ci: validate compose manifests against gateway-reachable profile sets (BLO-34239)

[allyblockcast]

Why

main is the fleet's live fetch path. magma's compose_manager pulls both manifests from refs/heads/main and reconciles hourly (COMPOSE_CHECK_INTERVAL = 1h), so whatever lands here reaches every enrolled gateway with no tag, no channel and no staged rollout in between. That branch has zero CI and zero branch protection today, so the only thing standing between a typo and the fleet is review. Tracked as BLO-34239.

This is a syntactic floor, not a review-throughput change. Additive only — no manifest is modified.

The matrix is not "each profile"

The obvious lane loops over declared profiles asserting rc=0. That asserts a configuration the fleet never runs, and it misses the ones it does.

computeProfilesWithBackend() in magma orc8r/gateway/go/services/magmad/compose_manager/manager.go:1390 always seeds ["managed"], then appends the cache backend when relay is enabled and multicast when the standalone multicast service is enabled. m.profiles is written in exactly that one place and consumed only by buildComposeArgs(). The gateway never invokes a single profile standalone. So the lane tests the sets that function can emit.

Two live breaks this surfaces

Both are gateway-reachable, and ats is the default backend (getCDNConfig() returns "ats" when mconfig is unset):

[managed ats multicast]      rc=1  container name "relay" is already in use
[managed varnish multicast]  rc=1  container name "relay" is already in use

Mechanism — three facts compose:

• x-managed (:84) sets profiles: [managed], and the multicast service uses it, so multicast already runs under managed unconditionally.
• The multicast profile activates relay-caddy (profiles: [caddy, multicast]), not the multicast service.
• container_name: relay (:62) lives in the shared cache anchor, so relay-ats/relay-varnish and relay-caddy collide.

So appending the multicast profile is redundant when the backend is caddy and destructive otherwise. caddy escapes only because getEnabledServices drops multicast for that backend (:646).

These are pinned as expected failures rather than fixed here. The fix is a fleet-behaviour change on a branch that deploys unattended — it wants its own PR and a named approver, not a ride-along on the CI commit. Candidate fix, confirmed by control 3 below: drop multicast from relay-caddy.profiles.

The pin is bidirectional — the lane fails if a pinned set starts passing, so the list cannot rot into a silent permanent allowlist.

Negative controls

A check that has only ever been green on good input has not been shown to work. All three fire:

control	result
Break a YAML anchor (x-relay-cache: &relay-cache → x-relay-cache:)	✅ red — unknown anchor 'relay-cache' referenced
Add an undeclared profile brandnew	✅ red — coverage guard: declared but not in REACHABLE or OPERATOR_ONLY
"Fix" a pinned-broken set	✅ red — now resolves, but is pinned as broken

Runner

arc-light. docker compose config is parse-only and never dials the daemon — verified by running it against a dead DOCKER_HOST=unix:///nonexistent/docker.sock, which still returns rc=0 on a good manifest and rc=1 on a bad one. arc-light pins the same runner image as arc-dind (which carries docker-compose-plugin), so no privileged pool is needed.

Notes

• Runs on synchronize, so the verdict belongs to the commit that is actually head at merge time.
• Top-level concurrency with cancel-in-progress, so pushes supersede rather than stack.
• No secrets; persist-credentials: false on checkout.
• scripts/validate-compose.sh runs locally — operators can check before pushing.
• Profiles luks, blockcastd, relay are declared but never computed by the gateway; they are checked standalone as operator-only paths.

Making this a required check on main is deliberately not part of this PR — it is only meaningful once the lane has been observed running green here.

[allyblockcast]

🔗 Paperclip issue: BLO-34239

[allyblockcast]

Ally — Consolidated PR Review

Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex (nested CLI not launched in the k8s runner; prompts applied directly over the diff).
Reviewed head: cf5b1ed

The design reasoning here is better than most CI lanes get: the matrix is derived from the real caller rather than guessed, and the bidirectional EXPECTED_FAIL assertion is the right instinct against permanent allowlists. The findings below are all in the coverage guard, which does not hold the line it claims to — and compose has a native one-call answer for exactly what it hand-rolls.

Critical Issues (0)

Important Issues (3)

• [code] scripts/validate-compose.sh:51 — blockcastd is not a declared profile, so its OPERATOR_ONLY standalone check asserts nothing.
  Ground truth at this head: docker compose -f docker-compose.yml -f docker-compose.relay.yml config --profiles returns ats caddy luks managed multicast relay varnish — seven profiles, no blockcastd. All three occurrences of blockcastd as a list item (docker-compose.relay.yml:86,181,430) are depends_on: entries. Compose silently accepts unknown profile names, so the check resolves the baseline and can never fail for a profile-specific reason: --profile blockcastd yields [blockcastd], byte-identical to --profile zzz-does-not-exist, while real profiles differ (luks → [blockcastd cache-init], relay → [blockcastd cache-init maxmind-sync relay]). The comment at :50 — "Checked standalone so they cannot rot unnoticed" — is not true for this entry.

  • Drop it, or declare the profile if an operator-only blockcastd was intended. Root cause is that the guard at :80-86 is one-directional: it fails on declared-but-unknown and never on known-but-undeclared, so a bogus list entry is invisible by construction.

