Skip to content

smelt: .env.published was missing the indexer, and nothing could notice - #23

Draft
Peeja wants to merge 13 commits into
mainfrom
claude/env-published-indexer
Draft

Peeja wants to merge 13 commits into
mainfrom
claude/env-published-indexer

Conversation

@Peeja

@Peeja Peeja commented Sep 23, 2026 •

Copy link
Copy Markdown
Contributor

Checks a reviewer can merge without waiting for

Everything except ci / unit smelt and ci / guards.

$ git diff --name-only origin/main...HEAD
smelt/.env.published                          one entry + header
smelt/CLAUDE.md                               documentation
smelt/pkg/stack/options.go                    comments only
smelt/pkg/stack/published_images_test.go      new, in-package tests

unit smelt runs the new tests. guards runs gofmt -s -l . over the whole tree, which now has a new Go file in it.

Why nothing else can see it, derived rather than asserted

options.go is comments only — every changed line is blank or a // line:

$ git diff origin/main HEAD -- smelt/pkg/stack/options.go \
    | grep -E '^[+-]' | grep -vE '^(\+\+\+|---)' | sed -E 's/^[+-]//' \
    | grep -vE '^\s*(//.*)?$'
(no output)

e2e and the two itest modules reach pkg/stack — but the new file is a TestGoFiles entry, and an importer compiles GoFiles only:

$ go list -deps -test ./tests/e2e/...            | grep -c forge/smelt/pkg/stack → 0
$ go list -deps -test -tags e2e ./tests/e2e/...  | grep -c forge/smelt/pkg/stack → 1
$ go list -f '{{.ImportPath}} GoFiles={{len .GoFiles}} TestGoFiles={{len .TestGoFiles}}' ./pkg/stack/
github.com/fil-forge/forge/smelt/pkg/stack GoFiles=10 TestGoFiles=4

Nothing outside ci / unit smelt reads smelt/.env.published's contents. Three things hand the path to compose by --env-file without opening it: smelt/Makefile's ENV_FILES (line 23), composeArgs in smelt/pkg/snapshot/stack.go, and that package's internal test. The Makefile never runs in CI — no workflow runs make. The other two do run, under unit smelt: TestComposeArgs is an untagged test in package snapshot. It makes no difference, because composeArgs only os.Stats the path and the test writes its own empty fixtures into t.TempDir(). The only thing anywhere that reads the file's contents is this PR's new test, in a job already named as not skippable.

ci.yml is untouched in the net diff.

What it costs if that is wrong: the ignored checks still run, and a red one after merge leaves main red.


The bug

make up fails at compose up with INDEXER_IMAGE unset. .env.published supplied seven of the eight images this repository builds; publishedImages in pkg/stack/options.go listed all eight.

Why nothing caught it

.env.published's own header said:

The same list lives in pkg/stack/options.go (publishedImages). That duplication is deliberate … Neither copy can drift silently — a missing entry fails at compose up naming the variable.

Both halves are true, and together they are not enough. It did fail at compose up, and it did drift. Nothing in CI runs make up, so not silent and not noticed were the same thing. Every Go path fills all eight through WithPublishedImages(), whose table does list the indexer, so no test could see it either.

The two checks

TestPublishedImageSetAgrees compares the three statements of the set — .env.published, publishedImages, envImageOptions — name by name and reference by reference. It pairs a variable to a reference by behaviour: it applies each override option and each published getter to a fresh config with a sentinel and sees which field moved, so nothing has to agree about how PIRI_IMAGE relates to piriImage, and a renamed field cannot silently break the join. It also asserts every published reference is reachable by some option — without that, an entry whose field no option reaches is compared against nothing, which was the one way two of the three could genuinely disagree and pass.

TestBuiltImagesAreRequiredNotDefaulted asserts each of those images appears as a required compose interpolation and appears with a fallback nowhere. Downgrading ${HILT_IMAGE:?…} to ${HILT_IMAGE:-ghcr.io/fil-forge/hilt:main} is the exact silent-fallback regression .env.published's header and smelt/CLAUDE.md 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.

Why the second check can read the compose files when an earlier version could not — the transferable bit

An earlier revision derived the complete required set from the compose files, and six review rounds went into every way that derivation could miss a file: include: targets of any name, compose.yaml, the four *.override.* spellings, extends: file:, # comments, $$ escapes, YAML map keys, _test.go. 285 of its 433 lines were that machinery, and a stub proof showed none of it was load-bearing — with requiredImageVars replaced by a constant, the original bug and the swapped-values case both still fail, because the expected set comes from the Go tables and never from the compose files.

The new check asks a different question, and the direction of the question decides which way incompleteness errs:

question a missed file means so
is each of these known names required somewhere? a name is not found → fails under-reading is safe
what is the complete set of required names? a name is never demanded → passes under-reading is fatal

Framed the first way it needs neither of the two most intricate parts of the deleted machinery — auto-discovery of the eight override filenames, and extends: file: resolution. It is 225 lines including its two helpers against that 285, a 21% saving; the file as a whole went 433 → 392. It does follow include:, which the defaulted half below needs.

Over-reading is the unsafe direction for this framing, and three of them each let the :? → :- downgrade pass. $${X:?}, compose's escape for a literal $, requires nothing and is blanked before matching. The file set is what the project loads and this repository commits — the root compose file and its include: targets, transitively — rather than anything compose-shaped on disk, because systems/telemetry/ and systems/stress-tester/ are loaded by nothing and a requirement planted in one satisfied a name defaulted everywhere compose looks. And the generator is called, its output scanned, rather than its source read, so a string literal it never emits does not count.

The check is two-sided: each built-here name must appear required, and must appear defaulted nowhere. One-sided, a name required in two places — HILT_IMAGE and UPLOAD_IMAGE both are — kept satisfying "required somewhere" when one of the two was downgraded.

What is still not checked — one of these is wider than it looks

That compose requires exactly this set and no more. A service added to a compose file with a required image variable, and to neither Go table nor .env.published, passes. So does a not-built-here image promoted from ${GUPPY_IMAGE:-…} to ${GUPPY_IMAGE:?…}.

That every occurrence is an interpolation at all. isRequired is existential — it asks whether a name appears required somewhere. So replacing a compose image: outright with a digest-pinned literal is caught only sometimes:

the literal replaces caught?
a not-built-here image (${GUPPY_IMAGE:-…}) no — by nothing at all
the sole occurrence of a built-here name (SWARF_IMAGE) yes — isRequired goes false
one of two occurrences of a built-here name (HILT_IMAGE, UPLOAD_IMAGE) no — the other still satisfies it

Measured at this head: ${HILT_IMAGE:?…} → ghcr.io/fil-forge/hilt:main@sha256:… in one of its two places leaves check-stack-images.sh at rc=0 and both tests passing. Two-sidedness does not help — a literal leaves no defaulted form for isDefaulted to find.

check-stack-images.sh does not cover any of these: its rule is pinned by digest or a variable, so a digest-pinned literal is precisely what it accepts. Only a non-digest literal turns it red.

Closing the third row is cheap, and is deliberately not done here: a third side rejecting any publishedImages reference that appears as a literal in a walked compose file — those repository paths are already in that table, so it needs no new derivation. It is a different regression class from the one this PR fixes, and this test has taken three rounds' worth of growth already; it belongs in its own change.

That closes the third row only. The first is out of its reach by construction: GUPPY_IMAGE is not in publishedImages, and a check keyed on that table cannot see an image this repository does not build. Catching that one needs the complete-set direction — for which, ask compose rather than reimplementing it: docker compose config with an empty environment, read back the variable it names as missing, repeat. Exact by construction, no daemon, measured at 1.43 s over 9 invocations. Not --variables, which was measured to under-report on an extends.file shape compose itself enforces.

