Skip to content

ci: build the images a PR changes, and pin the digests nothing asserted - #975

Merged
nikhilunni merged 3 commits into
mainfrom
dependabot-qa
Aug 3, 2026
Merged

nikhilunni merged 3 commits into
mainfrom
dependabot-qa

Conversation

@nikhilunni

@nikhilunni nikhilunni commented Aug 3, 2026 •

Copy link
Copy Markdown
Contributor

Prompted by the 18 open Dependabot PRs. Reviewing why they're stuck turned up
two faults, and only one of them is staleness.

A Dockerfile-only change runs nothing and reports green

bake-images.yml is on: push: [main], and ci.yml's bake-demo-image is gated
on push-to-main. No workflow builds a container image on a pull request.

The lane detector correctly reports images=true and no test lane for a
docker/ change. The CI Gate passes when every lane is "succeeded-or-skipped".
So all-skipped is a pass.

That is how #130 has been green since June 8 having proved nothing — rust
1.95→1.97, node 20→25, nginx 1.27→1.31, busybox 1.36→1.38 across five
Dockerfiles. Its only passing checks are the YAML linter, the detector, the
auto-approve job, and the Gate. The first real build would have happened on the
push to main that deploys.

build-images builds every image a PR changes, without pushing. Publishing
stays main's job, so this cannot churn the SHA tag the host-fleet DaemonSet pins
— the 2026-07-20 incident where a spurious rebake rolled every FC host four
times in 90 minutes.

Two content-addressing invariants are asserted by nothing

ChunkHash::of is a chunk's address in the blob store. Manifest::content_ref
exists so a deterministic re-bake produces the same id — its doc comment
describes the bug where a random id made every re-bake look like new content at
every layer above.

Neither has a known-answer test. The existing
content_ref_is_deterministic_and_content_sensitive checks only
self-consistency: same input equals itself, different inputs differ. A changed
digest satisfies every one of those assertions
while orphaning every chunk and
every disk_manifest_content_ref already stored.

Two tests now pin them to literals — ChunkHash::of against the FIPS 180-4
vectors, content_ref against the exact UUID a fixed manifest derives. If either
fails, the answer is a migration, not a re-blessing.

The auto-merge classifier now fails loudly

It used to shrug when fetch-metadata returned no dependencies — report "not
auto-mergeable" and succeed. A bump of that action could therefore stop the
gate working with every check green and nothing to see. dependabot/fetch-metadata
v2→v3 is sitting in #938 right now.

What this deliberately does not add

An earlier draft of this PR carried a hand-maintained dependency→risk map that
selected extra test lanes per bump. It was the wrong shape:

  • The Rust gates it named were strict subsets of cargo nextest run --workspace,
    which already runs on any Cargo.lock change. Zero added coverage, plus a
    second 8-vCPU runner.
  • Its escalate flag was unenforced — the report claimed a PR would not
    auto-merge while dependabot-auto-merge.yml independently merged it.
  • A dependency missing from the list got no gate and nothing said so. That is
    exactly the silently-permissive drift detect-rebake-lanes.py was written to
    replace ("a hand-maintained path denylist ... silently drifts"), reintroduced
    in the file beside it.

The knowledge belongs in a test, not a list. A known-answer test is checked on
every change by machinery that already exists, fails loudly, and catches the
invariant breaking for any reason rather than only on a Dependabot PR.

Test

chunk_hash_is_sha256_of_the_bytes and content_ref_never_moves, with the
vectors verified against shasum -a 256 so they are canonical rather than
self-agreeing. The detect job gained a step asserting the detector still names
images and images_matrix — build-images is gated on outputs this PR
surfaces for the first time, and a rename would silently reopen the hole.

just check: 2264 passed.

Follow-ups this does not do

🤖 Generated with Claude Code

@engrams-agent

engrams-agent Bot commented Aug 3, 2026 •

Copy link
Copy Markdown
Contributor

✅ engrams review — complete. 4 findings posted. · View details

@github-actions

github-actions Bot commented Aug 3, 2026 •

