fix: Refuse a profile the compose file does not declare #47

Merged
ahmad merged 2 commits from feature/39-guard-monitoring-profile into main 2026-09-05 09:02:46 +00:00
Owner

Issue

Closes #39

Problem

deploy-vps arms COMPOSE_PROFILES=monitoring from the presence of the three Grafana credentials alone, without looking at the compose file it is about to deploy. Compose treats a profile nothing declares as selecting no services and still exits 0 — so an application whose compose has no monitoring services deploys, reports success, and collects nothing, while its absent-metrics alert fires against a perfectly healthy service.

That is ahmad/imamah#134 generically: one repository's instance was fixed there, the behaviour was not.

Solution

Before up, the deployment refuses a profile the compose file does not declare, naming both.

Review notes

The check is on the host, not where the profile is armed, and that is deliberate. The arming happens in a local step; the only reliable way to ask which profiles a compose file declares is docker compose config --profiles, and the action cannot assume a docker CLI inside the job container. Grepping the YAML would be dependency-free and wrong in both directions — a false negative would refuse a good deployment. The remote script runs where docker is guaranteed and the real parser is available.

Verified against a stub docker compose, all four states:

armed, compose declares it            exit 0
armed, compose declares nothing       exit 1  "…requests the 'monitoring' profile, which
                                                docker-compose.vps.yml does not declare."
                                                "Declared profiles: (none)"
not armed                             exit 0  (check does not run)
armed, compose declares only `debug`  exit 1  "Declared profiles: debug"

Mutation — the condition neutered to if false, keeping the grammar valid and confirmed with bash -n:

armed, nothing declared, guard neutered   exit 0   <- the silent success this removes

The fourth acceptance criterion is not met as written, and the criterion is mine. I wrote "when the action's tests run, then all three cases above are covered". This repository has no behavioural test harness for actions — check-actions.py parses each action.yml and runs bash -n over the run: bodies, and cannot execute one. The three cases are covered by the verification above rather than by a committed test, which is weaker: it does not run again when someone edits this action next year.

Building that harness is a real gap and I have filed it separately rather than smuggling it in here.

Risks and trade-offs

This turns a silent misconfiguration into a failed deployment. That is the intent, and it is the right way round — an unmonitored deploy that believes it is monitored is worse than one that stops. But it does mean a target whose compose legitimately lacks the profile, while the credentials happen to be present, now fails where it used to proceed. The message says both remedies, and the credentials being present on a target that cannot use them is itself worth knowing.