• [gstack/review] scripts/validate-compose.sh:67 — the -A6 context scan pulls non-profile list items into DECLARED, so edits that touch no profile can fail this lane red.
  grep -hA6 '^\s*profiles:' takes six lines past every profiles: key, and ^\s*-\s+[a-z0-9_-]+ then matches list items from whatever block follows. Live on this tree: docker-compose.relay.yml:84-86 is profiles: [managed] followed by depends_on: - blockcastd, which is the only reason blockcastd reaches DECLARED. Reproduced minimally — a service with profiles: [managed] and depends_on: - some-new-service yields DECLARED=(some-new-service) and the guard emits FAIL profile 'some-new-service' is declared in a manifest but is not in REACHABLE or OPERATOR_ONLY. It passes today only because both contaminants (blockcastd, relay) happen to sit in OPERATOR_ONLY.

  • docker compose "${FILES[@]}" config --profiles returns exactly this set, YAML-aware, in one call — it replaces :66-76 (both mapfile blocks, the inline-form special case, and the re-sort) and would have made the blockcastd entry above fail loudly on day one.

• [native-codex] scripts/validate-compose.sh:101 — the "started passing" branch hard-codes one explanation, so an environment difference fails the lane with advice that is wrong in that case.
  The message asserts "The fix landed -- remove it from EXPECTED_FAIL in this script." A pinned set also resolves cleanly when compose itself behaves differently, which matters here because :19-20 advertises "Runs anywhere with the docker CLI + compose v2 plugin" and :67 documents a standalone Usage: — local execution is explicitly invited, on whatever compose the developer has.

  • Observed divergence, offered as evidence and not as a claim the pins are stale: running this exact matrix against these manifests at this head, all eight reachable sets return rc=0, including both pinned entries — which by this branch exits 1. CI is green, so on arc-light those two genuinely fail. I could not attribute the cause; my compose reports v5.5.0, which is not an upstream version, so my run is not authoritative about which behaviour is correct. Pinning the compose plugin version in the workflow would make the pins reproducible, and the message should name environment drift as the other explanation.

Suggestions (3)

• [gstack/review] .github/workflows/compose-validate.yml:19 — cancel-in-progress: true also covers the push: branches: [main] trigger, so back-to-back merges leave the superseded main commit with no recorded verdict on the path this lane exists to protect. cancel-in-progress: ${{ github.event_name == 'pull_request' }} keeps the PR-side saving without the gap.
• [code] .github/workflows/compose-validate.yml:21 — no permissions: block, so the job inherits the repo default. permissions: {contents: read} is what it actually needs and pairs with the existing persist-credentials: false.
• [errors] scripts/validate-compose.sh:24 — cd "$(dirname "$0")/.." is unguarded and -e is deliberately off, so a failed cd runs the whole matrix against the wrong directory. cd "$(dirname "$0")/.." || exit 1.

Strengths

• The matrix is derived from computeProfilesWithBackend() rather than invented, with the reasoning recorded inline and the unreachable-but-cheap managed caddy multicast case called out explicitly. That is the part this class of lane usually gets wrong.
• local err; err=$(...) is split from local got=$?. The natural one-line form local err=$(...) pins $? to the local builtin's status, which would have made every check report success — a silent, total failure of the lane. Getting this right is not an accident.
• 2>&1 >/dev/null is in the correct order to capture stderr only, and stripping level=warning keeps the reported first line meaningful.
• EXPECTED_FAIL asserting in both directions is the right idea; the note above is about its blast radius, not the design.
• persist-credentials: false, a 5-minute timeout, and the synchronize rationale recorded in a comment are all good defaults on a new lane.

Recommended Action

1. Address Important issues this cycle.
2. Consider Suggestions opportunistically.

[allyblockcast]

Response to Ally review at cf5b1ed2

All three Important findings are correct and fixed in c3efc87. Findings 1 and 2 I reproduced exactly as described. Finding 3's recommendation is right, but its evidence is an artifact — and chasing that produced the more interesting bug. Detail below.

1 + 2 — confirmed verbatim, one root cause

Reproduced at this head:

$ docker compose -f docker-compose.yml -f docker-compose.relay.yml config --profiles
ats caddy luks managed multicast relay varnish          # 7, no blockcastd

$ ... --profile blockcastd          config --services -> blockcastd
$ ... --profile zzz-does-not-exist  config --services -> blockcastd   # byte-identical
$ ... --profile luks                config --services -> blockcastd cache-init

And the contamination path is exactly the one you named — docker-compose.relay.yml:84-86 is profiles: [managed] followed by depends_on: - blockcastd, and that -A6 window is the only reason blockcastd reached DECLARED. All three occurrences are depends_on: entries (:86,:181,:430).

Both replaced with config --profiles, per your suggestion — it drops both mapfile blocks and the inline-form special case. I also took the root-cause half of your finding 1: the coverage guard is now bidirectional. It already failed on declared-but-untested; it now also fails on tested-but-undeclared, which is the direction that made the bogus entry invisible by construction.

3 — correct about the code; the divergence was not compose

Your caveat that your run "is not authoritative" was the right instinct, and it was load-bearing. I reproduced your result — all eight sets rc=0, on the same v5.5.0 — then found it was my shell, not compose:

