feat(deploy): replace the PM2 build-on-server deploy with Docker images (#1121)

[frankbria]

Closes #1121.

What changed

The deploy SSHed in, git reset --hard, uv venv --clear && uv sync, npm ci && npm run build, then pm2 delete && pm2 start. A failure anywhere in that sequence left the box holding new code and a dead process, with no previous build to fall back to — and no artifact had ever been proven to build before it reached a host.

CI now builds two images and pushes them to GHCR tagged by commit SHA. The deploy is docker compose pull && docker compose up -d. Nothing is built on the server.

Dockerfile	backend: python:3.11-slim, uv sync --frozen, non-root, git present (workspaces and worktrees need it at runtime)
web-ui/Dockerfile	frontend: multi-stage → Next.js standalone runner
docker-compose.yml + .staging.yml / .production.yml	loopback-published, volume-backed, health-checked

Decisions the issue asked to be made explicitly

One image per environment. NEXT_PUBLIC_* are inlined into the client bundle at build time, so they are build args. BACKEND_ORIGIN turned out to be the same — standalone output snapshots next.config.js into required-server-files.json, so a runtime value is ignored. Found by watching the container still dial localhost:8000 and 500 on /api/*, not by reading docs.

Guard, don't delete, for production. Both paths are converted so nothing is half-migrated, but production has never run (#1143's preflight still refuses it) and the compose override says so rather than implying otherwise.

Delegated coding agents are not in the image. claude-code / codex / opencode are absent from the container, so those engines will not work on a containerised host. The built-in ReAct engine is unaffected. This was the migration risk the issue flagged; it is documented, not silently broken.

Safety properties carried over, not assumed

• Loopback only. 127.0.0.1:14200 / 127.0.0.1:14100. 0.0.0.0 inside a container is fine; publishing on 0.0.0.0 is not — a test asserts every mapping in both overrides ([P1.20] Terminate TLS in front of the services (they bind 0.0.0.0 over plaintext HTTP/ws) #747).
• State on volumes. DATABASE_PATH=/data/codeframe.db, WORKSPACE_ROOT=/workspaces. Anything written inside the image is gone the moment a new tag is pulled.
• The npm audit gate moved rather than vanished. [P2.35] Decide the four remaining web-ui advisories, then raise the deploy audit gate above critical #1131's --audit-level=high ran as part of build-on-server; deleting that step would have deleted the gate with it. It now runs in the image build, where it is stronger — an image carrying a high advisory cannot be built at all.
• [P1.15] Harden deployment: quote the release tag in the production SSH heredoc and add security headers to the Caddy example #933's release-tag hardening is intact and simpler. The production deploy no longer checks out the tag; the tag still crosses base64-encoded.
• [P0.18] Scope pm2 stop/delete to the staging apps instead of all #912's shared-VPS invariant is carried over. pm2 delete all once stopped every unrelated app on this box. docker system prune and docker stop $(docker ps -q) are the same blunt instrument, so the scanner now catches both — verified against four host-wide forms and three scoped ones.

Two P1s from review, both real

The deploy jobs could not pull what the build jobs pushed. They inherit permissions: contents: read, and GHCR packages are private by default even for a public repo — so the images would build and the deploy would fail at compose pull. Fixed with packages: read.

The first containerised deploy would have come up with an empty database. Staging holds 152K of real SQLite on the host, including the operator's own login account. The deploy now seeds the volume from .env.<env>'s DATABASE_PATH when the volume has none. Verified on the actual box, not reasoned about:

run 1:  copied into the volume
run 2:  volume already has a database — left alone
ls -ln: -rw-r--r-- 1 10001 10001 155648 codeframe.db

The chown is not decoration: Docker applies the image directory's ownership only when it initialises an empty named volume, and this one is pre-populated — so without it the DB lands root-owned and the non-root backend cannot write to it. Also caught on the box.

Retired

ecosystem.{staging,production}.config.js, the root package.json (it existed only for PM2's dotenv), systemd/codeframe-staging.service, scripts/{deploy,start}-staging.sh.

Verified locally, end to end, before any CI change

Both images build; the stack comes up healthy; /health 200; the API is auth-gated (401); the frontend serves and its /api proxy reaches the backend (401, not 500); /data survives a down/up.

Full backend suite	6510 passed, 49 skipped
ruff, actionlint (shellcheck on)	clean
Deploy-config tests	rewritten against the compose stack; 82 pass across the four deploy test modules

Still to do on this PR

A real staging deploy is the acceptance criterion CI cannot satisfy — including a rollback to a prior SHA. I will run it from this branch and report the result here before merging.

A note on my own testing

Three times in this PR I wrote a test that grepped the raw workflow for a forbidden command and matched the comment explaining why the command is gone. Same substring-as-classifier mistake as #1113/#1116/#1064. The deploy assertions now parse the YAML and strip comments via a shared _deploy_commands() helper.

[github-actions]

Claude finished @frankbria's task in 13m 56s —— View job

---

GLM precision review — PR #1154 (Docker deploy)

Bug-hunting pass: logic errors, security, data loss, race conditions, breaking contracts. (Style, architecture, naming, and coverage are handled by the separate general review — not covered here.)

• Gather PR context (gh pr view, gh pr diff, existing comments)
• Analyze deploy workflow + Dockerfiles + compose files
• Analyze deploy-config test rewrites
• Confirm each candidate defect against real file context
• Post inline findings + summary

Two concrete defects found, posted as inline review comments:

Severity	File:line	Finding
major	.github/workflows/deploy.yml:812	Production deploy ships no git checkout, but the preceding "Create pre-deployment backup" step still runs git rev-parse HEAD / git log -1 under set -e → first production deploy fails at the backup step, before compose pull. Carried over from main, where it worked only because the deploy did git fetch && git reset --hard first.
minor	docker-compose.production.yml:12	Override hardcodes the published host port to 14400, decoupling it from secrets.API_PORT; the verify step still curls secrets.API_PORT → false-negative health failure if API_PORT ≠ 14400 (e.g. the 8000 documented in .env.production.example). Staging only works because both happen to be 14200.

Notes on things checked and cleared:

• Staging DB→volume migration (deploy.yml seeding block): idempotent, correct volume name (codeframe_codeframe-data), chown 10001 is necessary not decorative, grep|cut keeps the assignment exit-clean under set -e. Verified the name: codeframe project matches the volume prefix.
• [P1.15] Harden deployment: quote the release tag in the production SSH heredoc and add security headers to the Caddy example #933 release-tag hardening: tag still crosses base64-encoded; no attacker-controlled tag_name/ref_name interpolated into a run: script (the workflow's own test pins this).
• [P0.18] Scope pm2 stop/delete to the staging apps instead of all #912 shared-VPS invariant: compose up --remove-orphans is scoped to project codeframe via explicit -f files; no host-wide docker prune/stop $(docker ps -q).
• packages: read on both deploy jobs — the P1 GHCR-pull fix is present.
• Frontend npm audit --audit-level=high gate moved into the image build (stronger, not deleted); runs before npm run build.
• Loopback-only publishing on every override; volumes back /data + /workspaces; backend environment: overrides stale DATABASE_PATH/WORKSPACE_ROOT.

Not modified, not pushed — review only.

[claude]

Review

Solid piece of infra work overall — the CI-builds-then-pulls shape is the right fix for the original problem, the compose files are careful about loopback binding and volumes, the DB-migration step is idempotent and was verified on the actual box, and the test rewrite (parsing YAML instead of grepping the raw workflow) closes a real self-identified gap (#1113/#1116/#1064-style false positives from matching explanatory comments). The two P1s fixed in the second commit (packages: read, DB seeding) were real and are fixed correctly.

One gap worth closing before calling the migration complete:

scripts/health-check.sh + systemd/codeframe-health-check.{service,timer} are left dangling

This PR retires ecosystem.staging.config.js, scripts/start-staging.sh, and systemd/codeframe-staging.service — but not the health-check script/timer that depend on them:

scripts/health-check.sh:106:    pm2 stop "$PROJECT_ROOT/ecosystem.staging.config.js" 2>/dev/null || true
scripts/health-check.sh:108:    pm2 delete "$PROJECT_ROOT/ecosystem.staging.config.js" 2>/dev/null || true
scripts/health-check.sh:112:    if [ -f "$PROJECT_ROOT/scripts/start-staging.sh" ]; then
scripts/health-check.sh:114:        bash "$PROJECT_ROOT/scripts/start-staging.sh" >> "$LOG_FILE" 2>&1
scripts/health-check.sh:117:        pm2 start "$PROJECT_ROOT/ecosystem.staging.config.js" >> "$LOG_FILE" 2>&1

systemd/codeframe-health-check.service still points at this script, and legacydocs/STAGING_SERVER.md documents installing codeframe-health-check.timer on the box to run it periodically. After this PR, both files it falls back to (ecosystem.staging.config.js, scripts/start-staging.sh) no longer exist — the pm2 stop/delete calls will just no-op (harmless), but restart_services() will either try to bash a missing file or pm2 start a missing config, doing nothing to restart the actual Docker containers. If this timer is (or ever was) enabled on the staging host, a real outage would now go "handled" by a script that cannot actually fix anything — worse than no auto-restart at all, since it can mask the incident.

Given the PR's own stated bar ("Guard, don't delete... nothing is half-migrated"), this looks like exactly that case slipping through — the systemd unit that was touched (codeframe-staging.service) got deleted, but its sibling health-check unit did not. Worth either translating health-check.sh to docker compose ps/up -d (mirroring the diagnostics translation already done in deploy.yml's failure-path logging) or removing it plus the systemd unit alongside the rest of the PM2 retirement, whichever matches what is actually installed on the box today.

scripts/remote-setup.sh also still prints ./scripts/deploy-staging.sh and pm2 save && pm2 startup as next steps (~lines 288-296) — lower priority since it's a one-time host-bootstrap script rather than something that runs unattended, but same underlying staleness.

Minor: GHCR token exposure window on a shared box

echo "${GHCR_TOKEN}" | docker login ghcr.io -u ${{ github.actor }} --password-stdin

--password-stdin correctly keeps the token out of docker login's own argv, but the value is still a literal argument to echo for the instant it runs — visible via ps aux on the box during that window. Given deploy/README.md/#912 note this is a shared VPS with unrelated apps on it, and #933 in this same file already went to the trouble of base64-transporting the release tag specifically to avoid shell exposure, this is a small inconsistency in that same threat model. A herestring (--password-stdin <<<"${GHCR_TOKEN}") avoids the argv exposure without changing behavior. Low severity — the token is job-scoped and short-lived, and the deploy job's copy is packages: read only — but cheap to fix given the care taken elsewhere in this file.

Nit

docker volume create codeframe_codeframe-data in the migration step hardcodes the compose project-name + volume-name concatenation. It's documented in docker-compose.yml's header comment and correct today, but nothing enforces the coupling if name: codeframe or the volume name ever changes independently — a silent way to seed the wrong volume. Not asking for anything here, just flagging the one hardcoded assumption in an otherwise well-tested migration step.

Everything else — Dockerfile layering, non-root users, loopback-only port tests, the audit-gate relocation, the #933/#912 hardening carried into the compose world — looks correct and is backed by parsed-YAML tests rather than string matching.

[github-actions]

Claude finished @frankbria's task in 16m 23s —— View job

---

Precision bug-hunting review — PR #1154 (Docker deploy)

Bug-hunting pass on the synchronized commit 35ef536 ("retire the PM2 processes before the containers bind"), which was pushed after the prior reviews. Scope: logic errors, security, data loss, races, contract breaks. Style/architecture/coverage are handled by the separate general review.

• Gather PR context (gh pr view, gh pr diff, existing comments)
• Analyze deploy workflow + Dockerfiles + compose files
• Analyze deploy-config test rewrites
• Confirm each candidate defect against real file context
• Post inline findings + summary

✅ No new defects in 35ef536

The new commit's delta — the staging PM2 cutover block, the labelled volume-create, and the two test updates — is correct against every defect class in scope. No inline findings posted.

What I checked in the new code and cleared:

Area	Verdict
PM2 cutover quoting (deploy.yml:385-393)	Sound. ${LEGACY_BACKEND_NAME}/${LEGACY_FRONTEND_NAME} are deliberately runner-expanded (baking the non-secret process names in); \${legacy} is escaped so the loop iterates server-side. No backticks / $( ) in the unquoted heredoc → no #1130 command-substitution hazard.
#912 shared-VPS invariant	Held. pm2 delete targets per-process names from secrets, never all; the host-wide-docker regex and test_pm2_scoping_912 both pass. (The unconditional pm2 save rewrites the box's resurrection dump each deploy — not a true no-op as the commit message claims — but it stops/deletes nothing, so it is not a #912 violation. Low impact, noted not flagged.)
Cutover ordering / race	Safe. Delete-by-name runs under command -v pm2 + pm2 describe with || true, and completes before compose pull/up, so 14100/14200 are free before the containers bind. Idempotent on every later deploy (describe finds nothing).
Volume labelling (deploy.yml:401-404)	Correct. Labels com.docker.compose.project=codeframe + =codeframe-data match the compose name: and volume name, so compose adopts the pre-seeded volume. docker volume create is a no-op on the second deploy (volume exists, labels unchanged) — fine.
Test rewrite (test_deploy_config.py, test_pm2_scoping_912.py)	Correct. The forbidden list now targets pm2 start/pm2 restart (allows the cutover delete); the naming regex pm2 (stop|delete) "?\\?\$\{?\w+ matches the new pm2 delete "\${legacy}" form. The regex still rejects unscoped all (caught by _PM2_ALL).

Previously-reported findings still stand

35ef536 touched neither, so both remain valid on the current HEAD — not re-posted, just flagged as still open:

Severity	File:line	Finding	Status
major	.github/workflows/deploy.yml:745-746	Production "Create pre-deployment backup" still runs git rev-parse HEAD / git log -1 under set -e on a host with no checkout → first production deploy aborts at the backup step, before compose pull.	open
minor	docker-compose.production.yml:12	Override hardcodes published port 14400, decoupled from secrets.API_PORT that the verify step curls → false-negative health failure if API_PORT ≠ 14400.	open

(The general-review concerns — scripts/health-check.sh + its systemd unit left dangling on PM2 retirement, the echo "${GHCR_TOKEN}" argv exposure window, and the hardcoded codeframe_codeframe-data volume-name coupling — are also still open and likewise untouched by this commit.)

Not modified, not pushed — review only.

[claude]

Review (re-review after the PM2-cutover commit)

This is a solid migration — CI-builds-then-pulls is the right fix for the original build-on-server problem, and the third commit's fix (retiring PM2 by name before the containers bind) is a real, verified-on-the-box fix for the actual staging failure. The compose loopback-only binding, the idempotent DB-volume migration, and the audit-gate relocation into the image build all check out on inspection.

Two things flagged by the earlier GLM review are still present in the current deploy.yml / docker-compose.production.yml — I re-verified both against the code on this branch (not just re-reading the old comment) since the latest commit didn't touch either:

Still open — major: production's pre-deployment backup step will fail on the very first production deploy

.github/workflows/deploy.yml:745-746 (Create pre-deployment backup, which runs before any git/docker operations):

git rev-parse HEAD > ${TMP_BACKUP}/previous_commit.txt
git log -1 --format="%H %s" >> ${TMP_BACKUP}/previous_commit.txt

under set -e. The production deploy step below it no longer does git fetch/git checkout (that's the point of #1121 — nothing is built or synced from git on the box anymore). Per the PR description, production "has never been deployed," so PROJECT_PATH on that host has no reason to already be a git checkout. First real production deploy hits this step, git rev-parse HEAD fails outside any git repo, set -e kills the job before compose pull ever runs. Even on a host where .git exists from some earlier manual setup, this record would be permanently frozen since nothing ever advances it again — so it stops being useful as a "previous commit" marker.

Still open — minor: production's health check can false-negative regardless of deploy success

docker-compose.production.yml:12 hardcodes the published backend port to 14400:

ports:
  - "127.0.0.1:14400:14200"

but both health-check steps (deploy.yml:447 and :865) curl http://localhost:${{ secrets.API_PORT }}/health. Before this PR, the PM2 config derived its bind port from the same BACKEND_PORT/API_PORT-sourced value the health check used, so they couldn't drift. Now they're two independent sources — if whoever fills in the production API_PORT secret uses anything other than 14400 (e.g. the 8000 in .env.production.example, or 14200 to mirror staging's convention), the container comes up healthy and the workflow still reports a failed deploy.

Both of these are specific to the production path, which is why the extensive local/staging verification in the PR description didn't catch them — production genuinely can't be exercised from a PR per #1143's own preflight gate. Worth fixing before the first real production run rather than discovering them the way the PM2-port conflict was discovered on staging.

Also still open from my prior review (unchanged by the latest commit)

• scripts/health-check.sh + systemd/codeframe-health-check.{service,timer} still reference ecosystem.staging.config.js / scripts/start-staging.sh, both deleted by this PR — if that timer is enabled on the staging box, a crash now gets "handled" by a script that can't actually restart the containers, which is worse than no auto-restart. scripts/remote-setup.sh (~line 288-296) also still prints the retired deploy-staging.sh/pm2 save/pm2 startup as next steps.
• The GHCR token still crosses through echo "${GHCR_TOKEN}" | docker login ... --password-stdin, briefly visible via ps aux on the shared box during that instant — low severity, but inconsistent with the base64-transport care taken for the release tag two sections down in the same file.
• docker volume create codeframe_codeframe-data hardcodes the compose project-name + volume-name concatenation with nothing enforcing the coupling — just a note, not a blocker.

Nothing new to flag in the third commit itself — the PM2-retirement logic (name-checked via pm2 describe before pm2 delete, || true throughout, no pm2 delete all) and the volume-adoption labeling look correct and match the updated #912 tests.

[frankbria]

Staging is containerised, verified on the box

The acceptance criterion CI cannot satisfy. Deploy run 31569173089:

✓ Build images (staging)   4m27s
✓ Deploy to Staging          44s

44 seconds, down from ~9 minutes — because nothing is built on the server any more.

$ curl https://dev.codeframe.sh/health                  → 200
$ curl https://dev.codeframe.sh/                        → 200
$ curl https://dev.codeframe.sh/api/v2/settings/keys    → 401   (auth intact)

$ ssh staging 'docker compose ... ps'
backend   ghcr.io/frankbria/codeframe-backend:35ef536…   Up (healthy)
frontend  ghcr.io/frankbria/codeframe-frontend:35ef536…  Up (healthy)

$ ssh staging 'pm2 list | grep -c codeframe'
0

The first attempt failed, which is the point

failed to bind host port 127.0.0.1:14200/tcp: address already in use

The PM2 processes were still holding the ports. A state copy was not enough — the migration needed a cutover. That is precisely the class of thing the issue said could not be validated from a PR, and it would have shipped as a broken first deploy.

The deploy now retires the two legacy processes by name (from the PM2_* secrets, which exist only for this) — never a pm2 delete of all, which on this shared VPS takes out unrelated apps (#912). No-op forever after, and on any host that never ran PM2.

The database migration carried real data

$ docker run --rm -v codeframe_codeframe-data:/data alpine ls -ln /data
-rw-r--r-- 1 10001 10001 155648 codeframe.db     ← owned by the container's uid

$ docker compose exec backend python -c "…select count(*) from users"
users 1

That is the operator's own login account, which a fresh volume would have silently dropped.

Rollback, on the box, no rebuild

$ export IMAGE_TAG=6579b27…       # the previous commit
$ $COMPOSE pull && $COMPOSE up -d
backend   …codeframe-backend:6579b27…   Up (healthy)
frontend  …codeframe-frontend:6579b27…  Up (healthy)
health: 200

$ export IMAGE_TAG=35ef536…       # forward again
users after rollback+forward: 1   ← volumes untouched by either direction

Two image swaps, both healthy in under 20 seconds, database intact through both.

Every acceptance criterion

AC	Evidence
Images build in CI, pushed to GHCR by SHA	Build images (staging) 4m27s; the box is running :35ef536…
compose up -d brings both up; /health 200 through the proxy; UI over HTTPS	above
No pm2 start, uv sync, npm run build over SSH	a test parses the workflow and asserts it; the only PM2 left is the one-time delete
Rollback documented and verified	above, plus deploy/README.md
DB and WORKSPACE_ROOT survive a deploy	named volumes; user count survived two image swaps
.env generation, AUTH_SECRET/WORKSPACE_ROOT fail-fast, health-check diagnostics preserved	unchanged; diagnostics now compose ps + compose logs
Both environments converted	staging verified live; production converted and still gated by #1143's preflight
Docs + tests/test_deploy_config.py updated	rewritten against the compose stack
Idempotent; concurrency groups intact	migration only fires on an empty volume; cutover is a no-op once PM2 is gone

One more thing shellcheck caught

While writing the cutover comment I put backticks inside the unquoted heredoc — the exact #1130 bug, in the same file, hours after fixing it. actionlint failed the commit. That is the argument for having turned it on.

[github-actions]

Claude finished @frankbria's task in 7m 32s —— View job

---

Precision bug-hunting review — PR #1154 (Docker deploy)

Bug-hunting pass on the new synchronized commit 7278f85 ("one source of truth for the port, and a backup that survives no checkout"), pushed after the prior reviews. Scope: logic errors, security, data loss, races, contract breaks. Style/architecture/coverage are handled by the separate general review.

• Gather PR context (gh pr view, gh pr diff, prior comments)
• Read the 7278f85 delta specifically (4 files: deploy.yml + 3 compose files)
• Verify the two previously-reported findings were actually fixed
• Hunt for new defects introduced by the fix
• Confirm each candidate against real file context
• Post inline findings + summary

✅ No new defects in 7278f85

The delta is correct against every defect class in scope. No inline findings posted.

Both previously-reported findings are genuinely fixed (verified, not re-posted)

Prior finding	Status	Why it's actually fixed
major — deploy.yml backup step git rev-parse HEAD / git log -1 aborted under set -e on a checkout-less host	resolved	The git calls are now wrapped in { git rev-parse HEAD 2>/dev/null || echo "no-checkout"; git log -1 … 2>/dev/null || true; }. Under set -e (L728, note: -e only, no pipefail), commands participating in a || list are exempt from errexit, and the group's exit status is the last command (git log … || true → 0). The added docker compose … images … 2>/dev/null || true is likewise safe. First production deploy no longer dies at the backup step.
minor — production published port hardcoded 14400, verify step curled secrets.API_PORT	resolved	The verify step curls secrets.API_PORT (L882); the deploy now export BACKEND_PORT="$(grep '^API_PORT=' .env.production | cut -d= -f2-)" (L859), and the override binds 127.0.0.1:${BACKEND_PORT:-14400}:14200. .env.production's API_PORT is itself generated from secrets.API_PORT (L682) and is in the REQUIRED fail-fast list (L554), so published-port == verify-port, both rooted in the one secret. Staging is symmetric (L372, default 14200).

What I checked in the new code and cleared

• grep \| cut \|\| true inside $(…) under set -euo pipefail (L372, L859): the \|\| true is inside the command substitution, so a no-match grep (exit 1) cannot trip pipefail and abort — BACKEND_PORT just lands empty, then the :-14200 / :-14400 default applies. Correct.
• The in-container listen port is the load-bearing assumption — and it holds. The backend CMD is … --port 14200 (Dockerfile:53, EXPOSE 14200), an explicit CLI flag that overrides server.py:956's BACKEND_PORT/PORT default. So the container always listens on 14200 regardless of any env. The override's right-side port stays :14200 and the in-container healthcheck (docker-compose.yml:49) curls 127.0.0.1:14200 — both match the fixed bind. This is what makes "vary only the host port" safe.
• API_PORT vs BACKEND_PORT conflation is intentional, not a bug. API_PORT (from the secret → .env) drives the host publication + verify curl; the server ignores it for binding. The deploy bridges the two by reading .env's API_PORT into the BACKEND_PORT compose-substitution var. The host-shell export BACKEND_PORT=… is NOT passed into the container (not in environment: or env_file; the generated .env contains API_PORT, not BACKEND_PORT), so it cannot perturb the in-container bind.
• Compose substitution precedence. --env-file .env.<env> does not define BACKEND_PORT, so the shell-exported value wins uncontested (shell > env-file). No stale value can shadow it.
• Base docker-compose.yml dropping ports: (the append-double-bind fix): verified correct. With no override the service is simply unpublished (documented design); with either override, exactly one mapping applies. The frontend's depends_on: service_healthy still works because the backend healthcheck uses the in-container port, which needs no host mapping. FRONTEND_PORT is never exported, so it always takes the override default (14300/14100) — harmless, and there is no frontend verify curl to drift against.
• set -e scope: the backup step and the deploy step are separate ssh heredocs (L727/820, L844/872); each set -e is local. The backup's docker compose images may emit an empty previous_images.json on a first deploy (no containers / compose files not yet scp'd), but the \|\| true makes that a non-issue.

Notes

• The general-review concerns from prior rounds (scripts/health-check.sh + its systemd unit left dangling on PM2 retirement, the echo "${GHCR_TOKEN}" argv-exposure window, the hardcoded codeframe_codeframe-data volume-name coupling) are out of scope for this precision pass and were untouched by 7278f85; still open as previously reported.
• Not modified, not pushed — review only.

[frankbria]

Both bot findings fixed, and one of them exposed a bug of mine

The pre-deployment backup ran git on a host that no longer has a checkout. Since the deploy pulls an image, git rev-parse HEAD legitimately finds no repository — and under set -e that aborted the whole backup step on a fresh production host. The commit is now recorded when it happens to be knowable, and the image list (which is the version now) is recorded always.

Production hardcoded the published port to 14400 while the verify step curled ${{ secrets.API_PORT }}. An operator setting API_PORT to anything else would get a container on one port and a health check on another. Both overrides now read ${BACKEND_PORT}/${FRONTEND_PORT}, and the deploy exports them from the generated .env's API_PORT — one secret drives both.

Fixing that surfaced a real bug in my own compose layout. Compose appends port lists across files rather than replacing them, so the base mapping plus the override meant production would try to bind both 14200 and 14400:

$ docker compose -f docker-compose.yml -f docker-compose.production.yml config | grep published
published: "14200"     ← from the base
published: "14400"     ← from the override
published: "14300"

The base file now declares no ports at all; each environment override owns its mapping:

$ … -f docker-compose.staging.yml config | grep -E "host_ip|published"
host_ip: 127.0.0.1   published: "14200"
host_ip: 127.0.0.1   published: "14100"

$ … -f docker-compose.production.yml config | grep published
published: "14400"
published: "14300"

Staging never hit this because both its mappings named the same port. Production would have failed on its first deploy — the environment that cannot be tested.

Re-deploying to staging to confirm the port refactor against the live stack; will report.

[claude]

Re-review — latest commit (7278f85, "one source of truth for the port, and a backup that survives no checkout")

Checked this commit's changes directly against the two still-open findings from the prior rounds of review on this PR:

Fixed — major: pre-deployment backup git rev-parse HEAD under set -e

.github/workflows/deploy.yml:754-757 now wraps the git calls:

{
  git rev-parse HEAD 2>/dev/null || echo "no-checkout"
  git log -1 --format="%H %s" 2>/dev/null || true
} > ${TMP_BACKUP}/previous_commit.txt

This is correct — a host with no checkout (the first production deploy) no longer aborts the backup step, and the image list (docker compose images --format json, already || true) remains the always-present version record the commit message describes.

Fixed — minor: production port drift between the published container port and the health check

docker-compose.production.yml and docker-compose.staging.yml both now read ${BACKEND_PORT:-14400} / ${BACKEND_PORT:-14200} instead of a hardcoded literal, and deploy.yml (staging: line 372-373, production: line 859-860) derives BACKEND_PORT by grepping the generated .env.{staging,production}'s API_PORT= line — which in turn is written verbatim from secrets.API_PORT. That's the same value the verify step curls (secrets.API_PORT directly), so the two can no longer independently drift. Verified the chain end-to-end; correct.

FRONTEND_PORT is never exported and just falls back to its compose default (14100/14300) — fine, since nothing health-checks the frontend port against a secret, so there's no drift risk there.

Gap: neither fix has a regression test

This PR's own description calls out getting burned three times by weak grep-based deploy tests and rewrote test_deploy_config.py to parse YAML instead. Neither of these two real bugs (a bare git rev-parse that dies under set -e outside a repo; a hardcoded port vs. an env-driven one) has a test locking in the fix, unlike the #933/#912 hardening which each got a dedicated test module. test_published_ports_are_loopback_only still passes trivially either way since "127.0.0.1:${BACKEND_PORT:-14400}:14200" also starts with "127.0.0.1:". Worth a small addition — e.g. assert the production/staging compose overrides reference BACKEND_PORT rather than a bare port literal, and that the backup step's git invocation has a fallback — so a future edit can't silently reintroduce either regression.

Still open (unchanged by this commit, previously flagged, non-blocking)

• scripts/health-check.sh (and the systemd unit/timer that runs it) still falls back to ecosystem.staging.config.js / scripts/start-staging.sh, both deleted by this PR — if that timer is enabled on the staging box, a crash now gets "handled" by a script that can't restart the containers. scripts/remote-setup.sh (~line 288-296) also still prints the retired PM2 next-steps.
• echo "${GHCR_TOKEN}" | docker login ... --password-stdin (both staging and production deploy steps) briefly exposes the token via ps aux on the shared box — low severity, but inconsistent with the base64-transport care taken for the release tag in the same file.
• docker volume create codeframe_codeframe-data still hardcodes the compose project-name + volume-name concatenation with nothing enforcing the coupling to docker-compose.yml's name: field.

None of these three are new — flagging only because they remain true against the current HEAD.

Overall

Both findings from earlier review rounds are now correctly fixed, verified by inspection against the actual env-generation and health-check code paths. The staging deploy was independently verified live on the box (images running, DB migrated with real data, rollback tested) per the PR author's own comment. Nothing new introduced by this commit. The dangling health-check script and the GHCR token echo remain worth cleaning up, but neither blocks merge given production is still gated behind #1143's preflight and has zero secrets configured.

[frankbria]

Re-deploy after the port refactor: ✓ 41s. Both containers healthy, single loopback mapping each, database intact.

$ docker compose ps
backend    Up 44 seconds (healthy)   127.0.0.1:14200->14200/tcp
frontend   Up 38 seconds (healthy)   127.0.0.1:14100->14100/tcp

$ curl https://dev.codeframe.sh/health                  → 200
$ curl https://dev.codeframe.sh/                        → 200
$ curl https://dev.codeframe.sh/api/v2/settings/keys    → 401
$ …exec backend python -c 'select count(*) from users'  → 1

Exactly one published port per service now — the double-bind the base+override append would have caused in production is gone, and staging is unaffected by the change. All checks green, no unresolved threads.