Two files compose really does load are also out, deliberately: compose.override.yml, which compose auto-discovers next to compose.yml (the Makefile's COMPOSE passes no -f at all unless the workspace override exists, and an explicit -f list is what suppresses auto-discovery), and generated/compose/workspace.override.yml, which 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 not a verdict. If this repository ever commits an override file, it belongs in the walk — the test's comment says so.

The fourth copy of the disproven sentence was in the file the check was parsing

The "neither copy can drift silently" claim was corrected in .env.published and dropped from smelt/CLAUDE.md — and left standing in pkg/stack/options.go, which holds one of the two copies "neither copy" refers to.

smelt/CLAUDE.md had the same problem: it named six of eight, listed swarf and the indexer under "not built here" long after both were imported, and closed with "SWARF_IMAGE joins the required set when the branch that builds swarf lands." That branch landed.

Verified

Twelve mutations at this head, each asserting it applied
scenario test expected got
fixed tree both pass pass
the original bug — INDEXER_IMAGE absent from .env.published Agrees fail fail
PIRI_IMAGE and HILT_IMAGE values swapped Agrees fail fail
a publishedImages entry no option reaches Agrees fail fail
one of HILT_IMAGE's two :? occurrences downgraded to :- Required fail fail
the same downgrade in the generator's emitted output Required fail fail
control — that name planted as ${…:?} in a compose file the project loads Required pass pass
…the same plant escaped as $${…:?} Required fail fail
…the same plant in a compose file no project loads Required fail fail
…the same plant in the generator's source, never emitted Required fail fail
a compose file dropped from the root include: (under-reading must fail, not pass) Required fail fail
restored both pass pass

The three over-read rows neutralise HILT_IMAGE to a literal first, so the planted occurrence is the only thing that could satisfy "required somewhere". The control row is what makes them mean anything — without a plant that demonstrably works, a row failing because the plant was invisible reads exactly like a row failing because the check excluded it.

The generator is called once per (DB, Blob) pair. A single zero-value call reached none of buildPostgresService, buildPostgresInitService or buildMinioService — and the committed smelt.yml resolves to {postgres, s3}, so the three services make up actually runs were the three it never looked at. generated/ is skipped rather than read: it is gitignored, CI has no generate step, and reading it made the verdict differ between a developer's machine and CI in both directions.

All seven guards pass, gofmt -s -l . empty, go vet, staticcheck and go build clean under GOWORK=off, ci.yml parses. The three TestBuild*ImageTagFormat in the same package fail here and identically on an untouched origin/main worktree: they skip on exec.LookPath("docker") and this environment has the client without a daemon.

Twelve rounds, and the one thing worth taking from them

Every round up to the sixth found the same shape — a deriver reading something other than what its consumer reads:

round it read what the consumer reads
one the two lists as sets pairs — which reference a given variable carries
two only ${X:?}; only files named compose.yml ${X?} too; include: targets of any name; compose.yaml
three raw bytes, so # comments and $$ escapes counted parsed values
four YAML map keys, _test.go, non-compose YAML values, in the files compose opens
five four auto-discovered names eight, plus extends: file:
six every string literal in pkg/generate, emitted or not what the generator emits
eight any compose-shaped file, $${…} escapes, the generator's source again the files make up loads, $ unescaped, what the generator emits
nine the generator at one storage shape of four; gitignored build output all four shapes; only what the repository commits

Round seven found the round-six cut had given up more than it documented. Round eight found that cut had traded one failure direction for the other. Round nine found the sharpest version: making the check two-sided gave its halves opposite incompleteness directions — isRequired existential, so a missed file fails loudly; isDefaulted universal, so a missed file passes silently — and the file set had been narrowed on an argument that only covers the first.

Rounds ten, eleven and twelve found nothing in either test, and ten findings in this pull request body — several of them inside corrections the previous round had just asked for.

Not one of the findings across twelve rounds was a defect in the one-line fix that repairs make up. They were in the guard written for it, and then in the prose written about the guard.

One decision taken here

The second check stays in this PR rather than moving to a follow-up. Round nine recommended splitting it, and the case was real: TestBuiltImagesAreRequiredNotDefaulted is 225 of this file's 392 lines and absorbed most of rounds seven through nine. The stated condition for keeping it was that round nine's findings 1–3 collapse into a single push rather than another round — and they did, in one commit. The flip condition was round ten finding anything further in that test; it did not, and neither did rounds eleven or twelve.

One thing noticed and deliberately not fixed here

AGENTS.md says of the guards: "That directory is the list — this file deliberately does not enumerate them." But ci.yml enumerates them by hand, one named step each, so a check-*.sh added without a step silently never runs — the same class as the bug this PR fixes, one layer up. Not changed here because collapsing the steps into a glob loses the per-guard step names, which is what tells you which guard failed without opening the log. The check name is unaffected either way: all seven are steps of the single guards job, and GitHub creates one check run per job, so ci / guards stays ci / guards. The trade is legibility, not a required-status-check rename.


🤖 Generated with Claude Code

https://claude.ai/code/session_01CGAGAib517Ae1kg8SCdcEt

`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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CGAGAib517Ae1kg8SCdcEt
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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CGAGAib517Ae1kg8SCdcEt

@Peeja Peeja left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[From Claude:] Not satisfied — 5 findings. Two of them let check-published-images.sh print "agreed" and exit 0 on a tree where the lists genuinely disagree, which is the check's headline claim. One is a fourth copy of the very sentence this PR exists to disprove, in options.go — the file that is the other half of the duplication the sentence is about. The entry, the wiring, and every row of the body's Verified table reproduce exactly.

Findings: 5.

1. The guard compares ref sets, never variable→ref pairs, so it passes on a mismatched mapping (exit 0)

required/provided are compared as sets of variable names; env_refs/go_refs as sets of references. Nothing ever checks that a given variable carries the reference publishedImages declares for that service. Two constructions pass:

(a) Swap two values. No duplicates, both lists identical as sets:

$ sed -i 's|^PIRI_IMAGE=.*|PIRI_IMAGE=ghcr.io/fil-forge/hilt:main|;
          s|^HILT_IMAGE=.*|HILT_IMAGE=ghcr.io/fil-forge/piri:main|' smelt/.env.published
$ .github/scripts/check-published-images.sh
check-published-images: 8 required images, agreed across compose, .env.published and publishedImages.
EXIT=0

make up boots hilt's image as piri and piri's as hilt, and the guard reports agreement.

(b) A duplicate key. Append one line to the file:

$ printf 'PIRI_IMAGE=ghcr.io/fil-forge/hilt:main\n' >> smelt/.env.published
$ .github/scripts/check-published-images.sh
check-published-images: 8 required images, agreed across compose, .env.published and publishedImages.
EXIT=0

sort -u collapses the repeated name out of provided, and the duplicate's value is already in env_refs from the other service's line, so both comparisons see identical sets. PIRI_IMAGE is now bound twice to conflicting values — a disagreement whichever binding compose resolves, so this does not rest on dotenv's last-wins rule (not verifiable here, no Docker).

Both are the failure the check is named for: .env.published and publishedImages do not say the same thing, and it says they do. The mapping is derivable on both sides — publishedImages keys on a config field, and envImageOptions (finding 5) already states the field↔variable correspondence by hand — so comparing VAR=ref pairs is available rather than blocked.

2. A required interpolation in smelt/compose.yml — the root file make up runs — is outside the search root

find smelt/systems -name compose.yml is rooted one directory too deep. smelt/compose.yml is a compose file, it is the one make up resolves (COMPOSE = $(DOCKER) compose $(ENV_FILES) $$(test -f … && echo "-f compose.yml -f …"), and compose auto-discovers it otherwise), and it already interpolates ${SMELT_NETWORK_EXTERNAL:-true}. It declares no services today, so nothing is broken now — but the guard is blind there:

$ printf '\nservices:\n  extra:\n    image: ${EXTRA_IMAGE:?must be set}\n' >> smelt/compose.yml
$ .github/scripts/check-published-images.sh
check-published-images: 8 required images, agreed across compose, .env.published and publishedImages.
EXIT=0

make up fails on unset EXTRA_IMAGE; the guard says eight images agree. This is the same shape as the bug the PR fixes, at the one compose file systems/ does not contain. Rooting the find at smelt keeps the -name compose.yml restriction the header argues for (nothing else in the tree is named compose.yml, and snapshots/*/smelt.yml and generated/compose/piri.yml stay excluded).

3. The "refusing to report that as agreement" exit-2 path cannot fire from the compose side

required is the union of the two extractions and only the union is tested for emptiness. smelt/pkg/generate/compose.go always contributes PIRI_IMAGE, so the [ -z "$required" ] branch is unreachable unless both halves vanish. Losing either half alone is reported as a disagreement:

$ mv smelt/systems away && .github/scripts/check-published-images.sh; echo EXIT=$?
::error::smelt/.env.published does not match the required compose interpolations.
@@ -1 +1,8 @@
+DELEGATOR_IMAGE … +UPLOAD_IMAGE          (7 images "set in .env.published", not required)
EXIT=1

$ mv smelt/pkg/generate away && .github/scripts/check-published-images.sh; echo EXIT=$?
+PIRI_IMAGE
EXIT=1

Both are "could not read one of the lists" arriving as rc=1 with a diff that accuses .env.published of carrying seven entries nobody asked for. CI is red either way, so this is a misdiagnosis rather than a false pass — but the body's table maps rc=2 to refuse, and the header's reasoning holds only for the simultaneous case. Testing the two extractions separately (or asserting the systems/ half is non-empty) closes it.

4. The fourth copy is in options.go, and it is the disproven sentence itself, not just a stale list

smelt/pkg/stack/options.go:409-411, the comment directly above publishedImages:

// 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.

That is the sentence this PR quotes from .env.published's header under "Why nothing caught it", rewrites there, and drops from smelt/CLAUDE.md. It survives verbatim in the one remaining place — and options.go is not an incidental third party: it holds one of the two copies "neither copy" refers to. A reader who opens the Go table still gets the reasoning the PR demonstrates is insufficient, with no mention of the guard that now makes the conclusion true by a different mechanism.

The body says rule-5 enumeration "found smelt/CLAUDE.md carrying its own hand-written version". It stopped one file short. The same comment block (:413-415) also carries a hand-written not built here list — guppy, ipni, plc, blockchain, minio — which is currently correct, but is a fourth uncontrolled copy of that set, and the doc's copy of it is the one that had gone stale.

5. envImageOptions is a fourth statement of the same service set, and the guard does not cover it

smelt/pkg/stack/options.go:386-403 lists all thirteen image variables, and its own comment asserts by hand the derivation the script performs:

// The variable names match the ones the compose files already
// interpolate (systems/*/compose.yml, and PIRI_IMAGE in pkg/generate), so a
// caller that can set them for `docker compose` can set them for a Go test

OptionsFromEnv() iterates it, so a service added to compose + .env.published + publishedImages but not here silently gets no environment override in Go, and the guard stays green. It is complete today (13 = the 8 required + the 5 defaulted), so this is a coverage gap rather than a present defect — but it is the identical failure mode one list over, asserted rather than derived, in the file the script already parses. The script header's "stated in three places" is four.

Verified — what checked out

Run at c66c88f8, GOWORK=off, no Docker. Mutations applied in a sandbox copy, reverted and re-confirmed pristine (EXIT=0) after each.

The fix itself. The eight required variables are exactly right: seven across systems/*/compose.yml (DELEGATOR, HILT, INDEXER, INGOT, SIGNER, SWARF, UPLOAD) plus PIRI_IMAGE from pkg/generate/compose.go:70. .env.published now sets those eight; publishedImages names the eight matching refs. PIRI_IMAGE does appear in no compose file — every other occurrence in the tree is a comment (Makefile:18, CLAUDE.md:465, pkg/snapshot/stack.go:19, .env.published:6).

The body's Verified table. All seven rows reproduce, by execution:

scenario claimed got
fixed tree rc=0 rc=0, "8 required images, agreed"
INDEXER_IMAGE dropped from env file rc=1 rc=1, names it in both diffs
entry dropped from publishedImages rc=1 rc=1
env ref tag differs from Go table rc=1 rc=1
new required compose variable rc=1 rc=1
publishedImages renamed away rc=2 rc=2
.env.published emptied rc=2 rc=2

Also: .env.published deleted outright → rc=2; options.go deleted → rc=2.

|| true. Every extraction's emptiness is reachable — required, env_refs and go_refs explicitly, provided transitively (a non-empty required can never equal an empty provided, so empty always fails). cmd | cmd || true binds to the whole pipeline, so pipefail is caught as intended. No path I could construct exits 0 on a failure other than findings 1 and 2, neither of which is a || true problem.

The awk range. /^}$/ vs /^}/ reasoning is correct — the declaration closes with }{. Survives a trailing comment on the var line (rc=0, still 8). Moving the table into a var ( … ) block makes ^var publishedImages = unmatchable and the script exits 2 rather than passing — fail-safe. The grep -oE '"[^"]+/[^"]+:[^"]+"' picks up nothing spurious from the range; the func(c *config) *string closures carry no quoted strings.

The container-environment claim. BAO_ADDR and INGOT_OPENBAO_TOKEN (systems/ingot/openbao/init.sh:28-29) and PIRI_POSTGRES_DATABASES (systems/piri/postgres-init.sh:13) exist at the named paths, are in .sh files compose does not interpolate, and are the only non-*_IMAGE ${X:?} forms under smelt/ — so the note that a recursive grep misses them only by naming accident is accurate.

smelt/CLAUDE.md. The quoted derivation runs verbatim from smelt/, rc=0, and emits exactly the seven, PIRI_IMAGE absent as the doc says. The "not built here" list — guppy, ipni, plc, blockchain, minio — is exactly the five ${X_IMAGE:-} variables in the tree, all five pinned by digest in their systems/*/compose.yml, and 5 + 8 = 13 matches the unchanged "Available variables" line above it.

The skippable-checks block's central claim. Unrestricted grep -rn '\.env\.published' . (no --include): the only Go reference is pkg/snapshot/stack.go:30, which os.Stats the path and passes --env-file, never opening it; stack_internal_test.go:18 writes nil content into t.TempDir(). GOWORK=off go test ./pkg/snapshot/... → ok 0.009s. The only other consumer is Makefile:23 at runtime, not in CI. Contents cannot affect a check. The block's enumeration of ci.yml mentions omits .gitignore:5 (a comment about the tidy check) — it executes nothing, so the conclusion is unaffected.

Wiring and hygiene. Eight check-*.sh on disk, eight steps in ci.yml's guards, one-to-one — "all eight guards pass" is consistent. ci.yml parses (jobs: guards, unit). shellcheck clean, bash -n clean, gofmt -s -l . empty under smelt.

Not a missed source. smelt/cmd/smelt/cmd/workspace.go also writes a compose file (generated/compose/workspace.override.yml) from outside pkg/generate/, but it emits binary mounts over published images and no image: line, so it contributes no required interpolation today.


Generated by Claude Code

… 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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CGAGAib517Ae1kg8SCdcEt
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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CGAGAib517Ae1kg8SCdcEt

@Peeja Peeja left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[From Claude:] Round two on 1945c7d1, against c66c88f8. Five findings: two in the new test, both of which let it pass on a tree where make up fails; two derivations in the PR body that do not produce what the body says they produce; one stale statement on the wiki. Round one's five fixes were all checked and are real — the shell script and its ci.yml step are gone with no dangling reference, the swap and the duplicate binding are caught, the walk starts at the smelt root, envImageOptions is now joined rather than asserted, and the fourth copy of the disproven sentence is corrected.

The two test findings are the same shape as round one's finding 2: the deriver recognises less than compose does, so a required variable can be added in a form or a file it does not see, and all four lists then agree on a set that is missing it.

1. The regex accepts only ${X_IMAGE:?…}; compose's other required form ${X_IMAGE?…} is missed, and the test stays green

requiredImageVars matches \$\{([A-Z0-9_]+_IMAGE):\?. Compose has two required-variable forms, not one: ${VAR:?err} errors when unset or empty, ${VAR?err} errors when unset. The second is a different form, not a typo, and it is required in exactly the sense this test cares about:

$ printf 'services:\n  a:\n    image: ${NEWTHING_IMAGE?must be set}\n' > compose.yml
$ docker compose config
error while interpolating services.a.image: required variable NEWTHING_IMAGE is missing a value: must be set
rc = 1                                           (Docker Compose v5.1.1)

Appending the same service to smelt/compose.yml in each spelling:

${NEWTHING_IMAGE:?must be set}   test fails (caught)
${NEWTHING_IMAGE?must be set}    test PASSES  (missed)

A variable introduced that way is absent from required, so if it is also absent from envImageOptions, publishedImages and .env.published — which is the case this test exists to catch — all four sets agree on the smaller set and the test is green while make up dies at compose up with the variable unset. That is this PR's own bug, reachable again through the one spelling the regex does not read.

:?\? covers both: \$\{([A-Z0-9_]+_IMAGE):?\?.

2. The walk keys on the filename compose.yml, so a required variable in an included file with another name — or in a compose.yaml — is invisible

The walk scans d.Name() == "compose.yml" and nothing else. Two ways past it, both green:

systems/newsvc/newsvc.yml  + an include line in smelt/compose.yml   test PASSES
systems/newsvc/compose.yaml                                          test PASSES

Neither filename is hypothetical:

  • The root file already includes one. smelt/compose.yml's include: list names generated/compose/piri.yml. Include entries are explicit paths, so any filename is legal there; compose.yml is a convention the walk treats as an invariant. (piri.yml itself is covered, but only because the interpolation is a Go string literal under pkg/generate — if that template ever moves to an embedded .yml, this hole opens on the file the special case was written for.)
  • compose.yaml is compose's own preferred default name. It is auto-discovered, which matters for the standalone per-system runs smelt/compose.yml's header documents (cd systems/X && docker compose up -d):
$ ls; docker compose config
compose.yaml
error while interpolating services.a.image: required variable NEWTHING_IMAGE is missing a value

Matching compose*.y*ml, or deriving the file set from the root include: graph, closes both. Nested depth is fine as it stands — systems/indexing/indexer/compose.yml is reached, and a required variable added anywhere under systems/ in a file named compose.yml is caught (verified).

3. PR body: "seven across systems/*/compose.yml" — that glob matches six of them
$ grep -loE '\$\{[A-Z0-9_]+_IMAGE:\?' systems/*/compose.yml   | wc -l   → 6
$ grep -loE '\$\{[A-Z0-9_]+_IMAGE:\?' systems/*/*/compose.yml | wc -l   → 1

INDEXER_IMAGE is a level deeper, at systems/indexing/indexer/compose.yml. Eight total is right; the distribution is six + one + PIRI_IMAGE from the generator, not seven + PIRI_IMAGE.

Worth correcting rather than shrugging at, because #18's body records this exact miscount as having caught three review rounds "in both directions", and resolves to state the number with how it was counted. The count here is stated with a glob that returns a different number.

The test itself is not affected — the walk is recursive — and its own comment ("a glob over compose files alone reports seven of eight") is accurate.

4. PR body: the "options.go is comments only" command prints 17 lines, not "(no output)"

The claim is true — I checked the changed lines independently and every non-blank one begins with //. The derivation offered for it is not:

$ git diff origin/main HEAD -- smelt/pkg/stack/options.go \
    | grep -E '^[+-]' | grep -vE '^(\+\+\+|---)' | sed -E 's/^[+-]//' \
    | grep -vE '^\s*(//)?\s*$' | wc -l
17

^\s*(//)?\s*$ drops blank lines and lines that are a bare //; a comment line with text does not match it and survives the -v. grep -vE '^\s*(//.*)?$' is the filter that yields nothing.

This is the block that says "derived rather than asserted", and a reviewer who runs it gets 17 lines and no way to tell whether the claim holds.

5. The wiki's Current State says #18 is the only open forge PR; #23 is open too, and the paragraph below it still names #19 as open

[[Needs Human Work]] is current — it carries #23 at 1945c7d1 with round one's findings and "Round two running" (3c83df48, 26d6b351). Both of this PR's wiki commits touched that page only. Current-State.md was last written at 60d679e1, before #23 existed, and now states:

main is 4611cbcb (#22's merge) … [#18] is the only forge pull request open — and it is Open, not a draft.

Open PRs right now are #23 (draft) and #18 (Open). Three paragraphs later the same page still says "Two forge pull requests are open, both drafts", listing #18 and #19 — which is merged, and is in this PR's own base.

main's sha is right; the open-PR list is the other thing that page is for, and AGENTS.md puts it on the push: "Opened, pushed to … a PR? → update the wiki before you report what you did … If you touched the repository and the wiki still describes the previous state, you are not finished." The #19 half is older debt the section carries; the #18-only sentence is falsified by this PR.

Verification: 20 mutations, the PR body's claims, and what checked out

Baseline. GOWORK=off go test ./pkg/stack/ -run TestPublishedImageSetAgrees passes; also under CI's exact flags (-race -shuffle=on), and from the repository root in workspace mode — ../.. resolves to the smelt root under go test whatever directory invoked it, because the test binary's cwd is the package directory.

Mutations. Each applied to a clean tree, run, reverted.

mutation result
PIRI_IMAGE/HILT_IMAGE values swapped in .env.published fails
PIRI_IMAGE bound twice fails
INDEXER_IMAGE line removed (the original bug) fails
a tag differs (:main → :latest) in .env.published fails
a ref differs in publishedImages fails
entry dropped from publishedImages fails
entry dropped from envImageOptions fails
required var added to smelt/compose.yml (:?) fails
required var added to systems/telemetry/compose.yml (:?) fails
systems/hilt stops requiring HILT_IMAGE (:- default) fails
publishedImages gains guppy, compose keeps its :- fails
an option that sets no field fails
an option that sets two fields fails
a second variable aliased to a published field fails
export PIRI_IMAGE=… fails
PIRI_IMAGE="…" fails
PIRI_IMAGE=… # trailing comment fails
PIRI_IMAGE = … fails
PIRI_IMAGE=ghcr.io/…:${TAG} fails
pkg/generate prefix over-match (pkg/generatex) fails
whole file converted to CRLF passes — correct, compose reads CRLF too
required var in the ? form / in a non-compose.yml file passes — findings 1 and 2

fieldReachedBy is sound, and not by accident. reflect.Value.String() is the one getter that neither panics on a non-string kind nor consults the read-only flag, so reading unexported fields with it is documented behaviour rather than luck — flagRO is checked by Interface, Set* and Bytes, not by String. The Kind() == reflect.String guard means a field that stopped being a string is skipped, and the option then reports zero fields, which require.Len(…, 1) catches; a named string type still reports String. Options setting zero or two fields both fail (above). config is flat — no embedded or nested struct whose string fields a top-level-only scan would miss.

readEnvPublished diverges from compose's dotenv parser in six ways, all in the strict direction — quoted values, export , trailing comments, spaces around =, value-side ${} interpolation, and a repeated key (compose takes the last; this refuses). Each makes the test fail on a file compose would read, so none of them can pass on a file compose reads differently. I found no divergence in the loose direction. One consequence worth knowing: .env.published may contain nothing but the eight image variables, or the set comparison fails — strict, and safe.

One shape passes while two lists differ, and I am not filing it: a publishedImages entry whose config field no envImageOptions entry reaches. To matter it also needs a compose :- default for the same variable, and every image field has an option today.

buildEnv is a fifth statement of the variable↔field pairing and this test does not read it — but it is not uncovered: swapping PIRI_IMAGE/HILT_IMAGE there leaves TestPublishedImageSetAgrees green and turns TestWithPublishedImagesFillsUnsetFields red, so unit smelt still catches it.

CI placement. unit smelt runs go test -race -shuffle=on ./... in smelt/ with GOWORK=off, so pkg/stack is in it. No reference to check-published-images.sh survives anywhere at HEAD (git grep over the tree: none). .github/scripts/ holds seven check-*.sh and ci.yml names seven; six run clean here (the release-version one not run — it is untouched by this diff and slow by design). The check now runs after build/vet/staticcheck/tidy rather than in a job of its own, which is a real if minor change in when it is reached.

PR body claims, each re-derived: four changed files against 4611cbcb; ci.yml net diff empty; every changed line in options.go is a // line (finding 4 is about the command, not the claim); go list -deps -test ./tests/e2e/… → 0 and -tags e2e → 1; GoFiles=10 TestGoFiles=4; gofmt -s -l . empty. TestBuildImageTagFormat, TestBuildPiriImageTagFormat and TestBuildGuppyImageTagFormat fail here and fail identically in an untouched origin/main worktree — same three, same missing daemon. The rest of the smelt suite is green.

mainprobe is gone. No gitlink at HEAD (git ls-tree -r HEAD | awk '$1=="160000"' is empty), no .gitmodules, and the path is absent from both the tree and the working copy.


Generated by Claude Code

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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CGAGAib517Ae1kg8SCdcEt
Peeja pushed a commit that referenced this pull request Sep 23, 2026
The Go test that replaced round one's shell guard still recognised less than
compose does: it matched only `${X:?msg}` and not `${X?msg}`, which compose
rejects identically, and it keyed its walk on the filename compose.yml, so an
included file with another name or a compose.yaml was invisible. Both proved
with the real client rather than argued. Now `:?\?` over every .yml and .yaml
under the smelt root.

The body also repeated #18's nested-path miscount -- "seven across
systems/*/compose.yml", a glob that matches thirteen files and reaches none of
the indexer's -- in the pull request that fixes the nesting.

And the round caught Current State: it said #18 was the only open forge pull
request, and a later paragraph still listed #19, which merged and is #23's
base. Both corrected; the open pair is now #18 and #23.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CGAGAib517Ae1kg8SCdcEt

@Peeja Peeja left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[From Claude:]

Round three on cf5c105d. Three findings, none blocking. The widening is correct and every detection round one and two built still fires — all eleven rows of the verification table reproduce, plus ten more mutations. The findings are one inaccurate claim made for the widening (it has a false-pass direction, and the widening enlarges its surface), one grammar case the pattern diverges from compose on, and one sentence in the skippable-checks block this PR's own new file falsifies.

1. "Over-reading is the safe direction here" is not unconditionally true — there is a false-PASS direction, and widening the walk enlarges its surface

published_images_test.go:70-75 and the round-two section of the PR body both state that over-reading errs safe. That holds for a variable the Go tables do not carry: the extra name fails check 1 (is a required compose interpolation but is not in envImageOptions) and the test goes red. Mutations 12-14 below confirm it for non-compose YAML.

It inverts for a variable the Go tables do carry. wantVars is derived from envImageOptions ∩ publishedImages and never from required, so a spurious match at a name already in both tables does not add anything — it keeps a name that should have dropped out, and masks the disagreement the test exists to catch.

Realistic shape: pin an image literally and leave the old interpolation commented above it.

   swarf:
-    image: ${SWARF_IMAGE:?must be set (stack.WithPublishedImages() ...)}
+    # image: ${SWARF_IMAGE:?must be set (stack.WithPublishedImages() ...)}
+    image: ghcr.io/fil-forge/swarf:pinned-while-debugging

Compose stops requiring SWARF_IMAGE; .env.published, publishedImages and envImageOptions all still carry it; the test stays green.

                                    compose w/o SWARF_IMAGE   test
comment left behind                 exit=0 (not required)     ok     <- false pass
control: same edit, no comment      exit=0 (not required)     FAIL   (line 192)

Compose behaves identically in both rows — only the leftover comment changes the verdict, which is what makes it a masking case rather than a compose subtlety.

This is not introduced by the widening (round one's walk read whole files too), but the widening does widen the surface: the masking text no longer has to sit in a compose file. ${UPLOAD_IMAGE:?…} appearing in systems/upload/config/config.yaml, a grafana YAML, or a snapshots/*/smelt.yml now pins UPLOAD_IMAGE into required just as well.

Suggested: either drop the unqualified claim, or say which direction is safe and which is not.

2. `$${X_IMAGE:?}` is a literal to compose, and a required variable to the test

published_images_test.go:78-83 justifies the pattern against compose's grammar. Verified with the real client (v5.1.1) that it is right about the forms it names and the ones it excludes — ${X:?} and ${X?} both error, ${X:-}, ${X:+} and ${X-} do not. The one form it misses is the escape.

$ tail -1 smelt/compose.yml
x-probe: $${NEWSVC_IMAGE:?literal}

$ docker compose --env-file .env.published --env-file .env config | grep probe
x-probe: $${NEWSVC_IMAGE:?literal}          # emitted verbatim, no error

$ grep -oE '\$\{[A-Z0-9_]+_IMAGE:?\?' smelt/compose.yml
${NEWSVC_IMAGE:?                            # the test demands an entry for it

Same for an interpolation inside a YAML # comment: compose interpolates the parsed model, not the file text, so it never sees it.

Direction here is the safe one — NEWSVC_IMAGE is in neither Go table, so the test fails rather than passing. Worth a line in the grammar comment rather than a code change, since a fix means parsing YAML instead of grepping it and the failure is loud.

3. PR body: "`.env.published`'s contents reach no check" is falsified by this PR's own new file

The skippable-checks block says:

.env.published's contents reach no check. smelt/pkg/snapshot/stack.go:30 os.Stats the path and passes --env-file; it never opens the file … Grepped unrestricted — every other mention in the tree is prose.

stack.go:30 is right. But published_images_test.go:162 opens and parses the file:

env := readEnvPublished(t, filepath.Join(root, ".env.published"))

That runs in ci / unit smelt. So its contents do reach a check, and the "every other mention is prose" grep is wrong at this head — there are four code mentions in the new file.

The conclusion is unaffected: unit smelt is already named as not skippable, so nothing a reviewer would decide changes. The evidence is what is wrong, and it is the block AGENTS.md says to derive rather than assert.

Verification — what was run and what checked out

Checkout at cf5c105d, GOWORK=off, no Docker daemon, Compose client v5.1.1.

All eleven rows of the body's table reproduce, each seen against the committed tree first:

0.  committed tree                                        PASS
1.  INDEXER_IMAGE dropped from .env.published             FAIL
2.  PIRI/HILT values swapped                              FAIL
3.  PIRI_IMAGE bound twice                                FAIL
4.  SWARF_IMAGE tag differs from the Go table             FAIL
5.  required var added to smelt/compose.yml               FAIL
6.  entry dropped from publishedImages                    FAIL
7.  var dropped from envImageOptions                      FAIL
8.  ${X?msg} bare-question form                           FAIL
9.  required var in an included file named newsvc.yml     FAIL
10. required var in a compose.yaml                        FAIL

Ten more of my own:

11. required var in a deeply nested new compose.yml       FAIL  (walk depth ok)
12. required var in grafana datasources.yaml              FAIL  (over-read, safe dir)
13. required var in systems/upload/config/config.yaml     FAIL  (over-read, safe dir)
14. required var in snapshots/*/smelt.yml                 FAIL  (over-read, safe dir)
15. ${NEWSVC_IMAGE:-default}                              PASS  (correctly ignored)
16. ${NEWSVC_IMAGE:+alt}                                  PASS  (correctly ignored)
17. ${NEWSVC_IMAGE-dflt}                                  PASS  (correctly ignored)
18. $${NEWSVC_IMAGE:?literal}                             FAIL  -> finding 2
19. required var inside a YAML comment                    FAIL  -> finding 2
20. generated/compose/{piri.yml,workspace.override.yml}   PASS  (dirty checkout ok)

Does the walk reach everything compose does — yes, strictly more. All 12 root include: paths plus the two nested ones under systems/indexing/ are relative, inside the smelt root, and none is built at run time or comes from a variable. make up's COMPOSE passes no -f except -f compose.yml -f generated/compose/workspace.override.yml, both inside the root; SNAPSHOT=<path> feeds only the manifest, never a compose file. pkg/stack extracts EmbeddedFiles to a temp dir, and the embedded set is exactly the include set. generated/compose/piri.yml is gitignored and absent in a clean checkout — PIRI_IMAGE comes from pkg/generate/compose.go:70; when present it adds nothing new (mutation 20). The walk additionally reads 29 YAML files, of which only 7 carry matches, and two compose files (systems/telemetry/, systems/stress-tester/) that are in neither the include list nor embed.go.

One bounded gap, checked and closed: compose also interpolates --env-file contents, which the walk does not read —

$ cat envfile.env
A_IMAGE=${NEEDED_IMAGE:?required from inside an env file}
$ docker compose --env-file envfile.env config
failed to read envfile.env: required variable NEEDED_IMAGE is missing a value

Not a hole in practice: readEnvPublished does not interpolate, so such a line would be compared as its literal ${…} text against publishedImages and fail. Not reported as a finding.

Wrong-root canary still works. composeFiles now counts any YAML, so it no longer proves a compose file was read — but require.NotEmpty(t, vars) covers it. Rooted at systems/telemetry/ (YAML present, no matches): Should NOT be empty, but was map[].

Body numbers, all re-derived:

ls smelt/systems/*/compose.yml | wc -l                              13
  ... | grep -c indexer                                              0
go list -f '... GoFiles={{len .GoFiles}} TestGoFiles={{len .TestGoFiles}}' ./pkg/stack/
                                          GoFiles=10 TestGoFiles=4
go list -deps -test ./tests/e2e/...          | grep -c .../pkg/stack  0
go list -deps -test -tags e2e ./tests/e2e/.. | grep -c .../pkg/stack  1

The corrected comments-only command prints nothing (grep exits 1). "Seven named in YAML under smelt/, plus PIRI_IMAGE from the generator" is exact: DELEGATOR, HILT, INDEXER, INGOT, SIGNER, SWARF, UPLOAD in YAML; PIRI_IMAGE only in pkg/generate/compose.go. .env.published has 8 entries, publishedImages 8 rows.

Wiki (wiki branch, fetched fresh at 03342ba): Current State names #18 and #23 as the two open PRs, records main as 4611cbcb (matches origin/main), and no longer lists #19 — it names it merged as 82aaa35b, which is in this branch's history. #18 reads state: open, draft: false, mergeable_state: clean, matching "Open, not a draft … waiting on your merge". Needs Human Work line 32 carries #23 at cf5c105d, the current head, not stale.

Otherwise: GOWORK=off go test ./... across smelt is green except the three known daemon tests (build_test.go skips on exec.LookPath("docker"), which finds the client here — body's description accurate, fails the same on origin/main). go vet ./pkg/stack/ clean, gofmt -s -l . empty, all seven check-*.sh exit 0, ci.yml parses. No test writes YAML into the walk root (generate_test.go:252 uses t.TempDir()).


Generated by Claude Code

…e 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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CGAGAib517Ae1kg8SCdcEt

@Peeja Peeja left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[From Claude:] Round four, head b5ce12ec, scoped to cf5c105d..b5ce12ec plus the PR body.

Not satisfied — 3 findings. The parse rewrite is correct and loses nothing: across all 29 YAML files under the smelt root the parsed walk and a raw regex agree exactly, and every construction I could build (anchors, aliases, merge keys, block scalars, multi-document, flow style, quoted and non-string keys, !!binary, !reset/!override) reaches the walker. Round three's findings 1 and 2 are genuinely fixed — mutations 11 and 12 now fail. But the class finding 1 named survives in three other places, one of them newly introduced by this diff and justified by a comment that measures false; and the body is still at round two.

1. `requiredImageVars` still reads three sources compose never requires, and each masks drift at a name the Go tables carry

The commit says: "Parsing removes the case rather than documenting it." It removes two sub-cases (# comments, $${X:?}) and leaves three. The mechanism is unchanged from round three's finding 1 — wantVars comes from the two Go tables, never from what compose requires, so any over-read at an existing name keeps it in required and hides the drift.

Each was confirmed by pinning hilt's (or piri's) image literally, so compose stops requiring the variable, and checking the test still goes green. The control — same pin, without the over-read — fails correctly in all three.

over-read source pinned image + name present only there control
YAML map key pass (masked) fail
pkg/generate/*_test.go literal pass (masked) fail
non-compose YAML under the root pass (masked) fail

(a) Map keys — new in this diff. walkYAMLStrings calls fn(k) on keys, justified by "compose interpolates throughout, not only in values." Compose does not interpolate keys. Measured with the real client (v5.1.1) in four key positions — service name, environment key, labels key, top-level x- key — none required the variable, and compose re-escaped the $ on output, which is it telling you it treated the key as literal text:

$ cat compose.yml
services:
  hilt:
    image: ghcr.io/fil-forge/hilt:main
    labels:
      ${HILT_IMAGE:?must be set}: marker

$ env -u HILT_IMAGE docker compose config
...
    labels:
      $${HILT_IMAGE:?must be set}: marker
exit=0

Compose exits 0 with HILT_IMAGE unset. The test on the equivalent tree passes. That is round three's finding 1 reintroduced, with the justifying comment asserting the opposite of the measured behaviour.

(b) pkg/generate/*_test.go. The .go case matches every file under pkg/generate, test files included, and nothing in a _test.go is emitted into a compose file. This one is a live hazard rather than a contrived one: generate_test.go already asserts on emitted interpolations verbatim ("${SMELT_PIRI_0_PORT:-15100:3000}" at lines 115–116 and 143–145). The analogous assertion for the image line is the natural next test to write, and it would pin PIRI_IMAGE in required permanently. Confirmed: pin compose.go:70 to a literal and add _ = "${PIRI_IMAGE:?must be set}" to generate_test.go — test green; without the mention — fail.

(c) Non-compose YAML. Lines 99–104 still say "Over-reading is the safe direction here and costs nothing measurable" about scanning every .yml/.yaml, and lines 126–128 of the same function retract that exact sentence. The retraction is the correct one, and it applies to file selection too: appending zz_note: "see ${HILT_IMAGE:?must be set}" to systems/telemetry/config/prometheus.yml — a service config compose never loads — masks a pinned hilt image.

The "costs nothing measurable" half is still true today (I confirmed the pattern matches only compose files across all 29), but it is a fact about the tree, not about the check — which is the distinction the PR itself draws about resolve-rewrite-conflicts.sh and gofmt.

All three narrow the same way: require the match to come from a compose file's string values.

2. The body still claims `.env.published`'s contents reach no check — round three's finding 3, unfixed, and falsified by this PR's own test

The skippable-checks block still reads:

.env.published's contents reach no check. smelt/pkg/snapshot/stack.go:30 os.Stats the path and passes --env-file; it never opens the file, and its test writes empty fixtures in t.TempDir(). Grepped unrestricted — every other mention in the tree is prose.

The snapshot half is accurate (line 30's loop os.Stats and passes --env-file, never reading it). The claim is falsified by the file this PR adds:

env := readEnvPublished(t, filepath.Join(root, ".env.published"))

readEnvPublished opens and parses it, and four of the eleven table rows fail on nothing but its contents — missing entry, swapped values, duplicate binding, differing tag. I re-ran all four; all four fail.

The block's conclusion survives — unit smelt is one of the two checks it does not propose skipping — so nothing merges wrongly on this. But AGENTS.md is explicit that these blocks are derived, not asserted, and are being kept as the worked examples the real path-filtering will be designed from. A derivation that is false in its own PR is the wrong worked example to keep.

3. The body is still at round two: no round-three section, and it asserts a claim the head commit retracts

The body has not been refreshed since cf5c105d. Three things follow from that, all checkable:

It says two rounds. The heading is "The check, and two rounds of it being wrong" and "Getting there took two rounds", with <details> for rounds one and two. Round three changed the implementation substantively — raw byte scan to yaml.v3 plus go/parser — and has no section.

It asserts what the code retracts. The round-two <details> ends:

Over-reading is the safe direction and costs nothing measurable: across every YAML file there — grafana dashboards and service configs included — the pattern matches only compose files.

The head commit's own comment says that was "true only for names the tables do not carry". The body and the code it describes now disagree about the reasoning the change rests on.

The table is eleven rows and round three added two more. Every one of the eleven is still accurate — I re-ran all of them and all eleven fail as stated. But the two cases round three's rewrite exists for, commented-out interpolation with the image pinned and escaped $${X:?}, are not in it. Both now fail correctly, so they are rows the table has earned.

AGENTS.md rule 8's refresh-on-every-push applies to the range links; the skippable block carries the same instruction explicitly. The mutation table is the body's load-bearing evidence and is in the same position.

Verification — what I checked and what held

Checkout b5ce12ec, GOWORK=off throughout, compose client v5.1.1, no daemon.

Does parsing lose anything? No. A probe running the parsed walk and a raw regex over all 29 YAML files under the smelt root reports zero divergence in either direction, and zero parse errors. The eight required variables derive as the body says — seven from YAML (DELEGATOR, HILT, INDEXER, INGOT, SIGNER, SWARF, UPLOAD) plus PIRI_IMAGE from pkg/generate/compose.go:70.

walkYAMLStrings type coverage. yaml.v3 decodes mappings into map[string]any, falling back to map[any]any when any key is non-string (confirmed with int, bool and time.Time keys — note 2026-01-01 decodes to time.Time, not a string). Both are handled. Every construction reached the walker and the variable was still demanded: anchor+alias, alias to a scalar, merge key, block literal |, block folded >, multi-document, flow mapping, flow sequence, quoted key, plain key, non-string keys, !!binary, !!str, sequence-of-maps, and compose's own !reset / !override tags plus an unknown custom tag. Nothing string-shaped lands in a skipped type.

$$ blanking. Exact agreement with the real client on every run of $ I tried — the left-to-right pairing in ReplaceAll matches compose's lexer:

source compose test
$${X:?} not required not required
$$${X:?} required required
$$$${X:?} not required not required
$$$$${X:?} required required
${X:?}$$ required required
${X?msg} required required

scanGo. 369 STRING literals across the six pkg/generate files, 0 strconv.Unquote failures. Raw strings unquote correctly including a literal $$ and an embedded CR; a hex-escaped "\x24{E_IMAGE:?m}" unquotes to a match the raw byte scan would have missed. The fallback branch is unreachable for a literal from a file go/parser accepted. No _IMAGE interpolation in pkg/generate is built by concatenation or fmt.Sprintf — the only Sprintf-built interpolations are ports, and they use :- defaults, so neither approach is missing anything.

Parse-failure policy. Every current file parses under yaml.v3 specifically. Plausible additions that would fail, and so fail the whole test for an unrelated reason: an unquoted Helm template (image: {{ .Values.image }} → invalid map key), a Helm control block, a duplicate mapping key, tab indentation, a complex ? [a,b] key, a non-map merge target. GitHub Actions ${{ }} expressions parse fine, quoted Helm values parse fine. Direction: a loud unrelated failure, never a silent pass — the safe side.

Mutations — 15 run. All twelve established cases fail correctly: missing entry, swapped values, duplicate binding, differing tag, required var in smelt/compose.yml, bare-? form, included newsvc.yml, systems/compose.yaml, entry dropped from publishedImages, entry dropped from envImageOptions, commented-out interpolation with the image pinned, escaped $${X:?}. The three new ones are finding 1's table.

Hygiene. go vet ./pkg/stack/ clean; gofmt -s -l empty on the new file and the tree; go mod tidy leaves go.mod/go.sum byte-identical, and gopkg.in/yaml.v3 was already a direct require on origin/main, so the untouched go.mod is correct. TestBuild*ImageTagFormat fail for want of a daemon, identically on origin/main; ignored per the brief.

Body claims that held. git diff --name-only origin/main...HEAD is the four files listed. The options.go comments-only grep emits nothing, as printed. systems/*/compose.yml matches 13 files and reaches none of the indexer's. 7 check-*.sh guards. GoFiles=10 TestGoFiles=4. The e2e counts reproduce exactly — 0 without the tag, 1 with — when run from smelt/. ci.yml is untouched and parses. smelt/pkg/snapshot/stack.go:30 is the loop that os.Stats and passes --env-file without reading. publishedImages lists all eight including the indexer. I did not re-derive the independently reported 20-mutation round.


Generated by Claude Code

… 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/<x>/ 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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CGAGAib517Ae1kg8SCdcEt

@Peeja Peeja left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[From Claude:] Round five, b5ce12ec..b9423bc0 (one file) plus the rewritten body.

Not satisfied. Five findings. One is a measured claim that is wrong, in the committed comment and in the body. Four are gaps between the file set the test reads and the file set compose reads — all in the under-read direction, none live in this tree today. The set the test derives is exactly what compose demands right now: with generated/compose/piri.yml generated, docker compose --env-file .env.published --env-file .env config exits 0, and dropping any one of the eight makes compose name exactly that one. The nineteen-row matrix reproduces in full, including every "round N's test passed" annotation, each mutation proven applied.

1. `composeFileNames` omits the four `*.override.*` names compose also auto-discovers — a required variable in one is unchecked

composeFileNames says it is "the names docker compose discovers on its own in a directory it is run from". It is not. Compose auto-loads a base file and an override file. All four override spellings, against the client:

compose.override.yaml    -> AUTO-LOADED (required var seen)
compose.override.yml     -> AUTO-LOADED
docker-compose.override.yml   -> AUTO-LOADED
docker-compose.override.yaml  -> AUTO-LOADED
$ cat compose.yml           services: {a: {image: busybox:latest}}
$ cat compose.override.yml  services: {b: {image: "${OVERRIDE_ONLY_IMAGE:?set me}"}}
$ docker compose config
error while interpolating services.b.image: required variable OVERRIDE_ONLY_IMAGE is missing a value: set me

Against this tree, both the make up directory and a standalone system directory:

file added, carrying ${X_IMAGE:?} compose TestPublishedImageSetAgrees
smelt/compose.override.yml requires it pass
systems/hilt/compose.override.yml requires it pass

This is the live path, not a corner. COMPOSE passes no -f unless generated/compose/workspace.override.yml exists, so a plain make up runs with auto-discovery on — and -f is what suppresses the override (verified: with -f compose.yml -f extra.yml the override is not loaded). So the default make up is precisely the mode where an unseen compose.override.yml would break it, and SMELT_WORKSPACE=1 is the mode where it would not.

Adding the four names to the map is the whole fix.

2. `extends: file:` is a second way compose reads a file; the comment says `include:` is the only one

"and reaches anything else only through an include:" — published_images_test.go:146-148, and the same sentence in the body's Round four section.

Compose reads services.<x>.extends.file, and a required variable there is required:

$ cat compose.yml       services: {a: {extends: {file: base-parts.yml, service: tpl}}}
$ cat base-parts.yml    services: {tpl: {image: "${EXTENDS_IMAGE:?required via extends}"}}
$ docker compose config
error while interpolating services.tpl.image: required variable EXTENDS_IMAGE is missing a value: required via extends

Against this tree — systems/swarf/compose.yml gains an extends.file: base-parts.yml, the target carries ${EXTENDED_IMAGE:?…}: compose requires it, the test passes.

The target can be named anything, which is the same shape as round two's include:-target-named-newsvc.yml finding — that one is row 8 of the matrix and is caught; this one is its twin and is not. extends is absent from smelt's YAML today (grep -rn extends --include=*.yml --include=*.yaml smelt/ → nothing), so this is latent, but the comment asserting it cannot happen is what makes it worth fixing rather than noting.

3. The `os.Stat` skip leaves the test green on a tree where `docker compose config` will not load at all

"An include naming a file that is not there is compose's error to report, not this test's." — published_images_test.go:230-231

Compose's reaction to a missing include target is not a soft one:

$ cat compose.yml   include: [{path: nope.yml}] ; services: {a: {image: busybox:latest}}
$ docker compose config ; echo rc=$?
open …/nope.yml: no such file or directory
rc=1

The whole project fails to load — make up, make down, make status, all of it. Mistype one include path in smelt/compose.yml:

-  - path: systems/swarf/compose.yml
+  - path: systems/swraf/compose.yml

docker compose config → rc=1. TestPublishedImageSetAgrees → pass. The guard is green on a stack that cannot start, and it loses SWARF_IMAGE from required on the way without noticing, because the skip is silent.

The skip exists for exactly one target — generated/compose/piri.yml, which .gitignore excludes and which is absent in CI — and the comment does not say so, so a future reader has no way to tighten it safely. Narrowing the tolerance to that case (or to paths under an ignored directory) keeps CI working and closes the rest. Worth noting PIRI_IMAGE survives the skip today only because the pkg/generate string-literal scan is a second source; the include contributes nothing the test depends on.

4. Compose does not exit 0 on a `${X:?}` in a service name — it exits 1 before interpolation ever runs

published_images_test.go:71-76, repeated in the body's Round four section:

"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 $$"

Three of the four are right; I reproduced them exactly, $$ re-escaping and all:

services:
  a:
    environment:
      $${ENVKEY_IMAGE:?k3}: v
    labels:
      $${LABELKEY_IMAGE:?k4}: v
x-top-$${TOPKEY_IMAGE:?k1}: hello
=== exit 0 ===

The service name is not. Any ${…} there fails schema validation before interpolation:

name=svc-${A_IMAGE:?m}   rc=1  services additional properties 'svc-${A_IMAGE:?m}' not allowed
name=${B_IMAGE:?m}       rc=1  services additional properties '${B_IMAGE:?m}' not allowed
name=${C_IMAGE}          rc=1  services additional properties '${C_IMAGE}' not allowed
name=x$D                 rc=1  services additional properties 'x$D' not allowed

(--no-interpolate gives the same validation error, so it is the schema, not the interpolator.) The conclusion — keys are not interpolated — still holds, and the fix stands; it is the evidence sentence that names a case it did not measure. Dropping "a service name" from both copies is enough.

5. `include:` with `project_directory` redirects nested include resolution; `includePaths` always uses the including file's directory

includePaths(filepath.Dir(path), doc) is the default, but project_directory overrides it for relative paths inside the included file, nested include: among them:

compose.yml:  include: [{path: a/mid.yml, project_directory: b}]
a/mid.yml:    include: [{path: leaf.yml}]
a/leaf.yml:   ${LEAF_A_IMAGE:?req}      b/leaf.yml: ${LEAF_B_IMAGE:?req}

$ docker compose config
error while interpolating services.leaf_in_b.image: required variable LEAF_B_IMAGE is missing a value

Compose took b/leaf.yml. With the same shape grafted onto systems/indexing/compose.yml and only the real target present, compose requires RIGHTDIR_IMAGE and the test passes — it os.Stats a path that does not exist and drops it (finding 3 again). With both present it reads the wrong one.

Latent — no project_directory in the tree — and reading v["project_directory"] when present is a two-line change. Two smaller things from the same sweep, both latent and neither worth changing on its own: include.env_file supplies variables to the included file, so a required variable satisfied that way would make the test demand an .env.published entry nothing needs (false failure); and an include path that is a directory makes parse die on require.NoError from os.ReadFile with no message, where compose says is a directory.

Verification, and what checked out

Everything below is against b9423bc0 at the named checkout, Compose client v5.1.1, GOWORK=off, no daemon. Tree returned to b9423bc0 with git status --porcelain empty after every run.

The nineteen-row matrix, independently constructed. Every mutation prints a count proving it applied before the test runs — HILT_IMAGE and UPLOAD_IMAGE each appear twice in their compose files, so the mutations use SWARF_IMAGE (one occurrence, asserted ==1 in the patcher) and the .env.published rows assert the line count changed.

01 fixed tree                                    expected=pass  got=pass
02 INDEXER_IMAGE missing from .env.published     expected=fail  got=fail   occurrences 1->0
03 PIRI/HILT values swapped                      expected=fail  got=fail   PIRI=…/hilt:main HILT=…/piri:main
04 PIRI_IMAGE bound twice                        expected=fail  got=fail   PIRI_IMAGE lines=2
05 env ref whose tag differs                     expected=fail  got=fail   SWARF_IMAGE=…/swarf:v9.9.9
06 required var added to smelt/compose.yml       expected=fail  got=fail   NEWTHING_IMAGE=1
07 bare ${X?msg} form                            expected=fail  got=fail   bare-form=1
08 include: target named newsvc.yml              expected=fail  got=fail   include lines=1, file exists
09 a systems/compose.yaml                        expected=fail  got=fail   file exists
10 entry dropped from publishedImages            expected=fail  got=fail   indexing-service:main refs left=0
11 variable dropped from envImageOptions         expected=fail  got=fail   table entries left=0
12 interpolation commented out, image pinned     expected=fail  got=fail   live-interp=0, text still present=1
13 escaped $${X:?}                               expected=pass  got=pass   ESCAPED_IMAGE=1
14 interpolation inside a # comment              expected=pass  got=pass   COMMENTED_IMAGE=1
15 swarf pinned literally, no fake (control)     expected=fail  got=fail   live-interp=0, SWARF_IMAGE lines=0
16 control + fake in a YAML map key              expected=fail  got=fail   fake lines=1
17 control + fake in prometheus.yml              expected=fail  got=fail   prom fake lines=1
18 control + fake in pkg/generate/generate_test.go expected=fail got=fail   gen-test fake lines=1
19 restored                                      expected=pass  got=pass   git status clean

The italicised historical annotations reproduce too, by checking out each earlier revision of the test file into the same tree:

16/17/18 against b5ce12ec (round three)   pass / pass / pass   body: pass
15 control against b5ce12ec               fail                 body: fail
13/14 against cf5c105d (round two)        fail / fail          body: fail
07/08/09 against f9a86cfa (round one)     pass / pass / pass   body: pass

The set is right for this tree, end to end. go run ./cmd/smelt generate produces the missing include target, and then:

$ docker compose --env-file .env.published --env-file .env config   rc=0
$ docker compose config                                             SIGNER_IMAGE is missing a value

Dropping one variable at a time from .env.published makes compose name exactly that one, for all eight — delegator, hilt, indexer, ingot, piri-0, signing-service, swarf, upload. The test's required is [DELEGATOR HILT INDEXER INGOT PIRI SIGNER SWARF UPLOAD]. No over-read, no under-read, today. The test also passes with generated/compose/piri.yml present, so its result does not depend on whether the developer has generated.

Keys, the converse direction. Nothing a map[string]any/map[any]any rewrite could have dropped: decoded !override and !reset tags, merge keys, anchors and aliases, block scalars, flow style, multi-document files, and a map[interface{}]interface{} from non-string keys — in every case the values under them were reached. yaml.v3 yields map[string]interface{} for ordinary compose documents and falls back to map[interface{}]interface{} only for non-string keys, and both cases are handled.

Compose behaviours checked directly: profiles do not exempt a service from interpolation (an inactive-profile image is still required, so ignoring profiles is correct); ${X?msg} is required (row 7); $ runs of one to five pair left to right — odd required, even literal — which is what ReplaceAll(v, "$$", "\x00\x00") does; include must be a list and path may be a list (both matching includePaths); an include glob is not expanded by compose (correctly not expanded here either); a URL path is treated as a local path by this client; COMPOSE_FILE works but appears nowhere in the repo.

_test.go predicate. strings.HasSuffix(path, "_test.go") is exactly go/build's own rule, so nothing it excludes is a non-test file and nothing it keeps is a test file. pkg/generate has no subdirectories (five .go files, flat); WalkDir would descend into one if added, including testdata/, and parser.ParseFile(…, 0) ignores build constraints — both over-reads, both latent, neither worth a change.

PR body. Every derived number and command re-run: the four changed files; the options.go comments-only pipeline (empty); go list -deps -test ./tests/e2e/... → 0 and with -tags e2e → 1; GoFiles=10 TestGoFiles=4; systems/*/compose.yml matches 13; seven check-*.sh, all exit 0; gofmt -s -l . empty; go vet clean; ci.yml parses and does enumerate the seven guards one run: step each; ci / unit smelt is a real matrix leg. The corrected .env.published derivation holds — pkg/snapshot/stack.go os.Stats and passes --env-file without opening, its internal test writes fixtures in t.TempDir(), and the remaining mentions are prose. #18's body does say "three review rounds counted it wrong in both directions". The one drift: the body cites smelt/pkg/snapshot/stack.go:30 for the os.Stat, which is at line 31.


Generated by Claude Code

…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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CGAGAib517Ae1kg8SCdcEt

@Peeja Peeja left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[From Claude:]

Round six, on 1c27d270. Three findings.

Round five's five fixes are all real — each verified by execution, 34 mutation rows in all — and I could not reach a hole that any of them opened. On the brief's first question: the shape is a treadmill, and it does not have to be. docker compose config states the required set itself, in 1.4 s, with no daemon, and none of the 285 lines of hand-rolled compose grammar is load-bearing for the bug this PR fixes. One finding is a live masking hole at head, one is that recommendation, one is the PR body.

1. The shape: compose answers this itself in 1.4 s, and none of the 285 lines is load-bearing for the original bug — recommend replacing, not extending

Can the test ask compose instead? Yes, and it costs almost nothing.

docker compose config runs without a daemon. Looping it until it stops naming a missing variable yields exactly the eight, at the smelt root:

$ cd smelt && go run ./cmd/smelt generate        # generated/compose/piri.yml must exist
$ loop: docker compose config -q with an empty env, adding each named variable as a dummy
DELEGATOR_IMAGE  HILT_IMAGE  INDEXER_IMAGE  INGOT_IMAGE
PIRI_IMAGE       SIGNER_IMAGE  SWARF_IMAGE   UPLOAD_IMAGE
real 0m1.399s      (9 invocations: 8 failures + 1 success)

Repeating it in each of the 15 systems/ directories adds nothing to the set — measured, the union is the same eight.

Do not use docker compose config --variables. It is one call and 0.22 s, but it under-reports. Measured: with services.main.extends.file: base.yml, where base.yml has a second service carrying ${UNWANTED_IMAGE:?…}, --variables lists only WANTED_IMAGE — while compose itself exits 1 on UNWANTED_IMAGE. The iterative config -q loop matches enforcement; --variables does not.

--variables : required: ['WANTED_IMAGE']
config -q   : error ... required variable UNWANTED_IMAGE is missing a value    rc=1

How much grammar is load-bearing for the bug this PR fixes? None.

I stubbed requiredImageVars to a constant map — bypassing walkYAMLStrings, composeFileNames, includePaths, extendsPaths, the regex, the $$ blanking, the YAML parse loop, the include fixpoint and the pkg/generate AST scan — and re-applied both headline defects:

with all compose grammar stubbed out result
clean tree PASS
INDEXER_IMAGE removed from .env.published (the original bug) FAIL — "does not supply exactly the required images"
PIRI_IMAGE/HILT_IMAGE swapped (round one's headline catch) FAIL — "names a different image than publishedImages does"

wantVars is built from envImageOptions ∩ publishedImages and never from required, so the two assertions that catch the bug do not consult the grammar at all. The 285 lines buy exactly one property: compose requires this set and no more — rows 6–9, 12 and 15 of the matrix. That is the property compose states directly.

The dependency is already paid for. This is not a new requirement on ci / unit smelt:

  • pkg/stack itself shells out to docker in production code — build.go:29 (docker build), cleanup.go:29/46/57/99.
  • pkg/stack/build_test.go already calls docker in three tests.
  • ci.yml's own header: "The matrix needs a Docker daemon."

config needs only the client, so this is strictly weaker than what the job already depends on.

Recommendation: replace lines 67–351 (285 of 433) with the compose call. Keep lines 1–66 and 353–433.

Keep fieldReachedBy, readEnvPublished and the three assertions — that is the part that catches the bug. Drop walkYAMLStrings, composeFileNames, includePaths, extendsPaths and requiredImageVars, and with them the go/ast, go/parser, go/token, regexp and gopkg.in/yaml.v3 imports.

What that gains, beyond ending the treadmill:

  • both declared known limits close for free — I confirmed include.env_file really does supply a required variable (rc=0, image renders as alpine:from-include-env-file) and include.project_directory really does redirect nested include resolution (compose found altdir/deep.yml where sub/deep.yml does not exist);
  • the compose.yml-beats-docker-compose.yml precedence over-read closes — compose warns "Found multiple config files … Using compose.yml" and reads only one, while the basename walk counts both;
  • finding 2 below closes, which is a live masking hole today.

What it costs, honestly:

  • the test must fail, not skip, when docker is absent — a skip reintroduces exactly "not silent and not noticed";
  • it couples to one compose error string, required variable X is missing a value. That is new brittleness, but it fails loudly: if the string ever changes the loop cannot parse a variable out and must abort non-zero rather than concluding "no more variables". Written that way it cannot degrade into a false pass.

What would change my mind: if unit smelt ever stops needing a Docker daemon — if the testcontainers suites get tagged out of the default run — then the client stops being free and this becomes a real new dependency for the job, and the trade is worth re-deriving. Nothing else I measured argues for keeping the grammar.

2. Live at head: an unemitted string literal in `pkg/generate/*.go` counts as required, and masks a real drift

requiredImageVars scans every string literal in every non-test .go file under pkg/generate. A literal that never reaches the emitted compose file counts as a required variable just the same. That is round three's masking class — "a match on text compose does not interpolate keeps a name alive in required and hides the drift" — fixed for YAML by parsing, and still open for the generator.

Reproduced on 1c27d270:

Step 1  systems/swarf/compose.yml: image: ${SWARF_IMAGE:?...}  ->  image: ghcr.io/fil-forge/swarf:pinned
Step 2  pkg/generate/compose.go  += var _ = "${SWARF_IMAGE:?not emitted anywhere}"
Step 3  go run ./cmd/smelt generate
        grep -c SWARF_IMAGE generated/compose/piri.yml  ->  0      (never emitted)

Go test        : PASS   <-- drift masked
compose oracle : DELEGATOR_IMAGE HILT_IMAGE INDEXER_IMAGE INGOT_IMAGE
                 PIRI_IMAGE SIGNER_IMAGE UPLOAD_IMAGE            (SWARF_IMAGE absent)

So the tree ends up with .env.published and publishedImages both carrying an image nothing requires, and the guard says agreed. The control confirms the guard is otherwise working: the same swarf pin without the decoy fails, as do decoys placed in a YAML map key, in prometheus.yml, and in generate_test.go (matrix rows 15–18).

The existing comment anticipates the neighbouring case — "read its STRING LITERALS rather than its bytes … a commented-out interpolation would otherwise mask the same drift" — but a literal that is never emitted is not a comment, and it is not filtered.

The consumer here is compose reading generated/compose/piri.yml, not the generator's source. Reading the emitted file is what makes the deriver match its consumer, which is the principle every round so far has converged on; it is also what finding 1 does for free.

3. The PR body is stale for round five, and two of its round-four claims were measured false by round five

Not separate findings — one block, as asked.

Stale:

  • The heading "The check, and four rounds of it being wrong" — there have been five. Rounds one through four each have a <details> block; round five has none.
  • "Nineteen mutations, each seen to fail before being seen to pass" — the table carries no row for any round-five behaviour: no override-name row, no extends row, no mistyped-include row, no generated/ exception row.

Measured false, and already corrected in the code comments but not in the body:

  • Round four's block: "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". The service name does not. Measured with the client:

    services:
      "${SVCNAME_IMAGE:?in a service name}": {image: alpine}
    
    validating compose.yml: services additional properties
      '${SVCNAME_IMAGE:?in a service name}' not allowed          rc=1
    

    The other three do exit 0 and re-escape to $$, as claimed — I ran all four. walkYAMLStrings' comment was fixed in round five; the body still lists the service name among them.

  • Round four's block: "Compose discovers compose.yml/compose.yaml (and the docker-compose spellings) in a directory it is run from … and reaches anything else only through an include:, followed transitively." Both halves were falsified by round five — the four *.override.* names are auto-discovered too, and extends: {file: …} is a second way in. The body still states the totality claim the code no longer makes.

Not stale — re-derived at 1c27d270 and still correct: the skippable-checks block's file list (four files, unchanged), its options.go "comments only" derivation (the filter is still empty), and "All seven guards pass, gofmt -s -l . is empty, go vet clean" — I ran all seven guards (7 check-*.sh on disk, all PASS), gofmt -s -l . is empty and go vet ./pkg/stack/ is clean.

Verification: round five's five fixes, the holes probed for, and the 34-row matrix

Environment. Compose client v5.1.1, no daemon; docker compose config parses fine without one. TestBuild{,Piri,Guppy}ImageTagFormat fail for want of a daemon, identically on origin/main — ignored per the brief. Tree clean at 1c27d270 before and after; GOWORK=off throughout.

Round five's five fixes — each verified by execution, each naming the exact variable (causation, not just a red test).

fix how proved result
four *.override.* names a required var in each of the four files, at the smelt root all four FAIL, each naming its own var
extends: file: followed direct, and transitive over two hops (compose.yml→extended.yml→extended2.yml, var only in the last) both FAIL, naming EXT_DIRECT_IMAGE / EXT_TRANS_IMAGE
missing include fatal systems/swarf → systems/swraf in the root include FAIL — "…includes …/systems/swraf/compose.yml, which does not exist"
generated/ exception five tree states, below all PASS
comments match behaviour service name rc=1; environment/labels/top-level x- keys rc=0 with $$ re-escape; both known limits reproduced all as written

Compose independently agreed each mutation was real: config -q named OVERRIDE_YML_IMAGE, OVERRIDE_DCYAML_IMAGE, EXTENDS_DIRECT_IMAGE, EXTENDS_TRANSITIVE_IMAGE; and on the mistyped include, with all eight dummies supplied so it could get that far, open …/systems/swraf/compose.yml: no such file or directory rc=1.

Holes probed for in the round-five additions — none reachable.

  • extends as a string (short form). Compose accepts extends: base (rc=0); it names a service in the same file, so there is no file to follow. extendsPaths' type assertion correctly declines it. Confirmed PASS on a tree carrying one.
  • services not a map[string]any. yaml.v3 yields map[any]any only when a service key is non-string — and compose rejects exactly that: non-string key in services: 8080, rc=1. (on:, yes: and "on" all stay strings, checked.) Unreachable through a compose-valid file, so extendsPaths returning nil there cannot bite. Worth noting walkYAMLStrings handles map[any]any and includePaths/extendsPaths do not — the asymmetry is real but not reachable.
  • Queue dedupe. Built a file reached by both an include: and an extends.file in the same project: FAIL naming DEDUPE_IMAGE. Dedupe is on filepath.Abs, and filepath.Join cleans, so the two spellings collapse and the variable is still collected once.
  • Fatal-missing-include in legitimate tree states. Built all five: no generated/ at all (fresh clone / actions/checkout@v4); make nuke (generated/ present, generated/compose removed); empty generated/compose/; make clean (piri.yml survives, workspace override gone); and SMELT_WORKSPACE=1 with generated/compose/workspace.override.yml present. All five PASS. generated/compose/piri.yml is the only legitimately-absent include in the tree — the other 13 are tracked. The workspace override carries no _IMAGE interpolation (it only bind-mounts binaries), and its basename is not auto-discoverable, so it is correctly invisible.
  • Compose !reset / !override tags. yaml.v3 decodes them without error; test PASSES on a tree carrying one that compose accepts at rc=0.

Checked and judged not a finding. A malformed include: — a scalar instead of a list, or an entry whose path: is a number — is silently ignored by includePaths while compose fails the whole project (`include` must be a list). Structurally the same shape round five made fatal, but the consequence differs: it drops only include targets whose names are not auto-discoverable, everything else is still reached by the filesystem walk, and compose refuses the project unconditionally for everyone, so nothing can drift to main quietly. Flagging for the record rather than as a defect; finding 1 removes it along with the rest.

The matrix: 34 rows. Rows 1–19 are the PR body's nineteen, re-run at head; rows 20–34 are new. Each mutation was proved to have applied — by an occurrence count on the mutated file, by compose's own verdict on the same tree, or (for deletions) by the resulting failure message naming the deleted item. Two mutations initially failed to apply and were caught by that check before being read as results: a !reset probe that landed in a volumes: block, and an occurrence-count guard that read deletions backwards.

# row expected got
1 control: fixed tree pass pass
2 INDEXER_IMAGE removed from .env.published fail fail
3 PIRI/HILT values swapped fail fail
4 PIRI_IMAGE bound twice fail fail
5 an env ref whose tag differs fail fail
6 new required var in smelt/compose.yml fail fail
7 the bare ${X?msg} form fail fail
8 include target named newsvc.yml fail fail
9 a systems/compose.yaml fail fail
10 entry dropped from publishedImages fail fail
11 var dropped from envImageOptions fail fail
12 interpolation commented out, image pinned fail fail
13 escaped $${X:?} pass pass
14 interpolation inside a # comment pass pass
15 control: swarf pinned literally, no decoy fail fail
16 …+ decoy in a YAML map key fail fail
17 …+ decoy in prometheus.yml fail fail
18 …+ decoy in generate_test.go fail fail
19 …+ decoy in pkg/generate/compose.go (non-test) fail pass — finding 2
20–23 required var in each of the four *.override.* names fail fail ×4
24 extends.file direct fail fail
25 extends.file transitive, two hops fail fail
26 mistyped include fail fail
27 same file via both include and extends fail fail
28 compose !reset tag pass pass
29 extends short form ignored correctly pass pass
30–33 the four generated/ tree states pass pass ×4
34 restored pass pass

Row 19 is the only surprise, and it is finding 2.


Generated by Claude Code

… 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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CGAGAib517Ae1kg8SCdcEt

@Peeja Peeja left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[From Claude:]

Round seven, on 7f04a8b8. Two findings.

The simplification is sound and I could not break the part that remains: 29 mutations at this head, and the three assertions catch every one of the drift cases the PR exists for, including all eight of "an entry dropped or added in each of the three". fieldReachedBy and readEnvPublished re-verified from scratch — every way readEnvPublished diverges from compose's dotenv parser errs toward a false failure, measured against the client, and every degenerate option shape fails loudly rather than silently. The stub proof, the 285/433 figure, the six-round table, the nine-row table, ~1.4 s over 9 invocations and the --variables under-report all reproduce. Both findings are about what the remaining test's claims cover: one is a coverage delta the body's concrete enumeration does not name, one is a corner where two of the three can genuinely disagree and pass.

1. The measured coverage delta is wider than the one row the mutation table names — a required `${X:?}` reverting to a silent `${X:-…}` default now passes every test and every guard

The body's abstract sentence is complete. "That compose requires exactly this set and no more" covers everything below. But the concrete example — "a service added to a compose file with a required image variable, and to neither Go table nor .env.published" — and the table row "a required var only in a compose file — now unchecked, by decision" both state only the over-supply direction: compose gaining a name. The under-supply direction, compose stopping requiring a name this repository still publishes, is the one with a named consequence in two other documents, and it is not named here.

Measured, each mutation proved to have applied, run against both heads:

mutation at 1c27d270 at 7f04a8b8 other coverage
(a) systems/hilt/compose.yml: ${HILT_IMAGE:?…} → ${HILT_IMAGE:-ghcr.io/fil-forge/hilt:main} FAIL PASS all 7 guards green
(b) same file: interpolation → image: ghcr.io/fil-forge/hilt:main@sha256:0000… FAIL PASS all 7 guards green
(b′) same, literal without a digest FAIL PASS check-stack-images.sh RED
(c) systems/guppy/compose.yml: ${GUPPY_IMAGE:-…} → ${GUPPY_IMAGE:?…} FAIL PASS all 7 guards green
(d) {"PIRI2_IMAGE", WithPiriImage} added to envImageOptions and to .env.published FAIL PASS all 7 guards green
(e) new required var in a compose file, in neither Go table — the named row FAIL PASS all 7 guards green

(a) is the one with teeth. It is the exact regression the required form exists to prevent, and two documents in this PR's own diff say so:

  • .env.published, line 3: "the compose interpolations for these images used to carry a silent default (${HILT_IMAGE:-ghcr.io/fil-forge/hilt:main}), so a caller that forgot to pass an override booted a published image and never heard about it."
  • smelt/CLAUDE.md, "Built here": "That silent fallback used to let a test that forgot its overrides pass while exercising published code instead of the commit under test."

check-stack-images.sh does not cover it — its escape hatch is case "$value" in \$\{*) continue, so any ${…} value is skipped, :- and :? alike. Verified: the guard is green on (a) and on (b), and red only on (b′), where the literal lacks a digest. Nothing in pkg/stack notices either — I ran TestPublishedImageSetAgrees, TestWithPublishedImages* and TestOptionsFromEnv* together on each tree.

$ # (a), at 7f04a8b8
$ grep -c 'HILT_IMAGE:-' smelt/systems/hilt/compose.yml          2
$ go test ./pkg/stack/ -run 'TestPublishedImageSetAgrees|TestWithPublishedImages|TestOptionsFromEnv'
ok
$ for g in .github/scripts/check-*.sh; do bash "$g" >/dev/null || echo "RED: $g"; done
(no output)

(c) and (d) are not "in neither Go table". (c) is a required compose variable that is in envImageOptions; (d) is a variable in envImageOptions and .env.published that compose never asks for. Both were caught by wantVars == gotRequired, which is gone.

Not asking for the derivation back — the decision is taken and I agree with it. The ask is that the "what this deliberately no longer checks" block and the mutation table say both directions, since the table is what a reader scans, and that (a) is named explicitly given that two documents in this diff describe its consequence as the reason the required form exists.

2. A `publishedImages` entry whose field no `envImageOptions` entry reaches is compared against nothing — the one way two of the three genuinely disagree and pass

wantVars is built by iterating varToField and keeping the vars whose field is also in fieldToRef. fieldToRef is never iterated. So a publishedImages entry whose config field no override option reaches is dropped from the join silently: it is not required in .env.published, its reference is never compared, and nothing reports it.

Realistic shape — a new service where the option is the copy that gets forgotten:

pkg/stack/options.go:
  config += newsvcImage string
  publishedImages += {"ghcr.io/fil-forge/newsvc:main", func(c *config) *string { return &c.newsvcImage }}
  (no envImageOptions entry, no .env.published line)

go test ./pkg/stack/ -run 'TestPublishedImageSetAgrees|TestWithPublishedImages|TestOptionsFromEnv'   ok
7 guards                                                                                             all green

WithPublishedImages() then fills a field that reaches no compose variable, and make up has no reference for it — the same failure mode as the bug this PR fixes, arrived at from the other side. Forgetting only .env.published is caught (that is the original bug); forgetting envImageOptions and .env.published is not.

This is not a regression from the simplification — 1c27d270 passed on it too, because required never contained a name for a variable that does not exist. But three documents are new or rewritten at this head and all three state the property this corner breaks:

  • test header: "The published image set is stated in three places, and they must agree"
  • .env.published: "it checks this file against publishedImages and envImageOptions, name by name AND reference by reference"
  • options.go: "this table against .env.published and envImageOptions above, name by name and reference by reference"

Five lines close it, and they are green on the tree as it stands — verified both ways:

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)
}
control, patched                      ok
+ the mutation above                  FAIL: publishedImages publishes newsvcImage,
                                            which no envImageOptions entry reaches

With that, wantVars covers all of publishedImages by construction and the "name by name" claim is true without a corner.

Verification: 29 mutations at this head, the parser divergences measured against the client, and every PR-body number re-derived

Environment. Compose client v5.1.1, no daemon; docker compose config parses without one. TestBuild{,Piri,Guppy}ImageTagFormat — exactly three, each gated on exec.LookPath("docker") at build_test.go:13/41/61 — fail here and identically on origin/main; ignored per the brief. GOWORK=off throughout. Tree clean before and after; the smelt/generated/ I produced for the compose oracle was removed.

The 29 mutations. Each proved to have applied (occurrence count on the mutated file, or the failure message naming the deleted item). "old" is the same mutation run against 1c27d270's test file on the same tree.

# mutation expected new old
1 control: fixed tree pass pass pass
2 .env.published: INDEXER_IMAGE dropped — the original bug fail fail —
3 .env.published: FOO_IMAGE added fail fail —
4 publishedImages: indexer entry dropped fail fail —
5 publishedImages: a guppy entry added fail fail —
6 envImageOptions: INDEXER_IMAGE dropped fail fail —
7 envImageOptions: NEWPIRI_IMAGE → WithPiriImage added fail fail —
8 .env.published: PIRI_IMAGE tag differs fail fail —
9 .env.published: PIRI/HILT values swapped fail fail —
10 .env.published: PIRI_IMAGE bound twice, same value fail fail —
11 .env.published: PIRI_IMAGE bound twice, different value fail fail —
12 .env.published: value carrying a literal = fail fail —
13 .env.published: spaces around key and value fail fail —
14 .env.published: double-quoted value fail fail —
15 .env.published: export prefix fail fail —
16 .env.published: trailing # comment fail fail —
17 .env.published: CRLF throughout pass pass —
18 envImageOptions: duplicate env name (PIRI_IMAGE twice) fail fail —
19 envImageOptions: two names → piriImage, both supplied fail pass fail
20 publishedImages: same field twice fail fail —
21 an option that sets zero fields fail fail —
22 an option that sets two fields fail fail —
23 publishedImages: entry for a field no option reaches fail pass pass
24 …realistic: new field + publishedImages, nothing else fail pass pass
25 …same but .env.published line added, option still missing fail fail fail
26 compose: new required var, in neither Go table pass, by decision pass fail
27 compose: HILT_IMAGE :? → :- default fail pass fail
28 compose: hilt pinned literally, with a digest fail pass fail
29 compose: GUPPY_IMAGE :- → :? fail pass fail
— restored pass pass pass

Rows 19, 23–24 and 27–29 are findings 1 and 2. Row 17 is correct behaviour, not a gap — strings.TrimSpace strips the \r and compose resolves the same value; verified against the client.

readEnvPublished vs compose's dotenv parser. Each form run through docker compose --env-file … config on a one-service project. Every divergence is in the direction of a false failure; none can produce a false pass.

form compose readEnvPublished errs
export PIRI_IMAGE=… accepts, strips export key becomes export PIRI_IMAGE → set mismatch fails
PIRI_IMAGE="…" / '…' accepts, strips quotes quotes kept → reference mismatch fails
PIRI_IMAGE = … accepts, trims both key/value keep inner spaces fails
PIRI_IMAGE=… # comment accepts, strips comment comment kept in value fails
duplicate binding accepts, last wins explicit require.False(dup) fails (deliberate, documented)
PIRI_IMAGE=a=b value a=b strings.Cut on first = → a=b agrees
CRLF accepts TrimSpace strips \r agrees

Also stricter and untested-for because they cannot arise silently: a multi-line quoted value (continuation line has no = → hard fail), a BOM, and ${…} expansion inside the env file.

fieldReachedBy. Reading unexported fields through reflect.ValueOf(c).Field(i).String() is legal (only Interface() panics) and demonstrably works — rows 21–22 report piriImage and [piriImage hiltImage] by name. Non-string fields are skipped by the Kind() guard, so an option writing into serviceBinaries, a nested struct or a slice yields found == 0 and require.Len(found, 1) fails loudly; two string fields yield found == 2 and fail the same way. The sentinel cannot collide: var c config is zero-valued and mutate is the only writer. A nested-struct refactor of config would fail this test rather than pass it quietly. Note publishedImages' distinct-field guard is also asserted independently by TestWithPublishedImagesEachEntryHitsADistinctField in options_env_test.go — harmless duplication, flagged only so it is not mistaken for the only copy.

The stub proof, reproduced. requiredImageVars replaced by a constant map of the eight, the five helper imports dropped, everything else at 1c27d270:

clean tree                                          PASS
INDEXER_IMAGE removed from .env.published           FAIL  "does not supply exactly the required images"
PIRI_IMAGE/HILT_IMAGE swapped                       FAIL  "names a different image than publishedImages does"

Exactly as the body claims. wantVars never consults required.

285 of 433. The old file is 433 lines; walkYAMLStrings' doc comment through the close of requiredImageVars — walkYAMLStrings, composeFileNames, includePaths, extendsPaths, requiredImageVars and their comments — is lines 67–351, 285 lines. New file is 144. git diff --numstat: 42 331.

The six-round table. Each row checked against the commit that fixed it: one f9a86cfa (sets not pairs, and the wrong root), two cf5c105d (${X?}; compose.yml-only filename keying), three b5ce12ec (raw bytes: # comments and $$), four b9423bc0 (map keys, _test.go, non-compose YAML), five 1c27d270 (four auto-discovered names → eight, plus extends: file:), six (every string literal under pkg/generate, emitted or not). All six accurate. The round-six review's own heading was "Live at head: an unemitted string literal in pkg/generate/*.go counts as required", reproduced there with an injected decoy — the body's narrowing to compose.go matches where the decoy was placed; there is no such literal in the tree today, and the body does not claim there is. pkg/generate carries exactly two _IMAGE literals: compose.go:70 (${PIRI_IMAGE:?…}, emitted unless node.Image is set) and compose.go:190 (${MINIO_IMAGE:-…}, which the old regex never matched).

The compose oracle. Reproduced at the smelt root after go run ./cmd/smelt generate:

$ loop: docker compose config -q with an empty env, adding each named variable as a dummy
invocations: 9
DELEGATOR_IMAGE HILT_IMAGE INDEXER_IMAGE INGOT_IMAGE
PIRI_IMAGE SIGNER_IMAGE SWARF_IMAGE UPLOAD_IMAGE
real 0m1.518s

Nine invocations, the same eight, 1.5 s against the claimed ~1.4 s. --variables under-reporting also reproduced, on the exact shape described:

compose.yml : services.main.extends.{file: base.yml, service: main}
base.yml    : main -> ${WANTED_IMAGE:?}, other -> ${UNWANTED_IMAGE:?}

config --variables : {"WANTED_IMAGE": {... "Required": true}}          <- UNWANTED_IMAGE absent
config -q (WANTED supplied) : required variable UNWANTED_IMAGE is missing a value   rc=1

The three documents. .env.published's header, smelt/CLAUDE.md:474 and options.go:415 all now describe a three-way name-and-reference comparison and all three say it does not read the compose files. No leftover "does the whole derivation", "four statements" or "compose interpolations" claim anywhere: grep -rn "whole derivation\|four statements\|four places\|stated in four\|requires exactly this set" over the tree returns only .env.published's own two lines, both correct. The surviving drift silently hits are the two deliberate "this file used to claim…" corrections. CLAUDE.md's derivation snippet still yields seven and PIRI_IMAGE still appears in no compose file — both re-run.

The origin/main claims in the fourth-copy block. Verified verbatim: origin/main's smelt/CLAUDE.md names six (PIRI, HILT, INGOT, UPLOAD, DELEGATOR, SIGNER), lists indexer and swarf under "Not built here", and closes with "SWARF_IMAGE joins the required set when the branch that builds swarf lands."

The skippable-checks block, re-derived at this head. Four files in git diff --name-only origin/main...HEAD. The options.go comments-only filter is empty. go list -deps -test ./tests/e2e/... → 0, with -tags e2e → 1, and GoFiles=10 TestGoFiles=4. .env.published's contents are read by this test alone — pkg/snapshot/stack.go:30 only os.Stats it before passing --env-file; nothing under .github/ mentions it at all; every other hit is prose or a t.TempDir() fixture. ci.yml untouched, parses, and both named jobs exist (unit smelt in the matrix, guards running gofmt -s -l .). The block's closing cost sentence is present.

Housekeeping. All 7 check-*.sh pass, gofmt -s -l . empty, go vet ./pkg/stack/ clean, staticcheck ./pkg/stack/ clean, ci.yml parses. The body's note that ci.yml enumerates the guards by hand is correct — 7 scripts on disk, 7 named steps, none missing today.


Generated by Claude Code

…e 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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CGAGAib517Ae1kg8SCdcEt

@Peeja Peeja left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[From Claude:] Round eight — 7 findings. The new test's central claim splits: its under-reading half holds in every shape I could build, and its over-reading half does not. Three distinct shapes let the exact :? → :- downgrade the test exists to catch pass, and a fourth passes on the comment's own HILT_IMAGE example. Two figures in the rewritten body are wrong where the body says they were measured.

1. `$${X:?}` counts: the YAML parse removes # comments but not $$ escapes, so an escaped interpolation satisfies the check while compose requires nothing

smelt/pkg/stack/published_images_test.go:206:

required := regexp.MustCompile(`\$\{([A-Z0-9_]+_IMAGE):?\?`)

$${SWARF_IMAGE:?…} contains the substring ${SWARF_IMAGE:?, so the regex matches. Compose renders $$ as a literal $ and requires nothing. Parsing the YAML does not help — $$ survives into the decoded scalar.

Repro at b35c8275, systems/swarf/compose.yml: line 10 downgraded to ${SWARF_IMAGE:-ghcr.io/fil-forge/swarf:main}, and one line added under environment::

      - NOTE=$${SWARF_IMAGE:?not really required, this is an escaped dollar}
$ go test ./pkg/stack/ -run TestBuiltImagesAreRequiredNotDefaulted
ok      github.com/fil-forge/forge/smelt/pkg/stack

Compose's own answer on the same tree (the config-until-clean oracle):

invocations=8
required: SIGNER_IMAGE DELEGATOR_IMAGE INDEXER_IMAGE PIRI_IMAGE UPLOAD_IMAGE HILT_IMAGE INGOT_IMAGE

SWARF_IMAGE is gone from what compose requires, and the test is green. This is the $$ half of round three's finding; the body's table pairs # comments and $$ escapes as one row, and only the comment half is fixed. A leading-context guard ((^|[^$])\$\{) or stripping $$ before matching closes it.

2. Two compose-named files are read that no compose project in this repository loads; a required occurrence in one satisfies the check for a variable defaulted everywhere compose looks

isComposeFile (:209-211) matches by filename shape. Sixteen files under smelt/ match. Fourteen are reachable from smelt/compose.yml's include: (:18-30, plus systems/indexing/compose.yml's own two). Two are not:

systems/stress-tester/compose.yml
systems/telemetry/compose.yml

Neither is named anywhere outside its own directory:

$ grep -rn 'systems/telemetry\|systems/stress-tester' --exclude-dir=.git . \
    | grep -v '^./smelt/systems/telemetry/\|^./smelt/systems/stress-tester/'
(no output)

— not in smelt/compose.yml, not in smelt/Makefile, not in embed.go, not in smelt.yml — and no service of either appears in docker compose config --services at the smelt root.

Repro: downgrade systems/swarf/compose.yml:10 to ${SWARF_IMAGE:-…} and add to systems/telemetry/compose.yml

  swarf-exporter:
    image: ${SWARF_IMAGE:?must be set}
Required: PASS

while the compose oracle on that tree returns the same 7-name set as finding 1. Name shape is not the same predicate as "a file compose opens", and the comment at :172-179 rests on it being one.

3. A never-emitted string literal in pkg/generate satisfies the check — the round-six row, reintroduced

The body's own seven-round table, row six: "it read every string literal in pkg/generate, emitted or not | what the generator emits". The branch at :222-242 reads every string literal in every non-test .go under pkg/generate again. go/parser removes comments; it does not distinguish emitted from dead.

Repro: downgrade pkg/generate/compose.go:70 to "${PIRI_IMAGE:-ghcr.io/fil-forge/piri:main}" and append

// deadPiriImage is never used to emit anything.
var deadPiriImage = "${PIRI_IMAGE:?must be set}"

func init() { _ = deadPiriImage }
Required: PASS

The branch comment at :224-228 states the right principle — "over-reading is the unsafe direction for this question" — and then names only the commented-out case, which is the one the parse does handle.

4. The comment's own HILT_IMAGE example passes: a variable referenced twice needs only one occurrence to stay required

:185-190: "Downgrading ${HILT_IMAGE:?...} to ${HILT_IMAGE:-ghcr.io/fil-forge/hilt:main} puts that back, and nothing else notices."

HILT_IMAGE is required at systems/hilt/compose.yml:10 (hilt) and :74 (hilt-init); UPLOAD_IMAGE at systems/upload/compose.yml:8 and :50. Downgrading any one of the four, alone:

downgraded line TestBuiltImagesAreRequiredNotDefaulted
systems/hilt/compose.yml:10 PASS
systems/hilt/compose.yml:74 PASS
systems/upload/compose.yml:8 PASS
systems/upload/compose.yml:50 PASS

Both HILT lines together fail, which is what the nine-row table's row measured.

On this tree the surviving occurrence is in the same compose project, so compose up still errors and nothing boots a published image — so this is not a live hole by itself. It is the reason findings 1–3 are holes rather than false alarms: any one satisfying occurrence anywhere is enough. The failure message at :266-269 ("…is not a required compose interpolation anywhere") is accurate; the function's doc line and the body's "asserts each of those images is a required compose interpolation, not one with a fallback" read as universal and are not.

5. "What is still not checked", item 3 is wrong in both halves — and the section says the list is measured

The row: "an interpolation replaced outright by a digest-pinned literal (that last one turns check-stack-images.sh red, so it is not invisible)", under a section closing "The list above is measured."

mutation Required test check-stack-images.sh
systems/swarf/compose.yml:10 → ghcr.io/fil-forge/swarf:main@sha256:96e7… FAIL green
both systems/hilt/compose.yml lines → digest-pinned literal FAIL green
one of the two hilt lines → digest-pinned literal pass green
systems/swarf/compose.yml:10 → ghcr.io/fil-forge/swarf:main (no digest) FAIL red

(a) It is checked, for every built-here image whose interpolation is not duplicated — swarf, indexer, delegator, signer, ingot, piri — and for hilt/upload when both occurrences go.

(b) A digest-pinned literal is precisely the form the guard wants (check-stack-images.sh:41, *@sha256:*) echo "ok …"), so it never turns it red. Only a non-digest literal does, and then the test fails too. There is no shape in which the test passes and the guard goes red.

The one shape that does pass is the partial replacement, which is finding 4, not what the row says.

Items 1 and 2 of the same list reproduce exactly as written: a new service carrying ${NEWSVC_IMAGE:?must be set} and in neither Go table nor .env.published passes both tests; ${GUPPY_IMAGE:-…} promoted to ${GUPPY_IMAGE:?…} passes both.

6. The compose-oracle figures reproduce only on a generated tree; on a clean checkout the oracle cannot run at all, which the stated cost omits

The numbers are right — I reproduced them to the digit:

invocations=9
required: SIGNER_IMAGE DELEGATOR_IMAGE INDEXER_IMAGE PIRI_IMAGE UPLOAD_IMAGE SWARF_IMAGE HILT_IMAGE INGOT_IMAGE
real    0m1.422s

but only after creating generated/compose/piri.yml, which smelt/compose.yml:24 includes and which smelt/.gitignore excludes. On a clean checkout:

$ docker compose --env-file /dev/null -f compose.yml config
open …/smelt/generated/compose/piri.yml: no such file or directory     (rc=1)

$ oracle loop
invocations=4 STOPPED: open …/generated/compose/piri.yml: no such file or directory

So the trade as stated — "It costs a dependency on the compose binary in unit smelt, which is the trade to weigh" (test comment :52-60, and the body's "Exact by construction, no daemon, ~1.4 s") — is missing half its cost: it also requires the generator to have run, and no ci.yml step does that. Worth naming, since this note is the documented path back to the property that was given up.

7. Minor — the skippable-checks block calls smelt/Makefile:23 prose; it is a live --env-file .env.published

The block: "smelt/pkg/snapshot/stack.go os.Stats the path and passes --env-file without opening it; every other mention in the tree is prose."

smelt/Makefile:23:ENV_FILES := --env-file .env.published --env-file .env

That is the same category the sentence just carved out for pkg/snapshot/stack.go, not prose. The block's conclusion is unaffected — nothing in CI runs make up, which is the bug this PR fixes — but a reviewer using that sentence to decide what can see the file is using a false one.

Verified, and what checked out

Head, hygiene. b35c8275; both tests pass. On smelt: go vet clean, staticcheck 2026.2.1 (0.8.1) clean, gofmt -s -l . empty tree-wide, go mod tidy no diff — gopkg.in/yaml.v3 was already a direct require (go.mod:16), so the new import adds no churn. All seven .github/scripts/check-*.sh pass, and the directory holds exactly seven.

Interference. -race -shuffle=on (what ci.yml runs) passes at seeds 1, 2, 3 over the two new tests plus options_env_test.go's five. No shared mutable state: imageSentinel is a const, fieldReachedBy allocates a fresh config per call, walkStrings and readEnvPublished are pure, neither new test uses t.Setenv or t.Parallel, and neither reads the process environment. No ordering dependence.

Under-reading is safe — all four shapes asked about.

shape result
sole required occurrence in an include: target not named compose.* (systems/swarf/extra.yml, added to the root include) FAIL (safe), compose still requires it
compose file renamed off the pattern FAIL (safe)
generated/ absent and the generator's literal split so the regex misses it FAIL (safe)
multi-document compose file both documents read; compose v5 also reads document 2 — measured, it errors on services.swarf-doc2.image

Two claims in the comment, both correct. Keys are not interpolated (:273-275): labels: {"${KEYVAR:?must be set}": "x"} → docker compose config exits 0 and the output re-escapes to $${KEYVAR:?must be set}. And a commented-out ${HILT_IMAGE:?…} left above a downgraded line does not mask it — FAIL, as claimed. Separately, yaml.v3 decodes compose's !reset tag into any without error, so a v5 override file does not hard-fail the walk.

Nothing else catches the downgrade. check-stack-images.sh:36 does skip every ${…} value (\$\{*|\'\$\{*|\"\$\{*) continue ;;). No Go path compares the two forms — every ${…:?} / ${…:-} occurrence in smelt's Go is a doc comment or an emitted literal, none a comparison. The downgrade in the generator fails, as claimed.

The reached check (first test). Fires at :154 with publishedImages publishes swarfImage, which no envImageOptions entry reaches. Load-bearing: on a tree with SWARF removed from envImageOptions, from options_env_test.go's independent imageFields, and from .env.published, it is the only thing in the package that fails — buildEnv is hand-written, so TestWithPublishedImagesFillsUnsetFields still passes. Deleting the block from that tree leaves the package green but for the three daemon tests. No misfire: an alias option pointing at an already-published field passes (reached is a set of fields); the five options with no published entry (GUPPY / IPNI / PLC / BLOCKCHAIN / MINIO) are exempt by construction, since the loop iterates fieldToRef and not varToField, and 13 options against 8 published entries is green at head.

Nine-row table. All nine reproduce. Row 7, "a compose file deleted", holds for every compose file carrying a built-here variable; for completeness, deleting systems/telemetry/compose.yml passes (it carries none), and deleting smelt/compose.yml or systems/common/compose.yml "fails" as an embed.go compile error rather than as a test failure.

Seven-round table — every row checks out against the revision it describes.

row verified against
one the shell guard's sort -u over names and refs separately
two f9a86cfa: regex \$\{([A-Z0-9_]+_IMAGE):\? and case d.Name() == "compose.yml"
three cf5c105d: os.ReadFile + regex over raw bytes
four b5ce12ec: walkYAMLStrings(k, fn) on keys; HasSuffix(path, ".yml"/".yaml") for all YAML; pkg/generate branch with no _test.go exclusion
five b9423bc0: composeFileNames with exactly four names, zero extends mentions; 1c27d270 adds the four override names and extendsPaths
six 1c27d270: every string literal in every non-test .go under pkg/generate

285 of 433. The round-five file (1c27d270) is 433 lines; lines 67–351 — the walkYAMLStrings doc comment through the closing brace of requiredImageVars — are exactly 285. Round six's numstat on that file is 42 331.

Stub proof reproduced on a worktree at 1c27d270 with requiredImageVars replaced by a constant: unmutated passes; INDEXER_IMAGE removed from .env.published → ".env.published does not supply exactly the required images"; PIRI/HILT swapped → "HILT_IMAGE in .env.published names a different image than publishedImages does".

--variables under-reports, reproduced. compose.yml extending a service from base.yml, where base.yml also holds a non-extended other: image: ${OTHER_IMAGE:?must be set}:

$ docker compose -f compose.yml config --variables
NAME                REQUIRED            DEFAULT VALUE       ALTERNATE VALUE
$ docker compose -f compose.yml config
error while interpolating services.other.image: required variable OTHER_IMAGE is missing a value

Skippable block, the derived parts. git diff --name-only origin/main...HEAD returns exactly the four files listed; the options.go comments-only grep is empty; go list -deps -test ./tests/e2e/... → 0 and -tags e2e → 1; GoFiles=10 TestGoFiles=4; ci.yml untouched in the net diff. Only the "every other mention is prose" sentence is wrong (finding 7).

Ignored as instructed: TestBuildImageTagFormat, TestBuildPiriImageTagFormat, TestBuildGuppyImageTagFormat fail here for want of a Docker daemon, on origin/main too.


Generated by Claude Code

… 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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CGAGAib517Ae1kg8SCdcEt
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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CGAGAib517Ae1kg8SCdcEt

@Peeja Peeja left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[From Claude:]

Round nine, head a11c459d. Not satisfied: 4 findings. Round eight's four problems are each independently reproduced and each now fails, with a control per case proving the plant was visible. All four findings below are in TestBuiltImagesAreRequiredNotDefaulted; none is in TestPublishedImageSetAgrees or the .env.published fix.

Three of them share one root: the check became two-sided this round, and the two sides have opposite incompleteness directions. isRequired is existential (a missed file → loud false failure); isDefaulted is universal (a missed file → silent pass). The file set was narrowed this round on the strength of an argument that only covers the first.

A scope recommendation on whether this second test belongs in this pull request is at the bottom, in its own block. It is a recommendation, not a finding.

1. The generator is called with one of four storage shapes, and this repository's own smelt.yml uses a different one

The test calls the generator with a zero-value StorageSpec:

generate.GeneratePiriCompose([]manifest.ResolvedPiriNode{{Name: "piri-0", Index: 0}})

The committed smelt/smelt.yml resolves to something else — and manifest.Resolve() never produces the zero value, because firstNonEmpty fills it with sqlite/filesystem:

manifest: ../../../smelt.yml
resolved: [{Name:piri-0 Index:0 Image: Storage:{DB:postgres Blob:s3}}]

needsPostgres/needsS3 are therefore false in the test and true in the project, so buildPostgresService, buildPostgresInitService and buildMinioService are never reached. What each call emits:

generator called with image interpolations in the output
as the test calls it — 1 node, zero-value Storage ${PIRI_IMAGE…}
1 node, sqlite + filesystem ${PIRI_IMAGE…}
1 node, postgres + filesystem ${PIRI_IMAGE…}
1 node, sqlite + s3 ${MINIO_IMAGE…}, ${PIRI_IMAGE…}
1 node, postgres + s3 — what smelt.yml says ${MINIO_IMAGE…}, ${PIRI_IMAGE…}
3 nodes, mixed ${MINIO_IMAGE…}, ${PIRI_IMAGE…}
1 node with node.Image set (none)

Multiple nodes add nothing new — every node goes through the same buildPiriService — so node count is not the gap. Storage is. And the real generated/compose/piri.yml, generated from the committed manifest, confirms it:

$ go run ./cmd/smelt generate && grep -n image: generated/compose/piri.yml
5:   image: ${PIRI_IMAGE:?must be set …}
64:  image: ${MINIO_IMAGE:-ghcr.io/fil-forge/minio:RELEASE.2025-10-15…}
93:  image: postgres:16-alpine@sha256:cf78e766…
115: image: postgres:16-alpine@sha256:cf78e766…

Because isDefaulted is a universal, a missed branch is a silent pass. Demonstrated by planting a defaulted built-here image in each branch in turn:

plant branch reached by the test's call? test
${HILT_IMAGE:-…} as buildPostgresService's image no ok
${HILT_IMAGE:-…} inside buildPiriService's env yes fails: "has a DEFAULTED form in pkg/generate (emitted)"

Inert today, and only by luck of a table: MINIO_IMAGE is in envImageOptions but not in publishedImages, so built excludes it. The moment anything in that branch is added to publishedImages, the check goes quiet about it.

The comment says "the generator is CALLED rather than read, so what counts is what it emits." What it emits for this repository's own manifest is strictly more than what is scanned. Deriving rather than asserting (rule 3) means sweeping the branches — one call per (DB, Blob) pair is four calls and covers them all regardless of what any manifest says.

2. The file set was narrowed on an argument that covers only the required half — and compose.override.yml is a file make up loads and the test does not read

The header states one direction for the whole test:

So under-reading is safe here and OVER-reading is not, which is what the file set and the two exclusions below are for.

and again for the file set:

Missing one is safe by the direction above, so this needs none of the auto-discovery or extends: machinery the deleted derivation needed.

That holds for isRequired. It is the reverse for isDefaulted, which is new this round:

half quantifier a missed file means
isRequired[v] must be true ∃ the name is not found → loud false failure (safe)
isDefaulted[v] must be empty ∀ the default is not found → silent pass (unsafe)

So the same push that narrowed the file set added the check whose safety direction wants it wider. The two exclusions themselves are correct — systems/telemetry/compose.yml and systems/stress-tester/compose.yml are in no include list, no Makefile target and no embed set (smelt/CLAUDE.md even says stress-tester is "present but not wired into Makefile"), so a default in them is genuinely harmless. It is the stated reason, not the exclusions, that does not hold.

And the narrowing has a present-day hole. docker compose auto-discovers compose.override.yml in the project directory; make up runs docker compose $(ENV_FILES) up -d with no -f whenever the workspace override is absent, so auto-discovery applies. compose.override.yml is in no include list, is not gitignored, and is not read:

$ cat > smelt/compose.override.yml <<'Y'
services:
  hilt:
    image: ${HILT_IMAGE:-ghcr.io/fil-forge/hilt:OVERRIDE-MARKER}
Y
$ docker compose --env-file .env.published --env-file .env config --no-interpolate | grep HILT_IMAGE
212:    image: ${HILT_IMAGE:-ghcr.io/fil-forge/hilt:OVERRIDE-MARKER}      <- wins for `hilt`
249:    image: ${HILT_IMAGE:?must be set …}

$ GOWORK=off go test -count=1 ./pkg/stack/ -run TestBuiltImagesAreRequiredNotDefaulted
ok      github.com/fil-forge/forge/smelt/pkg/stack      0.039s

That is make up silently booting a published hilt when HILT_IMAGE is unset — the exact fault the required form exists to prevent — with the check green. b35c8275's file set did read this file (isComposeFile("compose.override.yml") is true); this round's does not.

Same class, empty today: generated/compose/workspace.override.yml, which the Makefile chains with -f under SMELT_WORKSPACE=1 and which pkg/stack/stack.go:585 writes for the SDK path. workspace.RenderOverride marshals a struct whose only field is Volumes []string, so no image line can appear in it as written — a hole with nothing in it, worth knowing about rather than fixing.

3. The test now reads gitignored build output, and its verdict flips with it — in both directions

generated/compose/piri.yml is in the root include: list, so the new walk reads it whenever it exists. It is .gitignored (smelt/.gitignore: generated/) and is written by make generate / make up. ci.yml's unit smelt job is checkout → go mod download → build/vet/staticcheck/tidy/test, with no generate step, so CI never has it and a developer who has run the stack always does.

b35c8275 did not read it — piri.yml is not compose.*-shaped — so this is new in this round's diff.

tree generated/compose/piri.yml present (developer) absent (CI)
generator's piri image changed to a plain literal; stale file still carries ${PIRI_IMAGE:?…} ok fails, "not a required compose interpolation anywhere"
generator correct; stale file carries ${PIRI_IMAGE:-…} (predates the :-→:? change on main) fails, "has a DEFAULTED form in ../../generated/compose/piri.yml" ok

The second row is the likely one in practice: anyone whose generated/ predates the required-form change gets a failure naming a gitignored file they never edited, and the message does not say make generate.

The code's own comment already supplies the argument against reading it — "its content is covered by calling the generator below" — so when the file happens to be on disk it contributes nothing the generator call does not, except staleness. (Finding 1 is why "covered" is currently only partly true; the answer to that is to fix the call, not to trust the artifact.)

Minor, in the same lines: the absent-include escape hatch is strings.Contains(path, "generated"), so an absent systems/generated-configs/compose.yml is also skipped silently — verified. A prefix check against the known generated directory would be exact.

4. The PR body describes machinery this head no longer has

Refreshing the body on every push is rule 8's habit and the skippable-checks block's rule; three load-bearing sentences describe b35c8275 or earlier.

…so it looks only at compose-shaped filenames and parses both the YAML and the generator's Go

Neither clause holds at a11c459d. The file set is the root compose plus its transitive include: targets: generated/compose/piri.yml is not compose-shaped and is read; systems/telemetry/compose.yml is compose-shaped and is not. And the generator is called, not parsed — which is the headline fix of this round.

Framed the first way it costs 60 lines instead of 285 and needs no include: following, no override names, no extends:.

It follows includes; includeTargets is 35 lines of exactly that, and the nested systems/indexing/compose.yml → ipni/indexer hop is how INDEXER_IMAGE — this PR's bug — is reached at all. The second check is now 186 of the file's 352 lines (38 comment, 90 body, 58 helpers), not 60.

Also stale, less critically: the "Nine mutations at this head" table predates the four cases this round added ($$ escape, unloaded-file plant, never-emitted literal, one-of-two downgrade), and the rounds table stops at seven.

The skippable-checks block itself still checks out — git diff --name-only origin/main...HEAD is the same four files, options.go is still comments-only (0 non-comment changed lines), and the unit smelt + guards call is right.

Verification, and what checked out

GOWORK=off throughout, in a worktree of my own; Compose client v5.1.1, no daemon.

The head moved from f693c50c to a11c459d mid-round — the S1011 spread, queue = append(queue, includeTargets(…)...). Behaviour-preserving, and not a finding. The matrix below was run at f693c50c; every row that could see that line was re-run at a11c459d with the same result, including all four findings above and the dropped-include row.

Round eight's four problems, each reproduced against this head, each with a control. The controls matter: a mutation that fails because the plant was never visible proves nothing.

# mutation expected got
A0 HILT's two required forms → plain literal, no plant (control: is the test sensitive?) fail fail
A1 + plain ${HILT_IMAGE:?} planted in included systems/common/compose.yml (control: is the plant visible?) pass pass
A2 + $${HILT_IMAGE:?} in the same place fail fail
B1 HILT neutralised, required form planted in systems/telemetry/compose.yml fail fail
B2 same, planted in systems/stress-tester/compose.yml fail fail
B3 round eight's exact case: both real HILT forms downgraded to :-, required planted in telemetry fail fail
C0 generator's piri image → plain literal, no plant (control) fail fail
C1 + a never-emitted "${PIRI_IMAGE:?…}" string literal in pkg/generate/compose.go fail fail
C2 never-emitted literal added, real required form left intact (control) pass pass
D1 hilt-init (line 74) downgraded, hilt (line 10) left required fail fail (isDefaulted)
D2 hilt downgraded, hilt-init left required fail fail
D3 upload-init (line 50) downgraded, upload (line 8) left required fail fail
D4 sole SWARF_IMAGE downgraded to the colon-less ${X-default} fail fail

The file set matches what compose actually loads. Replicating the test's walk and set-diffing against docker compose config --no-interpolate with the generated include present: identical, all thirteen names and every count, including HILT_IMAGE×2, UPLOAD_IMAGE×2, IPNI_IMAGE×2, MINIO_IMAGE×2. All eight built-here names are required and none is defaulted, which is the verdict the test gives. Include paths resolve against the including file's directory, which is what compose does — systems/indexing/compose.yml's relative ipni/compose.yml lands correctly.

Include following terminates, dedupes, and fails loudly on the right absences.

expected got
cycle: systems/common includes the root compose back terminate, pass pass
self-include + diamond (common and swarf both include indexing) terminate, pass pass
absent include that is not generated/ fail fail
the real absent generated/compose/piri.yml (the CI state) pass pass

Both regexes are exact. Twenty forms; every one classified as compose would:

${X_IMAGE:?e} ${X_IMAGE?e} ${X_IMAGE:?} ${X_IMAGE?}     -> REQUIRED
${X_IMAGE:-d} ${X_IMAGE-d} ${X_IMAGE:-} ${X_IMAGE-}     -> defaulted
${X_IMAGE}  $X_IMAGE  ${X_IMAGE:+a}  ${X_IMAGE+a}       -> neither
$${X_IMAGE:?e}  $${X_IMAGE:-d}                          -> neither ($$ escape)
$$${X_IMAGE:?e}                                         -> REQUIRED (escape then real)
${X_IMAGE:?-weird}                                      -> REQUIRED (not misread as defaulted)
${x_image:-d}  ${X_IMAGE_TAG:-d}                        -> neither (case, suffix)
${X_IMAGE:-${Y_IMAGE:?e}}                               -> X defaulted AND Y required

${X:-…} and ${X-…} and nothing else, as asked. Names not ending in _IMAGE match neither regex, which fails loudly on the required side before the defaulted side can go quiet.

Tree and CI. gofmt -s -l pkg/stack/ empty, go vet ./pkg/stack/ clean, go mod tidy no diff. GOWORK=off go test -shuffle=on ./... in smelt: everything green except TestBuildPiriImageTagFormat, TestBuildGuppyImageTagFormat, TestBuildImageTagFormat, which fail identically on an untouched origin/main worktree for want of a daemon — ignored as instructed.

Recommendation on scope — NOT a finding: split, do not drop

The premise checks out. On an origin/main worktree with ${HILT_IMAGE:?…} downgraded to ${HILT_IMAGE:-…} at systems/hilt/compose.yml:74:

check-assert-released-version.sh  rc=0     check-module-paths.sh   rc=0
check-base-images.sh              rc=0     check-pg-healthchecks.sh rc=0
check-goreleaser-ldflags.sh       rc=0     check-replaces.sh       rc=0
check-stack-images.sh             rc=0

$ GOWORK=off go test -shuffle=on ./...   # in smelt/
only the three TestBuild*ImageTagFormat failures, which fail unmutated too

Nothing. And this PR touches no compose file and not the generator — git diff --stat origin/main..HEAD is .env.published, CLAUDE.md, options.go (comments only, 0 non-comment changed lines) and the new test file. So the second test is an addition, not a restoration.

Is the property at risk, or only hypothetically? At risk, and this diff is part of why.

  • check-stack-images.sh names the rule in its own failure text — "the compose entry should be ${SERVICE_IMAGE:?...} instead" — while its case "$value" in \$\{*) continue skips every ${…} value. A guard that states the rule and does not enforce it is exactly the shape rule 5 warns about.

  • This diff adds a consumer of the property. smelt/CLAUDE.md's new block stops hand-listing the built-here set and tells the reader to derive it — with a command keyed on :?:

    $ find systems -name compose.yml -exec grep -hoE '\$\{[A-Z_]+_IMAGE:\?' {} + | sed … | sort -u
    DELEGATOR_IMAGE HILT_IMAGE INDEXER_IMAGE INGOT_IMAGE SIGNER_IMAGE SWARF_IMAGE UPLOAD_IMAGE
    
    $ # after downgrading HILT_IMAGE to :- …
    DELEGATOR_IMAGE INDEXER_IMAGE INGOT_IMAGE SIGNER_IMAGE SWARF_IMAGE UPLOAD_IMAGE
    

    Hilt disappears, silently. So "merely adjacent" understates it: the diff introduces a documented derivation that is wrong if the property breaks. That is a dependency.

But correcting one detail of the framing: .env.published's assertion of the property ("used to carry a silent default … They are now required") is context in this diff, not added by it — the only changed lines in that header are "seven"→"eight" and the drift paragraph. smelt/CLAUDE.md's assertion is rewritten here, and carries the new derivation. One of the two documents makes checking it here coherent; the other does not.

What it costs either way. 186 of the new file's 352 lines are the second test (38-line comment, 90-line body, 58 lines of helpers) — 53% of the diff, for a property the make up fix does not touch. It has now consumed rounds seven, eight and nine, and every finding in all three has been in it: this round, four findings, all four in the second test, none in the first or in the fix. Carrying it keeps a one-line repair to a broken make up behind a check that is still moving.

Would either half stand alone? Both, cleanly.

  • The cut is at line 166. walkStrings and includeTargets have exactly two call sites, both inside the second test, and nothing outside the file uses them. The first test uses only imageSentinel, fieldReachedBy and readEnvPublished, all above the cut.
  • The follow-up needs nothing from this PR: built is derived from envImageOptions × publishedImages, and options.go's diff here is comments only, so both tables are byte-identical to main.

Recommendation: split. Merge the fix — the INDEXER_IMAGE line, the .env.published header, options.go's comments, TestPublishedImageSetAgrees and the CLAUDE.md rewrite — and move TestBuiltImagesAreRequiredNotDefaulted to its own pull request. Not drop: the property has a live consumer in this very diff and no enforcement anywhere, so the test should exist; it just should not be what make up's repair waits on.

What would change it. Keep it here if findings 1–3 collapse into one push rather than another round — they are small (sweep the four storage combinations; read the override names compose auto-discovers; skip paths under generated/) — or if you would simply rather take one more round than carry a second PR; the cost of being wrong is a round, not a red main. Drop it only if the decision is that this property belongs to the ask-compose check the file's own header already describes (docker compose config against an empty environment, ~1.4 s, no daemon), in which case this test is scaffolding for its own replacement and CLAUDE.md's derivation should stop keying on :?.


Generated by Claude Code

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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CGAGAib517Ae1kg8SCdcEt

@Peeja Peeja left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[From Claude:]

Round ten, head d92200e6. Not satisfied: 3 findings — all in the PR body, none in the code.

Round nine's three code findings are each independently reproduced as fixed, with a control per case. Nothing came back in TestBuiltImagesAreRequiredNotDefaulted: its four storage shapes are exactly what manifest.validateStorage admits, every generator branch that can emit an image is now reached and each is caught by a planted default, the generated/ skip gives an identical verdict in both directions and at the prefix boundary, and all twelve mutation rows in the body re-derive at this head. All three findings below are wrong numbers and wrong history in the body's prose.

1. "225 lines … against 433 for the revision it replaced" sets a check against a whole file; the like-for-like number is 285

Framed the first way it needs neither of the two most intricate parts of the deleted machinery … It is 225 lines including its two helpers, against 433 for the revision it replaced.

225 is right, and so is 225 of this file's 392 lines — the file splits 166/1/225 at the blank line 167. 433 is also right as a file line-count. But the two are not the same unit, and the comparison reads as a halving that did not happen.

At 1c27d270 — the 433-line revision — the file held one test function:

$ git show 1c27d270:smelt/pkg/stack/published_images_test.go | grep -c '^func Test'
1
$ git show d92200e6:smelt/pkg/stack/published_images_test.go | grep -c '^func Test'
2

That one function was TestPublishedImageSetAgrees, which still exists at this head and is not part of the 225. So the 433 includes the other check, its helpers and the package header; 225 is the second check only.

The same block already carries the correct comparator two sentences earlier — "285 of its 433 lines were that machinery" — and the test file's own comment repeats it (line 44). Measured three ways, all of them smaller than the sentence implies:

same-unit comparison then now
the compose-reading machinery it replaced (body's own figure) 285 225
by function boundaries at 1c27d270 (walkYAMLStrings…requiredImageVars, lines 82–352) 271 225
the whole file 433 392

The load-bearing one is the first: 225 against 285, a 21% reduction, not 225 against 433. This matters because the body's own decision section cites the size to argue the split case both ways.

2. The body says round nine had three findings, twice, and two code findings once; it had four

Round nine's verdict line:

Round nine, head a11c459d. Not satisfied: 4 findings. … All four findings below are in TestBuiltImagesAreRequiredNotDefaulted

and in its scope block:

this round, four findings, all four in the second test, none in the first or in the fix.

The four, by their own headings: (1) one storage shape of four, (2) the file set narrowed on an argument covering only the required half / compose.override.yml, (3) gitignored build output, (4) the PR body describes machinery this head no longer has.

Three places in the body undercount them:

  • Nine rounds → "All three of round nine's findings live in that gap." Round nine said something different and weaker: "Three of them share one root" — three of four.
  • One decision taken here → "the same round's stated condition for keeping was that its three findings collapse into a single push". The condition itself is paraphrased correctly — round nine wrote "Keep it here if findings 1–3 collapse into one push" — but 1–3 were three of four, not all of them.
  • Verified → "Round nine's two code findings were each measured against round eight's shape as well as this one." Three of the four were code findings; only (4) was about the body. Both measurements it then gives are correct — I reproduced them — it is the count that is wrong.

The Nine rounds table's round-nine row has the same gap: the generator at one storage shape of four; gitignored build output names findings 1 and 3 and omits 2, although the body elsewhere documents 2's fix at length (the compose.override.yml paragraph).

Round nine's finding 4 was that the body described machinery the head no longer had. A body that then miscounts that same round three times is the same class recurring, one layer up.

3. "that last one turns check-stack-images.sh red" is backwards — a digest-pinned literal is exactly what that guard accepts

So does a not-built-here image being promoted from ${GUPPY_IMAGE:-…} to ${GUPPY_IMAGE:?…}, and an interpolation replaced outright by a digest-pinned literal (that last one turns check-stack-images.sh red, so it is not invisible).

The guard's own header states the rule it enforces:

# Every image a compose file names must be pinned by digest, or be a variable.

A digest-pinned literal satisfies it. Measured at this head, replacing ${GUPPY_IMAGE:-…} with ghcr.io/…:main@sha256:96e7…:

new test                    pass   (correct — GUPPY is not built here)
check-stack-images.sh       rc=0   GREEN

Only a literal without a digest turns it red:

…/guppy:main (no digest)    check-stack-images.sh  rc=1   RED

So that row is not "not invisible" — it is the one item on the list that nothing catches at all, which is the opposite of what the parenthetical tells a reader.

Under the other reading of the same clause — a built-here interpolation replaced by a digest-pinned literal — the claim fails differently: the new test catches it, so it does not belong on a "still not checked" list.

${HILT_IMAGE:?…} ×2 -> ghcr.io/…@sha256:96e7…
  new test                  FAIL   published_images_test.go:326 (HILT_IMAGE not required anywhere)
  check-stack-images.sh     rc=0   GREEN

The note under this section says "The list above is measured." This row is the one that is not.

Verification, and what checked out

Own detached worktree at d92200e6, removed after. Every mutation asserted present in the tree before its run; generated/compose/ removed explicitly between rows, since git clean -fd leaves ignored paths behind.

Brief 1 — the generator's storage shapes. manifest.validateStorage admits exactly DB ∈ {sqlite, postgres} × Blob ∈ {filesystem, s3}, so the test's 2×2 loop is the complete enumeration. Those four shapes reach every service the generator can emit — buildPiriService (all), buildPostgresService + buildPostgresInitService (needsPostgres), buildMinioService (needsS3) — plus the two conditional env blocks inside buildPiriService. A defaulted form planted in each, one at a time:

1 buildPiriService base image (:? -> :-)            expect=fail got=fail
2 buildPiriService postgres env branch              expect=fail got=fail
3 buildPiriService s3 env branch                    expect=fail got=fail
4 buildPostgresService                              expect=fail got=fail
5 buildPostgresInitService                          expect=fail got=fail
6 buildMinioService                                 expect=fail got=fail

And the counterfactual that makes the loop load-bearing — round eight's single zero-value call, with the buildPostgresService plant still in place:

round-8 shape + buildPostgresService plant          expect=pass got=pass

smelt.yml sets no per-node image:, so the generator's input here matches what make generate really resolves on the ${PIRI_IMAGE:?…} branch.

Separately, all eight built-here names are genuinely covered, not just the ones the table exercises — removing each one's requirement everywhere in turn:

PIRI_IMAGE (2 sites)  HILT_IMAGE (2)  INGOT_IMAGE (1)  UPLOAD_IMAGE (2)
DELEGATOR_IMAGE (1)   SIGNER_IMAGE (1) SWARF_IMAGE (1) INDEXER_IMAGE (1)
  -> all eight: expect=fail got=fail

Brief 2 — gitignored build output. Verdict identical with and without smelt/generated/compose/piri.yml, both directions:

stale file carrying a DEFAULT          present: pass   absent: pass
source lost PIRI's requirement         present: fail   absent: fail

The filepath.Rel prefix test cannot be fooled and cannot miss. Defaulted plants that must be READ (fail) and SKIPPED (pass):

systems/regenerated/compose.yml   read  -> fail       generated/compose/extra.yml     skipped -> pass
generatedstuff/compose.yml        read  -> fail       generated/nested.yml via ../../ skipped -> pass
systems/generated/compose.yml     read  -> fail

The trailing separator in "generated"+string(filepath.Separator) is what stops generatedstuff/, and filepath.Join normalisation is what stops a ../../ route from escaping the check. require.NoError fires on a missing non-generated/ target (published_images_test.go:288, reading ../../systems/nope/compose.yml). Deleting an existing include target instead fails earlier, at smelt/embed.go's //go:embed — so that path never reaches the assertion, which is why the plant above adds a target rather than removing one. And CI's unit smelt job is checkout → build → vet → staticcheck → test with no generate step, as the comment says.

One thing checked and left alone: the skip is keyed to the root-relative prefix, so a nested directory named generated/ would be read even though smelt/.gitignore's pattern would ignore it at any depth. No such directory exists and nothing in the include graph reaches one, so there is nothing wrong at this tree — noting it only because "only what the repository commits" is a slightly wider claim than the prefix implements.

Brief 3 — compose.override.yml. The reasoning is sound and the comment states it accurately. Makefile:36 is COMPOSE = $(DOCKER) compose $(ENV_FILES) $$(test -f $(WORKSPACE_OVERRIDE) && echo "-f compose.yml -f $(WORKSPACE_OVERRIDE)") — no -f unless the workspace override exists, so auto-discovery is live in the common case, exactly as written. Neither file is committed (git ls-files finds no compose override; workspace.override.yml is under the ignored generated/). I would not read it: the only regression it could catch lives on one developer's disk and can never reach main, while reading it reintroduces precisely the CI-vs-local split round nine found in generated/ — and on the universal isDefaulted side, where a file present locally and absent in CI flips the verdict silently. The comment's contingency ("If this repository ever commits an override file, it belongs in this walk") is the right trigger.

Brief 4 — the twelve-row table, re-derived independently. Every row holds.

 1 fixed tree                                         both      pass   pass
 2 INDEXER_IMAGE absent from .env.published           Agrees    fail   fail
 3 PIRI_IMAGE / HILT_IMAGE values swapped             Agrees    fail   fail
 4 publishedImages entry no option reaches            Agrees    fail   fail
 5 one of HILT's TWO :? downgraded to :-              Required  fail   fail  (line 330, isDefaulted)
 6 same downgrade in the generator's EMITTED output   Required  fail   fail
 7 control: ${HILT:?} planted in a LOADED file        Required  pass   pass
 8 same plant escaped as $${…:?}                      Required  fail   fail
 9 same plant where no project loads                  Required  fail   fail  (telemetry AND stress-tester)
10 same plant in the generator's SOURCE only          Required  fail   fail
11 a file dropped from the root include:              Required  fail   fail
12 restored                                           both      pass   pass

Rows 7–10 neutralise both of HILT_IMAGE's genuine :? occurrences to a literal first, as the body says. The sanity row that makes that meaningful — neutralised with no plant at all — fails, so the plant really is the only thing that could satisfy row 7.

Brief 5 — the body's other claims, all correct. options.go is comments-only (the body's own command emits nothing). walkStrings and includeTargets have call sites only at lines 298–299, inside the second test. go list -deps -test ./tests/e2e/... greps 0 and -tags e2e greps 1; GoFiles=10 TestGoFiles=4; both itest modules do reach pkg/stack. .env.published's contents are read by nothing but the new test — pkg/snapshot/stack.go:30 only os.Stats it. Seven check-*.sh guards exist and all seven pass; ci.yml does enumerate them by hand, one run: step each. gofmt -s -l . empty, GOWORK=off go build and go vet clean; the three TestBuild*ImageTagFormat fail for want of a daemon, unrelated. CLAUDE.md's derive command emits the seven names and misses PIRI_IMAGE exactly as stated. The first two rows of What is still not checked measure as claimed (a new required var in neither table passes; GUPPY_IMAGE promoted to :? passes) — only the third is wrong, as finding 3 says.


Generated by Claude Code

Peeja pushed a commit that referenced this pull request Sep 23, 2026
Round ten reproduced all three of round nine's code fixes as genuinely fixed
and found nothing in either test, so the scope call's flip condition did not
fire and the second check stays in #23. That is now settled rather than
pending.

All three of its findings were false claims in the pull request body, which
makes the last four findings across two rounds prose about the code rather
than the code.

One of them is a fact about this repository and not only about the body: a
compose image replaced by a digest-pinned LITERAL is accepted by
check-stack-images.sh, whose rule is "pinned by digest or a variable" -- so for
a not-built-here image that change is caught by nothing at all. Measured, not
read: guard rc=0 and the new test passing on the same mutated tree. Recorded
here because closing it is a change to the guard, not to #23.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CGAGAib517Ae1kg8SCdcEt

@Peeja Peeja left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[From Claude:]

Round eleven, head d92200e6. Not satisfied: 4 findings — all in the PR body; none in the code.

Brief was numbers and facts only, so no prose is reported. Two of the four are in the corrections round ten prompted: the 225-vs-286 rewrite disagrees with the file it describes, and the reversed check-stack-images.sh note over-generalises its last clause from a measurement that used both of HILT_IMAGE's occurrences. One is round eight's finding 7, unchanged. The round-nine/round-ten accounting is now correct in every place I checked, including the table rows and "The two round-nine findings that changed code", which the a11c459→d92200e6 diff confirms.

1. 286 of its 433 lines is 285 in the tree, and the other 147 lines are not what that sentence says they are

Three separate problems in the <sub> note and the sentence it explains.

(a) 286 contradicts the file it describes. The tree states this number twice, and both say 285:

$ grep -n '285 of its 433' smelt/pkg/stack/published_images_test.go
44:// rate was the signal: 285 of its 433 lines reimplemented a slice of compose's

$ git log -1 --format=%s 7f04a8b8
Round six: 285 of the test's 433 lines were not load-bearing, so they are gone

285 is also what the previous revision of this body said, and what round ten called "the correct comparator". Measured directly, under the same convention that gives 225: the machinery is walkYAMLStrings through requiredImageVars, doc comments included, blank separators excluded — lines 67–351 of 1c27d270, 285. Getting 286 requires annexing one adjacent blank line to the machinery and not to the 225, so the two "comparable units" are counted by different rules.

(b) 147 inherits it, and 148 is not the test plus the helpers either. 433 − 285 = 148, and here is what those 148 lines are:

lines n what
1–43 43 file header comment, package, import block
45 1 const imageSentinel
47–65 19 fieldReachedBy (+ doc)
67–351 285 the compose-grammar machinery
353–374 22 readEnvPublished
376–433 58 TestPublishedImageSetAgrees
44, 46, 66, 352, 375 5 blank separators
433

TestPublishedImageSetAgrees and the shared helpers come to 58 + 19 + 22 + 1 = 100. The other 48 are the file-level header comment, the package clause, the imports and the blanks — neither the test nor a helper. And "all of which still exist" is false for that 43-line header: round six rewrote it (git diff --numstat 1c27d270 7f04a8b8 on this file is 42 331).

(c) 225 + 147 + 286 is not a decomposition of anything — 225 is measured in the new file (lines 168–392, verified exact) and 147/286 in the old one, so they do not sum to 433 or to 392. The two coherent statements are 433 = 285 + 148 and 392 = 225 + 167.

The percentages survive either way: (285−225)/285 = 21.1% and (433−225)/433 = 48.0%, so "21%" and "~48%" are both right. Only the two line counts and the composition sentence are wrong.

2. "so the gap is exactly the not-built-here case" is false for HILT_IMAGE and UPLOAD_IMAGE, which this body names two paragraphs earlier

Four of the five claims in the reversed note reproduce exactly. The fifth does not.

Verified at this head, one mutation at a time, worktree confirmed clean between rows:

mutation guard TestBuiltImagesAreRequiredNotDefaulted
none (baseline) rc=0 pass
${GUPPY_IMAGE:-…} → the same ref as a bare digest-pinned literal rc=0 pass
…→ ghcr.io/fil-forge/guppy:main-dev, no digest rc=1 —
${GUPPY_IMAGE:-…} → ${GUPPY_IMAGE:?…} rc=0 pass
sole ${SWARF_IMAGE:?…} → digest-pinned literal rc=0 fail — "SWARF_IMAGE is built here but is not a required compose interpolation anywhere"
one of HILT_IMAGE's two :? → digest-pinned literal rc=0 pass
one of UPLOAD_IMAGE's two :? → digest-pinned literal rc=0 pass

So the last sentence holds only where a built-here name has a single occurrence. HILT_IMAGE (systems/hilt/compose.yml:10,74) and UPLOAD_IMAGE (systems/upload/compose.yml:8,50) have two each — the same fact the body cites one paragraph up to justify the two-sidedness — and two-sidedness does not help here: a literal leaves no defaulted form for isDefaulted to find, so both halves stay quiet where a :- downgrade would have been caught.

Round ten's own measurement of this row neutralised both of HILT_IMAGE's occurrences (${HILT_IMAGE:?…} ×2 -> ghcr.io/…@sha256:96e7…), which is why it failed there. The body generalised that to every built-here name. As written, "the gap is exactly the not-built-here case" understates it by two of the eight.

3. ".env.published's contents are read by exactly one thing" — make up reads them, which is this PR's bug; round eight's finding 7, unchanged

From the skippable-checks block:

smelt/.env.published's contents are read by exactly one thing, and it is this PR's own new test … smelt/pkg/snapshot/stack.go os.Stats the path and passes --env-file without opening it; every other mention in the tree is prose.

$ grep -rn '\.env\.published' smelt/Makefile
20:# variable. .env.published comes first and the developer's own .env
23:ENV_FILES := --env-file .env.published --env-file .env

$ grep -n 'COMPOSE =' smelt/Makefile
36:COMPOSE = $(DOCKER) compose $(ENV_FILES) $$(test -f $(WORKSPACE_OVERRIDE) && …)

smelt/Makefile:23 is the same category the sentence carves out for pkg/snapshot/stack.go, not prose — and it is the path whose reading of this file failed with INDEXER_IMAGE unset, which is the bug at the top of this body. Also not prose: smelt/pkg/snapshot/stack_internal_test.go:24,27, which writes its own fixture and asserts the --env-file argument.

This is round eight's finding 7 verbatim, still open (#23 (review)).

The block's conclusion is unaffected and I re-derived it: no workflow runs make, so no CI job reads this file outside unit smelt. It is the supporting sentence a reviewer would check that is false.

4. "every finding in all three landed in it" — round seven's second finding landed in the other test, and three of round eight's landed in this body

From One decision taken here:

TestBuiltImagesAreRequiredNotDefaulted is 225 of this file's 392 lines, it has absorbed rounds seven, eight and nine, and every finding in all three landed in it while none landed in the one-line fix that repairs make up.

Round seven had two findings. Its second was "A publishedImages entry whose field no envImageOptions entry reaches is compared against nothing — the one way two of the three genuinely disagree and pass", and its fix went into the other test, in the same commit that created this one:

$ git diff 7f04a8b8 b35c8275 -- smelt/pkg/stack/published_images_test.go | grep '^+.*reachable'
+	// Every published reference must be reachable by an override variable.

It is at published_images_test.go:143-155 today, inside TestPublishedImageSetAgrees — and this body's own mutation table rows it as Agrees, not Required.

Round eight's 5, 6 and 7 were the "What is still not checked" item, the compose-oracle cost figure and the Makefile:23 sentence — the PR body, not the test. Round nine's 4 was the body too, which this section elsewhere states correctly.

The neighbouring claim — "none landed in the one-line fix that repairs make up" — checks out: I read all ten reviews' finding headings and none is about the INDEXER_IMAGE line. So "Not one of the findings across those ten rounds was a defect in the one-line fix" stands; it is "every finding in all three landed in it" that does not.

Verification, and what checked out

Own detached worktree at d92200e6, removed after. GOWORK=off throughout. Compose client v5.1.1, docker client present, no daemon. Exit codes captured directly; smelt/generated/ handled explicitly (it holds two tracked files, .gitignore and generate-proofs.sh, so rm -rf on it is a tracked deletion — restored and git status --porcelain --ignored=matching empty before and after every row).

Line counts, measured not copied.

1c27d270  published_images_test.go   433 lines      d92200e6  392 lines
new-file block 168–392 (doc comment through EOF: test + includeTargets + walkStrings)   = 225
old-file block  67–351 (walkYAMLStrings doc through requiredImageVars close)            = 285

Per-revision: f9a86cfa 189, cf5c105d 201, b5ce12ec 283, b9423bc0 359, 1c27d270 433, 7f04a8b8 144, b35c8275 293, f693c50c 354, a11c459d 352, d92200e6 392. "225 of this file's 392 lines" and "433 → 392" are both exact.

Round nine and round ten, against what the reviews say. Round nine: "Not satisfied: 4 findings"; three share the two-sided-incompleteness root (1 generator storage shape, 2 file set / compose.override.yml, 3 gitignored output), the fourth is the body. Round ten: "Not satisfied: 3 findings — all in the PR body, none in the code", and "Round nine's three code findings are each independently reproduced as fixed … Nothing came back in TestBuiltImagesAreRequiredNotDefaulted". So: the four/three-plus-body split, both Ten rounds table rows, "findings 1–3 collapse into a single push" (round nine wrote exactly "Keep it here if findings 1–3 collapse into one push", and a11c459d..d92200e6 is one commit), "the flip condition … It did not", and "reproduced all three of round nine's code fixes" all check out.

"The two round-nine findings that changed code" is right, and is the fix round ten asked for: the diff changes code for finding 1 (the 2×2 storage loop) and finding 3 (the filepath.Rel generated/ skip); finding 2's response is comment-only, the walk's file set is unchanged.

The Ten rounds table's rounds one–six and eight rows each match the corresponding review's finding headings.

Everything else in the block and the body that is checkable.

$ git diff --name-only 4611cbcb..d92200e6
smelt/.env.published  smelt/CLAUDE.md  smelt/pkg/stack/options.go  smelt/pkg/stack/published_images_test.go
  • options.go comments-only: the body's own command prints nothing.
  • go list -f '… GoFiles={{len .GoFiles}} TestGoFiles={{len .TestGoFiles}}' ./pkg/stack/ → GoFiles=10 TestGoFiles=4.
  • go list -deps -test ./tests/e2e/... greps 0; -tags e2e greps 1. hilt/itest and ingot/itest each reach pkg/stack (1 each).
  • ci.yml untouched in the net diff, and it parses.
  • walkStrings / includeTargets: call sites only at published_images_test.go:298-299, inside the second test.
  • Seven check-*.sh in .github/scripts/, all seven rc=0 at this head; ci.yml:60-72 lists all seven by hand, one named run: step each — so the "added guard never runs" observation holds. guards also runs gofmt -s -l . (ci.yml:73+); empty tree-wide. GOWORK=off go build ./... and go vet ./... clean in smelt. staticcheck is not installed in this environment, so that one is unre-checked rather than confirmed.
  • TestBuildImageTagFormat, TestBuildPiriImageTagFormat, TestBuildGuppyImageTagFormat exist in build_test.go, each guarded by exec.LookPath("docker"); the client is present so they run and fail on dial unix /var/run/docker.sock. As described.
  • ~1.4 s over 9 invocations reproduces exactly. smelt generate, then docker compose --env-file /dev/null config -q in a loop, feeding back each named variable: SIGNER, DELEGATOR, INDEXER, PIRI, UPLOAD, SWARF, HILT, INGOT → success on the 9th. elapsed 1.43 s, no daemon.
  • "34 rows at one point" — round six's review: "The matrix: 34 rows. Rows 1–19 are the PR body's nineteen … rows 20–34 are new."
  • HILT_IMAGE and UPLOAD_IMAGE are each required in exactly two places; PIRI_IMAGE in none of the committed YAML (it comes from the generator), the other five in one each.
  • At 4611cbcb: .env.published had 7 entries and no INDEXER_IMAGE; publishedImages had 8 including indexing-service; options.go:409 carried "Neither copy can drift silently" — the fourth copy. smelt/CLAUDE.md:465 listed six built-here names, :466 put swarf and indexer under "Not built here" and closed with "SWARF_IMAGE joins the required set when the branch that builds swarf lands."
  • check-stack-images.sh's rule is "pinned by digest, or be a variable" (its own header line 2), and its case "$value" in \$\{*) continue skips every ${…} value. .env.published is not in smelt/embed.go.
  • The twelve-row mutation table and the Go code were out of scope this round and not re-reviewed.

Generated by Claude Code

@Peeja Peeja left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[From Claude:]

Round twelve, head d92200e6. Not satisfied: 3 findings — all in the PR body; none in the code.

Brief was numbers and facts only, so nothing about prose, phrasing or what the body no longer says is reported. The two passages rewritten since round eleven are the highest-risk ones and they split: the "what is still not checked" table reproduces exactly, row for row, including which of HILT_IMAGE/UPLOAD_IMAGE are the only built-here names with a second occurrence — but the sentence that follows it over-states what the deferred fix would close, and the rewritten .env.published sentence in the skippable-checks block draws an inference its own evidence does not carry. The four line counts, the round-by-round table, 1.43 s over 9 invocations, the generator/smelt.yml claim and the generated/ claim all check out; details in the verification block.

1. "so none of them runs in CI" is false for two of the three things that sentence just named — unit smelt runs both

From the skippable-checks block:

Three things hand it to compose by --env-file without opening it — smelt/Makefile's ENV_FILES (line 23), smelt/pkg/snapshot/stack.go, and that package's internal test — and no workflow runs make, so none of them runs in CI.

The three named things are right, and "no workflow runs make" is right — every make in .github/ is inside a comment:

$ grep -rn "make" .github/ | grep -v '\.sh:'
ci.yml:21, ci.yml:130, release.yml:101,102,167,209,474,500,616, itest.yml:90,161
   (all prose: "Dispatch makes", "make its graph the largest", "a local `make itest`", …)

But that premise only covers the Makefile. The other two are ordinary untagged Go code in the smelt module, and ci / unit smelt runs go test … ./... over the whole module:

$ sed -n '.../ci.yml unit job/p'
      - name: test
        run: go test ${{ matrix.build_flags }} -race -shuffle=on ./...

$ head -1 smelt/pkg/snapshot/stack_internal_test.go
package snapshot                      # no build tag

$ GOWORK=off go test ./pkg/snapshot/ -run TestComposeArgs -v -count=1
=== RUN   TestComposeArgs
--- PASS: TestComposeArgs (0.00s)

So TestComposeArgs runs in CI on every push, and it calls composeArgs in pkg/snapshot/stack.go — the second and third items — under unit smelt.

The block's conclusion is unaffected and I re-derived it: neither reads smelt/.env.published. composeArgs only os.Stats the path (stack.go:30), and the test writes its own empty fixtures into t.TempDir() (stack_internal_test.go:17-19), so no CI job outside unit smelt touches the file and nothing but this PR's new test reads its contents. It is the supporting inference a reviewer would check that is false — and this is the same sentence round eleven's finding 3 reported, failing a second way after the rewrite.

2. "Closing it is cheap" closes one of the two uncaught rows in its own table; the not-built-here row cannot be closed by a publishedImages check

Closing it is cheap and is deliberately not done here: a third side that rejects any publishedImages reference appearing as a literal in a walked compose file — those repository paths are already in that table, so it needs no new derivation.

Both halves of the mechanism check out, measured:

  • No new derivation. publishedImages (options.go:422-431) already holds all eight references, so their repository paths need nothing derived.
  • No false positives against this tree. The only ghcr.io/fil-forge/* strings in the walked file set are filecoin-localdev, minio, storetheindex, guppy and did-method-plc — every one inside a ${VAR:-…} default, and not one of them in publishedImages:
$ grep -Hn 'ghcr.io/fil-forge' <root compose.yml + its 12 includes + 2 nested>
systems/blockchain/compose.yml:8    ${BLOCKCHAIN_IMAGE:-ghcr.io/fil-forge/filecoin-localdev:…}
systems/common/compose.yml:47       ${MINIO_IMAGE:-ghcr.io/fil-forge/minio:…}
systems/indexing/ipni/compose.yml:13,35  ${IPNI_IMAGE:-ghcr.io/fil-forge/storetheindex:…}
systems/guppy/compose.yml:7         ${GUPPY_IMAGE:-ghcr.io/fil-forge/guppy:…}
systems/plc/compose.yml:12          ${PLC_IMAGE:-ghcr.io/fil-forge/did-method-plc:…}

What does not hold is the scope. The table directly above has two rows marked uncaught, and publishedImages is by construction the eight images this repository builds — GUPPY_IMAGE's reference is not in it and cannot be:

row marked closed by the proposed third side?
not-built-here image (${GUPPY_IMAGE:-…}) → literal no — by nothing at all no — ghcr.io/fil-forge/guppy:main-dev@sha256:… is in no Go table
sole occurrence of a built-here name (SWARF_IMAGE) yes already caught
one of two occurrences (HILT_IMAGE, UPLOAD_IMAGE) no yes

So "closing it" closes one of the two open rows, not the item. Either the sentence wants "closing the built-here half of it", or the first row wants its own note saying a publishedImages-keyed check will not reach it.

3. "changes check names" — the seven guards are steps of one job, so collapsing them into a glob cannot change a check name

Not changed here, because collapsing the steps into a glob loses per-guard names in the checks UI and changes check names, which is a trade rather than a fix.

ci.yml has one guards job whose name: is guards, with the seven scripts as steps inside it:

$ sed -n '44,70p' .github/workflows/ci.yml
  guards:
    name: guards
    …
      - name: every in-repo require has a replace
        run: .github/scripts/check-replaces.sh
      - name: every in-repo module reference uses its monorepo path
      …   (7 such steps, then the gofmt step)

GitHub creates a check run per job, not per step, so replacing those steps with one glob loop leaves the check named ci / guards exactly as it is now. What is lost is the per-guard step names inside the job — which is the sentence's first clause, already stated. The body's own skippable-checks block uses the job-level names (ci / unit smelt, ci / guards), so the two halves of this sentence disagree about what a check is.

Only the second clause is wrong; the trade the paragraph describes is real, just one consequence smaller than stated.

Verification, and what checked out

Own detached worktree at d92200e6, git status --porcelain --ignored=matching empty before and after every row, worktree removed after. GOWORK=off throughout. Exit codes captured directly. smelt/generated/ handled explicitly (it holds two tracked files): rm -rf then git checkout -- smelt/generated/. Compose client v5.1.1; docker client present, no daemon.

The "what is still not checked" table — all three rows reproduce, one mutation at a time. Digest used: @sha256:aaaa…9990.

mutation check-stack-images.sh Agrees Required
none (baseline) rc=0 pass pass
${GUPPY_IMAGE:-…} → digest-pinned literal rc=0 pass pass
sole ${SWARF_IMAGE:?…} → digest-pinned literal rc=0 pass fail — "SWARF_IMAGE is built here but is not a required compose interpolation anywhere"
${HILT_IMAGE:?…} at systems/hilt/compose.yml:10 (1 of 2) → digest-pinned literal rc=0 pass pass
…the same at line 74 instead (the other of the 2) rc=0 pass pass
${UPLOAD_IMAGE:?…} at systems/upload/compose.yml:8 (1 of 2) → digest-pinned literal rc=0 pass pass
${SWARF_IMAGE:?…} → literal with no digest rc=1 — —

So "Measured at this head … leaves check-stack-images.sh at rc=0 and both tests passing", "its rule is pinned by digest or a variable" (the guard's own line 2, verbatim) and "Only a non-digest literal turns it red" are all exact.

HILT_IMAGE and UPLOAD_IMAGE really are the only two. Occurrence counts over the walked set (root compose.yml + its 12 include: targets + systems/indexing/'s two nested ones; generated/compose/piri.yml skipped): HILT_IMAGE 2, UPLOAD_IMAGE 2, SWARF_IMAGE/SIGNER_IMAGE/INGOT_IMAGE/INDEXER_IMAGE/DELEGATOR_IMAGE 1 each, all :?. PIRI_IMAGE has no compose occurrence and exactly one emitting site (pkg/generate/compose.go:70, used once at :101) — so it behaves as a sole occurrence, which I confirmed by mutating that line to a digest-pinned literal: Required fails, "PIRI_IMAGE is built here but is not a required compose interpolation anywhere". No built-here name other than those two survives a single-site literal.

Line counts, measured. 1c27d270 433 lines, d92200e6 392. Under one convention — first doc-comment line through the last closing brace of a contiguous run, trailing blank excluded — the old machinery is walkYAMLStrings (67) … requiredImageVars (351) = 285, and the new block is TestBuiltImagesAreRequiredNotDefaulted (168) … walkStrings (392) = 225. (285−225)/285 = 21.05% → "21%"; "433 → 392" and "225 of this file's 392 lines" exact.

The compose oracle. Reproduced the probing loop with an empty environment (env -i), after make generate so the generated/compose/piri.yml include resolves:

1 SIGNER_IMAGE  2 DELEGATOR_IMAGE  3 INDEXER_IMAGE  4 PIRI_IMAGE
5 UPLOAD_IMAGE  6 SWARF_IMAGE      7 HILT_IMAGE     8 INGOT_IMAGE   9 SUCCESS
total invocations=9 elapsed=1.41s

Exactly 9, and 1.41 s here against the body's 1.43 s. --variables under-reporting also reproduces, on the shape described — a required variable in a non-extended service of an extends.file target:

$ docker compose config          -> error: required variable EXTRA_IMAGE is missing a value
$ docker compose config --variables
NAME       REQUIRED
B_IMAGE    true                  # EXTRA_IMAGE absent

Skippable-checks block. git diff --name-only 4611cbcb...HEAD is the four files listed; .env.published is one entry + header; the body's own options.go comments-only command prints nothing; go list -deps -test ./tests/e2e/... greps 0 and -tags e2e greps 1; GoFiles=10 TestGoFiles=4; hilt/itest and ingot/itest each reach pkg/stack (1 each); ci.yml untouched and it parses; smelt/Makefile:23 is ENV_FILES := --env-file .env.published --env-file .env; pkg/snapshot/stack.go:30 os.Stats only.

Verified block. All seven check-*.sh in .github/scripts/ exit 0 (seven is the whole directory); gofmt -s -l . empty; GOWORK=off go build ./... and go vet ./... clean. ci.yml enumerates the guards by hand, one named step each, and contains no generate step; smelt/generated/ is ignored by smelt/.gitignore:2. The generator loop is 2×2 and a zero-value StorageSpec matches neither manifest.DBPostgres nor manifest.BlobS3, so buildPostgresService, buildPostgresInitService and buildMinioService are each unreachable from it (compose.go:34,41,45); the committed smelt.yml is one node at db: postgres / blob: s3. The three TestBuild*ImageTagFormat fail here — docker is at /usr/bin/docker, so the exec.LookPath skip does not fire, and each dies on dial unix /var/run/docker.sock; build_test.go is untouched by this PR. staticcheck I could not run — the only version I could fetch is built with go1.24.7 and this module needs go1.27 — so that one claim is unverified rather than checked.

History. Eleven prior review rounds on this PR. Round ten: "Not satisfied: 3 findings — all in the PR body, none in the code" and "Nothing came back in TestBuiltImagesAreRequiredNotDefaulted"; round eleven: "Not satisfied: 4 findings — all in the PR body; none in the code" and "Two of the four are in the corrections round ten prompted" — so "seven findings … two of them inside corrections round ten had just asked for" is exact. Round nine wrote "Keep it here if findings 1–3 collapse into one push", and a11c459d..d92200e6 is one commit. The round table's rows one–six, eight and nine each match the corresponding review's finding headings, and round nine's second finding — the one the table's row omits — is the subject of the paragraph immediately below it. I read all eleven rounds' finding headings: none is about the INDEXER_IMAGE line, so "Not one of the findings across eleven rounds was a defect in the one-line fix" stands.

smelt/CLAUDE.md. At origin/main the "Built here" list named six (PIRI, HILT, INGOT, UPLOAD, DELEGATOR, SIGNER), put guppy, indexer, ipni, swarf, plc, blockchain, minio under "Not built here", and closed with "SWARF_IMAGE joins the required set when the branch that builds swarf lands." — all three as the body describes. The AGENTS.md quote is verbatim. systems/telemetry/compose.yml and systems/stress-tester/compose.yml appear in no include list, no Makefile target and no embed.go directive; no compose.override.yml or generated/compose/workspace.override.yml is committed, and COMPOSE passes no -f unless $(WORKSPACE_OVERRIDE) exists.


Generated by Claude Code

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants