Deployment review: finish the compose-project fix, survive a reboot, and stop a slow migration reading as unhealthy - #17
Merged
Merged
Conversation
Follow-up to a47ddf8, whose message claimed it named the project "in every command it runs and prints". It did not, the docs it shipped contradicted themselves, and the tests it added could not detect either gap. Four faces of one mistake, so one commit. The probe at worker_ok() spells out `docker compose` rather than using compose(), because `timeout` needs a real command — and it was the single invocation left without -p. Harmless while both paths resolve `agrippa`; it diverges when the deployed commit's `name:` differs from the tree's at script start, since COMPOSE_PROJECT is read before the reset. `up -d` would act on one project while the probe queried another, and a healthy deploy would be rolled back with "worker never became ready". Both manuals printed a restore block WITHOUT -p, directly beneath the paragraph a47ddf8 added promising that it has one. That is the more dangerous of the two: an operator recovering under pressure reads the sentence, pastes the block, and drops a database on whatever stack the tree happens to name. The project derivation also sat above both die() and the `-f` guard, making those guards dead code. A missing compose file died on awk's own error under `set -e`; a compose file with no `name:` — reachable by rolling back to any pre-v0.2.0 commit — reached a `die` that did not exist yet and exited 127 with `die: command not found` and no explanation. Moved below the guards. The tests were the actual failure. `expect(r.log).toContain("-p agrippa")` asserts over the concatenation of every stub invocation, so it passes if a single call carries the flag; the probe never had one and they were green. Now every line in the log that invokes compose must carry it. Getting that right took two attempts, both caught by mutation rather than by reading. The first strengthened version still passed with the probe's -p removed, because it ran under STUB_UP_RC=1 — rollback's own `up -d` then fails too and the script dies before stack_ok, so the probe never executes. The second failed on the baseline: the stub log is a file that accumulates across run() calls within a test, so the second run's assertion saw the first run's lines. Pinning now asserts on the success path, one run per test.
…orker No compose service declared a restart policy, and nothing else supervises the stack — infra/janus/agrippa-deploy@.service is the one-shot deploy, and the infra/vm units belong to the other topology. A host reboot therefore left the instance down, silently, until somebody noticed and ran a deploy. Nothing alerts, because nothing is running to fail a healthcheck. The policy cannot be added on its own, which is why this is one commit rather than a one-line change. worker_ok() explicitly relied on its absence: compose sets no restart policy, so a crashed worker leaves an exited container that this catches With `restart: unless-stopped`, a crash-looping worker is "running" between restarts, and it registers its executors BEFORE it can die during pg-boss consumer setup — so it satisfies both the replica count and the fresh registration on the upswing of each loop. The failing deploy would have reported success and pruned the image that worked. worker_ok() now also fails when the workers' summed RestartCount moves during verification, which is what distinguishes looping from up. The baseline is taken once per stack_ok() call, after `up -d` has recreated the containers, so it measures change rather than level. Mutation-checked: deleting the RestartCount comparison makes the new case pass, i.e. the deploy reports success against a crash-looping worker. The stub increments the count per probe rather than returning a fixed value, because a fixed value cannot reproduce the thing being detected. This does not close the gap already recorded in worker_ok's comment and in issue #15: a worker that stays up but wedges *after* registering is still invisible. That needs a readiness signal written after boss.work() returns, which is an apps/worker change.
… dev-stack holes Three compose-only fixes; no script change. The api healthcheck had no start_period. Migrations, seeding and builtin template publication all run under top-level await before the listener opens, so every probe until then is a connection refusal — counted as a failure from the first one, reaching `unhealthy` at ~75s. It is survivable today only because deploy.sh polls for "healthy" in a loop and docker flips it back, but it would be fatal for anything that gates on the api's health during `up`, and in the meantime it reports a healthy deploy as unhealthy to anyone watching. start_period: 180s, matching HEALTH_TIMEOUT's intent, with start_interval: 3s so the healthy transition stays prompt once the listener does open. The dev stack had no `name:`, so its project resolved to the parent directory — `infra`, which is exactly what the production project was called before `name: agrippa` pinned it, and this file declares a volume also named `pgdata`. On a host predating that change, `down -v` here would delete the production volume; short of that, two clones of the repo silently share one dev database. It also published postgres and redis on every interface with a known password, while .env.example already teaches the loopback-binding technique for the production port. Docker's published ports insert their own iptables DNAT rules ahead of ufw, so a host firewall does not cover this. POSTGRES_PASSWORD defaulted to the literal `agrippa` published in the compose file, while the two secrets on the same lines use `:?` and refuse to start. .env.example compounded it with `change-me`, a placeholder that WORKS — so an operator who filled in only what compose complained about shipped a documented password with no signal. Now it hard-fails like its siblings and the example ships blank. Verified against real docker compose on the host, in /tmp, away from production: the dev file resolves project agrippa-dev with both ports on 127.0.0.1; the production file refuses to render without POSTGRES_PASSWORD and otherwise reports restart: unless-stopped on all four services and start_period 3m0s on the api.
📝 WalkthroughWalkthroughThe deployment stack now requires explicit PostgreSQL credentials, pins Compose project names, tolerates slow API startup, binds development databases to loopback, validates database URLs, and verifies worker restart stability. Deployment tests and English and Chinese documentation reflect these behaviors. ChangesDeployment readiness and Compose configuration
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant deploy.sh
participant DockerCompose
participant Workers
participant Database
deploy.sh->>DockerCompose: Render and start pinned Compose project
DockerCompose->>Workers: Start worker containers
deploy.sh->>Workers: Capture restart baseline
deploy.sh->>Database: Run readiness probe
deploy.sh->>Workers: Verify registrations and restart counts
Workers-->>deploy.sh: Stable readiness result
🚥 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: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/manual/en/06-operations.md`:
- Line 137: Update every documented Docker Compose command in
docs/manual/en/06-operations.md (anchor, lines 137-137) and
docs/manual/zh-CN/06-operations.md (sibling, lines 136-136) to include the -p
agrippa project pin. Keep the “every compose invocation” claim in CHANGELOG.md
(sibling, lines 41-41) unchanged because it is valid once both manuals are
updated.
In `@infra/docker-compose.yml`:
- Line 26: Ensure the API DATABASE_URL at infra/docker-compose.yml:26 and worker
DATABASE_URL at infra/docker-compose.yml:90 cannot receive URI-unsafe raw
passwords by applying a consistent URI-safe password constraint or encoding
strategy; update the password-generation guidance at
infra/env/.env.example:76-79 to use a URI-safe generator such as hex output, and
keep the same contract across all three sites.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: b59bd131-d062-4a75-b779-816ab6f27c91
📒 Files selected for processing (9)
CHANGELOG.mddocs/design/08-deployment.mddocs/manual/en/06-operations.mddocs/manual/zh-CN/06-operations.mdinfra/deploy.shinfra/deploy.test.tsinfra/docker-compose.dev.ymlinfra/docker-compose.ymlinfra/env/.env.example
| - "${AGRIPPA_PORT:-3000}:3000" | ||
| environment: | ||
| DATABASE_URL: postgres://agrippa:${POSTGRES_PASSWORD:-agrippa}@postgres:5432/agrippa | ||
| DATABASE_URL: postgres://agrippa:${POSTGRES_PASSWORD:?set POSTGRES_PASSWORD}@postgres:5432/agrippa |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Require URI-safe database passwords or encode them.
openssl rand -base64 24 can emit /, but both DATABASE_URL values interpolate the password unescaped; a generated password can therefore make API and worker database connections fail. Constrain passwords to URI-safe characters (for example, openssl rand -hex 24) and validate that contract, or percent-encode before URI construction.
infra/docker-compose.yml#L26-L26: avoid placing an arbitrary raw password in the API URI.infra/docker-compose.yml#L90-L90: apply the same password encoding/constraint to the worker URI.infra/env/.env.example#L76-L79: replace the base64 generation guidance with a URI-safe generator.
📍 Affects 2 files
infra/docker-compose.yml#L26-L26(this comment)infra/docker-compose.yml#L90-L90infra/env/.env.example#L76-L79
🤖 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 `@infra/docker-compose.yml` at line 26, Ensure the API DATABASE_URL at
infra/docker-compose.yml:26 and worker DATABASE_URL at
infra/docker-compose.yml:90 cannot receive URI-unsafe raw passwords by applying
a consistent URI-safe password constraint or encoding strategy; update the
password-generation guidance at infra/env/.env.example:76-79 to use a URI-safe
generator such as hex output, and keep the same contract across all three sites.
Reported by review on this PR, and introduced by this PR: the commit that made POSTGRES_PASSWORD mandatory recommended `openssl rand -base64 24` in the same breath. That value is interpolated unescaped into DATABASE_URL, and base64's alphabet includes `/`, which terminates a URL's authority component. Measured, not estimated: 74 of 200 generated base64 passwords make the URL unparseable, against 0 of 200 hex ones. So the change meant to stop operators shipping a published password instead handed roughly 37% of them a stack that could not boot, failing with a parse error naming neither the password nor the variable. Now `openssl rand -hex 24`, which is what infra/vm/install.sh:112 has always generated — so the two topologies agree rather than one being right by accident. AGRIPPA_SECRET_KEY and BETTER_AUTH_SECRET stay base64: neither goes into a URL. The sharper edge, found by sweeping for the same class rather than reported: Postgres reads POSTGRES_PASSWORD ONLY when the data directory is empty, while compose builds DATABASE_URL from it forever. Making the variable required therefore forces existing operators to set something, and setting anything other than their current password leaves the role unchanged — api and worker fail authentication, health never comes up, deploy.sh rolls back, and the rollback cannot recover it either, because infra/env/.env is untracked and survives `git reset --hard`. Terminal state is ROLLBACK UNHEALTHY with the error pointing at the deploy. Anyone hitting the base64 bug and "fixing" it by regenerating the password walks straight into this. The `:?` hard-fail is still right — defaulting to a password published in the compose file is the bug it replaced — so this is documented as a breaking change with a required action instead: .env.example states the initdb constraint and the upgrade value, both manuals gain a rotation recipe with the ALTER ROLE ordering, both quick-starts list the variable they now fail without, and CHANGELOG carries the action-required note. createDb() is the durable half: it now parses DATABASE_URL and, on failure, names percent-encoding and the hex generator rather than letting an opaque error surface at api boot. That catches a bad password whatever its origin, which grepping .env.example for a recommended command never would — so no test asserts on that prose. Mutation-checked: removing the guard fails the new case. Our own host is not affected — its password is set, contains no `/`, and matches the role. Establishing that took two invalid probes first: `compose exec postgres psql -U agrippa` goes over the local unix socket, and `psql -h 127.0.0.1` inside the container hits the 127.0.0.1 trust rule. Neither tests the password. The valid evidence is that the api authenticates over the docker network, which is the only path scram-sha-256 applies to.
Both lines were falsified by the restart-policy commit earlier on this branch, and one of them was reported by review. "compose sets no restart policy, so a crashed worker leaves an exited container that this catches" is now the opposite of true, and it sat directly above the worker_restarts helper whose own comment explains why the policy breaks that assumption. "verify two things" undercounts: it is three, and the third — RestartCount — was added precisely because the policy invalidated the other two on their own. The undercount is the one that could bite. A reader who counts two checks is liable to treat the RestartCount comparison as leftover instrumentation and remove it, which restores exactly the masking that commit fixed: a crash-looping worker reads as running between restarts and has already registered before it dies, so the deploy reports success, last-good advances to the bad commit, and prune deletes the image that worked. The comment now says all three are load-bearing and what deleting the third one costs. This is the same self-contradicting-documentation defect fixed in the manuals two commits earlier on this branch, committed again immediately afterwards — the fix and the repeat were in the same review round.
Reported by CodeRabbit on this PR, and correct. I pinned -p in deploy.sh and in
the restore procedure it prints, then wrote a CHANGELOG line opening "Every
compose invocation…" while leaving roughly a dozen documented compose commands
in the manuals unpinned. Same shape as the two rounds before it: the claim
outran the change.
The sharpest instance is destructive. The pre-v0.2.0 migration recipe in both
manuals opens with an unpinned `down -v`, and its very next line is a
deliberate `-p infra`. With COMPOSE_PROJECT_NAME set, step 0 deletes the
volumes of whatever stack that variable names — in a procedure the operator is
running precisely because they have two stacks on one host.
Every executable example that operates a stack now names its project:
production commands `-p agrippa`, dependency-only commands `-p agrippa-dev`
(the dev compose file gained `name: agrippa-dev` earlier on this branch).
Four kinds of occurrence are deliberately left unpinned, which is the whole
judgement here:
- prose illustrating the anti-pattern — "a bare `git pull && docker compose
up -d` restarts the OLD code", and "a plain `docker compose up -d` would
build a second, empty stack". Pinning these would dress up the wrong
command as sanctioned.
- the `# 0. ONLY if you already ran docker compose up -d` comment, which
refers to something the reader did before, not something to run.
- `-p infra` in the migration recipe: that is the old stack, named on purpose.
- `docs/design/08-deployment.md`'s `--scale worker=3`, which is a separate
known defect — it contradicts the WORKER_REPLICAS mechanism deploy.sh
verifies against, and is deferred with the other tier-4 findings.
Pinning trades one foot-gun for another if left unexplained: an operator
running a drill who copies a pinned command would now hit production instead of
their own stack, which is exactly the accident the restore drill caught. So
both manuals now state up front that examples pin `agrippa`, that the flag is
redundant until something sets COMPOSE_PROJECT_NAME, and that a second stack
substitutes its own project name rather than dropping the flag.
The CHANGELOG claim is scoped to what deploy.sh does and now separately states
that the documented commands are pinned too, so both halves are true rather
than one covering for the other.
CodeRabbit's other comment — URI-unsafe database passwords — was already fixed
by e342d27 earlier on this branch; it was reviewing the pushed head, which
predates it. No change needed.
…ast examples
Two follow-ups from one review round. Both are mine.
FIRST HUNK — infra/deploy.sh, behavioural.
The tag assertion was a single pipeline:
compose config 2>/dev/null | grep -q "agrippa-api:${sha}" ||
rollback "AGRIPPA_VERSION did not reach compose; images would be mistagged"
`2>/dev/null` discarded compose's stderr, which is the only place the offending
variable is named, and every non-zero outcome funnelled into one message about
image tagging. Verified against the real binary on the host: a failing `compose
config` writes `required variable POSTGRES_PASSWORD is missing a value` to
stderr and 0 bytes to stdout — so the grep finds nothing and the `||` fires,
which is exactly how an unrelated failure acquired a tagging diagnosis.
That path is not hypothetical any more, because of my own change earlier on
this branch: `${POSTGRES_PASSWORD:?…}` means an installation that relied on the
removed default fails precisely here, and gets told its images would be
mistagged. Same class as the CHANGELOG overclaim and the self-contradicting
manuals — the message and the reality diverge.
Validation is now separate from the tag assertion and keeps stderr. Rendering
twice costs nothing; it is a local template expansion. Mutation-checked:
restoring the single-pipeline form fails the new case, which asserts both that
compose's own diagnostic survives AND that it is not relabelled — the second
assertion is the one the fix earns.
SECOND HUNK — three runnable examples, documentation only.
infra/env/.env.example, infra/docker-compose.yml and infra/docker-compose.dev.yml
each carry a usage command that omitted -p, keeping the wrong-stack risk the
previous commit set out to remove.
That commit claimed "the audit that found this, re-run as the check". The audit
covered docs/, README.md and CONTRIBUTING.md and never grepped infra/ — complete
only over a scope I chose silently and then described as exhaustive. Re-run
across the whole repo, the only unpinned occurrences left are prose, anti-pattern
illustrations ("a bare `git pull && docker compose up -d` restarts the OLD
code"), historical CHANGELOG entries, test names and comments, and the deliberate
`-p infra` in the migration recipe.
…ICAS Two review follow-ups, plus a harness defect found while checking one of them. THE UPGRADE SYMPTOM. deploy.sh edits take effect one deploy later — established and documented two rounds ago — so the first upgrade to this release runs the PREVIOUS release's script. Confirmed: f5e86a4:321 is the old single-pipeline `compose config 2>/dev/null | grep -q`, and its compose file still defaults POSTGRES_PASSWORD. That script resets the tree onto the new required-password compose file, discards compose's stderr, and reports "AGRIPPA_VERSION did not reach compose; images would be mistagged". The improved diagnostic exists and cannot run on the one deploy that needs it. Bounded, which is why documenting beats staging: rollback restores the previous commit, whose compose still carries the default, so the stack comes back healthy. The deploy fails with a wrong message; production is not down. Staging the enforcement across two releases would only protect operators who deploy every release in order — anyone skipping a version still hits it, while the error string is what someone actually searches for. So the exact string is now in the CHANGELOG upgrade note and in both manuals' troubleshooting tables, along with the `compose config` command that shows the real cause. SCALING. docs/design/08-deployment.md recommended `docker compose up --scale worker=3`. worker_ok reads WORKER_REPLICAS from the env file and requires exact equality, so that recommendation desynchronises the two and the next deploy fails verification for the full HEALTH_TIMEOUT and rolls back a healthy stack. Review asked for `-p` on that command; pinning it would have dressed up a broken recommendation, so it now documents WORKER_REPLICAS and says why. No runnable unpinned compose command remains anywhere in the repo. THE HARNESS DEFECT, found because a mutation contradicted my prediction. I expected `-eq` → `-ge` to fail only the new more-workers-than-expected case and leave the dead-worker case green. Both failed, and the reason is that BSD seq counts DOWN when first > last: `seq 1 0` emits "1 0" on macOS and nothing under GNU. So STUB_WORKERS_RUNNING=0 meant "two workers" on every developer machine and "no workers" in CI — the dead-worker case has been asserting a count mismatch locally, not a dead worker, and its own comment said otherwise. The stub now counts with a while loop. With that fixed the mutation discriminates as predicted: only the new case fails. The new case is worth its own test rather than trusting the existing one: relaxing `-eq` to `-ge` is a plausible refactor that keeps the dead-worker case green while silently letting an out-of-band-scaled stack pass verification.
… the rolled-back one The troubleshooting row added last round to explain a misleading message was self-defeating. It told operators to run `compose config` in /opt/agrippa — but rollback() resets the checkout to the previous commit (deploy.sh:208) before they ever see the failure, and in the scenario the row documents that previous compose file still defaulted POSTGRES_PASSWORD. So the prescribed command renders cleanly, reports nothing, and points away from the cause. I demonstrated the defect by accident while checking the replacement: rendering the server's HEAD — f5e86a4, the pre-PR commit — with the password stripped returned rc=0. That is exactly what an operator following the old row would have seen. The row now leads with the cause and the fix (set POSTGRES_PASSWORD, redeploy, the stack is already rolled back and running), states plainly that checking in place will mislead and why, and gives a command that reproduces the real error by rendering the commit that actually failed: cd /opt/agrippa git show <failed-sha>:infra/docker-compose.yml | docker compose -p agrippa -f - --env-file infra/env/.env config Verified against the real binary on the host, both directions: that file on stdin without the password exits 1 and names POSTGRES_PASSWORD; with it, it renders clean and exits 0. The failed SHA comes from the deploy's own "after a failed deploy of …" line, so the operator has it without extra digging. CHANGELOG needs no change — its upgrade note says "set the variable and redeploy" and never prescribed the broken check. No behavioural change, so no mutation check applies here; the gate below is a regression guard rather than evidence for the fix.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
infra/deploy.sh (1)
263-270: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winFail closed and bound the worker restart probe.
Lines 269-270 turn empty
docker inspectoutput into0, making Docker API failures indistinguishable from a healthy zero restart count. Since both the baseline and later comparison use this helper, readiness can pass without validating RestartCount. The inspect call is also unbounded, so a hung Docker daemon can bypassHEALTH_TIMEOUT.Return failure for inspect errors/empty output, apply the probe timeout, and propagate failures at Lines 298 and 326.
Proposed direction
- docker inspect -f '{{.RestartCount}}' $ids 2>/dev/null | - awk '{s += $1} END {print s + 0}' + local counts + counts="$(timeout "${probe_budget:-${HEALTH_TIMEOUT:-10}}" \ + docker inspect -f '{{.RestartCount}}' $ids 2>/dev/null)" || return 1 + [ -n "$counts" ] || return 1 + awk '$1 !~ /^[0-9]+$/ { exit 1 } { s += $1 } END { if (NR == 0) exit 1; print s + 0 }' \ + <<<"$counts" - restarts="$(worker_restarts)" + restarts="$(worker_restarts)" || return 1 - worker_restart_baseline="$(worker_restarts)" + worker_restart_baseline="$(worker_restarts)" || return 1Also applies to: 296-298, 324-327
🤖 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 `@infra/deploy.sh` around lines 263 - 270, Update worker_restarts to fail when docker inspect errors or returns empty output instead of converting either case to zero, and apply the existing health/probe timeout to bound the Docker call. At the baseline and later comparison sites in the readiness flow, including the logic around worker_restarts calls, propagate a failed probe and prevent readiness from passing; preserve valid RestartCount aggregation for successful inspections.
🤖 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 `@infra/env/.env.example`:
- Around line 88-89: Update the documented postgres password-rotation command in
the env example to include the deployment Compose file and env-file options,
specifically `infra/docker-compose.yml` and `infra/env/.env`, while preserving
the existing project name and psql arguments.
---
Outside diff comments:
In `@infra/deploy.sh`:
- Around line 263-270: Update worker_restarts to fail when docker inspect errors
or returns empty output instead of converting either case to zero, and apply the
existing health/probe timeout to bound the Docker call. At the baseline and
later comparison sites in the readiness flow, including the logic around
worker_restarts calls, propagate a failed probe and prevent readiness from
passing; preserve valid RestartCount aggregation for successful inspections.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 2f84e15c-99e9-4681-b23e-5280891f11ec
📒 Files selected for processing (16)
CHANGELOG.mdCONTRIBUTING.mdREADME.mddocs/design/08-deployment.mddocs/design/09-testing-and-ci.mddocs/manual/en/01-getting-started.mddocs/manual/en/06-operations.mddocs/manual/zh-CN/01-getting-started.mddocs/manual/zh-CN/06-operations.mdinfra/deploy.shinfra/deploy.test.tsinfra/docker-compose.dev.ymlinfra/docker-compose.ymlinfra/env/.env.examplepackages/db/src/client.test.tspackages/db/src/client.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- infra/docker-compose.dev.yml
- infra/docker-compose.yml
- CHANGELOG.md
- docs/manual/en/06-operations.md
- docs/manual/zh-CN/06-operations.md
- docs/design/08-deployment.md
…ecrets The troubleshooting recipe added last round ran `docker compose … config` with no redirect. That renders the fully interpolated model to stdout, and this compose file carries AGRIPPA_SECRET_KEY, BETTER_AUTH_SECRET, ANTHROPIC_API_KEY and the database password in service environments. Measured on the host, counting lines rather than echoing them: six secret-bearing lines went to stdout. An operator following those instructions mid-incident would paste them into a terminal, a ticket, or a chat log. `--quiet` validates without printing. Verified against compose 5.3.1 in both directions, because a quiet command that also swallowed the error would be worse than the one it replaces: with a valid env file it exits 0 with zero bytes of stdout and zero secret-bearing lines; with POSTGRES_PASSWORD stripped it exits 1 and still names the variable on stderr. The rationale ships next to the flag on purpose. A bare `--quiet` invites the next reader to drop it in order to see the output; the sentence explaining what the output contains is what makes it stick. Nothing else needed changing. deploy.sh's two `compose config` calls already discard stdout — one captures stderr via `2>&1 >/dev/null`, the other pipes into `grep -q` — so neither reaches a terminal or the janus-readable deploy log. DECLINED, with evidence: review also reported that .env.example's rotation command omits `-f`/`--env-file` and so "may fail to resolve the stack or the required POSTGRES_PASSWORD". Tested directly — `docker compose -p agrippa exec -T postgres psql …` returns 0 from /opt/agrippa and from a neutral /tmp. Compose resolves `exec` against running containers by project label and never renders the model, so no interpolation happens and no env file is needed. The proposed fix would make it worse: adding --env-file forces interpolation, so the command would then require POSTGRES_PASSWORD to be set, breaking it for the case the surrounding comment documents — rotating the role BEFORE putting the new value in the env file. Left as-is deliberately.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Final review of the deployment feature, after it went live and survived two drills (rollback, and a database restore against a throwaway stack). Three reviewers swept
deploy.sh, the container/runtime layer, and the automation+docs surface; every finding was verified against the code rather than accepted on report.Ten commits across five review rounds. Four of the original findings were defects in a commit I landed the same day, and each later round found more of the same shape — a claim that outran the change. That history is kept in the commits deliberately.
What it fixes
Compose project naming.
deploy.shnamed the project in some invocations and not others; the worker readiness probe — the one call that spells outdocker composebecausetimeoutneeds a real command — never did. UnderCOMPOSE_PROJECT_NAMEit would query a different project thanup -dacted on, rolling back a healthy deploy. Both manuals also printed a restore block without-p, directly beneath a paragraph promising it had one; an operator recovering under pressure would have dropped a database on the wrong stack.Failing with a message. The project derivation sat above
die()and the-fguard, so a compose file with noname:— reachable by rolling back to any pre-v0.2.0commit — exited 127 withdie: command not found.Surviving a reboot. No service declared a restart policy and nothing else supervises the stack, so a host reboot left the instance down silently. That could not be fixed alone:
worker_okrelied on its absence, and a crash-looping worker registers its executors before dying, so it would have satisfied both readiness checks on each upswing and reported a failed deploy as successful.worker_oknow also fails when the workers' summedRestartCountmoves during verification.Healthcheck, dev stack, password.
start_period: 180sso boot-time migration stops reading as unhealthy. The dev stack resolved to projectinfra— the production name before it was pinned, with a volume also calledpgdata— and published postgres/redis on every interface.POSTGRES_PASSWORDdefaulted to a value published in this file while its two siblings hard-failed.What review found afterwards
.env.examplerecommendedopenssl rand -base64 24in the same commit that made the variable mandatory. Base64 emits/, which terminates a URL's authority component. Measured: 74/200 base64 passwords makeDATABASE_URLunparseable, 0/200 hex. Nowopenssl rand -hex 24, matching whatinfra/vm/install.shalways generated.POSTGRES_PASSWORDis applied only at initdb. Making it mandatory forces existing operators to set something, and setting anything but their current password leaves the role unchanged — and the rollback cannot recover it, becauseinfra/env/.envis untracked and survivesgit reset --hard. Documented as a breaking change with a required action, plus a rotation recipe with theALTER ROLEordering.deploy.shchanges take effect one deploy later, so the first upgrade runs the previous script, which discards compose's stderr and reportsAGRIPPA_VERSION did not reach compose. Documented rather than staged across two releases — staging only protects operators who deploy every release in order, while the error string is what someone actually searches for.--scale worker=3in the design doc would have rolled back a healthy stack:worker_okrequires the running count to equalWORKER_REPLICAS. Corrected rather than pinned — pinning would have sanctioned a broken recommendation.rollback()resets the checkout before the operator reads it, socompose configin place renders the rolled-back file, which still had a password default — it succeeds and shows nothing. Now renders the failed commit explicitly viagit show … | docker compose -f -.configprints the resolved model: 6 secret-bearing lines includingAGRIPPA_SECRET_KEYand the database password. Nowconfig --quiet, with the reason next to the flag.One finding declined, with evidence
Review reported that
.env.example's rotation command needs-f/--env-file. Tested:docker compose -p agrippa exec -T postgres psql …returns 0 from/opt/agrippaand from a neutral/tmp— compose resolvesexecby project label and never renders the model. The proposed fix would introduce a failure, since--env-fileforces interpolation and would then requirePOSTGRES_PASSWORD— breaking the documented case of rotating the role before writing the new value.A defect no review found
STUB_WORKERS_RUNNING=0meant "two workers" on macOS and "none" in CI: BSDseq 1 0counts down. The dead-worker test was asserting a different property per platform while its comment claimed otherwise. Found because a mutation contradicted a prediction — the test was green in both configurations, it just wasn't testing what it said.Verification
bun run check·bun test317 pass / 0 fail / 0 skipped ·templates:validate·build·shellcheck -S warning· commitlint.Mutation-checked individually, each reported rather than assumed: removing
-pfrom the probe or fromcompose(); reverting the guard ordering; removing theRestartCountcomparison; restoring the single-pipeline config check; relaxing-eqto-ge; removing thecreateDbURL guard. Two of those took multiple attempts before the test could fail at all — recorded in the commits.Compose behaviour verified against the real binary on the host, in
/tmp, away from production: project resolution, the required-variable hard-fail, the-f -stdin diagnostic, and--quietemitting zero bytes while still reporting the error.Not verified live:
deploy.shchanges take effect one deploy later, so theworker_okchange needs a second deploy after merge before it is actually exercised.Reviewed and deliberately deferred
All verified real, out of scope here:
STATE_DIR/last-goodnot project-scoped, so a drill without an explicitSTATE_DIRoverwrites production's rollback target; the hardcodedagrippa-api-1first-deploy fallback; no memory/PID limits on the worker that runs untrusted agent code; test gaps (STUB_BUILD_RC/STUB_DUMP_RCunused, no rollback test asserts the tree was restored, no test passes an explicit SHA); the polkit grant being scoped to thejanususer rather than to this pipeline; andinfra/vm/, which restarts an API that migrates on boot with no pre-deploy dump, no rollback and no worker verification.