Copy link
Copy Markdown

The latest Buf updates on your PR. Results from workflow CI / buf (pull_request).

BuildFormatLintBreakingUpdated (UTC)
✅ passed⏩ skipped⏩ skipped✅ passedAug 3, 2026, 1:44 PM

@engrams-agent engrams-agent Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Engrams review

Verdict: 4 findings posted inline.
Severity: Critical 0 · High 1 · Medium 3 · Low 0
Categories: 🎯 Functional Correctness: 3 · 🚀 Performance & Scalability: 1

View the full engrams review

Comment thread .github/workflows/dependabot-qa.yml Outdated
Comment on lines +80 to +120
- name: Install the toolchain
if: matrix.gate != 'images' && matrix.gate != 'web-runtime'
uses: dtolnay/rust-toolchain@stable

- uses: taiki-e/install-action@v2
if: matrix.gate != 'images' && matrix.gate != 'web-runtime'
with:
tool: cargo-nextest

# Mirrors ci.yml's `web` job. There is no web/.nvmrc, so the version is
# pinned here the same way it is there.
- uses: pnpm/action-setup@v6
if: matrix.gate == 'web-runtime'
with:
version: 9

- uses: actions/setup-node@v6
if: matrix.gate == 'web-runtime'
with:
node-version: 20
cache: pnpm
cache-dependency-path: web/pnpm-lock.yaml

- name: Install web dependencies
if: matrix.gate == 'web-runtime'
run: cd web && pnpm install --frozen-lockfile

# `images` is not a shell command — ci.yml owns the PR-time image build,
# which is the gate. Asserting it here would duplicate that job's matrix.
- name: Run the gate
if: matrix.gate != 'images'
env:
GATE: ${{ matrix.gate }}
run: |
RUN=$(jq -r --arg g "$GATE" '.gates[$g].run' .github/dependency-risk.json)
if [ -z "$RUN" ] || [ "$RUN" = "null" ]; then
echo "no run command for gate $GATE" >&2
exit 1
fi
echo "::notice::$(jq -r --arg g "$GATE" '.gates[$g].why' .github/dependency-risk.json)"
bash -eo pipefail -c "$RUN"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🎯 Functional Correctness · HIGH — QA gate job never installs protoc, so every Rust gate fails at build time

WHAT: The gate job installs only rust-toolchain + cargo-nextest, but never installs protoc, so every Rust gate that builds engram-protocol fails to compile — the QA gate proves nothing.