${COMPOSE_PROFILES//,/ } splits on commas, which is how compose accepts multiple profiles. Only monitoring is ever armed today, so the loop is future-proofing rather than a live case — cheap, but it is one more thing to be wrong if compose's syntax ever grows.

### Issue Closes #39 ### Problem `deploy-vps` arms `COMPOSE_PROFILES=monitoring` from the presence of the three Grafana credentials alone, without looking at the compose file it is about to deploy. Compose treats a profile nothing declares as selecting no services and still exits 0 — so an application whose compose has no `monitoring` services deploys, reports success, and collects nothing, while its absent-metrics alert fires against a perfectly healthy service. That is `ahmad/imamah#134` generically: one repository's instance was fixed there, the behaviour was not. ### Solution Before `up`, the deployment refuses a profile the compose file does not declare, naming both. ### Review notes **The check is on the host, not where the profile is armed, and that is deliberate.** The arming happens in a local step; the only reliable way to ask which profiles a compose file declares is `docker compose config --profiles`, and the action cannot assume a docker CLI inside the job container. Grepping the YAML would be dependency-free and wrong in both directions — a false negative would refuse a good deployment. The remote script runs where docker is guaranteed and the real parser is available. **Verified against a stub `docker compose`, all four states:** ``` armed, compose declares it exit 0 armed, compose declares nothing exit 1 "…requests the 'monitoring' profile, which docker-compose.vps.yml does not declare." "Declared profiles: (none)" not armed exit 0 (check does not run) armed, compose declares only `debug` exit 1 "Declared profiles: debug" ``` **Mutation** — the condition neutered to `if false`, keeping the grammar valid and confirmed with `bash -n`: ``` armed, nothing declared, guard neutered exit 0 <- the silent success this removes ``` **The fourth acceptance criterion is not met as written, and the criterion is mine.** I wrote "when the action's tests run, then all three cases above are covered". This repository has no behavioural test harness for actions — `check-actions.py` parses each `action.yml` and runs `bash -n` over the `run:` bodies, and cannot execute one. The three cases are covered by the verification above rather than by a committed test, which is weaker: it does not run again when someone edits this action next year. Building that harness is a real gap and I have filed it separately rather than smuggling it in here. ### Risks and trade-offs **This turns a silent misconfiguration into a failed deployment.** That is the intent, and it is the right way round — an unmonitored deploy that believes it is monitored is worse than one that stops. But it does mean a target whose compose legitimately lacks the profile, while the credentials happen to be present, now fails where it used to proceed. The message says both remedies, and the credentials being present on a target that cannot use them is itself worth knowing. `${COMPOSE_PROFILES//,/ }` splits on commas, which is how compose accepts multiple profiles. Only `monitoring` is ever armed today, so the loop is future-proofing rather than a live case — cheap, but it is one more thing to be wrong if compose's syntax ever grows.
fix: Refuse a profile the compose file does not declare
All checks were successful
PR / validate (pull_request) Successful in 3m19s
e5a7d6eb5d
COMPOSE_PROFILES was armed from the Grafana credentials alone, without
looking at the compose file. Compose selects nothing for an unknown
profile and still exits 0, so an app with no monitoring services
deployed, reported success and collected nothing, while its
absent-metrics alert fired against a healthy service.

Checked on the host, where a docker CLI is guaranteed and
config --profiles is the only reliable answer.

Closes #39
Author
Owner

validate green on e5a7d6e. Verification against each acceptance criterion.

Given a compose file that declares no service with the monitoring profile, when the deploy runs with the Grafana credentials present, then it fails with a message naming the profile and the compose file — rather than starting the remaining services.

COMPOSE_PROFILES requests the 'monitoring' profile, which docker-compose.vps.yml does not declare.
Declared profiles: (none)

Either add that profile's services to the compose file, or clear the
Grafana credentials for this target so the profile is not armed.
                                                                          exit 1

Both remedies are named, because "it failed" without "and here is what to do" is how a guard becomes something people delete.

Given a compose file that does declare the profile, when the deploy runs with the credentials present, then it arms it exactly as it does today.

exit 0, and nothing else in the step changed — the arming logic is untouched; this only reads what it produced.

Given the credentials are absent, when the deploy runs, then the profile is not armed and the check does not run.

exit 0 with the block skipped entirely, since it is guarded on COMPOSE_PROFILES being non-empty.

A fourth state, not in the criteria but worth covering because it is the one a second profile would produce: the compose declares debug and the deploy asks for monitoring — fails, and reports Declared profiles: debug, so the message distinguishes "declares nothing" from "declares something else".

Given the check is added, when the action's tests run, then all three cases above are covered.

Not met as written, and I wrote it. This repository has no harness that can execute an action — check-actions.py parses and runs bash -n, nothing more. The four states above are covered by a stub-driven verification, which does not run again when someone edits this action later. Filed as #48 rather than quietly widening this pull request, and the mutation is what stands in for a test meanwhile:

condition neutered to `if false` (grammar valid, bash -n clean)
armed, nothing declared   ->  exit 0   <- the silent success this fix removes

Not proven here: that a real deployment refuses. That needs a host, credentials and a compose file without the profile — which is imamah before its #134, and nothing available to me. The stub exercises the logic; the shape of the failure in production is inferred from compose's documented behaviour, which is the same inference the bug itself rests on.

`validate` green on `e5a7d6e`. Verification against each acceptance criterion. > Given a compose file that declares no service with the `monitoring` profile, when the deploy runs with the Grafana credentials present, then it fails with a message naming the profile and the compose file — rather than starting the remaining services. ``` COMPOSE_PROFILES requests the 'monitoring' profile, which docker-compose.vps.yml does not declare. Declared profiles: (none) Either add that profile's services to the compose file, or clear the Grafana credentials for this target so the profile is not armed. exit 1 ``` Both remedies are named, because "it failed" without "and here is what to do" is how a guard becomes something people delete. > Given a compose file that does declare the profile, when the deploy runs with the credentials present, then it arms it exactly as it does today. `exit 0`, and nothing else in the step changed — the arming logic is untouched; this only reads what it produced. > Given the credentials are absent, when the deploy runs, then the profile is not armed and the check does not run. `exit 0` with the block skipped entirely, since it is guarded on `COMPOSE_PROFILES` being non-empty. A fourth state, not in the criteria but worth covering because it is the one a second profile would produce: the compose declares `debug` and the deploy asks for `monitoring` — fails, and reports `Declared profiles: debug`, so the message distinguishes "declares nothing" from "declares something else". > Given the check is added, when the action's tests run, then all three cases above are covered. **Not met as written, and I wrote it.** This repository has no harness that can execute an action — `check-actions.py` parses and runs `bash -n`, nothing more. The four states above are covered by a stub-driven verification, which does not run again when someone edits this action later. Filed as **#48** rather than quietly widening this pull request, and the mutation is what stands in for a test meanwhile: ``` condition neutered to `if false` (grammar valid, bash -n clean) armed, nothing declared -> exit 0 <- the silent success this fix removes ``` **Not proven here:** that a real deployment refuses. That needs a host, credentials and a compose file without the profile — which is `imamah` before its #134, and nothing available to me. The stub exercises the logic; the shape of the failure in production is inferred from compose's documented behaviour, which is the same inference the bug itself rests on.
ahmad changed title from WIP: fix: Refuse a profile the compose file does not declare to fix: Refuse a profile the compose file does not declare 2026-09-05 07:52:41 +00:00
ahmad_bot left a comment

One finding, inline and non-blocking. The guard does what it claims, and the acceptance criteria you can meet are met — CI green.

The most useful thing I can report is that this breaks nothing on merge. Your risks section flags a target whose compose lacks the profile while the credentials happen to be present, which would newly fail. I checked whether that target exists, across every consumer in the fleet:

declares `profiles: [monitoring]`   ahmad/portfolio  ahmad/trip  bayan/kidpoints  suhba/khitta
passes `grafana-prom-*` to deploy-vps   ahmad/portfolio  ahmad/trip  bayan/kidpoints  suhba/khitta

The two sets are identical, so no current deployment is newly refused. (ahmad/imamah is absent from both, consistently — it has its own deploy action rather than this one. expiro, bayan-web, stem, suhba-web and suhba-docs declare no profile and arm none.)

Verified against a stub docker, all four documented states plus the not-armed case:

declares monitoring        proceeding          exit 0
declares only debug        refused, names both exit 1
declares nothing           refused, "(none)"   exit 1
profile not armed          proceeding          exit 0

On the fourth criterion you declared unmet: saying so, naming why (check-actions.py parses and bash -ns but cannot execute an action), and filing the harness separately is the right handling. It is worth being blunt about what that costs, since the trade recurs in this repository: the verification above holds today and will not run again when someone edits this guard next year. The behaviour most at risk from that is the one in my comment — an error path nobody exercises by hand.

On placing the check on the host rather than where the profile is armed: agreed, and the reasoning generalises. config --profiles is the real parser; a grep over the YAML would be wrong in both directions, and the false-negative direction refuses a good deployment, which is the worse half.

One finding, inline and non-blocking. The guard does what it claims, and the acceptance criteria you can meet are met — CI green. **The most useful thing I can report is that this breaks nothing on merge.** Your risks section flags a target whose compose lacks the profile while the credentials happen to be present, which would newly fail. I checked whether that target exists, across every consumer in the fleet: ``` declares `profiles: [monitoring]` ahmad/portfolio ahmad/trip bayan/kidpoints suhba/khitta passes `grafana-prom-*` to deploy-vps ahmad/portfolio ahmad/trip bayan/kidpoints suhba/khitta ``` The two sets are identical, so no current deployment is newly refused. (`ahmad/imamah` is absent from both, consistently — it has its own deploy action rather than this one. `expiro`, `bayan-web`, `stem`, `suhba-web` and `suhba-docs` declare no profile and arm none.) **Verified against a stub `docker`**, all four documented states plus the not-armed case: ``` declares monitoring proceeding exit 0 declares only debug refused, names both exit 1 declares nothing refused, "(none)" exit 1 profile not armed proceeding exit 0 ``` **On the fourth criterion you declared unmet:** saying so, naming why (`check-actions.py` parses and `bash -n`s but cannot execute an action), and filing the harness separately is the right handling. It is worth being blunt about what that costs, since the trade recurs in this repository: the verification above holds today and will not run again when someone edits this guard next year. The behaviour most at risk from that is the one in my comment — an error path nobody exercises by hand. **On placing the check on the host rather than where the profile is armed:** agreed, and the reasoning generalises. `config --profiles` is the real parser; a grep over the YAML would be wrong in both directions, and the false-negative direction refuses a good deployment, which is the worse half.
@ -325,0 +333,4 @@
# the only reliable answer; grepping the YAML would be wrong in both
# directions.
if [ -n "${COMPOSE_PROFILES:-}" ]; then
DECLARED="$(docker compose -f docker-compose.vps.yml config --profiles 2>/dev/null || true)"
Member

2>/dev/null || true collapses "docker could not answer" into "the answer is nothing", and the message that follows then blames the compose file. Driven against a stub docker, the four documented cases behave exactly as your table says — and so do two that are not in it:

compose file INVALID          -> "…which docker-compose.vps.yml does not declare."  exit 1
docker daemon unreachable     -> "…which docker-compose.vps.yml does not declare."  exit 1

In both, the compose file may declare monitoring perfectly well. The deployment fails either way — the next line is docker compose pull, which would fail too — so this is a diagnosis fault, not a correctness one. But it is the specific kind this fleet keeps paying for: a red that names the wrong cause sends whoever reads it to edit a compose file that was never wrong, and the real error (yaml: mapping values are not allowed, Cannot connect to the Docker daemon) was discarded to /dev/null before anyone saw it.

Concrete fix — keep the output, distinguish the two failures:

if ! DECLARED="$(docker compose -f docker-compose.vps.yml config --profiles 2>&1)"; then
  echo "Could not read the profiles declared by docker-compose.vps.yml:" >&2
  printf '%s\n' "${DECLARED}" >&2
  exit 1
fi

That still stops the deployment, and it stops it saying what actually went wrong.

Minor, while here: grep -qx treats ${want} as a basic regular expression, so a profile name containing . would match more than itself. grep -qxF is what you use in the reporters in this same repository, and it costs one letter.

`2>/dev/null || true` collapses "docker could not answer" into "the answer is nothing", and the message that follows then blames the compose file. Driven against a stub `docker`, the four documented cases behave exactly as your table says — and so do two that are not in it: ``` compose file INVALID -> "…which docker-compose.vps.yml does not declare." exit 1 docker daemon unreachable -> "…which docker-compose.vps.yml does not declare." exit 1 ``` In both, the compose file may declare `monitoring` perfectly well. The deployment fails either way — the next line is `docker compose pull`, which would fail too — so this is a diagnosis fault, not a correctness one. But it is the specific kind this fleet keeps paying for: a red that names the wrong cause sends whoever reads it to edit a compose file that was never wrong, and the real error (`yaml: mapping values are not allowed`, `Cannot connect to the Docker daemon`) was discarded to `/dev/null` before anyone saw it. Concrete fix — keep the output, distinguish the two failures: ```sh if ! DECLARED="$(docker compose -f docker-compose.vps.yml config --profiles 2>&1)"; then echo "Could not read the profiles declared by docker-compose.vps.yml:" >&2 printf '%s\n' "${DECLARED}" >&2 exit 1 fi ``` That still stops the deployment, and it stops it saying what actually went wrong. Minor, while here: `grep -qx` treats `${want}` as a basic regular expression, so a profile name containing `.` would match more than itself. `grep -qxF` is what you use in the reporters in this same repository, and it costs one letter.
ahmad changed title from fix: Refuse a profile the compose file does not declare to WIP: fix: Refuse a profile the compose file does not declare 2026-09-05 08:32:47 +00:00
fix: Say when docker could not read the compose file
All checks were successful
PR / validate (pull_request) Successful in 1m33s
dd17f639c3
Discarding stderr and the exit status turned docker being unable to
answer into the answer being nothing, so an invalid compose file or an
unreachable daemon produced a message blaming the compose file for a
profile it may well declare.

Also grep -F: a profile name is a literal, not a regular expression.

Refs #39
Author
Owner

Both taken, fixed in dd17f63.

The swallowed error is the better find, and it is the failure mode I was warning about elsewhere in this same change. 2>/dev/null || true made "docker could not answer" indistinguishable from "the answer is nothing", so the message blamed a compose file that might declare the profile perfectly well — while the real cause went to /dev/null. Now:

compose file invalid      "Could not read the profiles declared by docker-compose.vps.yml:"
                          "yaml: mapping values are not allowed in this context"        exit 1
daemon unreachable        same, with "Cannot connect to the Docker daemon at unix://…"  exit 1

The four documented states are unchanged and still behave:

declares monitoring     exit 0
declares nothing        refused, "Declared profiles: (none)"    exit 1
declares only debug     refused, "Declared profiles: debug"     exit 1
profile not armed       exit 0

The -F point was not cosmetic, and I checked which direction it fails in. A profile whose name contains . matched a different declared profile — so the previous form could have let an undeclared profile through, which is the failure this guard exists to prevent rather than a harmless over-match:

wants "mon.toring", compose declares "monXtoring"
  grep -qxF   refused    exit 1     (correct)
  grep -qx    accepted   exit 0     (the bug)

No profile in use today contains a regex metacharacter, so nothing was live — but the guard would have been silently weaker than it reads, which is the same class of defect as the one it was written to catch.

Thank you for checking the merge is safe across the fleet. Enumerating which consumers declare profiles: [monitoring] against which pass grafana-prom-* — and finding the sets identical — is the evidence my risks section asserted without measuring. I wrote "a target whose compose legitimately lacks the profile now fails"; you established that no such target exists, which is a stronger statement than the one I hedged.

On the harness cost you named: agreed, and it is the right thing to keep saying out loud. The verification in this pull request holds today and will not run again, and the behaviour most exposed by that is exactly the error path you found — nobody exercises it by hand twice. That is #48.

Both taken, fixed in `dd17f63`. **The swallowed error is the better find, and it is the failure mode I was warning about elsewhere in this same change.** `2>/dev/null || true` made "docker could not answer" indistinguishable from "the answer is nothing", so the message blamed a compose file that might declare the profile perfectly well — while the real cause went to `/dev/null`. Now: ``` compose file invalid "Could not read the profiles declared by docker-compose.vps.yml:" "yaml: mapping values are not allowed in this context" exit 1 daemon unreachable same, with "Cannot connect to the Docker daemon at unix://…" exit 1 ``` The four documented states are unchanged and still behave: ``` declares monitoring exit 0 declares nothing refused, "Declared profiles: (none)" exit 1 declares only debug refused, "Declared profiles: debug" exit 1 profile not armed exit 0 ``` **The `-F` point was not cosmetic, and I checked which direction it fails in.** A profile whose name contains `.` matched a *different* declared profile — so the previous form could have let an undeclared profile through, which is the failure this guard exists to prevent rather than a harmless over-match: ``` wants "mon.toring", compose declares "monXtoring" grep -qxF refused exit 1 (correct) grep -qx accepted exit 0 (the bug) ``` No profile in use today contains a regex metacharacter, so nothing was live — but the guard would have been silently weaker than it reads, which is the same class of defect as the one it was written to catch. **Thank you for checking the merge is safe across the fleet.** Enumerating which consumers declare `profiles: [monitoring]` against which pass `grafana-prom-*` — and finding the sets identical — is the evidence my risks section asserted without measuring. I wrote "a target whose compose legitimately lacks the profile now fails"; you established that no such target exists, which is a stronger statement than the one I hedged. **On the harness cost you named:** agreed, and it is the right thing to keep saying out loud. The verification in this pull request holds today and will not run again, and the behaviour most exposed by that is exactly the error path you found — nobody exercises it by hand twice. That is `#48`.
Author
Owner

validate green on dd17f63; back to ready.

The delta since the reviewed head is the two findings, both verified above: the six states (four documented plus the invalid-compose and unreachable-daemon paths that previously reported the wrong cause), and the literal-match case showing the old form let an undeclared profile through.

Everything else stands as verified, including the fourth criterion still being met by stub rather than by a test that runs again — #48.

`validate` green on `dd17f63`; back to ready. The delta since the reviewed head is the two findings, both verified above: the six states (four documented plus the invalid-compose and unreachable-daemon paths that previously reported the wrong cause), and the literal-match case showing the old form let an undeclared profile through. Everything else stands as verified, including the fourth criterion still being met by stub rather than by a test that runs again — `#48`.
ahmad changed title from WIP: fix: Refuse a profile the compose file does not declare to fix: Refuse a profile the compose file does not declare 2026-09-05 08:36:19 +00:00
ahmad_bot approved these changes 2026-09-05 09:01:37 +00:00
ahmad_bot left a comment

Approved. Both halves of the finding are closed. Re-reviewed only the delta (e5a7d6eb5d..dd17f639c3); CI green.

Driven against a stub docker, eight cases — the four you documented, the two I raised, and two the fix newly makes possible:

declares monitoring          proceeding                                              exit 0
declares only debug          refused, names both                                     exit 1
declares nothing             refused, "(none)"                                       exit 1
not armed                    proceeding                                              exit 0

compose file INVALID         "Could not read the profiles… yaml: mapping values…"    exit 1
daemon unreachable           "Could not read the profiles… Cannot connect to the…"   exit 1

warns, declares monitoring   proceeding                                              exit 0
warns, declares only debug   refused, names both                                     exit 1

The last two are the reason I checked rather than just reading: capturing 2>&1 means a docker compose that succeeds while warning on stderr now folds that warning into DECLARED. It is benign in both directions — grep -qx matches any line, so a warning cannot mask a profile that is declared, and no plausible warning is the literal string monitoring, so it cannot invent one that is not. The only cost is a warning line appearing in the Declared profiles: message on the failure path, where the extra context is more use than harm.

The -F is not cosmetic, and I should have said so more plainly the first time:

printf 'axb\n' | grep -qx  -- 'a.b'   ->  MATCHED
printf 'axb\n' | grep -qxF -- 'a.b'   ->  no match

Without it a profile name containing . matches a different declared profile, which would silently pass a mismatched deployment — the exact failure this guard exists to stop, reintroduced through the check itself.

Nothing further. The earlier verification stands: the four consumers that arm monitoring are still exactly the four that declare it, so this remains a no-op for every live deployment.

Approved. Both halves of the finding are closed. Re-reviewed only the delta (`e5a7d6eb5d..dd17f639c3`); CI green. **Driven against a stub `docker`, eight cases — the four you documented, the two I raised, and two the fix newly makes possible:** ``` declares monitoring proceeding exit 0 declares only debug refused, names both exit 1 declares nothing refused, "(none)" exit 1 not armed proceeding exit 0 compose file INVALID "Could not read the profiles… yaml: mapping values…" exit 1 daemon unreachable "Could not read the profiles… Cannot connect to the…" exit 1 warns, declares monitoring proceeding exit 0 warns, declares only debug refused, names both exit 1 ``` The last two are the reason I checked rather than just reading: capturing `2>&1` means a `docker compose` that succeeds while warning on stderr now folds that warning into `DECLARED`. It is benign in both directions — `grep -qx` matches any line, so a warning cannot mask a profile that is declared, and no plausible warning is the literal string `monitoring`, so it cannot invent one that is not. The only cost is a warning line appearing in the `Declared profiles:` message on the failure path, where the extra context is more use than harm. **The `-F` is not cosmetic, and I should have said so more plainly the first time:** ``` printf 'axb\n' | grep -qx -- 'a.b' -> MATCHED printf 'axb\n' | grep -qxF -- 'a.b' -> no match ``` Without it a profile name containing `.` matches a different declared profile, which would silently *pass* a mismatched deployment — the exact failure this guard exists to stop, reintroduced through the check itself. Nothing further. The earlier verification stands: the four consumers that arm `monitoring` are still exactly the four that declare it, so this remains a no-op for every live deployment.
ahmad merged commit 0300efe82c into main 2026-09-05 09:02:46 +00:00
ahmad deleted branch feature/39-guard-monitoring-profile 2026-09-05 09:02:48 +00:00
Sign in to join this conversation.
No description provided.