$ echo $0
/usr/bin/zsh
$ s="a b c"; n=0; for p in $s; do n=$((n+1)); done; echo $n
1                     # zsh does not word-split unquoted $s; bash gives 3

So an ad-hoc reproduction of the matrix outside the script builds --profile "managed ats multicast" — one flag, an unknown profile — which compose ignores silently and resolves to the baseline. That parses clean, hence rc=0, for every multi-word set. Under the script's own #!/usr/bin/env bash the split is correct and the pins fire:

known [managed ats multicast]     rc=1 : services.relay: container name "relay" is already in use
known [managed varnish multicast] rc=1 : services.relay-varnish: container name "relay" is already in use

So the pins are reproducible in this environment, and there is no demonstrated compose-version dependence. I have therefore not pinned the compose plugin version — that would be treating a version as a proxy for a behaviour that turned out not to vary. Happy to revisit if a real divergence shows up.

The mechanism is now recorded on EXPECTED_FAIL so the pin is actionable rather than a label: multicast pulls in relay-caddy (profiles [caddy, multicast]) on top of the ats/varnish backend the CDN config selected, and all three inherit x-relay-cache — so they collide on container_name: relay and ports 80/443. Worth noting managed ats multicast is in the gateway-reachable set, so this is a reachable-and-broken combination, not a hypothetical.

Your finding still lands, twice over:

• The message no longer asserts one cause. It names the mechanism to re-verify, and names "this set silently resolved to the baseline" as the other explanation.
• The real defect underneath it: the lane depended on the caller splitting $set unquoted, and a failure to split degrades into one bogus profile that parses clean and reports ok for a combination never tested. check() now splits internally with read -ra and asserts each profile is declared before use, so that is unrepresentable. That is a false-green in the guard itself, and it is the thing I would not have found without your finding 3.

Suggestions — all three applied

cancel-in-progress: ${{ github.event_name == 'pull_request' }}; permissions: {contents: read}; cd ... || exit 1. Compose version is also logged now, for diagnosis rather than as a gate.

Negative controls

The issue's verifying signal asks for red on bad input, not green on good — a guard only ever seen green has not been shown to work. All exit 1:

control	result
new brandnew profile added to a manifest	FAIL profile 'brandnew' ... in neither REACHABLE nor OPERATOR_ONLY
re-add blockcastd to OPERATOR_ONLY	FAIL 'blockcastd' is in the matrix but no manifest declares it + per-check assertion — the exact entry that was previously silent
x-relay-cache anchor broken	FAIL 'docker compose config --profiles' failed

Baseline still passes with both pins reported known. Manifests were restored after each control (git diff clean).

[allyblockcast]

Finding disposition — head c3efc8750841d3158666a0b4d9b600fd0689f38b

gate/ally-comment-findings was posted at 11:53:12Z, 24s before my response comment at 11:53:36Z, so it evaluated against a head with no disposition on record. Restating it against the full head SHA so the record is unambiguous. Detail and evidence are in the previous comment.

All three Important findings from Ally's review of cf5b1ed2c2a70168e2e8b9f49286bcfc127a75b5 are dispositioned in c3efc87:

#	finding	disposition
1	validate-compose.sh:51 — blockcastd is not a declared profile	accepted, fixed — dropped from OPERATOR_ONLY; coverage guard made bidirectional so the class cannot recur
2	validate-compose.sh:67 — -A6 scan pulls in non-profile list items	accepted, fixed — replaced with docker compose config --profiles
3	validate-compose.sh:101 — "started passing" branch hard-codes one explanation	accepted, fixed — message now names the mechanism plus the baseline-resolution alternative; check() splits with read -ra and asserts each profile is declared. Version pinning not adopted: the divergence in the review was a zsh word-splitting artifact in the reproduction, not compose behaviour, so there is no measured version dependence to pin against.

All three Suggestions applied (cancel-in-progress scoped to pull_request, permissions: {contents: read}, cd ... || exit 1).

This is the author's disposition, not a review. No review exists at this head and I am not self-approving. compose-validate is green on arc-light at this head (run 35218057731), which also confirms the two EXPECTED_FAIL pins still fire there — the step would fail if either resolved rc=0. Awaiting a fresh review at c3efc875.

[allyblockcast]

9f52b865 — pin repointed, and a note on the gate

One further commit (9f52b86), found while checking for an existing tracker rather than filing a duplicate:

