From 14acba6b190f34d41eec57eaa2d6ffbfcb7c10cf Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 23 Sep 2026 00:08:37 +0000 Subject: [PATCH 01/13] smelt: .env.published was missing the indexer, and nothing could notice `make up` fails at `compose up` with INDEXER_IMAGE unset. Eight image variables are required `${X:?}` interpolations -- seven across systems/*/compose.yml plus PIRI_IMAGE, which appears in no compose file at all because pkg/generate/compose.go emits piri's service. `.env.published` supplied seven of the eight. The file's own header said the duplication between it and publishedImages in pkg/stack/options.go "cannot drift silently -- a missing entry fails at `compose up` naming the variable". Both halves are true and together they are not enough: it failed at `compose up`, and it drifted anyway, because nothing in CI runs `make up`. Not silent and not noticed turn out to be the same thing. Every Go path fills all eight through WithPublishedImages(), whose table does list the indexer, so no test could see it either. So the fix is the entry plus the check that makes the header's claim true. check-published-images.sh derives all three lists -- the required interpolations, the env file, and the Go table -- and fails when they disagree. No list lives in it; a service added or renamed needs no edit (rule 3). Two things found while writing it, both worth not rediscovering: - THE COMPOSE GREP MUST BE SCOPED TO compose.yml. Shell scripts that run inside the containers use the same `${X:?}` form for values compose never supplies: BAO_ADDR and INGOT_OPENBAO_TOKEN in systems/ingot/openbao/init.sh, PIRI_POSTGRES_DATABASES in systems/piri/postgres-init.sh. A recursive grep misses them today only because none is named *_IMAGE, which is not a property to rely on. - THE FIRST DRAFT OF THIS GUARD SHIPPED A SECOND CHECK THAT NEVER RAN. Its awk range ended at /^}/, which matches the `}{` closing the struct type, so the Go table read as empty -- and under `set -o pipefail` the empty grep killed the script at that line, before the emptiness check meant to catch exactly this. The early exit was indistinguishable from the first check having reported and finished. Every extraction now ends in `|| true` and is tested explicitly, which is the opposite of the `|| true` this repository removes elsewhere: there it masked a failure, here it stops one being masked. Verified in seven directions, each seen to fail before being seen to pass: missing env entry (rc=1), entry dropped from the Go table (rc=1), an env ref whose tag differs from the Go table's (rc=1), a new required compose variable nobody added (rc=1), publishedImages renamed away (rc=2), the env file emptied (rc=2), and the fixed tree (rc=0). All eight guards pass and gofmt -s is clean. NOT verified: that `make up` now boots. There is no Docker here. What is checked is that all eight variables resolve from the env files the Makefile passes, which is the interpolation that was failing. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01CGAGAib517Ae1kg8SCdcEt --- .github/scripts/check-published-images.sh | 101 ++++++++++++++++++++++ .github/workflows/ci.yml | 2 + smelt/.env.published | 14 ++- 3 files changed, 114 insertions(+), 3 deletions(-) create mode 100755 .github/scripts/check-published-images.sh diff --git a/.github/scripts/check-published-images.sh b/.github/scripts/check-published-images.sh new file mode 100755 index 00000000..126ebfa9 --- /dev/null +++ b/.github/scripts/check-published-images.sh @@ -0,0 +1,101 @@ +#!/usr/bin/env bash +# The published image set is stated in three places. They must agree. +# +# smelt's compose files require these images (`${HILT_IMAGE:?...}`) rather than +# defaulting them, because a silent default meant a caller who forgot an +# override booted a published image and never heard about it. That leaves two +# places to supply them: `.env.published` for `make up`, and `publishedImages` +# in pkg/stack/options.go for Go callers of `stack.WithPublishedImages()`. The +# Makefile shells out to `docker compose` directly, so the Go table cannot +# reach it and the duplication is unavoidable. +# +# `.env.published` used to say that duplication could not drift silently, +# because a missing entry fails at `compose up` naming the variable. It failed +# at `compose up` and it drifted anyway: indexing-service was added to the Go +# table and not to the env file, so `make up` had been broken on main for as +# long as the indexer had been in the stack. Nothing in CI runs `make up`, so +# "not silent" and "not noticed" turned out to be the same thing. +# +# This is the check that makes the claim true. Nothing here is a list: all +# three sides are derived, so a service added or renamed needs no edit. +# +# NOT COVERED, and deliberately: images the stack runs that this repository +# does NOT build -- guppy, ipni, plc, blockchain, minio. Those keep their `:-` +# compose defaults and are pinned by digest instead, which is +# check-stack-images.sh's job. This check is only about the required ones. +set -euo pipefail + +cd "$(dirname "$0")/../.." + +fail() { echo "::error::$*" >&2; status=1; } +status=0 + +# 1. REQUIRED: every `${X_IMAGE:?...}` compose interpolation. Two sources, and +# both are needed -- PIRI_IMAGE appears in no compose file at all, because +# pkg/generate/compose.go emits piri's service definition. A glob over +# systems/ alone reports seven of eight and looks complete. +# +# EVERY extraction below ends in `|| true` and is then tested for +# emptiness. Not a mask: under `set -o pipefail` a grep that matches +# nothing kills the script mid-way, and an early exit at the second +# extraction is indistinguishable from the first check having reported +# and finished. That is exactly how the first draft of this file shipped +# a second check that never ran. +# +# RESTRICTED TO compose.yml, not all of systems/. Shell scripts that run +# INSIDE the containers use the same `${X:?}` form for values compose never +# supplies -- BAO_ADDR and INGOT_OPENBAO_TOKEN in +# systems/ingot/openbao/init.sh, PIRI_POSTGRES_DATABASES in +# systems/piri/postgres-init.sh. Those come from the container environment +# and have nothing to do with `make up`'s interpolation. A recursive grep +# happens to miss them today only because none is named *_IMAGE, which is +# not a property worth relying on. +required=$( + { find smelt/systems -name compose.yml -exec grep -hoE '\$\{[A-Z_]+_IMAGE:\?' {} + 2>/dev/null || true + grep -rhoE '\$\{[A-Z_]+_IMAGE:\?' smelt/pkg/generate/ 2>/dev/null || true + } | sed -E 's/\$\{([A-Z_]+):\?/\1/' | sort -u +) +if [ -z "$required" ]; then + echo "::error::found no required \${X_IMAGE:?} interpolations at all." \ + "Refusing to report that as agreement." >&2 + exit 2 +fi + +# 2. PROVIDED: what .env.published sets. +provided=$(grep -oE '^[A-Z_]+_IMAGE=' smelt/.env.published 2>/dev/null | tr -d '=' | sort -u || true) + +if [ "$required" != "$provided" ]; then + fail "smelt/.env.published does not match the required compose interpolations." + diff <(printf '%s\n' "$required") <(printf '%s\n' "$provided") \ + --label 'required by compose' --label 'set in .env.published' -u >&2 || true +fi + +# 3. The two copies of the reference list must name the same images. Compared +# as refs rather than by variable name, because the Go table keys on the +# reference and a config field, not on the compose variable. +env_refs=$(grep -oE '^[A-Z_]+_IMAGE=.*' smelt/.env.published 2>/dev/null | cut -d= -f2- | sort -u || true) +# `/^}$/`, not `/^}/`: the declaration is `[]struct {` ... `}{` ... `}`, so a +# range ending at any line starting with `}` stops at the struct's own closing +# brace and never reaches a single entry. It read as an empty table. +go_refs=$(awk '/^var publishedImages = /,/^}$/' smelt/pkg/stack/options.go 2>/dev/null \ + | grep -oE '"[^"]+/[^"]+:[^"]+"' | tr -d '"' | sort -u || true) + +if [ -z "$go_refs" ] || [ -z "$env_refs" ]; then + echo "::error::could not read one of the two image lists" \ + "(publishedImages in smelt/pkg/stack/options.go, or smelt/.env.published)." \ + "Refusing to report that as agreement." >&2 + exit 2 +fi + +if [ "$env_refs" != "$go_refs" ]; then + fail "smelt/.env.published and pkg/stack/options.go name different images." + diff <(printf '%s\n' "$env_refs") <(printf '%s\n' "$go_refs") \ + --label '.env.published' --label 'publishedImages' -u >&2 || true +fi + +if [ "$status" -eq 0 ]; then + n=$(printf '%s\n' "$required" | grep -c .) + echo "check-published-images: $n required images, agreed across compose," \ + ".env.published and publishedImages." +fi +exit "$status" diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 2657e170..b92f8170 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -64,6 +64,8 @@ jobs: run: .github/scripts/check-base-images.sh - name: every pulled compose image is pinned by digest run: .github/scripts/check-stack-images.sh + - name: the three copies of the published image set agree + run: .github/scripts/check-published-images.sh - name: every pg_isready healthcheck probes TCP run: .github/scripts/check-pg-healthchecks.sh - name: every goreleaser -X ldflag names a package in its module diff --git a/smelt/.env.published b/smelt/.env.published index 6cec404c..fe91302c 100644 --- a/smelt/.env.published +++ b/smelt/.env.published @@ -1,4 +1,4 @@ -# Published image references for the seven services this repository builds. +# Published image references for the eight services this repository builds. # # Why this file exists: the compose interpolations for these images used to # carry a silent default (`${HILT_IMAGE:-ghcr.io/fil-forge/hilt:main}`), so a @@ -19,8 +19,15 @@ # # The same list lives in pkg/stack/options.go (publishedImages). That # duplication is deliberate: the Makefile shells out to `docker compose` -# directly, so a Go table cannot reach it. Neither copy can drift silently — -# a missing entry fails at `compose up` naming the variable. +# directly, so a Go table cannot reach it. +# +# This file used to claim the two copies could not drift silently, because a +# missing entry fails at `compose up` naming the variable. They drifted +# anyway: indexing-service was added to the Go table and not here, so +# `make up` was broken for as long as the indexer had been in the stack. +# Nothing in CI runs `make up`, so "not silent" and "not noticed" were the +# same thing. `.github/scripts/check-published-images.sh` is what makes the +# claim true now — it derives all three lists and runs in `guards`. PIRI_IMAGE=ghcr.io/fil-forge/piri:main HILT_IMAGE=ghcr.io/fil-forge/hilt:main @@ -29,3 +36,4 @@ UPLOAD_IMAGE=ghcr.io/fil-forge/sprue:main DELEGATOR_IMAGE=ghcr.io/fil-forge/delegator:main SIGNER_IMAGE=ghcr.io/fil-forge/piri-signing-service:main SWARF_IMAGE=ghcr.io/fil-forge/swarf:main +INDEXER_IMAGE=ghcr.io/fil-forge/indexing-service:main From c66c88f81f916578b79b3e35798f4c2c4d2c0de1 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 23 Sep 2026 00:09:50 +0000 Subject: [PATCH 02/13] smelt/CLAUDE.md carried a third copy of the image set, stale two ways Enumerating the class after fixing the first instance, as rule 5 asks, turned up a third hand-written copy of the built-here image list -- and it was worse than .env.published's. It named six of eight, and it still listed swarf and the indexer under "not built here" long after both were imported, with a trailing note that SWARF_IMAGE "joins the required set when the branch that builds swarf lands". That branch landed. Replaced with the derivation rather than extended with two more names: a `find ... compose.yml` one-liner that runs verbatim, the note that PIRI_IMAGE is not in it because pkg/generate/compose.go emits piri's service, and a pointer at the guard that checks all three copies agree. A fourth copy of a derivable list is the failure rule 3 names, and this paragraph is the proof that extending one does not stop it recurring. The "not built here" bullet now lists only what is actually pulled, and says those are pinned by digest by check-stack-images.sh instead. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01CGAGAib517Ae1kg8SCdcEt --- smelt/CLAUDE.md | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/smelt/CLAUDE.md b/smelt/CLAUDE.md index 1dcacc90..e48baff2 100644 --- a/smelt/CLAUDE.md +++ b/smelt/CLAUDE.md @@ -462,8 +462,17 @@ Available variables: `PIRI_IMAGE`, `GUPPY_IMAGE`, `DELEGATOR_IMAGE`, `INDEXER_IM Where the defaults come from depends on whether this repository builds the image: -- **Built here** — `PIRI_IMAGE`, `HILT_IMAGE`, `INGOT_IMAGE`, `UPLOAD_IMAGE`, `DELEGATOR_IMAGE`, `SIGNER_IMAGE`. These have **no compose default**: the interpolation is `${PIRI_IMAGE:?...}`, so compose fails with the variable's name rather than quietly booting `ghcr.io/fil-forge/piri:main`. That silent fallback used to let a test that forgot its overrides pass while exercising published code instead of the commit under test. Published references for these live in `.env.published`, which `make up` loads via `--env-file`; a Go test asks for them with `stack.WithPublishedImages()`. -- **Not built here** — everything else (guppy, indexer, ipni, swarf, plc, blockchain, minio). These keep an inline `:-` default in their `systems/*/compose.yml`, because nothing in this repository can produce an alternative. `SWARF_IMAGE` joins the required set when the branch that builds swarf lands. +- **Built here.** These have **no compose default**: the interpolation is `${PIRI_IMAGE:?...}`, so compose fails with the variable's name rather than quietly booting `ghcr.io/fil-forge/piri:main`. That silent fallback used to let a test that forgot its overrides pass while exercising published code instead of the commit under test. Published references for these live in `.env.published`, which `make up` loads via `--env-file`; a Go test asks for them with `stack.WithPublishedImages()`. + + **The set is not listed here**, because this paragraph carried a hand-written copy of it and went stale twice over: it named six when there were eight, and still called swarf and the indexer "not built here" long after both were imported. Derive it: + + ```sh + find systems -name compose.yml -exec grep -hoE '\$\{[A-Z_]+_IMAGE:\?' {} + \ + | sed -E 's/\$\{([A-Z_]+):\?/\1/' | sort -u + ``` + + That misses `PIRI_IMAGE`, which appears in no compose file — `pkg/generate/compose.go` emits piri's service definition. `.github/scripts/check-published-images.sh` does the whole derivation and fails when `.env.published`, `publishedImages` in `pkg/stack/options.go` and the compose interpolations disagree. +- **Not built here** — guppy, ipni, plc, blockchain, minio. These keep an inline `:-` default in their `systems/*/compose.yml`, because nothing in this repository can produce an alternative, and they are pinned by digest instead (`.github/scripts/check-stack-images.sh`). `.env` is yours: every line in it is commented out, and `make up` loads it *after* `.env.published`, so anything you uncomment there wins. From f9a86cfaa0e11ccc9cebb4a793f2525f40e0d67e Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 23 Sep 2026 00:24:27 +0000 Subject: [PATCH 03/13] Round one: the guard passed on three broken trees, so it is a Go test now A review round found five, two of them the guard reporting "agreed" and exiting 0 on a tree that was genuinely broken -- the same class it was written to catch. Reproduced all three before changing anything. IT COMPARED SETS, NEVER PAIRS. Variable names in one comparison, image references in the other, and never which reference a given variable carries. So it passed on `.env.published` with PIRI_IMAGE and HILT_IMAGE swapped -- each service booting the other's image -- and on a file binding PIRI_IMAGE twice, once to hilt's image, because `sort -u` collapsed the repeated name and the value was already present from hilt's own line. IT LOOKED IN THE WRONG ROOT. `find smelt/systems -name compose.yml` is one directory too deep: smelt/compose.yml is the file `make up` resolves and it sits outside systems/. A required variable added there went unseen. Nothing is broken today -- that file declares no services -- but it is this PR's own bug class at the one compose file systems/ does not contain. Closing the pairing hole in shell means recovering which config field each variable and each reference reaches, which is Go semantics. So the check is now TestPublishedImageSetAgrees in pkg/stack, and the shell script and its ci.yml step are gone. Both tables are already data in that package; the test reads them instead of parsing them, and pairs variable to reference BY BEHAVIOUR -- applying each option and each getter to a fresh config and seeing which field moved -- so nothing has to agree about how PIRI_IMAGE relates to piriImage, and a renamed field cannot silently break the join. That also covers the fourth statement of the set, envImageOptions, which the shell version ignored: a service added to compose, .env.published and publishedImages but not there silently loses its env override. AND THE FOURTH COPY OF THE DISPROVEN SENTENCE WAS IN options.go -- the file the script was parsing. The claim that neither copy can drift silently was corrected in .env.published and dropped from smelt/CLAUDE.md, and left standing in the third place, which is the fix-one-instance failure rule 5 names. Corrected there too, and its own hand-written "not built here" list replaced with a pointer at the test. Verified in eight directions, each seen to fail before being seen to pass: env missing the indexer, PIRI/HILT values swapped, a duplicate binding, an env ref whose tag differs, a required variable added to smelt/compose.yml, an entry dropped from publishedImages, a variable dropped from envImageOptions, and the fixed tree. All seven remaining guards pass and gofmt -s is clean. TestBuild*ImageTagFormat fail in this environment and do so identically on an untouched origin/main worktree: they skip on `exec.LookPath("docker")`, and here the client is present while the daemon is not. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01CGAGAib517Ae1kg8SCdcEt --- .github/scripts/check-published-images.sh | 101 ------------ .github/workflows/ci.yml | 2 - mainprobe | 1 + smelt/.env.published | 8 +- smelt/CLAUDE.md | 2 +- smelt/pkg/stack/options.go | 18 ++- smelt/pkg/stack/published_images_test.go | 189 ++++++++++++++++++++++ 7 files changed, 209 insertions(+), 112 deletions(-) delete mode 100755 .github/scripts/check-published-images.sh create mode 160000 mainprobe create mode 100644 smelt/pkg/stack/published_images_test.go diff --git a/.github/scripts/check-published-images.sh b/.github/scripts/check-published-images.sh deleted file mode 100755 index 126ebfa9..00000000 --- a/.github/scripts/check-published-images.sh +++ /dev/null @@ -1,101 +0,0 @@ -#!/usr/bin/env bash -# The published image set is stated in three places. They must agree. -# -# smelt's compose files require these images (`${HILT_IMAGE:?...}`) rather than -# defaulting them, because a silent default meant a caller who forgot an -# override booted a published image and never heard about it. That leaves two -# places to supply them: `.env.published` for `make up`, and `publishedImages` -# in pkg/stack/options.go for Go callers of `stack.WithPublishedImages()`. The -# Makefile shells out to `docker compose` directly, so the Go table cannot -# reach it and the duplication is unavoidable. -# -# `.env.published` used to say that duplication could not drift silently, -# because a missing entry fails at `compose up` naming the variable. It failed -# at `compose up` and it drifted anyway: indexing-service was added to the Go -# table and not to the env file, so `make up` had been broken on main for as -# long as the indexer had been in the stack. Nothing in CI runs `make up`, so -# "not silent" and "not noticed" turned out to be the same thing. -# -# This is the check that makes the claim true. Nothing here is a list: all -# three sides are derived, so a service added or renamed needs no edit. -# -# NOT COVERED, and deliberately: images the stack runs that this repository -# does NOT build -- guppy, ipni, plc, blockchain, minio. Those keep their `:-` -# compose defaults and are pinned by digest instead, which is -# check-stack-images.sh's job. This check is only about the required ones. -set -euo pipefail - -cd "$(dirname "$0")/../.." - -fail() { echo "::error::$*" >&2; status=1; } -status=0 - -# 1. REQUIRED: every `${X_IMAGE:?...}` compose interpolation. Two sources, and -# both are needed -- PIRI_IMAGE appears in no compose file at all, because -# pkg/generate/compose.go emits piri's service definition. A glob over -# systems/ alone reports seven of eight and looks complete. -# -# EVERY extraction below ends in `|| true` and is then tested for -# emptiness. Not a mask: under `set -o pipefail` a grep that matches -# nothing kills the script mid-way, and an early exit at the second -# extraction is indistinguishable from the first check having reported -# and finished. That is exactly how the first draft of this file shipped -# a second check that never ran. -# -# RESTRICTED TO compose.yml, not all of systems/. Shell scripts that run -# INSIDE the containers use the same `${X:?}` form for values compose never -# supplies -- BAO_ADDR and INGOT_OPENBAO_TOKEN in -# systems/ingot/openbao/init.sh, PIRI_POSTGRES_DATABASES in -# systems/piri/postgres-init.sh. Those come from the container environment -# and have nothing to do with `make up`'s interpolation. A recursive grep -# happens to miss them today only because none is named *_IMAGE, which is -# not a property worth relying on. -required=$( - { find smelt/systems -name compose.yml -exec grep -hoE '\$\{[A-Z_]+_IMAGE:\?' {} + 2>/dev/null || true - grep -rhoE '\$\{[A-Z_]+_IMAGE:\?' smelt/pkg/generate/ 2>/dev/null || true - } | sed -E 's/\$\{([A-Z_]+):\?/\1/' | sort -u -) -if [ -z "$required" ]; then - echo "::error::found no required \${X_IMAGE:?} interpolations at all." \ - "Refusing to report that as agreement." >&2 - exit 2 -fi - -# 2. PROVIDED: what .env.published sets. -provided=$(grep -oE '^[A-Z_]+_IMAGE=' smelt/.env.published 2>/dev/null | tr -d '=' | sort -u || true) - -if [ "$required" != "$provided" ]; then - fail "smelt/.env.published does not match the required compose interpolations." - diff <(printf '%s\n' "$required") <(printf '%s\n' "$provided") \ - --label 'required by compose' --label 'set in .env.published' -u >&2 || true -fi - -# 3. The two copies of the reference list must name the same images. Compared -# as refs rather than by variable name, because the Go table keys on the -# reference and a config field, not on the compose variable. -env_refs=$(grep -oE '^[A-Z_]+_IMAGE=.*' smelt/.env.published 2>/dev/null | cut -d= -f2- | sort -u || true) -# `/^}$/`, not `/^}/`: the declaration is `[]struct {` ... `}{` ... `}`, so a -# range ending at any line starting with `}` stops at the struct's own closing -# brace and never reaches a single entry. It read as an empty table. -go_refs=$(awk '/^var publishedImages = /,/^}$/' smelt/pkg/stack/options.go 2>/dev/null \ - | grep -oE '"[^"]+/[^"]+:[^"]+"' | tr -d '"' | sort -u || true) - -if [ -z "$go_refs" ] || [ -z "$env_refs" ]; then - echo "::error::could not read one of the two image lists" \ - "(publishedImages in smelt/pkg/stack/options.go, or smelt/.env.published)." \ - "Refusing to report that as agreement." >&2 - exit 2 -fi - -if [ "$env_refs" != "$go_refs" ]; then - fail "smelt/.env.published and pkg/stack/options.go name different images." - diff <(printf '%s\n' "$env_refs") <(printf '%s\n' "$go_refs") \ - --label '.env.published' --label 'publishedImages' -u >&2 || true -fi - -if [ "$status" -eq 0 ]; then - n=$(printf '%s\n' "$required" | grep -c .) - echo "check-published-images: $n required images, agreed across compose," \ - ".env.published and publishedImages." -fi -exit "$status" diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index b92f8170..2657e170 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -64,8 +64,6 @@ jobs: run: .github/scripts/check-base-images.sh - name: every pulled compose image is pinned by digest run: .github/scripts/check-stack-images.sh - - name: the three copies of the published image set agree - run: .github/scripts/check-published-images.sh - name: every pg_isready healthcheck probes TCP run: .github/scripts/check-pg-healthchecks.sh - name: every goreleaser -X ldflag names a package in its module diff --git a/mainprobe b/mainprobe new file mode 160000 index 00000000..4611cbcb --- /dev/null +++ b/mainprobe @@ -0,0 +1 @@ +Subproject commit 4611cbcb9693ae34390cc21e5a94169e13bf0332 diff --git a/smelt/.env.published b/smelt/.env.published index fe91302c..2e8abc0e 100644 --- a/smelt/.env.published +++ b/smelt/.env.published @@ -26,8 +26,12 @@ # anyway: indexing-service was added to the Go table and not here, so # `make up` was broken for as long as the indexer had been in the stack. # Nothing in CI runs `make up`, so "not silent" and "not noticed" were the -# same thing. `.github/scripts/check-published-images.sh` is what makes the -# claim true now — it derives all three lists and runs in `guards`. +# same thing. `TestPublishedImageSetAgrees` in pkg/stack is what makes the +# claim true now. It is a Go test rather than a check-*.sh because the first +# attempt was a shell script that compared the lists as SETS, and so reported +# agreement on a tree where PIRI_IMAGE and HILT_IMAGE had been swapped here — +# each service booting the other's image. Pairing a variable to a reference +# means knowing which config field each reaches, which is Go semantics. PIRI_IMAGE=ghcr.io/fil-forge/piri:main HILT_IMAGE=ghcr.io/fil-forge/hilt:main diff --git a/smelt/CLAUDE.md b/smelt/CLAUDE.md index e48baff2..1f2d4fa5 100644 --- a/smelt/CLAUDE.md +++ b/smelt/CLAUDE.md @@ -471,7 +471,7 @@ Where the defaults come from depends on whether this repository builds the image | sed -E 's/\$\{([A-Z_]+):\?/\1/' | sort -u ``` - That misses `PIRI_IMAGE`, which appears in no compose file — `pkg/generate/compose.go` emits piri's service definition. `.github/scripts/check-published-images.sh` does the whole derivation and fails when `.env.published`, `publishedImages` in `pkg/stack/options.go` and the compose interpolations disagree. + That misses `PIRI_IMAGE`, which appears in no compose file — `pkg/generate/compose.go` emits piri's service definition. `TestPublishedImageSetAgrees` in `pkg/stack` does the whole derivation and fails when the compose interpolations, `.env.published`, `publishedImages` and `envImageOptions` disagree — including when two of them name *different* images for the same service. - **Not built here** — guppy, ipni, plc, blockchain, minio. These keep an inline `:-` default in their `systems/*/compose.yml`, because nothing in this repository can produce an alternative, and they are pinned by digest instead (`.github/scripts/check-stack-images.sh`). `.env` is yours: every line in it is commented out, and `make up` loads it *after* `.env.published`, so anything you uncomment there wins. diff --git a/smelt/pkg/stack/options.go b/smelt/pkg/stack/options.go index f2c16fdb..b038871e 100644 --- a/smelt/pkg/stack/options.go +++ b/smelt/pkg/stack/options.go @@ -406,13 +406,19 @@ var envImageOptions = []struct { // builds, alongside the config field it lands in. It is the Go-side twin of // .env.published, which says the same thing for `make up` — the Makefile // shells out to `docker compose` directly, so it cannot read this table. -// Neither copy can drift silently: the compose interpolations for these are -// required (`${X:?...}`), so a missing entry fails at `compose up` naming -// the variable rather than quietly booting :main. // -// Only what this repository builds. The other images the stack runs (guppy, -// ipni, plc, blockchain, minio) come from elsewhere and keep their `:-` -// compose defaults, so there is nothing for this to fill in. +// THIS COMMENT USED TO SAY the two copies could not drift silently, because +// the interpolations are required (`${X:?...}`) so a missing entry fails at +// `compose up` naming the variable. That is true and it is not enough: they +// drifted anyway — indexing-service was added here and not to .env.published, +// and `make up` had been failing ever since, because nothing in CI runs it. +// TestPublishedImageSetAgrees is what checks it now, and it checks all four +// statements of this set against each other, including envImageOptions above. +// +// Only what this repository builds; the rest keep their `:-` compose defaults, +// so there is nothing for this to fill in. That set is not listed here — it +// was, and the copy in smelt/CLAUDE.md went stale twice over — it is whatever +// the test derives. var publishedImages = []struct { ref string get func(*config) *string diff --git a/smelt/pkg/stack/published_images_test.go b/smelt/pkg/stack/published_images_test.go new file mode 100644 index 00000000..c8f56082 --- /dev/null +++ b/smelt/pkg/stack/published_images_test.go @@ -0,0 +1,189 @@ +package stack + +import ( + "os" + "path/filepath" + "reflect" + "regexp" + "sort" + "strings" + "testing" + + "github.com/stretchr/testify/require" +) + +// The published image set is stated in four places, and they must agree: +// +// - the compose files, which require each one (`${HILT_IMAGE:?...}`) rather +// than defaulting it, so a caller who forgets an override fails loudly; +// - `.env.published`, which supplies them to `make up`; +// - publishedImages, which supplies them to WithPublishedImages(); +// - envImageOptions, which lets a Go caller override them by the same names. +// +// They had already drifted: indexing-service was added to publishedImages and +// not to .env.published, so `make up` failed at `compose up` with INDEXER_IMAGE +// unset, for as long as the indexer had been in the stack. Three comments in +// this repository asserted that could not happen silently, on the grounds that +// a missing entry fails at `compose up`. It does — and nothing in CI runs +// `make up`, so "not silent" and "not noticed" were the same thing. +// +// WHY THIS IS A GO TEST AND NOT A check-*.sh. The first version was a shell +// script, and it compared the lists as SETS: variable names in one comparison, +// image references in the other, never the pairing between them. It reported +// agreement, and exited 0, on a tree where PIRI_IMAGE and HILT_IMAGE had been +// swapped in .env.published -- each service booting the other's image. Closing +// that in shell means recovering which config field each variable and each +// reference reaches, which is Go semantics, so it belongs here. Both tables are +// already data in this package; the test reads them rather than parsing them. + +const imageSentinel = "SENTINEL-published-images-test" + +// fieldReachedBy applies mutate to a fresh config and returns the name of the +// single field it set. Pairing by BEHAVIOUR rather than by a name convention: +// nothing has to agree about how PIRI_IMAGE relates to piriImage, and a +// renamed field cannot silently break the join. +func fieldReachedBy(t *testing.T, mutate func(*config)) string { + t.Helper() + var c config + mutate(&c) + + v := reflect.ValueOf(c) + var found []string + for i := 0; i < v.NumField(); i++ { + if v.Field(i).Kind() == reflect.String && v.Field(i).String() == imageSentinel { + found = append(found, v.Type().Field(i).Name) + } + } + require.Len(t, found, 1, "expected exactly one config field to be set") + return found[0] +} + +// requiredImageVars walks every compose.yml under the smelt root, plus the +// generator, and returns the `${X_IMAGE:?...}` variables. +// +// ROOTED AT THE SMELT ROOT, not at systems/. smelt/compose.yml is the file +// `make up` resolves and it sits outside systems/; a walk rooted one directory +// deeper would let a required variable added there go unchecked. That is the +// same bug class this test exists for, at the one compose file systems/ does +// not contain. +func requiredImageVars(t *testing.T, root string) map[string]bool { + t.Helper() + re := regexp.MustCompile(`\$\{([A-Z0-9_]+_IMAGE):\?`) + vars := map[string]bool{} + + scan := func(path string) { + b, err := os.ReadFile(path) + require.NoError(t, err) + for _, m := range re.FindAllStringSubmatch(string(b), -1) { + vars[m[1]] = true + } + } + + composeFiles := 0 + require.NoError(t, filepath.WalkDir(root, func(path string, d os.DirEntry, err error) error { + if err != nil { + return err + } + if d.IsDir() { + return nil + } + // PIRI_IMAGE appears in no compose file at all -- pkg/generate emits + // piri's service definition -- so the generator is a second source and + // a glob over compose files alone reports seven of eight. + switch { + case d.Name() == "compose.yml": + composeFiles++ + scan(path) + case strings.HasPrefix(path, filepath.Join(root, "pkg", "generate")) && + strings.HasSuffix(path, ".go"): + scan(path) + } + return nil + })) + + require.NotZero(t, composeFiles, "found no compose.yml under %s", root) + require.NotEmpty(t, vars, "found no required ${X_IMAGE:?} interpolations") + return vars +} + +func readEnvPublished(t *testing.T, path string) map[string]string { + t.Helper() + b, err := os.ReadFile(path) + require.NoError(t, err) + + out := map[string]string{} + for i, line := range strings.Split(string(b), "\n") { + line = strings.TrimSpace(line) + if line == "" || strings.HasPrefix(line, "#") { + continue + } + k, v, ok := strings.Cut(line, "=") + require.True(t, ok, "%s:%d: not KEY=VALUE: %q", path, i+1, line) + // A repeat binding is a real defect and invisible to any set + // comparison: the earlier shell version passed on a file with + // PIRI_IMAGE bound twice to different images. + _, dup := out[k] + require.False(t, dup, "%s:%d: %s is set more than once", path, i+1, k) + out[k] = v + } + return out +} + +func TestPublishedImageSetAgrees(t *testing.T) { + root := filepath.Join("..", "..") + + // Pair each compose variable and each published reference to the config + // field it reaches, then join on that field. + varToField := map[string]string{} + for _, e := range envImageOptions { + varToField[e.env] = fieldReachedBy(t, func(c *config) { e.opt(imageSentinel)(c) }) + } + fieldToRef := map[string]string{} + for _, p := range publishedImages { + f := fieldReachedBy(t, func(c *config) { *p.get(c) = imageSentinel }) + _, dup := fieldToRef[f] + require.False(t, dup, "publishedImages sets %s twice", f) + fieldToRef[f] = p.ref + } + + required := requiredImageVars(t, root) + env := readEnvPublished(t, filepath.Join(root, ".env.published")) + + // 1. Every required compose variable must be overridable from Go, or a + // caller can set it for `docker compose` and not for a Go test. + for v := range required { + require.Contains(t, varToField, v, + "%s is a required compose interpolation but is not in envImageOptions", v) + } + + // 2. Required, supplied by .env.published, and present in publishedImages + // must be the same set -- and the REFERENCE each names must match, + // which is what a set comparison cannot see. + var wantVars []string + for v, f := range varToField { + if _, built := fieldToRef[f]; built { + wantVars = append(wantVars, v) + } + } + sort.Strings(wantVars) + + var gotRequired, gotEnv []string + for v := range required { + gotRequired = append(gotRequired, v) + } + for v := range env { + gotEnv = append(gotEnv, v) + } + sort.Strings(gotRequired) + sort.Strings(gotEnv) + + require.Equal(t, wantVars, gotRequired, + "the compose files and publishedImages disagree about which images this repository builds") + require.Equal(t, wantVars, gotEnv, + ".env.published does not supply exactly the required images") + + for _, v := range wantVars { + require.Equal(t, fieldToRef[varToField[v]], env[v], + "%s in .env.published names a different image than publishedImages does", v) + } +} From 1945c7d1f0098c5af049a12ccafd0f9131100f1c Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 23 Sep 2026 00:24:41 +0000 Subject: [PATCH 04/13] Remove a stray mainprobe gitlink committed by accident The previous commit's `git add -A` picked up a temporary worktree created to check whether TestBuild*ImageTagFormat fail on an untouched origin/main -- they do -- and committed it as a gitlink. It was never part of the change. Removed, the worktree deleted and pruned. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01CGAGAib517Ae1kg8SCdcEt --- mainprobe | 1 - 1 file changed, 1 deletion(-) delete mode 160000 mainprobe diff --git a/mainprobe b/mainprobe deleted file mode 160000 index 4611cbcb..00000000 --- a/mainprobe +++ /dev/null @@ -1 +0,0 @@ -Subproject commit 4611cbcb9693ae34390cc21e5a94169e13bf0332 From cf5c105d72381995140c254927185e8b3f0645a3 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 23 Sep 2026 00:46:49 +0000 Subject: [PATCH 05/13] Round two: the deriver recognised less than compose does, in two ways Round one replaced a shell guard that compared sets instead of pairs. Round two found the replacement under-reading its inputs -- the same shape one layer down, and both were proved with the real client rather than argued. `${X?msg}` IS ALSO A REQUIRED FORM. `${X:?msg}` errors when X is unset or empty; `${X?msg}` errors when unset. Compose rejects either identically -- "required variable NEWTHING_IMAGE is missing a value" -- and the pattern matched only the first, so the other spelling of the same line left this test green. Now `:?\?`. THE WALK KEYED ON THE FILENAME compose.yml. The root file pulls its parts in with `include:`, whose paths it names explicitly, so an included file called anything else was invisible -- and compose.yaml is compose's own auto-discovered default. Now every .yml and .yaml under the smelt root. Over-reading is the safe direction and costs nothing measurable: across every YAML file under that root -- grafana dashboards and service configs included -- the pattern matches only compose files. Verified: all three mutations that previously passed now fail (the no-colon form in smelt/compose.yml, an included file named newsvc.yml, a systems/compose.yaml), and the committed tree still passes. Also: `systems/*/compose.yml` in the PR body was said to match seven. It matches thirteen files and reaches none of the indexer's -- INDEXER_IMAGE is at systems/indexing/indexer/compose.yml, a level deeper. That is the same nested-path miscount #18's body records as having caught three rounds, made again in the body of the pull request that fixes the nesting. The body now states the number without the glob that does not produce it. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01CGAGAib517Ae1kg8SCdcEt --- smelt/pkg/stack/published_images_test.go | 22 +++++++++++++++++----- 1 file changed, 17 insertions(+), 5 deletions(-) diff --git a/smelt/pkg/stack/published_images_test.go b/smelt/pkg/stack/published_images_test.go index c8f56082..17afc4e4 100644 --- a/smelt/pkg/stack/published_images_test.go +++ b/smelt/pkg/stack/published_images_test.go @@ -58,17 +58,29 @@ func fieldReachedBy(t *testing.T, mutate func(*config)) string { return found[0] } -// requiredImageVars walks every compose.yml under the smelt root, plus the -// generator, and returns the `${X_IMAGE:?...}` variables. +// requiredImageVars walks the smelt root and returns the required image +// variables its YAML and its compose generator name. // // ROOTED AT THE SMELT ROOT, not at systems/. smelt/compose.yml is the file // `make up` resolves and it sits outside systems/; a walk rooted one directory // deeper would let a required variable added there go unchecked. That is the // same bug class this test exists for, at the one compose file systems/ does // not contain. +// +// EVERY .yml AND .yaml, not files named compose.yml. The root file pulls in +// its parts with `include:`, whose paths it names explicitly, so an included +// file called anything else would be missed -- and `compose.yaml` is compose's +// own auto-discovered default. Over-reading is the safe direction here and +// costs nothing measurable: across every YAML file under this root, the +// pattern matches only compose files. func requiredImageVars(t *testing.T, root string) map[string]bool { t.Helper() - re := regexp.MustCompile(`\$\{([A-Z0-9_]+_IMAGE):\?`) + // `:?` and `?` are BOTH required forms. `${X:?msg}` errors when X is unset + // or empty; `${X?msg}` errors only when unset. Compose rejects either the + // same way -- "required variable NEWTHING_IMAGE is missing a value" -- + // and an earlier version of this pattern matched only the first, so the + // other spelling of the same line left this test green. + re := regexp.MustCompile(`\$\{([A-Z0-9_]+_IMAGE):?\?`) vars := map[string]bool{} scan := func(path string) { @@ -91,7 +103,7 @@ func requiredImageVars(t *testing.T, root string) map[string]bool { // piri's service definition -- so the generator is a second source and // a glob over compose files alone reports seven of eight. switch { - case d.Name() == "compose.yml": + case strings.HasSuffix(path, ".yml"), strings.HasSuffix(path, ".yaml"): composeFiles++ scan(path) case strings.HasPrefix(path, filepath.Join(root, "pkg", "generate")) && @@ -101,7 +113,7 @@ func requiredImageVars(t *testing.T, root string) map[string]bool { return nil })) - require.NotZero(t, composeFiles, "found no compose.yml under %s", root) + require.NotZero(t, composeFiles, "found no YAML under %s", root) require.NotEmpty(t, vars, "found no required ${X_IMAGE:?} interpolations") return vars } From b5ce12ec8fc1ce1948472f1216f74cadcfa48e89 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 23 Sep 2026 01:05:52 +0000 Subject: [PATCH 06/13] Round three: "over-reading is the safe direction" was true in only one case Three findings, none in the shipped behaviour, two of them claims made FOR the change. The first is the one worth the round. "OVER-READING IS THE SAFE DIRECTION HERE" INVERTS FOR A NAME THE GO TABLES ALREADY CARRY. It holds for a name they do not: the extra entry fails check 1 and the test goes red. But `wantVars` is derived from envImageOptions intersected with publishedImages and NEVER from what the YAML requires, so a match on text compose does not interpolate keeps an existing name alive in `required` and hides the drift. Comment out an interpolation while pinning the image literally -- compose stops requiring the variable -- and the raw scan still saw it, so the test passed on an .env.published entry nothing needed. Measured both ways against the committed head: masked before, caught after. The other divergence is the same root cause: `$${X:?}` is compose's escape for a literal and was matched as an interpolation. Both are gone rather than documented, by reading the inputs the way their consumers do. YAML is parsed with yaml.v3 and every string scalar walked, keys included, so `#` comments are not in the input at all. The generator is parsed with go/parser and its string literals read, symmetric and for the same reason -- a commented-out interpolation there would mask the same way. `$$` is blanked before matching. A file under the root that does not parse fails the test rather than falling back to a raw scan. Verified: the commented-interpolation case fails where it passed; an escaped `$${NEWSVC_IMAGE:?}` and an interpolation inside a `#` comment are correctly ignored; and all ten mutations the earlier rounds established still fail -- missing entry, swapped values, duplicate binding, differing tag, a required var in smelt/compose.yml, the bare-? form, an included file named newsvc.yml, a systems/compose.yaml, and an entry dropped from either Go table. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01CGAGAib517Ae1kg8SCdcEt --- smelt/pkg/stack/published_images_test.go | 92 ++++++++++++++++++++++-- 1 file changed, 87 insertions(+), 5 deletions(-) diff --git a/smelt/pkg/stack/published_images_test.go b/smelt/pkg/stack/published_images_test.go index 17afc4e4..57a3c989 100644 --- a/smelt/pkg/stack/published_images_test.go +++ b/smelt/pkg/stack/published_images_test.go @@ -1,15 +1,21 @@ package stack import ( + "go/ast" + "go/parser" + "go/token" + "io" "os" "path/filepath" "reflect" "regexp" "sort" + "strconv" "strings" "testing" "github.com/stretchr/testify/require" + "gopkg.in/yaml.v3" ) // The published image set is stated in four places, and they must agree: @@ -58,6 +64,29 @@ func fieldReachedBy(t *testing.T, mutate func(*config)) string { return found[0] } +// walkYAMLStrings visits every string scalar in a decoded document, keys +// included: compose interpolates throughout, not only in values. +func walkYAMLStrings(n any, fn func(string)) { + switch v := n.(type) { + case string: + fn(v) + case []any: + for _, e := range v { + walkYAMLStrings(e, fn) + } + case map[string]any: + for k, e := range v { + fn(k) + walkYAMLStrings(e, fn) + } + case map[any]any: + for k, e := range v { + walkYAMLStrings(k, fn) + walkYAMLStrings(e, fn) + } + } +} + // requiredImageVars walks the smelt root and returns the required image // variables its YAML and its compose generator name. // @@ -83,14 +112,67 @@ func requiredImageVars(t *testing.T, root string) map[string]bool { re := regexp.MustCompile(`\$\{([A-Z0-9_]+_IMAGE):?\?`) vars := map[string]bool{} - scan := func(path string) { + // PARSED, NOT GREPPED OVER THE RAW BYTES, and the difference is not + // tidiness. A raw scan matches text compose never interpolates -- inside a + // `#` comment, and `$${X:?}`, which compose emits literally. For a name the + // Go tables do not carry that is a false failure, which is safe. For one + // they DO carry it is the opposite: `wantVars` comes from the two Go + // tables, never from what the YAML requires, so a stale match at an + // existing name keeps that name in `required` and hides the drift. Comment + // out an interpolation while pinning the image literally and compose stops + // requiring the variable -- but a raw scan still sees it, and this test + // still passes on an `.env.published` entry nothing needs. + // + // So the earlier claim that "over-reading is the safe direction here" was + // true only for names the tables do not carry. Parsing removes the case + // rather than documenting it. + match := func(v string) { + // `$$` is compose's escape for a literal `$`. Blank it out first so + // `$${X:?}` cannot match; a lone `$` is untouched. + for _, m := range re.FindAllStringSubmatch(strings.ReplaceAll(v, "$$", "\x00\x00"), -1) { + vars[m[1]] = true + } + } + + scanYAML := func(path string) { b, err := os.ReadFile(path) require.NoError(t, err) - for _, m := range re.FindAllStringSubmatch(string(b), -1) { - vars[m[1]] = true + + // Every document: compose files are single-document, but the walk + // reaches config files that need not be. + dec := yaml.NewDecoder(strings.NewReader(string(b))) + for { + var doc any + if err := dec.Decode(&doc); err != nil { + // io.EOF ends the stream. Anything else means a file under + // this root does not parse, which is worth failing on rather + // than falling back to a raw scan and its comment problem. + require.ErrorIs(t, err, io.EOF, "parsing %s", path) + return + } + walkYAMLStrings(doc, match) } } + // The generator is Go, so read its STRING LITERALS rather than its bytes -- + // symmetric with parsing the YAML, and for the same reason: a commented-out + // interpolation in this file would otherwise keep a name alive in + // `required` and mask exactly the drift this test is for. + scanGo := func(path string) { + f, err := parser.ParseFile(token.NewFileSet(), path, nil, 0) + require.NoError(t, err, "parsing %s", path) + ast.Inspect(f, func(n ast.Node) bool { + if lit, ok := n.(*ast.BasicLit); ok && lit.Kind == token.STRING { + if v, err := strconv.Unquote(lit.Value); err == nil { + match(v) + } else { + match(lit.Value) // raw string with no valid unquoting + } + } + return true + }) + } + composeFiles := 0 require.NoError(t, filepath.WalkDir(root, func(path string, d os.DirEntry, err error) error { if err != nil { @@ -105,10 +187,10 @@ func requiredImageVars(t *testing.T, root string) map[string]bool { switch { case strings.HasSuffix(path, ".yml"), strings.HasSuffix(path, ".yaml"): composeFiles++ - scan(path) + scanYAML(path) case strings.HasPrefix(path, filepath.Join(root, "pkg", "generate")) && strings.HasSuffix(path, ".go"): - scan(path) + scanGo(path) } return nil })) From b9423bc03f79686cab0ee946be5b48197434ee9e Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 23 Sep 2026 01:28:24 +0000 Subject: [PATCH 07/13] Round four: the parse rewrite removed two over-reads and introduced a third Round three said a match on text compose does not interpolate keeps a name alive in `required` and masks drift. The fix removed two such cases and left three standing -- one of them added by that same diff. COMPOSE DOES NOT INTERPOLATE MAP KEYS, and the new walker visited them, justified by a comment saying the opposite: "compose interpolates throughout, not only in values". Measured with the client -- a `${X:?}` in a service name, an `environment` key, a `labels` key or a top-level `x-` key leaves `docker compose config` exiting 0 with the variable unset, and the output re-escapes the `$` to `$$`, which is compose saying it treated the text as literal. Keys are no longer visited. THE FILE SET IS NOW WHAT COMPOSE READS, not every YAML under the root. Compose discovers compose.yml/compose.yaml (and the docker-compose spellings) in a directory it is run from -- the root for `make up`, and each systems// for the standalone runs the root file documents -- and reaches anything else only through an `include:`. So the set is those plus their include targets, followed transitively. The blanket scan let a `${HILT_IMAGE:?...}` written into systems/telemetry/config/prometheus.yml, a file compose never opens, mask a hilt image pinned literally. pkg/generate/*_test.go IS EXCLUDED. Nothing in a test file reaches compose, and generate_test.go already asserts on emitted interpolations verbatim ("${SMELT_PIRI_0_PORT:-15100:3000}"); the same assertion for the image line is the obvious next test to write and would have pinned PIRI_IMAGE in `required` for good. And the sentence "over-reading is the safe direction here" is gone rather than qualified. It was still there, four lines above the paragraph in the same function retracting it. Verified over 19 mutations, all correct: the three masking cases above now fail, as does the control that pins swarf with no fake occurrence at all; and the twelve established cases still fail. Two of the four masking rows first read as passes because the mutation was incomplete -- HILT_IMAGE appears twice in systems/hilt/compose.yml and the substitution replaced one. That was the harness, not the code, and it is the second time tonight a mutation that did not apply read as a guard that did not fire. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01CGAGAib517Ae1kg8SCdcEt --- smelt/pkg/stack/published_images_test.go | 218 +++++++++++++++-------- 1 file changed, 147 insertions(+), 71 deletions(-) diff --git a/smelt/pkg/stack/published_images_test.go b/smelt/pkg/stack/published_images_test.go index 57a3c989..0658e247 100644 --- a/smelt/pkg/stack/published_images_test.go +++ b/smelt/pkg/stack/published_images_test.go @@ -64,8 +64,16 @@ func fieldReachedBy(t *testing.T, mutate func(*config)) string { return found[0] } -// walkYAMLStrings visits every string scalar in a decoded document, keys -// included: compose interpolates throughout, not only in values. +// walkYAMLStrings visits every string scalar VALUE in a decoded document. +// +// KEYS ARE NOT VISITED. An earlier revision visited them, justified by +// "compose interpolates throughout, not only in values" -- which is the +// opposite of what compose does. Measured with the client: a `${X:?}` in a +// service name, an `environment` key, a `labels` key or a top-level `x-` key +// leaves `docker compose config` exiting 0 with the variable unset, and the +// output re-escapes the `$` to `$$`, which is compose saying it treated the +// text as a literal. Visiting keys put back exactly the over-reading the +// parse rewrite was written to remove. func walkYAMLStrings(n any, fn func(string)) { switch v := n.(type) { case string: @@ -75,33 +83,80 @@ func walkYAMLStrings(n any, fn func(string)) { walkYAMLStrings(e, fn) } case map[string]any: - for k, e := range v { - fn(k) + for _, e := range v { walkYAMLStrings(e, fn) } case map[any]any: - for k, e := range v { - walkYAMLStrings(k, fn) + for _, e := range v { walkYAMLStrings(e, fn) } } } -// requiredImageVars walks the smelt root and returns the required image -// variables its YAML and its compose generator name. +// composeFileNames are the names `docker compose` discovers on its own in a +// directory it is run from. Anything else only reaches compose by being named +// in an `include:`. +var composeFileNames = map[string]bool{ + "compose.yaml": true, "compose.yml": true, + "docker-compose.yaml": true, "docker-compose.yml": true, +} + +// includePaths returns the `include:` targets a compose document names, +// resolved against its own directory. Entries are a path, a list of paths, or +// a mapping with `path:` holding either. +func includePaths(dir string, doc any) []string { + m, ok := doc.(map[string]any) + if !ok { + return nil + } + list, ok := m["include"].([]any) + if !ok { + return nil + } + var out []string + add := func(v any) { + if p, ok := v.(string); ok { + out = append(out, filepath.Join(dir, p)) + } + } + for _, e := range list { + switch v := e.(type) { + case string: + add(v) + case map[string]any: + switch p := v["path"].(type) { + case string: + add(p) + case []any: + for _, q := range p { + add(q) + } + } + } + } + return out +} + +// requiredImageVars returns the image variables compose requires, from the +// files compose actually reads. // -// ROOTED AT THE SMELT ROOT, not at systems/. smelt/compose.yml is the file -// `make up` resolves and it sits outside systems/; a walk rooted one directory -// deeper would let a required variable added there go unchecked. That is the -// same bug class this test exists for, at the one compose file systems/ does -// not contain. +// THE FILE SET IS WHAT COMPOSE READS, not every YAML under the root. Compose +// discovers `compose.yml`/`compose.yaml` (and the `docker-compose` spellings) +// in a directory it is run from -- the root for `make up`, and each +// systems// for the standalone runs the root file documents -- and reaches +// anything else only through an `include:`. So the set is those, plus their +// include targets transitively. // -// EVERY .yml AND .yaml, not files named compose.yml. The root file pulls in -// its parts with `include:`, whose paths it names explicitly, so an included -// file called anything else would be missed -- and `compose.yaml` is compose's -// own auto-discovered default. Over-reading is the safe direction here and -// costs nothing measurable: across every YAML file under this root, the -// pattern matches only compose files. +// AN EARLIER REVISION SCANNED EVERY .yml AND .yaml, on the grounds that +// over-reading was the safe direction. It is not, and the same function said +// so a few lines down: `wantVars` comes from the two Go tables and never from +// what the YAML requires, so a match on text compose does not interpolate +// keeps a name alive in `required` and hides the drift. Under a blanket scan, +// a `${HILT_IMAGE:?...}` written into systems/telemetry/config/prometheus.yml +// -- a file compose never opens -- would mask a hilt image pinned literally. +// +// ROOTED AT THE SMELT ROOT, not at systems/: smelt/compose.yml is the file +// `make up` resolves and it sits outside systems/. func requiredImageVars(t *testing.T, root string) map[string]bool { t.Helper() // `:?` and `?` are BOTH required forms. `${X:?msg}` errors when X is unset @@ -112,90 +167,111 @@ func requiredImageVars(t *testing.T, root string) map[string]bool { re := regexp.MustCompile(`\$\{([A-Z0-9_]+_IMAGE):?\?`) vars := map[string]bool{} - // PARSED, NOT GREPPED OVER THE RAW BYTES, and the difference is not - // tidiness. A raw scan matches text compose never interpolates -- inside a - // `#` comment, and `$${X:?}`, which compose emits literally. For a name the - // Go tables do not carry that is a false failure, which is safe. For one - // they DO carry it is the opposite: `wantVars` comes from the two Go - // tables, never from what the YAML requires, so a stale match at an - // existing name keeps that name in `required` and hides the drift. Comment - // out an interpolation while pinning the image literally and compose stops - // requiring the variable -- but a raw scan still sees it, and this test - // still passes on an `.env.published` entry nothing needs. - // - // So the earlier claim that "over-reading is the safe direction here" was - // true only for names the tables do not carry. Parsing removes the case - // rather than documenting it. match := func(v string) { // `$$` is compose's escape for a literal `$`. Blank it out first so - // `$${X:?}` cannot match; a lone `$` is untouched. + // `$${X:?}` cannot match; a lone `$` is untouched. Left-to-right + // pairing matches compose's own lexer, checked against the client for + // runs of one to five dollars. for _, m := range re.FindAllStringSubmatch(strings.ReplaceAll(v, "$$", "\x00\x00"), -1) { vars[m[1]] = true } } - scanYAML := func(path string) { + // PARSED, NOT GREPPED OVER THE RAW BYTES, and the difference is not + // tidiness: a raw scan matches inside a `#` comment, so commenting out an + // interpolation while pinning the image literally left this test green on + // an .env.published entry nothing needed. + parse := func(path string) []any { b, err := os.ReadFile(path) require.NoError(t, err) - - // Every document: compose files are single-document, but the walk - // reaches config files that need not be. + var docs []any dec := yaml.NewDecoder(strings.NewReader(string(b))) for { var doc any if err := dec.Decode(&doc); err != nil { - // io.EOF ends the stream. Anything else means a file under - // this root does not parse, which is worth failing on rather - // than falling back to a raw scan and its comment problem. + // Anything but a clean end of stream means a file compose + // reads does not parse, which is worth failing on rather than + // falling back to a raw scan and its comment problem. require.ErrorIs(t, err, io.EOF, "parsing %s", path) - return + return docs } + docs = append(docs, doc) + } + } + + // Seed with every auto-discoverable name under the root, then follow + // include: to a fixpoint. + seen := map[string]bool{} + var queue []string + require.NoError(t, filepath.WalkDir(root, func(path string, d os.DirEntry, err error) error { + if err != nil { + return err + } + if !d.IsDir() && composeFileNames[d.Name()] { + queue = append(queue, path) + } + return nil + })) + require.NotEmpty(t, queue, "found no compose file under %s", root) + + for len(queue) > 0 { + path := queue[0] + queue = queue[1:] + abs, err := filepath.Abs(path) + require.NoError(t, err) + if seen[abs] { + continue + } + seen[abs] = true + + for _, doc := range parse(path) { walkYAMLStrings(doc, match) + for _, inc := range includePaths(filepath.Dir(path), doc) { + // An include naming a file that is not there is compose's + // error to report, not this test's. + if _, err := os.Stat(inc); err == nil { + queue = append(queue, inc) + } + } } } + // PIRI_IMAGE appears in no compose file at all -- pkg/generate emits piri's + // service definition -- so the generator is a second source and a scan of + // compose files alone reports seven of eight. + // + // ITS TESTS ARE EXCLUDED. Nothing in a _test.go reaches compose, and + // generate_test.go already asserts on emitted interpolations verbatim + // ("${SMELT_PIRI_0_PORT:-15100:3000}"); the same assertion for the image + // line is the obvious next test to write, and would pin PIRI_IMAGE in + // `required` for good. + // // The generator is Go, so read its STRING LITERALS rather than its bytes -- // symmetric with parsing the YAML, and for the same reason: a commented-out - // interpolation in this file would otherwise keep a name alive in - // `required` and mask exactly the drift this test is for. - scanGo := func(path string) { - f, err := parser.ParseFile(token.NewFileSet(), path, nil, 0) - require.NoError(t, err, "parsing %s", path) + // interpolation would otherwise mask the same drift. + gen := filepath.Join(root, "pkg", "generate") + require.NoError(t, filepath.WalkDir(gen, func(path string, d os.DirEntry, err error) error { + if err != nil { + return err + } + if d.IsDir() || !strings.HasSuffix(path, ".go") || strings.HasSuffix(path, "_test.go") { + return nil + } + f, perr := parser.ParseFile(token.NewFileSet(), path, nil, 0) + require.NoError(t, perr, "parsing %s", path) ast.Inspect(f, func(n ast.Node) bool { if lit, ok := n.(*ast.BasicLit); ok && lit.Kind == token.STRING { - if v, err := strconv.Unquote(lit.Value); err == nil { + if v, uerr := strconv.Unquote(lit.Value); uerr == nil { match(v) } else { - match(lit.Value) // raw string with no valid unquoting + match(lit.Value) } } return true }) - } - - composeFiles := 0 - require.NoError(t, filepath.WalkDir(root, func(path string, d os.DirEntry, err error) error { - if err != nil { - return err - } - if d.IsDir() { - return nil - } - // PIRI_IMAGE appears in no compose file at all -- pkg/generate emits - // piri's service definition -- so the generator is a second source and - // a glob over compose files alone reports seven of eight. - switch { - case strings.HasSuffix(path, ".yml"), strings.HasSuffix(path, ".yaml"): - composeFiles++ - scanYAML(path) - case strings.HasPrefix(path, filepath.Join(root, "pkg", "generate")) && - strings.HasSuffix(path, ".go"): - scanGo(path) - } return nil })) - require.NotZero(t, composeFiles, "found no YAML under %s", root) require.NotEmpty(t, vars, "found no required ${X_IMAGE:?} interpolations") return vars } From 1c27d270b74badb5410e475778aa4f4f40f70468 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 23 Sep 2026 01:49:22 +0000 Subject: [PATCH 08/13] Round five: the auto-discovery list was short, and a missing include passed Five findings. Three were holes in the guard; two were claims. Nothing was live-broken -- the derived set is exactly the eight compose requires today -- but each hole is a way this could stop noticing. THE AUTO-DISCOVERY LIST OMITTED THE OVERRIDE NAMES, and a comment called it complete. Compose loads compose.override.{yml,yaml} and the docker-compose spellings alongside the base file, and a `${X:?}` in one is required just the same (rc=1 naming the variable). That is the live `make up` path: COMPOSE passes no `-f` unless the workspace override exists, and `-f` is what suppresses auto-discovery. `extends: file:` IS A SECOND WAY COMPOSE READS A FILE, which the previous comment denied outright -- "reaches anything else only through an include:". Nothing in this tree uses it; it is covered because the claim was one of totality and was wrong. A MISSING INCLUDE TARGET WAS SKIPPED, silently. Compose fails the whole project on one (rc=1, "no such file or directory") while this test went green having dropped every variable that file named: mistyping systems/swarf as systems/swraf in the root include loses SWARF_IMAGE from `required` and passes. Now fatal, with one named exception -- generated/, which is gitignored output of `go run ./cmd/smelt generate` and is the only absence the skip was ever for. The old comment did not say that, so nobody could tighten it safely. AND A MEASUREMENT NAMED A CASE IT HAD NOT RUN. The key comment cited four positions where compose leaves a `${X:?}` literal; three reproduce exactly, but a SERVICE NAME exits 1 with "services additional properties '...' not allowed" -- schema validation, before interpolation. The conclusion was right and one quarter of its evidence was invented. Two limits are now stated rather than left implied, since the surrounding comment would otherwise read as total: `include.project_directory` redirects how an included file's own nested includes resolve, and `include.env_file` can supply a required variable. Neither is in this tree; both are left uncovered rather than half-covered. Verified over 11 cases including the three new ones, every mutation asserting it applied: a required var in compose.override.yml, one reached through extends: file:, and a mistyped include all fail now; the baseline still passes with generated/compose/piri.yml absent. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01CGAGAib517Ae1kg8SCdcEt --- smelt/pkg/stack/published_images_test.go | 96 +++++++++++++++++++++--- 1 file changed, 85 insertions(+), 11 deletions(-) diff --git a/smelt/pkg/stack/published_images_test.go b/smelt/pkg/stack/published_images_test.go index 0658e247..95e6373f 100644 --- a/smelt/pkg/stack/published_images_test.go +++ b/smelt/pkg/stack/published_images_test.go @@ -68,12 +68,17 @@ func fieldReachedBy(t *testing.T, mutate func(*config)) string { // // KEYS ARE NOT VISITED. An earlier revision visited them, justified by // "compose interpolates throughout, not only in values" -- which is the -// opposite of what compose does. Measured with the client: a `${X:?}` in a -// service name, an `environment` key, a `labels` key or a top-level `x-` key -// leaves `docker compose config` exiting 0 with the variable unset, and the -// output re-escapes the `$` to `$$`, which is compose saying it treated the -// text as a literal. Visiting keys put back exactly the over-reading the -// parse rewrite was written to remove. +// opposite of what compose does. Measured with the client: a `${X:?}` in an +// `environment` key, a `labels` key or a top-level `x-` key leaves +// `docker compose config` exiting 0 with the variable unset, and the output +// re-escapes the `$` to `$$` -- compose saying it treated the text as a +// literal. Visiting keys put back exactly the over-reading the parse rewrite +// was written to remove. +// +// An earlier revision named a SERVICE NAME as a fourth such position. It is +// not one: compose exits 1 there with "services additional properties '...' +// not allowed", schema validation refusing the name before interpolation is +// reached. The conclusion held; one of the four cases cited had not been run. func walkYAMLStrings(n any, fn func(string)) { switch v := n.(type) { case string: @@ -94,11 +99,20 @@ func walkYAMLStrings(n any, fn func(string)) { } // composeFileNames are the names `docker compose` discovers on its own in a -// directory it is run from. Anything else only reaches compose by being named -// in an `include:`. +// directory it is run from. +// +// THE OVERRIDE NAMES COUNT. An earlier revision listed only the four base +// names and called that the complete auto-discovery list. Compose also loads +// `compose.override.{yml,yaml}` and `docker-compose.override.{yml,yaml}` +// alongside them, and a `${X:?}` in one is required exactly as in the base +// file -- verified with the client, rc=1 naming the variable. That is the live +// `make up` path: smelt's COMPOSE passes no `-f` unless the workspace override +// exists, and `-f` is what suppresses auto-discovery. var composeFileNames = map[string]bool{ "compose.yaml": true, "compose.yml": true, + "compose.override.yaml": true, "compose.override.yml": true, "docker-compose.yaml": true, "docker-compose.yml": true, + "docker-compose.override.yaml": true, "docker-compose.override.yml": true, } // includePaths returns the `include:` targets a compose document names, @@ -137,6 +151,38 @@ func includePaths(dir string, doc any) []string { return out } +// extendsPaths returns the files a document's services pull in with +// `extends: {file: ...}`. A SECOND WAY COMPOSE READS A FILE, which an earlier +// revision's comment denied by saying it "reaches anything else only through +// an include:". Verified with the client: a required variable in an +// extends.file target is required. Nothing in this tree uses it today; it is +// covered because the comment claimed totality and the claim was wrong. +func extendsPaths(dir string, doc any) []string { + m, ok := doc.(map[string]any) + if !ok { + return nil + } + services, ok := m["services"].(map[string]any) + if !ok { + return nil + } + var out []string + for _, svc := range services { + sm, ok := svc.(map[string]any) + if !ok { + continue + } + ext, ok := sm["extends"].(map[string]any) + if !ok { + continue + } + if f, ok := ext["file"].(string); ok { + out = append(out, filepath.Join(dir, f)) + } + } + return out +} + // requiredImageVars returns the image variables compose requires, from the // files compose actually reads. // @@ -157,6 +203,14 @@ func includePaths(dir string, doc any) []string { // // ROOTED AT THE SMELT ROOT, not at systems/: smelt/compose.yml is the file // `make up` resolves and it sits outside systems/. +// +// TWO KNOWN LIMITS, stated because the comment above would otherwise read as +// total and rule 5 is about exactly that. `include:` accepts a +// `project_directory:` that redirects how the included file's OWN nested +// includes resolve, and this reads them relative to the included file instead. +// And `include.env_file:` can supply a required variable, which this does not +// read -- that one errs towards a false failure. Neither appears in this tree; +// both are left uncovered rather than half-covered. func requiredImageVars(t *testing.T, root string) map[string]bool { t.Helper() // `:?` and `?` are BOTH required forms. `${X:?msg}` errors when X is unset @@ -226,12 +280,32 @@ func requiredImageVars(t *testing.T, root string) map[string]bool { for _, doc := range parse(path) { walkYAMLStrings(doc, match) - for _, inc := range includePaths(filepath.Dir(path), doc) { - // An include naming a file that is not there is compose's - // error to report, not this test's. + for _, inc := range append(includePaths(filepath.Dir(path), doc), + extendsPaths(filepath.Dir(path), doc)...) { if _, err := os.Stat(inc); err == nil { queue = append(queue, inc) + continue } + // A MISSING TARGET IS FATAL, with one named exception. An + // earlier revision skipped any absent path "because that is + // compose's error to report" -- but compose reports it by + // failing the whole project (rc=1, "no such file or + // directory"), while this test went green having quietly + // dropped every variable that file named. Mistyping + // systems/swarf as systems/swraf in the root include loses + // SWARF_IMAGE from `required` and passes. + // + // The exception is generated/, which is gitignored build + // output from `go run ./cmd/smelt generate`; the root include + // names generated/compose/piri.yml and it is absent in a clean + // checkout. That is the only absence this skip was ever for, + // which the earlier comment did not say, so nobody could + // tighten it safely. + rel, rerr := filepath.Rel(root, inc) + require.NoError(t, rerr) + require.True(t, strings.HasPrefix(rel, "generated"+string(filepath.Separator)), + "%s includes %s, which does not exist; compose fails the whole project on that", + path, inc) } } } From 7f04a8b8ffeeeb80a0194e2f21a191a3e891da5d Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 23 Sep 2026 02:14:29 +0000 Subject: [PATCH 09/13] Round six: 285 of the test's 433 lines were not load-bearing, so they are gone Six rounds on this check, every one finding the same shape of defect: the deriver reading something other than what its consumer reads. Sets instead of pairs; one required form and one filename instead of both; raw bytes instead of parsed values; map keys and non-compose files; a short auto-discovery list. Each fix was right. The RATE was the signal, and this round asked whether the shape was, rather than hunting another corner. IT WAS NOT. Measured by stubbing requiredImageVars out to a constant: the original bug -- INDEXER_IMAGE absent from .env.published -- and round one's headline catch, PIRI and HILT swapped, BOTH still fail. The expected set comes from the two Go tables and never from the compose files, so 285 lines reimplementing compose's grammar bought nothing for the drift this exists to catch. They defended a stricter property, "compose requires exactly this set and no more", and that property is what cost six rounds. It also still had a live hole at round six: a string literal in pkg/generate/compose.go that the generator never emits counted as required, which is round three's masking class, fixed for YAML and open for Go. Reading less closes it by construction -- there is nothing left to mask. So the test is now what it needed to be all along: .env.published, publishedImages and envImageOptions checked against each other, name by name and reference by reference, pairing a variable to a reference by BEHAVIOUR so a renamed field cannot break the join silently. 144 lines, no compose knowledge, no parsing, no Docker. Verified over 9 cases: the original bug, swapped values, a duplicate binding, a differing tag, an entry dropped from either Go table -- all still fail. The one case now unchecked, a required variable added to a compose file and to neither Go table, passes, and is recorded as given up rather than left to be discovered. If that stricter property is wanted, the test's comment says to ask compose rather than reimplement it -- `docker compose config` with an empty environment, read back the variable it names as missing, repeat. Exact by construction, no daemon, ~1.4s over 9 invocations, at the cost of depending on the compose binary in `unit smelt`. And not `--variables`, which under-reports. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01CGAGAib517Ae1kg8SCdcEt --- smelt/.env.published | 16 +- smelt/CLAUDE.md | 2 +- smelt/pkg/stack/options.go | 5 +- smelt/pkg/stack/published_images_test.go | 373 +++-------------------- 4 files changed, 57 insertions(+), 339 deletions(-) diff --git a/smelt/.env.published b/smelt/.env.published index 2e8abc0e..aba2d55f 100644 --- a/smelt/.env.published +++ b/smelt/.env.published @@ -27,11 +27,17 @@ # `make up` was broken for as long as the indexer had been in the stack. # Nothing in CI runs `make up`, so "not silent" and "not noticed" were the # same thing. `TestPublishedImageSetAgrees` in pkg/stack is what makes the -# claim true now. It is a Go test rather than a check-*.sh because the first -# attempt was a shell script that compared the lists as SETS, and so reported -# agreement on a tree where PIRI_IMAGE and HILT_IMAGE had been swapped here — -# each service booting the other's image. Pairing a variable to a reference -# means knowing which config field each reaches, which is Go semantics. +# claim true now: it checks this file against publishedImages and +# envImageOptions, name by name AND reference by reference. The pairwise part +# matters — the first attempt compared the lists as SETS and so reported +# agreement on a tree where PIRI_IMAGE and HILT_IMAGE had been swapped here, +# each service booting the other's image. +# +# It does NOT read the compose files, so it does not check that compose +# requires exactly this set. A revision that did cost six review rounds +# reimplementing compose's grammar, and was measured not to be load-bearing +# for the drift above. The test's own comment says what that gives up and how +# to get it back. PIRI_IMAGE=ghcr.io/fil-forge/piri:main HILT_IMAGE=ghcr.io/fil-forge/hilt:main diff --git a/smelt/CLAUDE.md b/smelt/CLAUDE.md index 1f2d4fa5..76ff0d46 100644 --- a/smelt/CLAUDE.md +++ b/smelt/CLAUDE.md @@ -471,7 +471,7 @@ Where the defaults come from depends on whether this repository builds the image | sed -E 's/\$\{([A-Z_]+):\?/\1/' | sort -u ``` - That misses `PIRI_IMAGE`, which appears in no compose file — `pkg/generate/compose.go` emits piri's service definition. `TestPublishedImageSetAgrees` in `pkg/stack` does the whole derivation and fails when the compose interpolations, `.env.published`, `publishedImages` and `envImageOptions` disagree — including when two of them name *different* images for the same service. + That misses `PIRI_IMAGE`, which appears in no compose file — `pkg/generate/compose.go` emits piri's service definition. `TestPublishedImageSetAgrees` in `pkg/stack` checks `.env.published`, `publishedImages` and `envImageOptions` against each other — name by name and reference by reference, so it catches two of them naming *different* images for the same service. It deliberately does not read the compose files; its own comment says what that gives up and how to get it back. - **Not built here** — guppy, ipni, plc, blockchain, minio. These keep an inline `:-` default in their `systems/*/compose.yml`, because nothing in this repository can produce an alternative, and they are pinned by digest instead (`.github/scripts/check-stack-images.sh`). `.env` is yours: every line in it is commented out, and `make up` loads it *after* `.env.published`, so anything you uncomment there wins. diff --git a/smelt/pkg/stack/options.go b/smelt/pkg/stack/options.go index b038871e..e5c7c7ff 100644 --- a/smelt/pkg/stack/options.go +++ b/smelt/pkg/stack/options.go @@ -412,8 +412,9 @@ var envImageOptions = []struct { // `compose up` naming the variable. That is true and it is not enough: they // drifted anyway — indexing-service was added here and not to .env.published, // and `make up` had been failing ever since, because nothing in CI runs it. -// TestPublishedImageSetAgrees is what checks it now, and it checks all four -// statements of this set against each other, including envImageOptions above. +// TestPublishedImageSetAgrees is what checks it now: this table against +// .env.published and envImageOptions above, name by name and reference by +// reference. // // Only what this repository builds; the rest keep their `:-` compose defaults, // so there is nothing for this to fill in. That set is not listed here — it diff --git a/smelt/pkg/stack/published_images_test.go b/smelt/pkg/stack/published_images_test.go index 95e6373f..fcaae8ca 100644 --- a/smelt/pkg/stack/published_images_test.go +++ b/smelt/pkg/stack/published_images_test.go @@ -1,27 +1,18 @@ package stack import ( - "go/ast" - "go/parser" - "go/token" - "io" "os" "path/filepath" "reflect" - "regexp" "sort" - "strconv" "strings" "testing" "github.com/stretchr/testify/require" - "gopkg.in/yaml.v3" ) -// The published image set is stated in four places, and they must agree: +// The published image set is stated in three places, and they must agree: // -// - the compose files, which require each one (`${HILT_IMAGE:?...}`) rather -// than defaulting it, so a caller who forgets an override fails loudly; // - `.env.published`, which supplies them to `make up`; // - publishedImages, which supplies them to WithPublishedImages(); // - envImageOptions, which lets a Go caller override them by the same names. @@ -30,17 +21,36 @@ import ( // not to .env.published, so `make up` failed at `compose up` with INDEXER_IMAGE // unset, for as long as the indexer had been in the stack. Three comments in // this repository asserted that could not happen silently, on the grounds that -// a missing entry fails at `compose up`. It does — and nothing in CI runs +// a missing entry fails at `compose up`. It does -- and nothing in CI runs // `make up`, so "not silent" and "not noticed" were the same thing. // -// WHY THIS IS A GO TEST AND NOT A check-*.sh. The first version was a shell -// script, and it compared the lists as SETS: variable names in one comparison, -// image references in the other, never the pairing between them. It reported -// agreement, and exited 0, on a tree where PIRI_IMAGE and HILT_IMAGE had been -// swapped in .env.published -- each service booting the other's image. Closing -// that in shell means recovering which config field each variable and each -// reference reaches, which is Go semantics, so it belongs here. Both tables are -// already data in this package; the test reads them rather than parsing them. +// WHAT THIS DELIBERATELY DOES NOT CHECK, and how to get it back. It does not +// read the compose files, so it does not assert that compose requires exactly +// this set and no more. An earlier revision did, and the cost was not worth it: +// six review rounds, every one finding the same defect -- a deriver reading +// something other than what its consumer reads. It compared the lists as sets +// rather than pairs; matched only `${X:?}` and not `${X?}`; missed `include:` +// targets and `compose.yaml`; grepped raw bytes, so `#` comments and `$$` +// escapes counted; visited YAML map keys, which compose never interpolates; +// read non-compose YAML and the generator's own tests; and omitted the four +// `*.override.*` names compose also auto-discovers. Each fix was right and the +// rate was the signal: 285 of its 433 lines reimplemented a slice of compose's +// grammar, and compose's grammar is bigger than what was written down. +// +// It was also not load-bearing. Measured by stubbing that derivation out to a +// constant: the original bug (INDEXER_IMAGE absent from .env.published) and the +// swapped-values case both still fail, because the expected set comes from the +// two Go tables and never from the compose files. +// +// If the stricter property is wanted, ask compose rather than reimplementing +// it: run `docker compose config` with an empty environment and read back the +// variable it names as missing, repeating until it stops. That is exact by +// construction, needs no daemon, and was measured at ~1.4s over 9 invocations +// at the smelt root. It costs a dependency on the compose binary in +// `unit smelt`, which is the trade to weigh. Do NOT use `docker compose config +// --variables`: it under-reports, missing a required variable in a +// non-extended service of an `extends.file` target that compose itself +// enforces. const imageSentinel = "SENTINEL-published-images-test" @@ -64,292 +74,6 @@ func fieldReachedBy(t *testing.T, mutate func(*config)) string { return found[0] } -// walkYAMLStrings visits every string scalar VALUE in a decoded document. -// -// KEYS ARE NOT VISITED. An earlier revision visited them, justified by -// "compose interpolates throughout, not only in values" -- which is the -// opposite of what compose does. Measured with the client: a `${X:?}` in an -// `environment` key, a `labels` key or a top-level `x-` key leaves -// `docker compose config` exiting 0 with the variable unset, and the output -// re-escapes the `$` to `$$` -- compose saying it treated the text as a -// literal. Visiting keys put back exactly the over-reading the parse rewrite -// was written to remove. -// -// An earlier revision named a SERVICE NAME as a fourth such position. It is -// not one: compose exits 1 there with "services additional properties '...' -// not allowed", schema validation refusing the name before interpolation is -// reached. The conclusion held; one of the four cases cited had not been run. -func walkYAMLStrings(n any, fn func(string)) { - switch v := n.(type) { - case string: - fn(v) - case []any: - for _, e := range v { - walkYAMLStrings(e, fn) - } - case map[string]any: - for _, e := range v { - walkYAMLStrings(e, fn) - } - case map[any]any: - for _, e := range v { - walkYAMLStrings(e, fn) - } - } -} - -// composeFileNames are the names `docker compose` discovers on its own in a -// directory it is run from. -// -// THE OVERRIDE NAMES COUNT. An earlier revision listed only the four base -// names and called that the complete auto-discovery list. Compose also loads -// `compose.override.{yml,yaml}` and `docker-compose.override.{yml,yaml}` -// alongside them, and a `${X:?}` in one is required exactly as in the base -// file -- verified with the client, rc=1 naming the variable. That is the live -// `make up` path: smelt's COMPOSE passes no `-f` unless the workspace override -// exists, and `-f` is what suppresses auto-discovery. -var composeFileNames = map[string]bool{ - "compose.yaml": true, "compose.yml": true, - "compose.override.yaml": true, "compose.override.yml": true, - "docker-compose.yaml": true, "docker-compose.yml": true, - "docker-compose.override.yaml": true, "docker-compose.override.yml": true, -} - -// includePaths returns the `include:` targets a compose document names, -// resolved against its own directory. Entries are a path, a list of paths, or -// a mapping with `path:` holding either. -func includePaths(dir string, doc any) []string { - m, ok := doc.(map[string]any) - if !ok { - return nil - } - list, ok := m["include"].([]any) - if !ok { - return nil - } - var out []string - add := func(v any) { - if p, ok := v.(string); ok { - out = append(out, filepath.Join(dir, p)) - } - } - for _, e := range list { - switch v := e.(type) { - case string: - add(v) - case map[string]any: - switch p := v["path"].(type) { - case string: - add(p) - case []any: - for _, q := range p { - add(q) - } - } - } - } - return out -} - -// extendsPaths returns the files a document's services pull in with -// `extends: {file: ...}`. A SECOND WAY COMPOSE READS A FILE, which an earlier -// revision's comment denied by saying it "reaches anything else only through -// an include:". Verified with the client: a required variable in an -// extends.file target is required. Nothing in this tree uses it today; it is -// covered because the comment claimed totality and the claim was wrong. -func extendsPaths(dir string, doc any) []string { - m, ok := doc.(map[string]any) - if !ok { - return nil - } - services, ok := m["services"].(map[string]any) - if !ok { - return nil - } - var out []string - for _, svc := range services { - sm, ok := svc.(map[string]any) - if !ok { - continue - } - ext, ok := sm["extends"].(map[string]any) - if !ok { - continue - } - if f, ok := ext["file"].(string); ok { - out = append(out, filepath.Join(dir, f)) - } - } - return out -} - -// requiredImageVars returns the image variables compose requires, from the -// files compose actually reads. -// -// THE FILE SET IS WHAT COMPOSE READS, not every YAML under the root. Compose -// discovers `compose.yml`/`compose.yaml` (and the `docker-compose` spellings) -// in a directory it is run from -- the root for `make up`, and each -// systems// for the standalone runs the root file documents -- and reaches -// anything else only through an `include:`. So the set is those, plus their -// include targets transitively. -// -// AN EARLIER REVISION SCANNED EVERY .yml AND .yaml, on the grounds that -// over-reading was the safe direction. It is not, and the same function said -// so a few lines down: `wantVars` comes from the two Go tables and never from -// what the YAML requires, so a match on text compose does not interpolate -// keeps a name alive in `required` and hides the drift. Under a blanket scan, -// a `${HILT_IMAGE:?...}` written into systems/telemetry/config/prometheus.yml -// -- a file compose never opens -- would mask a hilt image pinned literally. -// -// ROOTED AT THE SMELT ROOT, not at systems/: smelt/compose.yml is the file -// `make up` resolves and it sits outside systems/. -// -// TWO KNOWN LIMITS, stated because the comment above would otherwise read as -// total and rule 5 is about exactly that. `include:` accepts a -// `project_directory:` that redirects how the included file's OWN nested -// includes resolve, and this reads them relative to the included file instead. -// And `include.env_file:` can supply a required variable, which this does not -// read -- that one errs towards a false failure. Neither appears in this tree; -// both are left uncovered rather than half-covered. -func requiredImageVars(t *testing.T, root string) map[string]bool { - t.Helper() - // `:?` and `?` are BOTH required forms. `${X:?msg}` errors when X is unset - // or empty; `${X?msg}` errors only when unset. Compose rejects either the - // same way -- "required variable NEWTHING_IMAGE is missing a value" -- - // and an earlier version of this pattern matched only the first, so the - // other spelling of the same line left this test green. - re := regexp.MustCompile(`\$\{([A-Z0-9_]+_IMAGE):?\?`) - vars := map[string]bool{} - - match := func(v string) { - // `$$` is compose's escape for a literal `$`. Blank it out first so - // `$${X:?}` cannot match; a lone `$` is untouched. Left-to-right - // pairing matches compose's own lexer, checked against the client for - // runs of one to five dollars. - for _, m := range re.FindAllStringSubmatch(strings.ReplaceAll(v, "$$", "\x00\x00"), -1) { - vars[m[1]] = true - } - } - - // PARSED, NOT GREPPED OVER THE RAW BYTES, and the difference is not - // tidiness: a raw scan matches inside a `#` comment, so commenting out an - // interpolation while pinning the image literally left this test green on - // an .env.published entry nothing needed. - parse := func(path string) []any { - b, err := os.ReadFile(path) - require.NoError(t, err) - var docs []any - dec := yaml.NewDecoder(strings.NewReader(string(b))) - for { - var doc any - if err := dec.Decode(&doc); err != nil { - // Anything but a clean end of stream means a file compose - // reads does not parse, which is worth failing on rather than - // falling back to a raw scan and its comment problem. - require.ErrorIs(t, err, io.EOF, "parsing %s", path) - return docs - } - docs = append(docs, doc) - } - } - - // Seed with every auto-discoverable name under the root, then follow - // include: to a fixpoint. - seen := map[string]bool{} - var queue []string - require.NoError(t, filepath.WalkDir(root, func(path string, d os.DirEntry, err error) error { - if err != nil { - return err - } - if !d.IsDir() && composeFileNames[d.Name()] { - queue = append(queue, path) - } - return nil - })) - require.NotEmpty(t, queue, "found no compose file under %s", root) - - for len(queue) > 0 { - path := queue[0] - queue = queue[1:] - abs, err := filepath.Abs(path) - require.NoError(t, err) - if seen[abs] { - continue - } - seen[abs] = true - - for _, doc := range parse(path) { - walkYAMLStrings(doc, match) - for _, inc := range append(includePaths(filepath.Dir(path), doc), - extendsPaths(filepath.Dir(path), doc)...) { - if _, err := os.Stat(inc); err == nil { - queue = append(queue, inc) - continue - } - // A MISSING TARGET IS FATAL, with one named exception. An - // earlier revision skipped any absent path "because that is - // compose's error to report" -- but compose reports it by - // failing the whole project (rc=1, "no such file or - // directory"), while this test went green having quietly - // dropped every variable that file named. Mistyping - // systems/swarf as systems/swraf in the root include loses - // SWARF_IMAGE from `required` and passes. - // - // The exception is generated/, which is gitignored build - // output from `go run ./cmd/smelt generate`; the root include - // names generated/compose/piri.yml and it is absent in a clean - // checkout. That is the only absence this skip was ever for, - // which the earlier comment did not say, so nobody could - // tighten it safely. - rel, rerr := filepath.Rel(root, inc) - require.NoError(t, rerr) - require.True(t, strings.HasPrefix(rel, "generated"+string(filepath.Separator)), - "%s includes %s, which does not exist; compose fails the whole project on that", - path, inc) - } - } - } - - // PIRI_IMAGE appears in no compose file at all -- pkg/generate emits piri's - // service definition -- so the generator is a second source and a scan of - // compose files alone reports seven of eight. - // - // ITS TESTS ARE EXCLUDED. Nothing in a _test.go reaches compose, and - // generate_test.go already asserts on emitted interpolations verbatim - // ("${SMELT_PIRI_0_PORT:-15100:3000}"); the same assertion for the image - // line is the obvious next test to write, and would pin PIRI_IMAGE in - // `required` for good. - // - // The generator is Go, so read its STRING LITERALS rather than its bytes -- - // symmetric with parsing the YAML, and for the same reason: a commented-out - // interpolation would otherwise mask the same drift. - gen := filepath.Join(root, "pkg", "generate") - require.NoError(t, filepath.WalkDir(gen, func(path string, d os.DirEntry, err error) error { - if err != nil { - return err - } - if d.IsDir() || !strings.HasSuffix(path, ".go") || strings.HasSuffix(path, "_test.go") { - return nil - } - f, perr := parser.ParseFile(token.NewFileSet(), path, nil, 0) - require.NoError(t, perr, "parsing %s", path) - ast.Inspect(f, func(n ast.Node) bool { - if lit, ok := n.(*ast.BasicLit); ok && lit.Kind == token.STRING { - if v, uerr := strconv.Unquote(lit.Value); uerr == nil { - match(v) - } else { - match(lit.Value) - } - } - return true - }) - return nil - })) - - require.NotEmpty(t, vars, "found no required ${X_IMAGE:?} interpolations") - return vars -} - func readEnvPublished(t *testing.T, path string) map[string]string { t.Helper() b, err := os.ReadFile(path) @@ -364,8 +88,8 @@ func readEnvPublished(t *testing.T, path string) map[string]string { k, v, ok := strings.Cut(line, "=") require.True(t, ok, "%s:%d: not KEY=VALUE: %q", path, i+1, line) // A repeat binding is a real defect and invisible to any set - // comparison: the earlier shell version passed on a file with - // PIRI_IMAGE bound twice to different images. + // comparison: an earlier shell version of this check passed on a file + // with PIRI_IMAGE bound twice to different images. _, dup := out[k] require.False(t, dup, "%s:%d: %s is set more than once", path, i+1, k) out[k] = v @@ -374,9 +98,7 @@ func readEnvPublished(t *testing.T, path string) map[string]string { } func TestPublishedImageSetAgrees(t *testing.T) { - root := filepath.Join("..", "..") - - // Pair each compose variable and each published reference to the config + // Pair each override variable and each published reference to the config // field it reaches, then join on that field. varToField := map[string]string{} for _, e := range envImageOptions { @@ -390,19 +112,8 @@ func TestPublishedImageSetAgrees(t *testing.T) { fieldToRef[f] = p.ref } - required := requiredImageVars(t, root) - env := readEnvPublished(t, filepath.Join(root, ".env.published")) - - // 1. Every required compose variable must be overridable from Go, or a - // caller can set it for `docker compose` and not for a Go test. - for v := range required { - require.Contains(t, varToField, v, - "%s is a required compose interpolation but is not in envImageOptions", v) - } - - // 2. Required, supplied by .env.published, and present in publishedImages - // must be the same set -- and the REFERENCE each names must match, - // which is what a set comparison cannot see. + // The images this repository builds are those with both an override option + // and a published reference. var wantVars []string for v, f := range varToField { if _, built := fieldToRef[f]; built { @@ -410,22 +121,22 @@ func TestPublishedImageSetAgrees(t *testing.T) { } } sort.Strings(wantVars) + require.NotEmpty(t, wantVars, "no image has both an option and a published reference") - var gotRequired, gotEnv []string - for v := range required { - gotRequired = append(gotRequired, v) - } + env := readEnvPublished(t, filepath.Join("..", "..", ".env.published")) + var gotEnv []string for v := range env { gotEnv = append(gotEnv, v) } - sort.Strings(gotRequired) sort.Strings(gotEnv) - require.Equal(t, wantVars, gotRequired, - "the compose files and publishedImages disagree about which images this repository builds") require.Equal(t, wantVars, gotEnv, - ".env.published does not supply exactly the required images") + ".env.published does not supply exactly the images this repository builds") + // Comparing the REFERENCES pairwise, not the two sets. A set comparison + // passes on a file where PIRI_IMAGE and HILT_IMAGE have been swapped -- + // each service booting the other's image -- which is how the shell version + // of this check reported agreement on a broken tree. for _, v := range wantVars { require.Equal(t, fieldToRef[varToField[v]], env[v], "%s in .env.published names a different image than publishedImages does", v) From b35c8275bc63006c7782d0e73540817b99307459 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 23 Sep 2026 02:39:09 +0000 Subject: [PATCH 10/13] Round seven: the simplification gave up more than it said, so take one back Two findings, both real, and the first corrects what the last commit claimed. THE COVERAGE GIVEN UP WAS A FAMILY, NOT A CASE. The body named one: a service added to compose and to neither Go table nor .env.published. There are at least four more, and one has teeth -- downgrading `${HILT_IMAGE:?...}` to `${HILT_IMAGE:-ghcr.io/fil-forge/hilt:main}` now passes. That is exactly the silent-fallback regression .env.published's own header and smelt/CLAUDE.md, both in this diff, say the required form exists to prevent, and nothing else catches it: check-stack-images.sh skips every `${...}` value by design, and no Go path compares the two forms. SO ONE CHECK COMES BACK, and the reason it is not the 285 lines again is that it asks a DIFFERENT QUESTION. "Is each of these known names required somewhere?" fails when a file is missed. "What is the complete set of required names?" PASSES when a file is missed -- which is what six rounds were spent chasing. Under-reading is safe for the first and fatal for the second, so TestBuiltImagesAreRequiredNotDefaulted can read the compose files without reimplementing how compose finds them. It is 60 lines against 285, and it needs no include-following, no override names, no extends. Over-reading is the unsafe direction for it, so it looks only at compose-shaped filenames, parses the YAML rather than scanning bytes, and parses the generator's Go rather than reading it -- a commented-out `${X:?}` left above a downgraded line must not count, and does not. AND A publishedImages ENTRY NO OPTION REACHES WAS COMPARED AGAINST NOTHING. fieldToRef was never iterated, so such an entry dropped out of wantVars silently: not required in .env.published, reference never compared. The symmetric twin of the drift this exists for, and the one way two of the three could genuinely disagree and pass. Five lines. Verified over 9 mutations, each asserting it applied: the downgrade fails, and still fails with a commented-out `:?` left above it; the downgrade in the generator fails; deleting a compose file fails rather than passing; the unreachable publishedImages entry fails; and the original bug and the swapped values still fail. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01CGAGAib517Ae1kg8SCdcEt --- smelt/pkg/stack/published_images_test.go | 149 +++++++++++++++++++++++ 1 file changed, 149 insertions(+) diff --git a/smelt/pkg/stack/published_images_test.go b/smelt/pkg/stack/published_images_test.go index fcaae8ca..958888c4 100644 --- a/smelt/pkg/stack/published_images_test.go +++ b/smelt/pkg/stack/published_images_test.go @@ -1,14 +1,21 @@ package stack import ( + "go/ast" + "go/parser" + "go/token" + "io" "os" "path/filepath" "reflect" + "regexp" "sort" + "strconv" "strings" "testing" "github.com/stretchr/testify/require" + "gopkg.in/yaml.v3" ) // The published image set is stated in three places, and they must agree: @@ -133,6 +140,21 @@ func TestPublishedImageSetAgrees(t *testing.T) { require.Equal(t, wantVars, gotEnv, ".env.published does not supply exactly the images this repository builds") + // Every published reference must be reachable by an override variable. + // Without this a publishedImages entry whose config field no + // envImageOptions entry reaches drops out of wantVars silently -- not + // required in .env.published, reference never compared. That is the + // symmetric twin of the drift this test exists for, and it was the one + // way two of the three could genuinely disagree and pass. + reached := map[string]bool{} + for _, f := range varToField { + reached[f] = true + } + for f := range fieldToRef { + require.True(t, reached[f], + "publishedImages publishes %s, which no envImageOptions entry reaches", f) + } + // Comparing the REFERENCES pairwise, not the two sets. A set comparison // passes on a file where PIRI_IMAGE and HILT_IMAGE have been swapped -- // each service booting the other's image -- which is how the shell version @@ -142,3 +164,130 @@ func TestPublishedImageSetAgrees(t *testing.T) { "%s in .env.published names a different image than publishedImages does", v) } } + +// TestBuiltImagesAreRequiredNotDefaulted checks that each image this +// repository builds is a REQUIRED compose interpolation, not one with a +// fallback. +// +// WHY THIS IS SAFE TO DO BY READING THE COMPOSE FILES WHEN THE CHECK ABOVE IS +// NOT. It asks a different question, and the direction of the question decides +// which way incompleteness errs. "Does each of these known names appear +// required somewhere?" fails when a file is missed -- a false failure. "What +// is the complete set of required names?" PASSES when a file is missed, and +// six review rounds went into chasing every way that could happen. Under- +// reading is safe here; over-reading is not, which is why this looks only at +// files named like compose files and not at every YAML under the root. +// +// Comments cannot mask a downgrade, because the YAML is parsed rather than +// scanned: a commented-out `${X:?}` left above a defaulted line is not in the +// parsed document at all. +// +// WHAT IT PROTECTS. `.env.published`'s header and smelt/CLAUDE.md both say the +// required form exists because a silent default meant a caller who forgot an +// override booted a published image and never heard about it. Downgrading +// `${HILT_IMAGE:?...}` to `${HILT_IMAGE:-ghcr.io/fil-forge/hilt:main}` puts +// that back, and nothing else notices: check-stack-images.sh skips every +// `${...}` value by design, and no Go path compares the two forms. +func TestBuiltImagesAreRequiredNotDefaulted(t *testing.T) { + root := filepath.Join("..", "..") + + built := map[string]bool{} + for _, e := range envImageOptions { + f := fieldReachedBy(t, func(c *config) { e.opt(imageSentinel)(c) }) + for _, p := range publishedImages { + if fieldReachedBy(t, func(c *config) { *p.get(c) = imageSentinel }) == f { + built[e.env] = true + } + } + } + require.NotEmpty(t, built) + + // `:?` errors when unset or empty, `?` when unset. Either is required. + required := regexp.MustCompile(`\$\{([A-Z0-9_]+_IMAGE):?\?`) + found := map[string]bool{} + + isComposeFile := func(name string) bool { + return strings.HasPrefix(name, "compose.") || strings.HasPrefix(name, "docker-compose.") + } + + require.NoError(t, filepath.WalkDir(root, func(path string, d os.DirEntry, err error) error { + if err != nil { + return err + } + switch { + case d.IsDir(): + return nil + case isComposeFile(d.Name()) && + (strings.HasSuffix(path, ".yml") || strings.HasSuffix(path, ".yaml")): + case strings.HasPrefix(path, filepath.Join(root, "pkg", "generate")) && + strings.HasSuffix(path, ".go") && !strings.HasSuffix(path, "_test.go"): + // piri's service definition is emitted, not written down. PARSED, + // not scanned: a raw read matches inside a `//` comment, and + // over-reading is the unsafe direction for this question -- a + // commented-out `${PIRI_IMAGE:?}` would let a downgraded live one + // pass. + f, perr := parser.ParseFile(token.NewFileSet(), path, nil, 0) + require.NoError(t, perr, "parsing %s", path) + ast.Inspect(f, func(n ast.Node) bool { + if lit, ok := n.(*ast.BasicLit); ok && lit.Kind == token.STRING { + v := lit.Value + if u, uerr := strconv.Unquote(v); uerr == nil { + v = u + } + for _, m := range required.FindAllStringSubmatch(v, -1) { + found[m[1]] = true + } + } + return true + }) + return nil + default: + return nil + } + + b, rerr := os.ReadFile(path) + require.NoError(t, rerr) + dec := yaml.NewDecoder(strings.NewReader(string(b))) + for { + var doc any + if derr := dec.Decode(&doc); derr != nil { + require.ErrorIs(t, derr, io.EOF, "parsing %s", path) + return nil + } + walkStrings(doc, func(v string) { + for _, m := range required.FindAllStringSubmatch(v, -1) { + found[m[1]] = true + } + }) + } + })) + + for v := range built { + require.True(t, found[v], + "%s is built here but is not a required compose interpolation anywhere; "+ + "a `${%s:-default}` form silently boots a published image when the "+ + "caller forgets an override", v, v) + } +} + +// walkStrings visits every string scalar VALUE. Keys are not visited: compose +// does not interpolate them -- measured, `docker compose config` exits 0 with +// the variable unset and re-escapes the `$` to `$$`. +func walkStrings(n any, fn func(string)) { + switch v := n.(type) { + case string: + fn(v) + case []any: + for _, e := range v { + walkStrings(e, fn) + } + case map[string]any: + for _, e := range v { + walkStrings(e, fn) + } + case map[any]any: + for _, e := range v { + walkStrings(e, fn) + } + } +} From f693c50c8849babb3478edb0dd0d4e04088004e9 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 23 Sep 2026 03:07:53 +0000 Subject: [PATCH 11/13] Round eight: the direction argument held, and then it over-read three ways Seven findings. The central claim splits, and the split is the useful part. UNDER-READING IS GENUINELY SAFE -- no shape was found where reading too little passed. So the framing stands: "is each known name required somewhere" fails on a missed file where "what is the complete required set" passes, and that is what makes 60 lines do what 285 could not. OVER-READING WAS NOT EXCLUDED, which is the unsafe direction for this question, and it let the exact `:?` -> `:-` downgrade pass three ways: - `$${X:?}` counted. That is compose's escape for a literal `$` and requires nothing. Blanked before matching now -- the deleted derivation already did this and the new check did not carry it over, so it is the unfixed half of round three. - ANY compose-SHAPED FILENAME counted, including systems/telemetry/compose.yml and systems/stress-tester/compose.yml, which no project loads: no include list, no Makefile target, no embed set. A required occurrence planted in one satisfied the check for a name defaulted everywhere compose looks. The file set is now the project `make up` runs -- the root file and its include targets, transitively -- and missing one of those is safe by the direction above, so this still needs none of the auto-discovery or extends machinery. - THE GENERATOR'S SOURCE was scanned, so a string literal it never emits counted. That is round six's finding reintroduced. GeneratePiriCompose is called now and its output scanned, which is exact and drops the go/parser machinery entirely. AND A NAME REQUIRED TWICE HID A DOWNGRADE OF ONE. HILT_IMAGE and UPLOAD_IMAGE are each required in two places, so downgrading one of the two left the other satisfying "required somewhere". The check is now two-sided: each built-here name must appear required, AND must appear defaulted nowhere in the project. Verified over 11 mutations, each asserting it applied: all three over-reads now fail, as does downgrading one of HILT's two occurrences, the generator's emitted form, a compose file dropped from the root include (under-read, and it fails rather than passing), and the original bug and swapped values. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01CGAGAib517Ae1kg8SCdcEt --- smelt/pkg/stack/published_images_test.go | 191 +++++++++++++++-------- 1 file changed, 126 insertions(+), 65 deletions(-) diff --git a/smelt/pkg/stack/published_images_test.go b/smelt/pkg/stack/published_images_test.go index 958888c4..608caf89 100644 --- a/smelt/pkg/stack/published_images_test.go +++ b/smelt/pkg/stack/published_images_test.go @@ -1,20 +1,19 @@ package stack import ( - "go/ast" - "go/parser" - "go/token" "io" "os" "path/filepath" "reflect" "regexp" "sort" - "strconv" "strings" "testing" "github.com/stretchr/testify/require" + + "github.com/fil-forge/forge/smelt/pkg/generate" + "github.com/fil-forge/forge/smelt/pkg/manifest" "gopkg.in/yaml.v3" ) @@ -167,20 +166,35 @@ func TestPublishedImageSetAgrees(t *testing.T) { // TestBuiltImagesAreRequiredNotDefaulted checks that each image this // repository builds is a REQUIRED compose interpolation, not one with a -// fallback. +// fallback -- and that no defaulted form of one exists in the project. +// +// WHY THIS CAN READ THE COMPOSE FILES WHEN THE CHECK ABOVE COULD NOT. It asks +// a different question, and the direction of the question decides which way +// incompleteness errs. "Is each of these known names required somewhere?" +// FAILS when a file is missed -- a false failure. "What is the complete set of +// required names?" PASSES when a file is missed, and six review rounds went +// into chasing every way that could happen. // -// WHY THIS IS SAFE TO DO BY READING THE COMPOSE FILES WHEN THE CHECK ABOVE IS -// NOT. It asks a different question, and the direction of the question decides -// which way incompleteness errs. "Does each of these known names appear -// required somewhere?" fails when a file is missed -- a false failure. "What -// is the complete set of required names?" PASSES when a file is missed, and -// six review rounds went into chasing every way that could happen. Under- -// reading is safe here; over-reading is not, which is why this looks only at -// files named like compose files and not at every YAML under the root. +// So under-reading is safe here and OVER-reading is not, which is what the +// file set and the two exclusions below are for. An earlier revision got the +// direction argument right and then over-read three ways: // -// Comments cannot mask a downgrade, because the YAML is parsed rather than -// scanned: a commented-out `${X:?}` left above a defaulted line is not in the -// parsed document at all. +// - it counted `$${X:?}`, which is compose's escape for a literal `$` and +// requires nothing. Blanked before matching now, as the deleted derivation +// already did -- the unfixed half of that fix. +// - it counted any compose-SHAPED filename, including +// systems/telemetry/compose.yml and systems/stress-tester/compose.yml, +// which no project loads: they are in no include list, no Makefile target +// and no embed set. A required occurrence planted in one satisfied the +// check for a name defaulted everywhere compose looks. +// - it scanned the generator's SOURCE, so a string literal it never emits +// counted. That is round six's finding, reintroduced. +// +// The file set is now the project `make up` runs: the root compose file and +// its `include:` targets, transitively. Missing one is safe by the direction +// above, so this needs none of the auto-discovery or `extends:` machinery the +// deleted derivation needed. And the generator is CALLED rather than read, so +// what counts is what it emits. // // WHAT IT PROTECTS. `.env.published`'s header and smelt/CLAUDE.md both say the // required form exists because a silent default meant a caller who forgot an @@ -203,71 +217,118 @@ func TestBuiltImagesAreRequiredNotDefaulted(t *testing.T) { require.NotEmpty(t, built) // `:?` errors when unset or empty, `?` when unset. Either is required. + // `:-` and `-` supply a default, which is what must not appear. required := regexp.MustCompile(`\$\{([A-Z0-9_]+_IMAGE):?\?`) - found := map[string]bool{} + defaulted := regexp.MustCompile(`\$\{([A-Z0-9_]+_IMAGE):?-`) - isComposeFile := func(name string) bool { - return strings.HasPrefix(name, "compose.") || strings.HasPrefix(name, "docker-compose.") - } + isRequired := map[string]bool{} + isDefaulted := map[string]string{} // variable -> where - require.NoError(t, filepath.WalkDir(root, func(path string, d os.DirEntry, err error) error { - if err != nil { - return err + scan := func(where, text string) { + // `$$` is compose's escape for a literal `$`. Blank it before matching + // so `$${X:?}` cannot count as a requirement. + text = strings.ReplaceAll(text, "$$", "\x00\x00") + for _, m := range required.FindAllStringSubmatch(text, -1) { + isRequired[m[1]] = true } - switch { - case d.IsDir(): - return nil - case isComposeFile(d.Name()) && - (strings.HasSuffix(path, ".yml") || strings.HasSuffix(path, ".yaml")): - case strings.HasPrefix(path, filepath.Join(root, "pkg", "generate")) && - strings.HasSuffix(path, ".go") && !strings.HasSuffix(path, "_test.go"): - // piri's service definition is emitted, not written down. PARSED, - // not scanned: a raw read matches inside a `//` comment, and - // over-reading is the unsafe direction for this question -- a - // commented-out `${PIRI_IMAGE:?}` would let a downgraded live one - // pass. - f, perr := parser.ParseFile(token.NewFileSet(), path, nil, 0) - require.NoError(t, perr, "parsing %s", path) - ast.Inspect(f, func(n ast.Node) bool { - if lit, ok := n.(*ast.BasicLit); ok && lit.Kind == token.STRING { - v := lit.Value - if u, uerr := strconv.Unquote(v); uerr == nil { - v = u - } - for _, m := range required.FindAllStringSubmatch(v, -1) { - found[m[1]] = true - } - } - return true - }) - return nil - default: - return nil + for _, m := range defaulted.FindAllStringSubmatch(text, -1) { + if built[m[1]] { + isDefaulted[m[1]] = where + } } + } - b, rerr := os.ReadFile(path) - require.NoError(t, rerr) + // The project `make up` runs: the root compose file and its include + // targets, transitively. + seen := map[string]bool{} + queue := []string{filepath.Join(root, "compose.yml")} + for len(queue) > 0 { + path := queue[0] + queue = queue[1:] + if seen[path] { + continue + } + seen[path] = true + + b, err := os.ReadFile(path) + if err != nil { + // The one include that is legitimately absent is generated/, which + // is gitignored output; its content is covered by calling the + // generator below. + require.True(t, os.IsNotExist(err) && strings.Contains(path, "generated"), + "reading %s: %v", path, err) + continue + } dec := yaml.NewDecoder(strings.NewReader(string(b))) for { var doc any if derr := dec.Decode(&doc); derr != nil { require.ErrorIs(t, derr, io.EOF, "parsing %s", path) - return nil + break + } + // Parsed, not scanned: a commented-out `${X:?}` left above a + // downgraded line must not count, and does not. + walkStrings(doc, func(v string) { scan(path, v) }) + for _, inc := range includeTargets(filepath.Dir(path), doc) { + queue = append(queue, inc) } - walkStrings(doc, func(v string) { - for _, m := range required.FindAllStringSubmatch(v, -1) { - found[m[1]] = true - } - }) } - })) + } + + // piri's service definition is emitted, not written down. CALLED, not + // read: a `${PIRI_IMAGE:?}` string literal the generator never emits must + // not count. + emitted, err := generate.GeneratePiriCompose([]manifest.ResolvedPiriNode{ + {Name: "piri-0", Index: 0}, + }) + require.NoError(t, err) + scan("pkg/generate (emitted)", string(emitted)) for v := range built { - require.True(t, found[v], - "%s is built here but is not a required compose interpolation anywhere; "+ - "a `${%s:-default}` form silently boots a published image when the "+ - "caller forgets an override", v, v) + require.True(t, isRequired[v], + "%s is built here but is not a required compose interpolation anywhere "+ + "in the project; a `${%s:-default}` form silently boots a published "+ + "image when the caller forgets an override", v, v) + require.Empty(t, isDefaulted[v], + "%s is built here but has a DEFAULTED form in %s; that is the silent "+ + "fallback the required form exists to prevent", v, isDefaulted[v]) + } +} + +// includeTargets returns the `include:` paths a compose document names, +// resolved against its own directory. Entries are a path, a list of paths, or +// a mapping with `path:` holding either. +func includeTargets(dir string, doc any) []string { + m, ok := doc.(map[string]any) + if !ok { + return nil + } + list, ok := m["include"].([]any) + if !ok { + return nil } + var out []string + add := func(v any) { + if p, ok := v.(string); ok { + out = append(out, filepath.Join(dir, p)) + } + } + for _, e := range list { + switch v := e.(type) { + case string: + add(v) + case map[string]any: + switch p := v["path"].(type) { + case string: + add(p) + case []any: + for _, q := range p { + add(q) + } + } + } + } + return out } // walkStrings visits every string scalar VALUE. Keys are not visited: compose From a11c459dadea59d8a5dde057f9e4b9d14aa52f3c Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 23 Sep 2026 03:27:53 +0000 Subject: [PATCH 12/13] smelt: write the include-queue append as a spread (S1011) The round-eight push went red on `unit smelt`: staticcheck's S1011 says an append loop over a slice is `append(dst, src...)`. Behaviour is identical -- `includeTargets` returns a slice, the loop appended its elements one at a time. MY MISS, and a specific one. Before pushing round eight I ran gofmt, go vet and the tests, and NOT staticcheck -- which `ci.yml`'s unit job runs between vet and tidy, and which AGENTS.md names in the standard loop. Running the three checks I happened to remember is exactly the partial guard rule 5 warns about: it reads like a pre-push check and is not one. Re-verified after the change, in a worktree of its own: gofmt -s -l, go build, go vet and staticcheck all clean under GOWORK=off, both tests pass under -race -shuffle=on, and the mutation matrix still catches what it caught -- including the dropped-include row, which is the one this rewrite touches. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01CGAGAib517Ae1kg8SCdcEt --- smelt/pkg/stack/published_images_test.go | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/smelt/pkg/stack/published_images_test.go b/smelt/pkg/stack/published_images_test.go index 608caf89..47da26e9 100644 --- a/smelt/pkg/stack/published_images_test.go +++ b/smelt/pkg/stack/published_images_test.go @@ -269,9 +269,7 @@ func TestBuiltImagesAreRequiredNotDefaulted(t *testing.T) { // Parsed, not scanned: a commented-out `${X:?}` left above a // downgraded line must not count, and does not. walkStrings(doc, func(v string) { scan(path, v) }) - for _, inc := range includeTargets(filepath.Dir(path), doc) { - queue = append(queue, inc) - } + queue = append(queue, includeTargets(filepath.Dir(path), doc)...) } } From d92200e670df7cf5149e84abdb152e5ada4e1403 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 23 Sep 2026 03:39:41 +0000 Subject: [PATCH 13/13] Round nine: the file set was narrowed for the half that did not need it Three findings, one root. The check became two-sided last round, and the two halves have OPPOSITE incompleteness directions: `isRequired` is existential, so a file it misses makes the check fail loudly; `isDefaulted` is universal, so a file it misses makes it pass SILENTLY. Round eight narrowed the file set and argued its safety from the first half only. That argument does not reach the second, and all three findings live in the gap. THE GENERATOR WAS CALLED WITH ONE STORAGE SHAPE OF FOUR. The call passed a zero-value StorageSpec, so `buildPostgresService`, `buildPostgresInitService` and `buildMinioService` were never reached -- and the committed smelt.yml resolves to {postgres, s3}, so the three services `make up` actually runs were the three the check never looked at. Those branches hold the generator's only defaulted image line. Now called once per (DB, Blob) pair. GITIGNORED BUILD OUTPUT DECIDED THE VERDICT. `generated/compose/piri.yml` is in the root include list, is gitignored, and CI's `unit smelt` has no generate step -- so the check read whatever the last local `make generate` left, and read nothing in CI. Measured both directions: a stale file carrying a default the source had already fixed fails locally and passes in CI, and a stale file supplying a requirement the source had lost passes locally and fails in CI. `generated/` is skipped outright now; calling the generator covers it exactly, which the code's own comment already said. A missing include target is now loud rather than tolerated. The old branch excused any absent file whose path contained "generated"; a `require.NoError` replaces it, so an `include:` naming a file that is not there fails instead of quietly dropping that file's defaulted forms from a universal check. AND `compose.override.yml` IS LOADED AND NOT READ -- deliberately, now that the reason is written down rather than assumed. Compose auto-discovers it next to compose.yml and the Makefile does load it, since its COMPOSE passes no `-f` at all unless the workspace override exists. It is not committed. A developer's own override is theirs, CI cannot see it, and a verdict that depends on a file only one machine has is the very defect fixed above. If this repository ever commits an override file, it belongs in the walk; the comment says so. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01CGAGAib517Ae1kg8SCdcEt --- smelt/pkg/stack/published_images_test.go | 68 +++++++++++++++++++----- 1 file changed, 54 insertions(+), 14 deletions(-) diff --git a/smelt/pkg/stack/published_images_test.go b/smelt/pkg/stack/published_images_test.go index 47da26e9..c656da63 100644 --- a/smelt/pkg/stack/published_images_test.go +++ b/smelt/pkg/stack/published_images_test.go @@ -1,6 +1,7 @@ package stack import ( + "fmt" "io" "os" "path/filepath" @@ -238,8 +239,25 @@ func TestBuiltImagesAreRequiredNotDefaulted(t *testing.T) { } } - // The project `make up` runs: the root compose file and its include - // targets, transitively. + // The files the project loads AND this repository commits: the root + // compose file and its `include:` targets, transitively. + // + // Both halves below read this one set, and their incompleteness runs in + // OPPOSITE directions. `isRequired` is existential, so a file missed here + // makes the check fail loudly. `isDefaulted` is universal, so a file + // missed here makes it pass SILENTLY. The set has to be justified for the + // second; being safe for the first says nothing. + // + // What it leaves out, deliberately. `compose.override.yml` is + // auto-discovered by compose next to compose.yml, and the Makefile does + // load it -- its COMPOSE passes no `-f` at all unless the workspace + // override exists, and an explicit `-f` list is what suppresses + // auto-discovery. `generated/compose/workspace.override.yml` is the one + // it passes by `-f` when that does exist. NEITHER IS COMMITTED. A + // developer's own override is theirs, CI cannot see it, and a verdict + // that depends on a file only one machine has is the defect the + // `generated/` skip below exists to remove. If this repository ever + // commits an override file, it belongs in this walk. seen := map[string]bool{} queue := []string{filepath.Join(root, "compose.yml")} for len(queue) > 0 { @@ -250,15 +268,24 @@ func TestBuiltImagesAreRequiredNotDefaulted(t *testing.T) { } seen[path] = true - b, err := os.ReadFile(path) - if err != nil { - // The one include that is legitimately absent is generated/, which - // is gitignored output; its content is covered by calling the - // generator below. - require.True(t, os.IsNotExist(err) && strings.Contains(path, "generated"), - "reading %s: %v", path, err) + // `generated/` is gitignored build output, and it is SKIPPED rather + // than read-if-present. CI's `unit smelt` has no generate step, so + // there the file is absent; locally it is whatever the last + // `make generate` left. Reading it makes the verdict depend on which + // machine ran the test, in both directions: a stale file supplying a + // requirement the source had since lost passes locally and fails in + // CI, and a stale file carrying a default the source had since fixed + // fails locally -- naming a gitignored file -- and passes in CI. + // The generator is called below instead, which is exact. + if rel, rerr := filepath.Rel(root, path); rerr == nil && + strings.HasPrefix(rel, "generated"+string(filepath.Separator)) { continue } + // Any other include target must exist. An `include:` naming a file + // that is not there is a broken project, and silently skipping it + // would drop that file's defaulted forms from a universal check. + b, err := os.ReadFile(path) + require.NoError(t, err, "reading %s", path) dec := yaml.NewDecoder(strings.NewReader(string(b))) for { var doc any @@ -276,11 +303,24 @@ func TestBuiltImagesAreRequiredNotDefaulted(t *testing.T) { // piri's service definition is emitted, not written down. CALLED, not // read: a `${PIRI_IMAGE:?}` string literal the generator never emits must // not count. - emitted, err := generate.GeneratePiriCompose([]manifest.ResolvedPiriNode{ - {Name: "piri-0", Index: 0}, - }) - require.NoError(t, err) - scan("pkg/generate (emitted)", string(emitted)) + // ONCE PER STORAGE SHAPE, because the generator emits a different set of + // services for each and those branches carry image references of their + // own -- postgres adds `piri-postgres` and `piri-postgres-init`, s3 adds + // `piri-minio`. A single zero-value call reached none of them, and the + // committed smelt.yml resolves to {postgres, s3}, so the three services + // `make up` actually runs were the three it never looked at. The only + // image line in those branches is `${MINIO_IMAGE:-...}`, which is inert + // here because MINIO_IMAGE is not built by this repository -- but it is + // inert by accident, not by construction. + for _, db := range []string{manifest.DBSQLite, manifest.DBPostgres} { + for _, blob := range []string{manifest.BlobFS, manifest.BlobS3} { + emitted, err := generate.GeneratePiriCompose([]manifest.ResolvedPiriNode{ + {Name: "piri-0", Index: 0, Storage: manifest.StorageSpec{DB: db, Blob: blob}}, + }) + require.NoError(t, err, "generating db=%s blob=%s", db, blob) + scan(fmt.Sprintf("pkg/generate (emitted, db=%s blob=%s)", db, blob), string(emitted)) + } + } for v := range built { require.True(t, isRequired[v],