Validate ESP-Matter on every branch, on the base that run produced - #14
Merged
Conversation
A pull request touching only images/esp-matter/** matched the workflow's paths
filter, so esp-idf-build ran on its unchanged context and reported success. The
check was not merely absent - it was misleading: a green "🐳 ESP-IDF Docker Image"
on such a PR said nothing about whether ESP-Matter still compiled, and CLAUDE.md
had to carry a paragraph warning readers not to trust it.
The cause was `needs: esp-idf-manifest` on a job that only runs on master: needs on
a skipped job skips the dependent too. The condition is now
`${{ !cancelled() && … && needs.esp-idf-manifest.result != 'failure' }}`, so the
job proceeds when that manifest was skipped but not when it failed.
The base differs by context, deliberately. On master it is the digest artifact
esp-idf-manifest just published - the exact-base guarantee stays intact. Elsewhere
that artifact does not exist, so the build falls back to the last published
idf-<version> tag. That validates this image's own layers, which is what a change
to it touches; a PR editing both images checks each against the other's released
version, and the new combination is exercised when they land together.
Worth being explicit about what this does not do: the workflows are not split into
one file per image. Splitting would have broken the digest handoff, since the
artifact is scoped to a single run - the alternative being a workflow_run trigger
fetching another run's artifact through the API, or a return to building on a
mutable tag. Both cost more than the duplicated esp-idf-build leg that a
matter-only PR now runs, which is 1.5 minutes.
Also excludes !images/**/*.md from both workflows' paths. The docs-only merge that
started this whole series set off a full rebuild including a 5.5-hour ESP-Matter
leg, and republished every tag for images whose contents had not changed.
…fest Three findings, two of them P1, and one predates this branch. Fork pull requests were running on this org's paid runner pools. On a PR *from* a fork, github.repository_owner is the base repository - jethome-iot - so the owner check every build job carries was already true, and the fork's own Dockerfile executed on our machines. Enabling ESP-Matter on PRs would have extended that to two 180-minute builds of a fork-controlled context. Every build job now also requires github.event.pull_request.head.repo.full_name == github.repository. The PR fallback resolved a base tag that need not exist. A coordinated bump raises the ESP-IDF version and Matter's base_tag in the same branch, but the new esp-idf image is published only when that branch lands - so the fallback would have failed at FROM on precisely the change PR-5 makes. It now probes the registry and drops to :latest with a warning when the tag is not there yet, which still validates this image's own layers. On master, a failed esp-idf-build leaves esp-idf-manifest `skipped`, not `failed` - so `!= 'failure'` would have launched the Matter matrix to die downloading digest artifacts nobody created, burying the real failure under two more. master now requires the manifest to have succeeded; elsewhere it is skipped by design and only an outright failure stops the build.
The previous commit validated ESP-Matter on pull requests but resolved its base from the registry, and the code review showed that hollowed out the very case the validation exists for. A coordinated bump raises the ESP-IDF version and Matter's base_tag in one branch; on the PR the new base is published under no tag, so the build fell back to :latest - the *old* image - and reported green. The real pairing would first meet on master, after the esp-idf tags had already been republished. esp-idf-build now pushes by digest on every run, not only on master, and esp-matter-build takes the digest for its own platform straight from that job's artifact. Nothing published this way is reachable by name: only the manifest jobs create tags and they remain master-only, so a pull request moves no tag and leaves untagged blobs behind, which want an occasional cleanup. In exchange a PR checks the pair it will actually become. This simplifies more than it adds. esp-matter-build depends on esp-idf-build rather than the master-only manifest job - hanging off that job was the original reason ESP-Matter went unvalidated on PRs - so both branches behave identically and the status-function gymnastics disappear along with the registry probe, the :latest fallback, and the manifest-digest artifact nobody reads any more. Also from the review: `!cancelled()` drops the implicit success() over *every* `needs`, so a failed prepare no longer stopped this job - it surfaced as `Unexpected end of JSON input` from an empty matrix instead of the message check-versions.sh had already written. Moot now that the condition is a plain one, but it was real. And the GHCR login in esp-matter-build is unconditional: the job pulls its base on every run, where an anonymous pull carries rate limits an authenticated one does not.
Two findings from Codex, both about the push that the previous commit made unconditional. The login in esp-idf-build was still gated on master while push=true had become unconditional, so every dev push, same-repo PR and non-master dispatch would have failed at the push with no credentials - taking the ESP-Matter validation this series just added down with it, since no digest artifact would exist. The login is unconditional now, matching the push. Dependabot cannot push packages at all: GitHub hands its runs a read-only GITHUB_TOKEN whatever the permissions block asks for. Since Dependabot was enabled in this same series and its PRs edit workflow files - which are in the paths filter - this would have failed on the first automated bump. Those runs now skip the push and the digest upload, and esp-matter-build sits them out. The action bump is still exercised: esp-idf-build compiles the image, which is what such a PR changes.
There was a problem hiding this comment.
Pull request overview
Updates the repository’s Docker image CI workflows and documentation so ESP-Matter is reliably build-validated on PRs/dev using the ESP-IDF image produced in the same workflow run, while reducing unnecessary rebuilds and tightening fork/Dependabot behavior.
Changes:
- Make
esp-matter-builddepend onesp-idf-buildand consume per-platform base-image digests from build artifacts (instead of the master-only manifest job). - Exclude
images/**/*.mdfrom workflowpaths:filters to prevent documentation-only changes from triggering image rebuilds. - Add a head-repo check to prevent fork PRs from running on org runner pools; special-case Dependabot to avoid failing on GHCR pushes.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| README.md | Updates repo-level docs to reflect new validation behavior and doc-only trigger exclusion. |
| CLAUDE.md | Updates the repo’s CI/Docker image guidance to match the new digest handoff and validation model. |
| .github/workflows/platformio.yml | Excludes images/**/*.md from triggers and adds fork PR runner-pool protections. |
| .github/workflows/esp-idf.yml | Implements digest push/artifact handoff enabling ESP-Matter validation on PR/dev; adds doc-only trigger exclusion and fork/Dependabot handling. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Copilot caught two places left describing the old behaviour. The esp-idf.yml header still said dev/PR runs were build-only, which stopped being true when esp-idf-build started pushing by digest so esp-matter-build could consume the artifact. And the README generalised that to all build jobs, when it is specific to ESP-IDF: it is the only image another one is built from, so the only one with a reason to push outside master. PlatformIO still uploads nothing there, and its header now says why.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
Suppressed comments (2)
README.md:139
- The README note says every image is validated on “pull requests”, but the workflows intentionally skip PRs from forks via the head-repo check. Without clarifying that, this line can be read as promising validation on all PRs (including forks), which isn’t true.
**Note:** every image is build-validated on pull requests and on `dev`, including
one built `FROM` another image of this repo, and on the base that same run
produced — so a change touching both images is checked as the pair it will become.
.github/workflows/esp-idf.yml:355
- This step assumes the downloaded artifact contains exactly one file and then picks the first filename as the digest. If the artifact ever contains multiple files (or an unexpected filename), this can silently select the wrong digest and build esp-matter on the wrong base. Since the comment asserts “One file”, it’s safer to enforce exactly one file and validate it’s a full 64-hex sha256 digest.
set -euo pipefail
# One file, named after the digest it stands for.
digest=$(find /tmp/base -type f -printf '%f\n' | head -1)
case "${digest}" in
[0-9a-f]*) ;;
*) echo "::error::no usable base digest in the artifact"; exit 1 ;;
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
A pull request touching only
images/esp-matter/**matched the workflow'spaths:filter, so
esp-idf-buildran on its unchanged context and reported success. Thecheck was not merely absent — it was misleading, and
CLAUDE.mdcarried aparagraph warning readers not to trust it.
The cause was one line:
needs: esp-idf-manifeston a job whose dependency onlyruns on master. A
needson a skipped job skips the dependent too.ESP-Matter now builds on every branch
esp-matter-builddepends onesp-idf-buildinstead, and takes the digest for itsown platform from that job's artifact. Both branches behave identically — no status
expressions, no registry probing, no fallbacks.
That required
esp-idf-buildto push by digest on every run, not just master.Nothing published that way is reachable by name: only the manifest jobs create
tags, and they stay master-only. So a pull request moves no tag and leaves untagged
blobs behind — they accumulate and want an occasional cleanup, which is the
price of this.
What it buys is that a branch bumping ESP-IDF and Matter's
base_tagtogether —exactly what the next PR does — is checked as the pair it will become. The first
version of this change resolved the base from the registry instead, and review
showed that hollowed out the very case the validation exists for: on such a PR the
new base is published under no tag, so the build fell back to
:latest(the oldimage) and reported green, with the real pairing first meeting on master after the
esp-idf tags had already been republished.
Documentation-only changes trigger nothing
!images/**/*.mdis excluded from both workflows'paths:. The docs-only mergethat started this series set off a full rebuild including a 5.5-hour ESP-Matter leg
and republished every tag for images whose contents had not changed.
Two holes closed along the way
Fork pull requests were running on this org's paid pools, and had been since
before this series. On a PR from a fork,
github.repository_owneris the baserepository —
jethome-iot— so the owner check every build job carries was alreadytrue, and a fork-controlled Dockerfile executed on our machines. Every build job
now also requires
github.event.pull_request.head.repo.full_name == github.repository.Dependabot cannot push packages: GitHub hands its runs a read-only
GITHUB_TOKENwhateverpermissions:asks for. Since Dependabot was enabled twoPRs ago and its updates edit workflow files — which are in the paths filter — the
first automated bump would have failed on the digest push. Those runs skip the push
and the upload, and
esp-matter-buildsits them out; the bump is still exercised byesp-idf-build compiling the image.
Not done, deliberately
The workflows are not split into one file per image. Splitting would break the
digest handoff, which is scoped to a single run — the alternatives being a
workflow_runtrigger fetching another run's artifact through the API, or a returnto building on a mutable tag. Both cost more than the duplicated
esp-idf-buildleg that a matter-only PR runs, which is ~1.5 minutes.
esp-matter-buildalso runs for PRs that only touch shared files (a PlatformIOversion bump reaches it through
images/versions.json). Gating on the changedpaths was considered and declined: ~$0.6 on those runs, against a config that stays
simple.
Reviewed by
codex-review(xhigh), three rounds — the last one clean. It caught the fork-PRhole, the coordinated-bump failure at
FROM, a manifest job reportedskippedrather than
failedafter a build failure, and then, on the reworked version, thatthe login had stayed master-only while the push became unconditional, plus the
Dependabot token.
The
code-reviewworkflow (high) — ten defects, all confirmed. Beyond those above:!cancelled()drops the implicitsuccess()over everyneeds, so a failedpreparesurfaced asUnexpected end of JSON inputfrom an empty matrix insteadof the message
check-versions.shhad already written; and the first draft'sdocker manifest inspectprobe swallowed every registry error as "tag notpublished", silently substituting a different base.