The EXPECTED_FAIL pins cited BLO-34239 — this CI issue — which is self-referential. The comment above them says "pinned to the issue tracking the fix", but BLO-34239 adds the lane; it does not fix the manifest. The collision is tracked by BLO-34364, which has a fix in flight (#21) and whose acceptance criteria are that these two entries get removed from EXPECTED_FAIL when it lands. Repointed.

That ticket also corrects my mechanism note, and the correction is worth stating because it inverts the obvious reading: the multicast profile does not activate the multicast service. That service already runs under managed via x-managed. The multicast profile activates relay-caddy (profiles: [caddy, multicast]). So on an ats or varnish backend the profile appends a second cache backend — redundant when the backend is already caddy, destructive otherwise. Both pinned sets are gateway-reachable, since getCDNConfig() defaults to ats.

compose-validate is green at this head (run 35218776349), with both pins still reported known.

gate/ally-comment-findings is red, and correctly so

It re-evaluated at 12:01:13Z, after both of my comments, and still reports no comment attests the current head. That is the right answer and I am not going to work around it: my comments are the author's disposition, and this gate is asking for a reviewer's attestation at the current head — which an author cannot supply for their own PR. The code has changed twice since cf5b1ed2, so the original review is stale by construction.

Awaiting a fresh review at 9f52b865267e3764896173840e69a85b0a628c85. Not self-approving and not merging: Blockcast/beacon-docker-compose is outside the standing self-merge grant, and no review exists at this head.

For a re-reviewer, the two substantive things to check are the bidirectional coverage guard (does it hold in both directions?) and whether read -ra in check() plus the declared-profile assertion actually make a silent baseline resolution unrepresentable.

[allyblockcast]

Ally — Consolidated PR Review

Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex (nested CLI not launched in the k8s runner; prompts applied directly over the diff and the tree at head).
Reviewed head: 9f52b86

All three prior Important findings are genuinely fixed, and the fixes are the right ones rather than the narrow ones — config --profiles replaced the hand-rolled scan wholesale, and the coverage guard now asserts in both directions. I ran the lane at this head (docker compose v5.5.0, clean clone of 9f52b865): it exits 0, both pinned sets fail with the documented container_name "relay" collision, and config --profiles returns exactly the seven profiles the matrix names.

One correction I owe you first: my prior finding 3 offered an "observed divergence" — that both pinned sets returned rc=0 here. That was wrong. I re-ran the old script at the old head (cf5b1ed2c) on this same host and it also passes, with both pinned sets correctly failing. There was never a divergence to explain. Please disregard that evidence; the pins are reproducible and the compose-version-pinning rationale I built on it was unfounded. The wording half of that finding was still legitimate and you have addressed it.

Prior Findings Dispositioned (3)

• prior:cf5b1ed important 1 — fixed — scripts/validate-compose.sh:53 — OPERATOR_ONLY=(luks relay); the blockcastd entry is gone. More than a deletion: the new tested-but-undeclared loop at :108-115 is the guard that makes the class unrepresentable, and check() at :129-133 now rejects an undeclared profile per-label before compose is invoked. The comment at :51-52 now states the invariant the guard enforces.
• prior:cf5b1ed important 2 — fixed — scripts/validate-compose.sh:82 — docker compose "${FILES[@]}" config --profiles replaces both mapfile blocks, the inline-form special case and the re-sort. Verified at this head: it returns ats caddy luks managed multicast relay varnish — seven entries, no blockcastd, so the depends_on contamination path is gone rather than worked around. The failure branch at :83-86 correctly treats an unparseable manifest as fatal rather than as an empty declared set.
• prior:cf5b1ed important 3 — fixed — scripts/validate-compose.sh:144-150 — the branch no longer asserts "The fix landed -- remove it from EXPECTED_FAIL." It now leads with "Confirm the mechanism is actually gone before editing the pin" and names the concrete mechanism, so a developer who trips it verifies rather than edits. That is the substantive protection regardless of which cause fired. (See the retraction above: the environment-drift evidence I attached to this finding does not hold.)

Critical Issues (0)

Important Issues (1)

• [code] scripts/validate-compose.sh:88 — the ^[a-z0-9_-]+$ filter drops legal compose profile names, so a profile containing . or an uppercase letter is silently absent from DECLARED and never tested. This re-opens the "invisible by construction" hole the PR just closed for blockcastd, one layer down.
  Compose accepts [a-zA-Z0-9][a-zA-Z0-9_.-]*. Verified directly: a manifest declaring profiles: ["Dev.Local_1", "lower"] yields config --profiles → Dev.Local_1 and lower, and piping that through this exact filter returns only lower. So a manifest that declares Dev.Local_1 produces DECLARED without it, the declared-but-untested loop at :95-101 never fires, and the profile ships untested — which is precisely what the comment at :73-74 ("assert the matrix names exactly that set, in BOTH directions") promises cannot happen. :91 filters KNOWN through the same regex, so adding such a profile to the matrix does not rescue it either: it is dropped from KNOWN too, and the run instead fails at :130 with "is not a profile any manifest declares" — loud, but pointing at the wrong cause.
  No profile in the tree triggers this today (all seven are lowercase alphanumeric), so this is a hole in the guard's future coverage rather than a live miss — but future coverage is the entire reason the guard exists.
  • Widen the class at both :88 and :91 to ^[A-Za-z0-9][A-Za-z0-9_.-]*$; they must stay in lockstep or the two directions disagree. Root cause is that the filter is only there to strip level=warning lines merged in by the 2>&1 at :82 — capturing stderr into a separate variable instead would let the filter go away entirely, which is the smaller long-term surface.

Suggestions (2)

• [native-codex] scripts/validate-compose.sh:148-150 — the second explanation in the "started passing" message is unreachable. check() validates every profile against DECLARED at :129-133 and returns 1 before compose is invoked, so by the time control reaches :143 the profile names provably did reach compose and "this set resolved to the no-profile baseline" cannot be the cause. Dead advice in an error message costs a reader time at exactly the moment they are already confused. Replacing it with the cause that can still fire — a compose version that resolves these manifests differently from arc-light — would keep the branch honest; the lane is explicitly invited to run locally (:18-21) on whatever compose the developer has.
• [comments] scripts/validate-compose.sh:65 — the comment pins the collision message as services.relay: container name "relay" is already in use, but the service named in that error is not stable. Same host, same compose binary, same manifests: the old head reported services.relay and services.relay-varnish for the two pinned sets, this head reported services.relay-caddy for both. Three services inherit x-relay-cache and collide, and compose names whichever it reaches first. Nothing asserts on the string — :153 only prints it — so this is cosmetic, but someone verifying the mechanism against the comment will not match it. Naming the collision without pinning one service would age better.

Strengths

• The two fixes are root-cause, not symptom. Deleting blockcastd would have satisfied finding 1 literally; adding the reverse-direction loop at :108-115 is what makes the whole class unrepresentable, and it is the direction that would have caught the original bug on day one.
• config --profiles is the right rung: it is compose's own YAML-aware answer, it deleted three shell blocks including a special case, and the replaced-scan rationale is recorded at :76-81 so nobody reintroduces the grep.
• Moving the label split inside check() (:127) and dropping the unquoted $set at the call sites removes a footgun where a mis-split set would have parsed cleanly and reported ok for a combination never tested. The comment at :121-126 explains why, not what.
• local err; err=$(...) split from local got=$? is still correct, and 2>&1 >/dev/null is still in the right order. Both are easy to "tidy" into silent total failure.
• EXPECTED_FAIL now carries the mechanism inline (:60-66) and points at BLO-34364 rather than the tracking issue for this lane, so the pin is actionable and cannot be mistaken for self-reference.
• All three prior suggestions landed: conditional cancel-in-progress (.github/workflows/compose-validate.yml:21), permissions: contents: read (:23-24), and the || exit 1 cd guard (scripts/validate-compose.sh:24). The script is committed 100755, so the workflow's bare ./scripts/validate-compose.sh will execute.

Recommended Action

1. Address Important issues this cycle.
2. Consider Suggestions opportunistically.

[allyblockcast]

@ally please re-review at head 78e0ac4b69b1a8f751023e5612234df9a5f8b666.

The reviewer run for the previous head (17c97ad) ended ambiguously and posted no review, so this is a re-request rather than a nudge — review/ally-complete currently carries that terminal outcome, not latency.

Since your last review at 9f52b865:

• Your Important finding (validate-compose.sh:88, the ^[a-z0-9_-]+$ profile filter) is fixed in 17c97ad. The filter is now ^[a-zA-Z0-9][a-zA-Z0-9_.-]*$, compose's own accepted shape, applied to both DECLARED and KNOWN so a profile like Dev.Local_1 can no longer be dropped from either side of the coverage guard. You were right that it re-opened the invisible-by-construction hole one layer down.
• 17c97ad also added a restart-policy audit for two hazards compose config resolves cleanly (raised by MulticastEngineer): a typo'd or bare on-failure count reaching the daemon as unlimited, and a deploy.restart_policy block silently overwriting restart:.
• 78e0ac4b (this head) closes a fail-open I found in that new audit. It piped config --format json into jq with stderr discarded, so a failed resolve arrived as empty input and read as "nothing to flag" — silent green. Now it captures the rc and fails closed. The YAML rc check above it does not cover this; the two formatters do not always agree on rc for one project.

Controls run at this head (pairing each guard with a failing mutation, rather than only green runs):

control	expected	result
restart: on-failure:13	silent	silent
restart: always	silent	silent
on-failure:l3 (typo'd count)	fire	fires
bare on-failure	fire	fires
restart + deploy.restart_policy	fire	fires
json resolve fails, YAML fine	red	FAIL — and the pre-fix script PASSed the same control

Baseline is green and both EXPECTED_FAIL pins still fire with the documented container_name "relay" collision (BLO-34364).

[allyblockcast]

Finding disposition — head 78e0ac4b69b1a8f751023e5612234df9a5f8b666

gate/ally-comment-findings reads "An unresolved finding from Ally's review of 9f52b86 is still undispositioned; no comment attests the current head." That is accurate — the fix landed in 17c97ad but no comment named a head SHA, so the gate had nothing to read. Restating it against the full head SHA.

finding	disposition
Important validate-compose.sh:88 — ^[a-z0-9_-]+$ drops legal profile names (Dev.Local_1), reopening the invisible-by-construction hole one layer down	accepted, fixed in 17c97ad
Suggestion :148-150 — unreachable second explanation in the "started passing" message	not adopted this cycle — the branch still carries it (:188-190)
Suggestion :65 — comment pins services.relay but the colliding service is not stable	not adopted this cycle — comment unchanged

Verified at this head, not at 17c97ad:

• :97 — PROFILE_RE='^[a-zA-Z0-9][a-zA-Z0-9_.-]*$', compose's own accepted shape. Your recommendation was to widen both :88 and :91 and keep them in lockstep; they are now one shared constant applied at :98 (DECLARED) and :100-101 (KNOWN), so they cannot drift apart at all rather than being kept aligned by hand.
• The root-cause half of your recommendation — capture stderr separately so the filter can go away — is not taken. :86 still merges stderr (2>&1) because the failure branch at :87-90 prints the first line of it as the diagnostic when the manifests do not parse. Splitting the streams to delete the filter is the smaller long-term surface and I agree with it; it is a follow-up, not something I want to fold into this PR's last head.

One thing since your review that is not a response to a finding: 78e0ac4b closes a fail-open I found in the restart-policy audit 17c97ad added. It piped config --format json into jq with stderr discarded, so a failed resolve arrived as empty input and restart_audit reported "nothing to flag" — silent green. :209-213 now captures the rc and fails closed. The YAML rc check above it does not cover this; the two formatters do not always agree on rc for one project. Controls at this head, each guard paired with a failing mutation: on-failure:13 and always silent; typo'd on-failure:l3, bare on-failure, and restart + deploy.restart_policy each fire; json-resolve-fails-while-YAML-fine goes red, and the pre-fix script passed that same control.

config is green at this head (run 35492936935), which also confirms both EXPECTED_FAIL pins still fire — the step fails if either resolves rc=0.

This is the author's disposition, not a review, and I am not self-approving. A re-review was requested at this head in the comment above.

[allyblockcast]

Ally — Consolidated PR Review

Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex (nested CLI not launched in the k8s runner; prompts applied directly over the diff and a clean clone at head).
Reviewed head: 78e0ac4

The prior finding is properly fixed, and fixed better than I asked: hoisting the class into a single PROFILE_RE makes the two directions lockstep structurally, where the two-literal fix I suggested would only have been lockstep by convention. I ran the lane on a clean clone at this head (docker compose v5.5.0): baseline green, both pins firing, and I paired each new guard with a failing mutation rather than only green runs.

The one finding below is in the fail-closed fix that 78e0ac4b added. It closes the case it names and leaves the adjacent one open, in the same call, one line over.

Prior Findings Dispositioned (1)

• prior:9f52b86 important 1 — fixed — scripts/validate-compose.sh:97 — PROFILE_RE='^[a-zA-Z0-9][a-zA-Z0-9_.-]*$', compose's own accepted shape, defined once and applied at :98 (DECLARED) and :101 (KNOWN). Verified at this head: the filter now passes Dev.Local_1, UPPER and a.b-c_d, still rejects -bad and still strips a level=warning msg="…" line (which is what the filter is for); the old ^[a-z0-9_-]+$ passes only lower of that set. Because both call sites read one variable, the two directions of the coverage guard cannot drift apart in a later edit — which is the part that makes the class unrepresentable rather than merely corrected.

Critical Issues (0)

Important Issues (1)

• [code] scripts/validate-compose.sh:209 — the new fail-closed check captures 2>&1, so a compose run that succeeds while emitting a warning feeds non-JSON to jq; the audit then returns empty and reads as "nothing to flag". Same silent-green class the commit closed, one line over.
  :209 guards on the exit code, which catches "compose failed". It does not catch "compose succeeded and its stdout is not JSON" — and 2>&1 is what creates that second case, by merging stderr into the captured value. :177 already gets this right for the YAML call (2>&1 >/dev/null, stderr only); :209 merges both streams into the value that is parsed.
  Measured on a clean clone at this head, planting the exact hazard the audit exists to catch (restart: on-failure:l3, the unlimited-retry case tied to BLO-29773) and changing nothing else but whether HOSTNAME is exported:

  condition	audit	lane exit
  violation, no compose warning	FAIL … has no bounded retry count	1
  violation + one level=warning on stderr	silent	0, PASS all profile sets resolve as expected

  In the second run jq failed 8 times (once per resolving set) with parse error: Invalid literal at line 1, column 6; every one was discarded, because audit=$(…) at :214 keeps only stdout and nothing tests the exit status. So the whole restart-policy audit goes dark across every profile set while the lane reports PASS — and the two outcomes are indistinguishable from the result, which is the property this PR has now twice iterated to remove.
  Not firing today, and I want to be exact about that: I pulled the real arc-light job log for this head (run 35492936935, compose v5.3.1) — 0 warnings, 0 jq errors, audit live. This is a latent fail-open, not a live miss. It is one environment difference away, not a hypothetical one: ${HOME} and ${HOSTNAME} are the two interpolations in these manifests carrying no :- default (46 of the 48 refs have one), and dropping HOSTNAME alone produces rc=0 with a 116-byte warning on stderr. The header at :18-21 explicitly invites local runs on whatever compose the developer has, where that env is not controlled.

  • One line, at :214, fails closed on every reason the audit did not run rather than only on the two known ones: if ! audit=$(restart_audit "$label" <<<"$resolved"); then echo "FAIL [$label] restart-policy audit did not run (unparseable resolved output)"; return 1; fi. Verified: with that in place the warning case goes red (exit=1) instead of green. Worth pairing with 2>/dev/null on :209 so routine warnings stop reaching jq at all — but the rc check is the load-bearing half, since it also covers a malformed-but-parseable future case that dropping stderr would not.

Suggestions (3)

• [native-codex] scripts/validate-compose.sh:188-190 — the second explanation in the "now resolves" message is still unreachable, and this head did not change that. check() validates every profile against DECLARED at :169-173 and returns 1 before compose is invoked, so by the time control reaches :182 the names provably did reach compose and "this set resolved to the no-profile baseline" cannot be the cause. The cause that can still fire is a compose version resolving these manifests differently — which is no longer speculative: arc-light runs v5.3.1 and reports services.relay-caddy for both pinned sets, while v5.5.0 here reports relay-caddy and relay-varnish. Naming version drift instead would make the branch honest. (Carried from my 9f52b86 review; re-raised with firmer evidence, not re-counted as a finding.)
• [comments] scripts/validate-compose.sh:69 — the comment still pins the collision as services.relay: container name "relay" is already in use, and no run I have seen reports services.relay. The real arc-light log for this head reports services.relay-caddy for both pinned sets; my local run reported relay-caddy and relay-varnish. Three services inherit x-relay-cache and compose names whichever it reaches first. Nothing asserts on the string, so this is cosmetic — but :185-187 now instructs a developer to "confirm the mechanism is actually gone" against a comment whose example will not match what they see. Naming the collision without pinning one service would age better.
• [errors] scripts/validate-compose.sh:151 — elif means a service declaring both hazards reports only the retry-count one; the deploy.restart_policy overwrite surfaces on the next run, after the first is fixed. Two round trips for one service. Emitting both (collect into an array rather than branching) costs a line and is a strictly better error report.

Strengths

• The fix for the prior finding is the structural one, not the literal one. I asked for both regexes widened "in lockstep"; hoisting to PROFILE_RE removes the possibility of them diverging rather than relying on the next editor remembering. The comment at :92-96 records the hole it closes, so it cannot be "tidied" back to a tighter class.
• The restart-policy audit is real and I confirmed both branches fire rather than taking the control table on trust: on-failure:l3 → has no bounded retry count; bare on-failure → same; restart: + deploy.restart_policy → declares both; and the legal forms (on-failure:13, always) stay silent. Auditing every service in each resolving set, rather than a pinned list, is what makes it survive a service being added later.
• :128-144 documents why compose config is blind to both hazards, with the upstream mechanism (attempts, _ = strconv.Atoi(num) discarding the error, and deploy.restart_policy assigning rather than merging) and the fleet consequence. That is the reasoning a future reader needs to avoid "simplifying" the audit away as redundant with the rc check.
• The jq presence check at :28 fails closed with its rationale inline — a missing jq would otherwise skip the audit silently, which is the same class as the finding above and was pre-empted here.
• local err; err=$(…) split from local got=$? survives another edit, and 2>&1 >/dev/null at :177 is still in the correct order. Both are trivially "tidied" into silent total failure.
• The bidirectional coverage guard and the per-label DECLARED check inside check() continue to hold: config --profiles returns exactly the seven declared profiles, and the matrix names exactly those.

Recommended Action

1. Address Important issues this cycle.
2. Consider Suggestions opportunistically.

[allyblockcast]

Finding addressed — d793325

Correct, and reproduced exactly as measured. Ran it on a clean clone at 78e0ac4b (docker compose v5.5.0), planting restart: on-failure:l3 on x-service and changing nothing else but whether HOSTNAME is set in the environment:

condition	audit	lane exit
violation, HOSTNAME present	20 × has no bounded retry count	1
violation, HOSTNAME absent	silent	0, PASS all profile sets resolve as expected

Same table. The 2>&1 at :209 merges the warning into the value jq parses; jq exits 5, its parse error goes to its stderr and is discarded, audit comes back empty, and [[ -n "$audit" ]] reads that as a clean audit.

The fix is two arms, because neither subsumes the other

Your one-line suggestion at :214 is the right shape and I took it — but it is not sufficient on its own, and that is worth stating because it is the same class one layer down:

$ echo -n "" | jq -r '.services // {} | to_entries[] | .key'; echo $?
0

jq exits 0 on empty input. So with stderr separated, a compose run that fails outright yields empty stdout → jq rc 0 → silent PASS. The rc check on compose is the only thing that catches that. Conversely the jq rc check is the only thing that catches rc=0-with-non-JSON. Both stay:

if ! resolved=$(docker compose … config --format json 2>/dev/null) \
   || ! audit=$(restart_audit "$label" <<<"$resolved"); then

Stderr separation matches :177's 2>&1 >/dev/null — stderr belongs in the diagnostic, never in the parsed value. I dropped the old head -1 <<<"$resolved" message with it (it was printing the merged stream) and replaced it with the reproduce command line, which is more useful than a truncated compose error for a case that only fires when the YAML call at :197 already passed.

Mutation-tested per guard, not only run green

mutation	result
revert 2>/dev/null → 2>&1 (keep jq arm)	baseline, no violation, HOSTNAME absent → exit 1, the restart-policy audit did not run — a false red
drop the compose-rc arm (keep jq arm)	restart_audit on empty input → rc=0, empty output — a failed compose passes silently

Each arm has a mutation that turns the check red (or, for arm 2, demonstrably blind), so neither is decoration.

After

All four conditions now agree — that one variable no longer changes the verdict:

	HOSTNAME present	HOSTNAME absent
baseline	PASS, exit 0	PASS, exit 0
on-failure:l3 planted	20 FAIL, exit 1	20 FAIL, exit 1

Thanks for the precision on "latent, not live" — pulling run 35492936935 to confirm 0 warnings on the real arc-light job is what made this triageable as a correctness fix rather than a suspected CI miss. The header at :18-21 does invite local runs on an uncontrolled shell, so the latent case is the one that matters.

[allyblockcast]

Ally — Consolidated PR Review

Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex (nested CLI not launched in the k8s runner; prompts applied directly over the diff and the file at head).
Reviewed head: d793325

The prior finding is fixed, and the fix does better than fail closed: dropping stderr off the parsed value means the audit now actually runs in the warning case rather than merely going red. I paired the guard with a failing mutation instead of only confirming the green path.

Prior Findings Dispositioned (1)

• prior:78e0ac4 important 1 — fixed — scripts/validate-compose.sh:223 — the json call is now 2>/dev/null and the audit is invoked inside the same if, so restart_audit's own rc is checked: if ! resolved=$(… config --format json 2>/dev/null) || ! audit=$(restart_audit "$label" <<<"$resolved").

  I verified the two halves separately rather than trusting the shape. restart_audit is a bare jq call, so its rc is jq's rc — measured on jq 1.7: valid JSON clean → 0, valid JSON with a violation → 0 (output non-empty), warning-prefixed non-JSON → 5, empty input → 0. So the new arm fires on exactly the case that was silent, and the comment's "jq exits 0 on EMPTY input" caveat at :220 is correct and is why the compose rc check must stay.

  Then the mutation, head against 78e0ac4b, same fake compose, only the two lines differing:

  compose behaviour	head d7933259	prior 78e0ac4b
  clean JSON	ok rc=0	ok rc=0
  violation	FAIL … unbounded retry rc=1	FAIL … unbounded retry rc=1
  violation + one level=warning on stderr	FAIL … unbounded retry rc=1	ok rc=0
  rc=0, stdout not JSON	FAIL audit did not run rc=1	ok rc=0
  compose rc≠0	FAIL audit did not run rc=1	FAIL could not resolve as json rc=1

  Both silent-green rows are closed with no regression on the other three. Note the third row specifically: head does not just fail closed there, it reports the planted on-failure:l3 violation — because with stderr off the value, the JSON parses and the audit genuinely runs. That is the stronger outcome, and it is the one the 2>/dev/null half buys that the rc check alone would not.

Critical Issues (0)

Important Issues (0)

Suggestions (4)

• [errors] scripts/validate-compose.sh:223 — 2>/dev/null discards compose's stderr, so on the rc≠0 branch the operator loses the error text the prior version printed; the message substitutes a reproduce command. I think that trade is defensible and I am not asking you to reverse it — it is the rung that needed no temp file, and :228 prints a copy-pasteable command with the real $PROJECT/${FILES[*]}/${args[*]}. The one place it bites is the branch's own stated cause: "the two formatters do not always agree on rc for one project" is a compose-version-dependent disagreement, so the environment that failed is the one whose diagnostic was dropped, and the reproduce command may not reproduce locally — this PR has already measured v5.3.1 on arc-light against v5.5.0 elsewhere. If it ever fires in CI, capturing stderr to a file for that branch only is the cheap follow-up. Not worth pre-empting now.
• [native-codex] scripts/validate-compose.sh:188-190 — the second explanation in the "now resolves" message is still unreachable: check() validates every profile against DECLARED at :169-173 and returns 1 before compose is invoked, so by :182 the names provably reached compose and "this set resolved to the no-profile baseline" cannot be the cause. Version drift is the cause that can still fire. (Carried from 9f52b86/78e0ac4b; unchanged by this head, re-raised not re-counted as a finding.)
• [comments] scripts/validate-compose.sh:69 — the collision is still pinned as services.relay: container name "relay" is already in use, and no run I have seen reports services.relay; the real arc-light log reports services.relay-caddy for both pinned sets. Three services inherit x-relay-cache and compose names whichever it reaches first. Cosmetic — nothing asserts on the string — but :185-187 sends a developer to "confirm the mechanism" against an example that will not match. (Carried.)
• [errors] scripts/validate-compose.sh:151 — elif still means a service declaring both hazards reports only the retry-count one; the deploy.restart_policy overwrite surfaces on the next run. Two round trips for one service. (Carried.)

Strengths

• The fix addresses the class, not the line I cited. I flagged one call; the replacement guards every reason the audit could fail to run, and :200-220 enumerates the two causes with an explicit note that neither subsumes the other — including the jq-exits-0-on-empty detail that is the whole reason the compose rc check cannot be dropped as redundant. That is the argument a future reader needs to not "simplify" one half away.
• The comment records the measurement (rc=1 and 20 FAIL lines with $HOSTNAME set, rc=0 and PASS without) rather than the conclusion, so the claim is checkable at the point of the code. It also names why $HOSTNAME is the trigger — one of only two interpolations carrying no :- default — which is what makes the hazard findable again.
• if ! A || ! B short-circuits correctly: a failed compose never reaches jq, and local resolved audit stays on its own line so neither assignment's rc is masked by the local builtin. That footgun has now survived four edits of this function.
• The error message states the property rather than the symptom — "The audit reports nothing in that state, which is indistinguishable from a clean audit" — which is the sentence that stops someone re-merging the streams later.
• 2>&1 >/dev/null at :177 is still in the correct order for the YAML call, and the two calls now differ deliberately rather than accidentally: stderr wanted there, unwanted here.

Recommended Action

1. No blocking changes requested.
2. Merge once the remaining required CI checks finish green.