WHEN: On any real Dependabot PR (the classify job's if: restricts this workflow to dependabot[bot]), a matched Rust gate runs cargo nextest run. The wire gate runs -p engram-protocol -p engram-core; engram-protocol/build.rs calls tonic_build...compile_protos(...), which requires a protoc binary on PATH. Every Rust job in ci.yml installs it explicitly via apt-get install protobuf-compiler (with a long comment noting it is not preinstalled), but this gate job has no such step, so the build fails before any test runs.

Scenario:

  1. Dependabot opens a sha2/prost/tonic/nom bump → classifier emits wire (and others).
  2. The gate matrix job runs cargo nextest run -p engram-protocol -p engram-core.
  3. engram-protocol's build.rs cannot find protoc → compile error → gate job is red.
  4. Every Rust gate (wire, egress, content-addressing, determinism, and the Rust half of auth) hits this — the entire mechanism this PR adds never actually validates an invariant.
Suggested change
- name: Install the toolchain
if: matrix.gate != 'images' && matrix.gate != 'web-runtime'
uses: dtolnay/rust-toolchain@stable
- uses: taiki-e/install-action@v2
if: matrix.gate != 'images' && matrix.gate != 'web-runtime'
with:
tool: cargo-nextest
# Mirrors ci.yml's `web` job. There is no web/.nvmrc, so the version is
# pinned here the same way it is there.
- uses: pnpm/action-setup@v6
if: matrix.gate == 'web-runtime'
with:
version: 9
- uses: actions/setup-node@v6
if: matrix.gate == 'web-runtime'
with:
node-version: 20
cache: pnpm
cache-dependency-path: web/pnpm-lock.yaml
- name: Install web dependencies
if: matrix.gate == 'web-runtime'
run: cd web && pnpm install --frozen-lockfile
# `images` is not a shell command — ci.yml owns the PR-time image build,
# which is the gate. Asserting it here would duplicate that job's matrix.
- name: Run the gate
if: matrix.gate != 'images'
env:
GATE: ${{ matrix.gate }}
run: |
RUN=$(jq -r --arg g "$GATE" '.gates[$g].run' .github/dependency-risk.json)
if [ -z "$RUN" ] || [ "$RUN" = "null" ]; then
echo "no run command for gate $GATE" >&2
exit 1
fi
echo "::notice::$(jq -r --arg g "$GATE" '.gates[$g].why' .github/dependency-risk.json)"
bash -eo pipefail -c "$RUN"
Add a protoc install step to the `gate` job for the Rust gates, mirroring `ci.yml`:
```yaml
- name: Install protoc
if: matrix.gate != 'images' && matrix.gate != 'web-runtime'
run: |
sudo apt-get update -qq
sudo apt-get install -y -qq --no-install-recommends protobuf-compiler

Comment thread .github/workflows/dependabot-qa.yml Outdated
Comment on lines +109 to +120
- name: Run the gate
if: matrix.gate != 'images'
env:
GATE: ${{ matrix.gate }}
run: |
RUN=$(jq -r --arg g "$GATE" '.gates[$g].run' .github/dependency-risk.json)
if [ -z "$RUN" ] || [ "$RUN" = "null" ]; then
echo "no run command for gate $GATE" >&2
exit 1
fi
echo "::notice::$(jq -r --arg g "$GATE" '.gates[$g].why' .github/dependency-risk.json)"
bash -eo pipefail -c "$RUN"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🎯 Functional Correctness · MEDIUM — auth gate runs bun test but the job never installs bun or the orchestrator deps

WHAT: The auth gate's command is cargo nextest run -p engram-git-github && cd orchestrator && bun test (dependency-risk.json), but the gate job only sets up bun/Node for the web-runtime gate, so the auth gate always fails at bun: command not found.

WHEN: A Dependabot bump to any auth-mapped package (cargo jsonwebtoken/rsa/pkcs1/pkcs8/ring, or npm jsonwebtoken/jose) makes the classifier emit the auth gate. The matrix job then runs the gate command. bun is only provisioned via the web-runtime-gated steps (oven-sh/setup-bun is not used at all in this workflow), so cd orchestrator && bun test fails immediately. Even if bun were present, no bun install runs first, so orchestrator/node_modules would be absent. The gate can never pass on its own trigger.

Suggested change
- name: Run the gate
if: matrix.gate != 'images'
env:
GATE: ${{ matrix.gate }}
run: |
RUN=$(jq -r --arg g "$GATE" '.gates[$g].run' .github/dependency-risk.json)
if [ -z "$RUN" ] || [ "$RUN" = "null" ]; then
echo "no run command for gate $GATE" >&2
exit 1
fi
echo "::notice::$(jq -r --arg g "$GATE" '.gates[$g].why' .github/dependency-risk.json)"
bash -eo pipefail -c "$RUN"
Provision bun (and `bun install` in `orchestrator/`) for the `auth` gate, e.g. add `oven-sh/setup-bun` + a `bun install` step guarded by `if: matrix.gate == 'auth'`, mirroring how `web-runtime` sets up pnpm/node and runs `pnpm install`.

Comment thread .github/workflows/dependabot-qa.yml Outdated
existing=$(gh api "repos/${GITHUB_REPOSITORY}/issues/${PR}/comments" \
--jq '[.[] | select(.user.login=="github-actions[bot]") | select(.body | startswith("## Dependency QA")) | .id] | first // empty')
if [ -n "$existing" ]; then
gh api --method PATCH "repos/${GITHUB_REPOSITORY}/issues/comments/${existing}" \

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🎯 Functional Correctness · MEDIUM — Rolling-comment update uses malformed gh api -f body@report.md, so updates fail

WHAT: The comment-update path calls gh api --method PATCH ... -f body@report.md, but -f/--raw-field takes a literal key=value string and does not read files — reading a file needs -F/--field body=@report.md. As written the argument has no = at all, so gh errors on field parsing and the PATCH never runs.

WHEN: The first QA run on a Dependabot PR has no existing comment, so it takes the else branch (gh pr comment --body-file report.md) and works. On every subsequent push to the same PR, existing is non-empty and the code takes the PATCH branch. gh api -f body@report.md fails to parse the field (-f requires key=value; @file is only honored by -F), the if returns non-zero as the last command in the step, and the report job goes red. The "one rolling comment per PR" behavior never happens — the comment is never updated, and each re-push posts a failed report job.

Suggested change
gh api --method PATCH "repos/${GITHUB_REPOSITORY}/issues/comments/${existing}" \
Use the typed-field form that reads from a file:
```sh
gh api --method PATCH "repos/${GITHUB_REPOSITORY}/issues/comments/${existing}" \
-F body=@report.md >/dev/null

Comment thread .github/workflows/ci.yml Outdated
Comment on lines +2194 to +2195
cache-from: type=gha,scope=ci-${{ matrix.image }}
cache-to: type=gha,mode=max,scope=ci-${{ matrix.image }}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🚀 Performance & Scalability · MEDIUM — build-images reintroduces the type=gha,mode=max cache that bake-images.yml deliberately removed

WHAT: The new build-images job sets cache-from/cache-to: type=gha,mode=max for each of up to five images — the exact GitHub Actions cache exporter that bake-images.yml deliberately dropped, per its own in-file comment, because it round-trips to GitHub's real cache service from the Blacksmith microVM at ~0.2 MB/s (~24 min/job) and blew past GitHub's hard 10 GB per-repo cache cap, evicting other caches.

WHEN: On every PR that changes a Dockerfile (or otherwise trips the images lane), each matrixed image writes a mode=max gha cache (all intermediate layers) under scope=ci-<image>. Because the 10 GB gha cache is shared repo-wide, this competes with and evicts the Swatinem/rust-cache@v2 shared-key: workspace-debug cache that lint, test-linux, and test-e2e-stack rely on — degrading hit rates on the hot Rust lanes — while the expensive layer here (RUN --mount=type=cache for cargo/pnpm) is never exported to gha anyway, so the cache buys little. This is the same failure mode bake-images.yml documents having already hit.

Suggested change
cache-from: type=gha,scope=ci-${{ matrix.image }}
cache-to: type=gha,mode=max,scope=ci-${{ matrix.image }}
Drop the `cache-from`/`cache-to: type=gha` lines (rely on Blacksmith's colocated BuildKit cache as `bake-images.yml` does), or at minimum use `mode=min` and a bounded, non-shared scope after confirming the gha cap impact.

## Problem

Reviewing the 18 open Dependabot PRs turned up two faults; only one of them is
staleness.

**A Dockerfile-only change runs nothing and reports green.** `bake-images.yml`
is `on: push: [main]`, and ci.yml's `bake-demo-image` is gated on push-to-main,
so no workflow builds a container image on a pull request. The lane detector
correctly reports `images=true` and no test lane; the CI Gate passes when every
lane is "succeeded-or-skipped"; so all-skipped is a pass. That is how #130 —
rust 1.95→1.97, node 20→25, nginx 1.27→1.31, busybox 1.36→1.38 across five
Dockerfiles — has been green and unvalidated since June 8. Its first real build
would have happened on the push to main that deploys.

**Two content-addressing invariants are asserted by nothing.** `ChunkHash::of`
is a chunk's address in the blob store, and `Manifest::content_ref` exists so a
deterministic re-bake produces the same id (its doc comment describes the bug
where a random id made every re-bake look like new content). Neither has a
known-answer test. `content_ref_is_deterministic_and_content_sensitive` only
checks self-consistency — same input equals itself, different inputs differ —
which a changed digest satisfies perfectly while orphaning every chunk and
manifest ref already stored.

## Fix

`build-images` builds every image a PR changes, WITHOUT pushing: publishing
stays main's job, so this cannot churn the SHA tag the host-fleet DaemonSet pins
(the 2026-07-20 incident where a spurious rebake rolled every FC host four times
in 90 minutes). `node-assets` is excluded — it fetches Firecracker, the guest
kernel and the RO bundles, and has its own lane. It joins the CI Gate's `needs:`,
per the rule that a lane outside the gate wedges the merge queue when it
path-skips.

Two known-answer tests pin the digests to literals: `ChunkHash::of` against the
FIPS 180-4 vectors, and `content_ref` against the exact UUID a fixed manifest
derives. If either fails, the answer is a migration, not a re-blessing.

The auto-merge classifier now fails loudly when `fetch-metadata` returns no
dependencies. It used to shrug and report "not auto-mergeable" while the job
SUCCEEDED, so a bump of that action could stop the gate working with every check
green — and `dependabot/fetch-metadata` v2→v3 is sitting in #938 right now.

## What this deliberately does NOT add

An earlier draft carried a hand-maintained dependency→risk map that chose extra
test lanes per bump. It was the wrong shape and I removed it:

- The Rust gates it named were strict subsets of `cargo nextest run --workspace`,
  which already runs on any `Cargo.lock` change. Zero added coverage.
- Its `escalate` flag was unenforced — the report claimed a PR would not
  auto-merge while `dependabot-auto-merge.yml` independently merged it.
- A dependency missing from the list got no gate and nothing said so. That is
  the silently-permissive drift `detect-rebake-lanes.py` was written to replace
  ("a hand-maintained path denylist ... silently drifts"), reintroduced beside it.

The knowledge belongs in a test, not a list: a known-answer test is checked on
every change by machinery that already exists, fails loudly, and catches the
invariant breaking for any reason rather than only on a Dependabot PR.

## Test

`chunk_hash_is_sha256_of_the_bytes` and `content_ref_never_moves`, verified
against `shasum -a 256` so the vectors are canonical rather than self-agreeing.
The `detect` job gained a step asserting the detector still names `images` and
`images_matrix` — `build-images` is gated on outputs this PR surfaces for the
first time, and a rename would silently reopen the hole.

`just check`: 2264 passed.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@nikhilunni nikhilunni changed the title ci: gate a dependency bump on the invariant it can break, not on the build ci: build the images a PR changes, and pin the digests nothing asserted Aug 3, 2026
@engrams-agent

engrams-agent Bot commented Aug 3, 2026 •

Copy link
Copy Markdown
Contributor

✅ engrams review — complete. 1 finding posted. · View details

@engrams-agent engrams-agent Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Engrams review

Verdict: 1 finding posted inline.
Severity: Critical 0 · High 0 · Medium 1 · Low 0
Categories: 📐 Maintainability & Code Quality: 1

View the full engrams review

Comment thread .github/workflows/ci.yml Outdated
Comment on lines +168 to +174
- name: Detector still names the images lane
run: |
printf 'docker/web.Dockerfile\n' \
| python3 .github/scripts/detect-rebake-lanes.py --stdin 2>&1 \
| tee /tmp/lanes.txt
grep -q 'images=True' /tmp/lanes.txt
grep -q "images_matrix=\['web'\]" /tmp/lanes.txt

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

📐 Maintainability & Code Quality · MEDIUM — Contract check greps the detector's stderr debug line, not the GITHUB_OUTPUT key build-images depends on

WHAT: The "Detector still names the images lane" step claims to protect the images/images_matrix job-output contract, but it greps the detector's human-readable stderr diagnostic (-> images=True, -> images_matrix=['web']) instead of the actual GITHUB_OUTPUT keys (images=true, images_matrix=["web"]) that the build-images job consumes — so the exact rename it promises to catch would pass green.

WHEN: The detector emits its output contract in two independent places: the stderr print print(f"-> images={images} ...", file=sys.stderr) / print(f"-> images_matrix={images_matrix}", ...), and the GITHUB_OUTPUT writes f.write(f"images={b(images)}\n") / f.write(f"images_matrix={json.dumps(images_matrix)}\n"). The two forms even differ: stderr prints the Python bool True and list repr ['web'] (single quotes); the real output is lowercase true and JSON ["web"] (double quotes). The grep patterns (images=True, images_matrix=\['web'\]) match only the stderr repr. The 2>&1 | tee /tmp/lanes.txt captures stdout+stderr only; the GITHUB_OUTPUT writes go to a separate file the grep never sees.

Scenario:

  1. A future edit renames the emitted key in the f.write(...) block (e.g. images -> container_images) but leaves the stderr debug print — or the detect job's outputs: mapping — out of sync.
  2. build-images's if: needs.detect.outputs.images == 'true' now reads an empty output and the job is silently skipped; the CI Gate treats "skipped" as pass.
  3. This contract step still greps the unchanged stderr line images=True, so it stays green — the "Dockerfile change unvalidated again" hole the step's own comment says it closes has reopened undetected.

The step does still catch a change to the detector's logic (if docker/web.Dockerfile stopped mapping to the images lane), so it is not useless — but the comment overpromises: it does not assert the output-key contract it names.

Suggested change
- name: Detector still names the images lane
run: |
printf 'docker/web.Dockerfile\n' \
| python3 .github/scripts/detect-rebake-lanes.py --stdin 2>&1 \
| tee /tmp/lanes.txt
grep -q 'images=True' /tmp/lanes.txt
grep -q "images_matrix=\['web'\]" /tmp/lanes.txt
Assert against the emitted outputs, not the stderr diagnostic. Run the detector with a temporary `GITHUB_OUTPUT` file and grep that file for the lowercase/JSON forms:

export GITHUB_OUTPUT=/tmp/lanes.env
printf 'docker/web.Dockerfile\n' | python3 .github/scripts/detect-rebake-lanes.py --stdin
grep -qx 'images=true' /tmp/lanes.env
grep -qx 'images_matrix=["web"]' /tmp/lanes.env


so a rename of the `f.write` output key fails the check.

Addresses the two live findings from the engrams review on #975. The other
three were against `dependabot-qa.yml`, which the reshape deleted.

## The contract check tested the debug print, not the contract

The "detector still names the images lane" step greps `images=True` and
`images_matrix=['web']`. Those are the detector's human-readable STDERR
diagnostic — the Python repr. The values `build-images` actually consumes are
the GITHUB_OUTPUT keys, and they differ: `images=true` (lowercase, via `b()`)
and `images_matrix=["web"]` (JSON, double quotes).

So renaming the emitted key leaves the stderr print untouched, the step stays
green, `build-images` reads an empty output and silently skips, and the CI Gate
passes on "skipped" — the exact hole the step exists to prevent, reopened
undetected. A check that cannot fail for the reason it was written.

It now runs the detector with GITHUB_OUTPUT pointed at a temp file (scoped to
that one command, so the job's own output file is untouched) and asserts the
real keys with `grep -qx`.

Verified by mutation rather than by reading: renaming the `f.write` key to
`container_images` FAILS the new check and PASSES the old one.

## build-images reintroduced a cache bake-images.yml deliberately removed

`type=gha,mode=max` is the exporter `bake-images.yml` documents having removed:
it round-trips to GitHub's real cache service from inside the Blacksmith
microVM (~0.2 MB/s, ~24 min/job), and the repo had already blown past the hard
10 GB per-repo cap and begun evicting — which would have taken the
`workspace-debug` rust-cache that lint, test-linux and test-e2e-stack rely on
with it. The expensive layer here is a build-local `RUN --mount=type=cache`
that BuildKit never exports to gha anyway.

Dropped, with a comment pointing at the reasoning so it is not added a third
time.

`just check`: 2264 passed.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@engrams-agent

engrams-agent Bot commented Aug 3, 2026 •

Copy link
Copy Markdown
Contributor

✅ engrams review — complete. 0 findings posted. · View details

@nikhilunni

Copy link
Copy Markdown
Contributor Author

Addressed. Two of the five findings were live; the other three were against
dependabot-qa.yml, which the reshape deleted before this review landed.

Contract check tested the debug print, not the contract — confirmed and
fixed. The detector emits images twice in different forms: stderr carries the
Python repr (images=True, ['web']), GITHUB_OUTPUT carries images=true and
JSON ["web"]. My grep matched only the stderr line, so renaming the emitted
key would have kept the step green while build-images silently skipped and the
Gate passed on "skipped".

It now runs the detector with GITHUB_OUTPUT pointed at a temp file (scoped to
that one command so the job's own output file is untouched) and asserts the real
keys with grep -qx.

I verified by mutation rather than by reading: renaming the f.write key to
container_images fails the new check and passes the old one. Worth
noting this was the same "test that cannot fail" pattern the PR description
criticizes, in the step written to prevent it.

type=gha cache — confirmed and dropped. bake-images.yml documents
removing that exporter: ~0.2 MB/s round-trip from the Blacksmith microVM
(~24 min/job), already past the 10 GB per-repo cap and evicting — which would
have taken the workspace-debug rust-cache that lint/test-linux/test-e2e-stack
depend on with it. And the expensive layer here is a build-local
RUN --mount=type=cache BuildKit never exports to gha anyway. Left a comment
pointing at the reasoning so it is not added a third time.

just check: 2264 passed.

@engrams-agent engrams-agent Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Engrams review

Verdict: No findings.
Severity: Critical 0 · High 0 · Medium 0 · Low 0

View the full engrams review

…ages lane

## Problem

`build-images` was gated on `needs.detect.outputs.images`, and that lane is much
broader than Dockerfiles: `IMAGES_PATHS` covers `web/`, `orchestrator/`,
`deploy/migrations/` and `BINARY_COMMON` (Cargo.lock, Cargo.toml). So the
previous commit would have put an image build on nearly every PR in the repo:

  docker/web.Dockerfile   -> ['web']
  web/src/App.tsx         -> ['web']
  orchestrator/src/*.ts   -> ['orchestrator']
  Cargo.lock              -> ['coordinator', 'host-agent', 'host-operator']
  any crate source change -> ['coordinator', 'host-agent', 'host-operator']

Three release Rust builds inside Docker on every Rust PR, for signal
`tests (linux)` already provides by compiling the same crates natively. That is
a CI regression, and it is not the hole this branch set out to close — #130 sat
green because a change to the build INSTRUCTIONS ran nothing.

## Fix

A `dockerfiles` lane flag (files under `docker/`), and `build-images` gates on
that. A Rust or web source change no longer builds an image; a Dockerfile change
does.

The residual risk is deliberate and now written down: a dependency that installs
on the runner but not inside the image's base still surfaces on main, exactly as
it does today. Closing that would cost three release builds per Rust PR.

The contract step asserts BOTH directions — a Dockerfile change sets
`dockerfiles=true`, and a `Cargo.lock` change sets it to `false` — so a future
widening of the flag fails the check rather than quietly restoring the cost.

Greps are `-qxF`. The bracket characters in `images_matrix=["web"]` are regex
metacharacters, and an unescaped pattern silently becomes a character class that
matches the wrong thing; fixed-string matching removes the footgun rather than
relying on escaping staying correct.

`just check`: 2264 passed. Step verified by extracting it from ci.yml and
running it verbatim.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@engrams-agent

engrams-agent Bot commented Aug 3, 2026 •

Copy link
Copy Markdown
Contributor

✅ engrams review — complete. 0 findings posted. · View details

@engrams-agent engrams-agent Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Engrams review

Verdict: No findings.
Severity: Critical 0 · High 0 · Medium 0 · Low 0

View the full engrams review

@nikhilunni
nikhilunni merged commit 7b360ae into main Aug 3, 2026
28 checks passed
@nikhilunni
nikhilunni deleted the dependabot-qa branch August 3, 2026 13:56
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.

1 participant