[claude]

Requested by Samuel Attard · Slack thread

Roller's build-images roll only rewrote a fixed list of files, and since electron/electron#51148 moved the CI build-image SHA into .github/actions/build-image-sha/action.yml, the workflow entries in that list matched nothing and the action file was never updated (as in electron/electron#55200, which bumped only the devcontainer). This adds the action file to the list, drops the workflow entries (verified against electron/electron main that none of them reference the build image SHA anymore), and points the shouldUpdateFiles check at the action file. Each file's own pin is now read and replaced (the build-image-sha= value in the action file, the ghcr.io/electron/devcontainer: tag in the devcontainer) rather than one shared SHA from the registry's previous tag, so files that have drifted apart are all bumped; if any file's pin is missing the roll fails loudly without opening a PR, and files are read from the roll branch being written to. Tests are updated to match, with new coverage for shouldUpdateFiles.

---

Generated by Claude Code

[claude]

Beyond the inline findings, I checked two things. shouldUpdateFiles does read files[0] (line 27), so it now checks action.yml. The OID swap uses a literal includes/split/join rather than a regex (lines 107-113), so the new build-image-sha=<oid> format needs no pattern change.

Extended reasoning...

Findings were reported inline. This note only records two follow-up checks on unchanged code that the change depends on, and both were ruled out by reading src/build-images-handler.ts.

Additional findings (outside the current diff — GitHub can't attach inline comments there):

• 🟣 src/build-images-handler.ts — Pre-existing: if a roll PR is already open when a newer image publishes, the newly added action.yml is never updated on that PR. Files are read from the default branch because getContent gets no ref (line 95-98). The writes, however, go to roller/build-images/main. Main still holds the older OID, so previousOid is not found, and the PR update is skipped (line 199). If a match did occur, main's blob sha would be stale for the branch and the write would be rejected. Fix: pass ref: branchName to getContent in updateFilesWithNewOid.

  Why this was flagged

  Trigger: roll PR #1 for OID B is open on roller/build-images/main, unmerged, and build-images publishes C. handleBuildImagesCheck takes the existing-PR branch at line 192. getPreviousOid returns B, the version right before C. updateFilesWithNewOid fetches each file with only ...REPOS.electron, path: filePath (lines 95-98), so it reads main, which still holds A. file.content.includes(previousOid) at line 107 is false for both files. updatedAny stays false, and line 199 logs 'No files were updated, skipping PR update'. The open PR stays on B. The base branch has the same flaw for docker-compose.yml. This diff routes the new action.yml target through the same path, so the CI image SHA is also left stale here. The try/catch at lines 94-124 only hides the 409 sha-mismatch error that a write would hit.

  Verification: pre-existing. Trigger: a roll PR on roller/build-images/main is still open and unmerged when a newer image is published. In src/build-images-handler.ts:95-98, getContent is called with no ref. Main still holds the older OID, so the check at line 107 is true for every file. The base commit had the same getContent call without ref and the same existing-PR flow.

[claude]

The pre-existing finding from the review summary is also fixed in 2e6f26a: updateFilesWithNewOid now passes ref: branchName to getContent, so an existing roll PR is updated against the content and blob shas on roller/build-images/main rather than main.

---

Generated by Claude Code

[claude]

The latest push brings back an issue flagged in an earlier review; replied in that comment's thread.

Still open from earlier reviews (1):

• 🔴 src/build-images-handler.ts:12 — action.yml can still go un-bumped after this merges if it does not hold exactly the previous package version's tag, giv…

If you have decided not to act on one of these findings, resolve its thread (a reply alone leaves it open) and the next review stops counting it. To review this commit again now, use Re-run on its "Claude Code Review" check.

[claude]

I reviewed the latest push (3bc7453) and found no bugs. It fixes the drift issue I raised earlier: each file is now bumped from its own current pin. The last human approval came before this commit, so a quick human look at the reworked flow would still be worthwhile.

What was reviewed:

• Per-file regexes in files: their capture groups pass through isValidOid before they are used, and the swap still uses literal split/join.
• getCurrentOids returns null when any file lacks a valid pin. In that case no PR is created or updated, so a roll can no longer quietly update only some of the files.
• The existing-PR path reads from the roll branch and skips the update when every file already matches the target. The new-PR path reads from the same sha that createRef uses.

Extended reasoning...

The change rewrites the build-images roller in src/build-images-handler.ts and its tests. It drops the GHCR previous-tag lookup and instead reads each pinned file's own 40-hex OID from action.yml and docker-compose.yml. Webhook-supplied and file-derived OIDs are still validated and substituted literally, so no new injection surface was found. CODEOWNERS assigns every path to the electron working groups, and the only human approval is on an earlier commit, so this review defers instead of approving.