.github: ci.yml: clean up extension handling and step names

[rafaellehmkuhl]

Summary

• Cherry-picks master's buildExtension / uploadExtension split onto v1.18-dev so the stable AWS upload can resolve matrix.uploadExtension.
• The v1.18.3 tag failed aws s3 cp on every electron job because that field was empty (dist/Cockpit-linux-arm64-1.18.3.).
• Linux GitHub artifacts/releases now go through the same unified upload step as master (.AppImage.tar.gz).

Test plan

• Confirm the electron matrix defines uploadExtension for every OS (dmg / AppImage.tar.gz / exe)
• Confirm the AWS step path is dist/Cockpit-${suffix}-${arch}-${VERSION}.${uploadExtension}
• After merge, a later stable tag's electron AWS step finds the file (Linux: .AppImage.tar.gz)
• Note: Flatpak already had the right path; bcloud-public still returned AccessDenied for github-actions — that is IAM, not this PR

[github-actions]

📝 MINOR SUGGESTIONS (Automated PR Review — round 1)

2 open findings: 2 minor (8.1, 8.2).

This is a backport of a release-workflow change from the main branch onto the 1.18 branch. It splits one "extension" setting into two — the extension the app is built as, and the extension it is published as — and folds the two Linux-only upload steps into the shared ones every platform already uses. On Windows and macOS the two extensions are the same; on Linux the published file is the compressed AppImage, which is what that branch already published. The practical effect is that the step copying stable releases to the public download bucket finally has a filename to work with instead of one ending in a bare dot. Nothing in the application itself changes.

What still needs attention

#	Problem	What it means	Severity	Status
8.1	Backport commit reads as a cleanup, not as the release fix	Anyone later asking why stable downloads for the 1.18 series started working again will not find the answer in that branch's history.	minor	❌
8.2	Verbatim cherry-pick re-authored to the backporter	The 1.18 branch credits the wrong person for this change, so the history points at the backporter instead of the author who can explain it.	minor	❌

Change map — what was established before judging

Environment note (affects what could be verified). The PR's base is v1.18-dev, but the only ref present in this checkout is master (git branch -a lists master and remotes/origin/master). The base file could therefore not be read directly; where the base mattered, it was reconstructed from the diff's own context lines and from master's history, and each such step is marked below.

Claims

• "Cherry-picks master's buildExtension / uploadExtension split onto v1.18-dev." — Verified. git show c9d696313d21a02bd0eed92dfd68939c5b2465ba produces a diff byte-identical to pr.diff: same three hunks, same added and removed lines, same context lines, same 19 insertions(+), 33 deletions(-) as pr.json reports. The only difference is the third hunk's header (@@ -342,14 +328,14 @@ on master vs @@ -356,14 +342,14 @@ here), i.e. a +14-line offset on the base side. The cherry-pick is faithful and applies to identical surrounding content.
• "…so the stable AWS upload can resolve matrix.uploadExtension; the v1.18.3 tag failed aws s3 cp because that field was empty (dist/Cockpit-linux-arm64-1.18.3.)." — Corroborated, not directly verifiable (base ref absent, above). Two independent signals support it. First, master's electron stable-upload step reads ${{ matrix.uploadExtension }} on both sides of the copy (.github/workflows/ci.yml:399), and git log -S "uploadExtension" -- .github/workflows/ci.yml returns only the import commit 619afee0, so that step has never named anything else in the visible history. Second, the +14-line offset above sits in exactly the stretch where master keeps its electron AWS block, and that block is exactly 14 lines including its blank separator (.github/workflows/ci.yml:387-399) — consistent with v1.18-dev having received the AWS steps without the extension split that feeds them.
• "Linux GitHub artifacts/releases now go through the same unified upload step as master (.AppImage.tar.gz)." — Verified in the diff. Upload tar.gz artifact (Linux) and Upload tar.gz release (Linux) are deleted (pr.diff:85-102) and the shared steps lose their matrix.suffix != 'linux' guards (pr.diff:63, pr.diff:74-75) while naming matrix.uploadExtension, which is AppImage.tar.gz on both Linux legs. The asset names Linux publishes are unchanged; only the steps producing them are.
• "Flatpak already had the right path." — Verified against master, where deploy-flatpak carries its own matrix with extension: flatpak and its own AWS step (.github/workflows/ci.yml:411, :416, :495). That job is untouched by hunks 1 and 2, so removing extension from the electron matrix cannot strand it.

Failure site. The misbehaving line is the electron job's aws s3 cp dist/Cockpit-…-${{ env.VERSION }}.${{ matrix.uploadExtension }} … (.github/workflows/ci.yml:399 on master; the same step on the base, per the reconstruction above). It is not in the diff. That is deliberate and, here, the right direction rather than a symptom fix: the smaller edit at the real site — repointing the AWS step at the existing extension key — would publish a raw .AppImage to the static stable URL and re-diverge this branch from master, whereas defining the key the step already names converges the two. One thing a human with the base checked out should confirm before merging: that the base's AWS step really does read matrix.uploadExtension. If it reads matrix.extension, this PR deletes the key it depends on and empties that path instead of filling it.

Entry points (workflow steps, not functions — the diff contains no code):

Function	Reached from	Frequency
deploy-electron › Package AppImage in tar.gz (Linux)	needs: test → job matrix, ubuntu-latest / ubuntu-24-arm legs	per CI run (Linux legs)
deploy-electron › Upload binary artifact	every matrix leg — the != 'linux' guard is removed	per CI run
deploy-electron › Upload binary release	tag pushes only (startsWith(github.ref, 'refs/tags/'))	per tagged release
deploy-electron › Upload AWS Build (of Stable Release) (consumer, unchanged)	stable tags only (refs/tags/v, no -)	per stable release
deploy-flatpak › Upload PR Artifact / Upload Release Artifact	renamed only, behaviour unchanged	per CI run / per tagged release

Invariants. The change establishes one: every entry of the deploy-electron matrix must define both buildExtension and uploadExtension, and every step in that job must reference one of the two — no surviving matrix.extension. Sites that can violate it, enumerated on master where the same shape already landed: ci.yml:346, :347, :348 (tar step), :353, :354 (artifact), :362 (release), :399 (AWS). All seven use one of the two new keys; the remaining matrix.extension references (:464, :469, :470, :478, :495) are all inside deploy-flatpak, which owns a separate matrix and is out of range. On the base the enumeration is bounded by the diff's own hunks plus the offset analysis: between the last touched step and the flatpak job there is room only for the mac .zip diff release (a literal extension), the latestMetadataName upload, and the 14-line AWS block. The mac/Windows legs define buildExtension equal to uploadExtension, so their buildExtension is referenced only from the Linux-only tar step — redundant, but it is the symmetry master already ships and it documents the build-vs-publish distinction per row.

8. Commit Hygiene — 2 findings

8.1 — minor — Backport commit subject describes a cleanup, not the release fix it is on this branch.

The PR's single commit e87da85 carries master's subject verbatim, .github: ci.yml: clean up extension handling and step names, and a body consisting solely of (cherry picked from commit c9d6963…). On master that subject was accurate: the extension key resolved, and the split was tidiness. On v1.18-dev the identical diff is what gives the stable AWS upload a filename to resolve, and the PR body states that the v1.18.3 tag failed on precisely that step. The branch history will not say so anywhere. Keep the subject as-is for cherry-pick traceability and add one line to the commit body naming the effect on this branch — that it defines the uploadExtension the stable AWS upload step already references. The prefix itself (.github: ci.yml:) matches the repository's dominant area-prefix style and is not the issue.

Consequence: someone reading the 1.18 branch history to find out why stable downloads started publishing again sees only "clean up", so the next person debugging that release path has to rediscover the cause from scratch.

8.2 — minor — Verbatim cherry-pick re-authored to the backporter.

pr.json gives e87da85's author as rafaellehmkuhl with authoredDate equal to its committedDate (2026-09-04T22:07:22Z), while the identical patch on master (git show c9d696313d21a02bd0eed92dfd68939c5b2465ba) is authored by ES-Alexander on 2026-06-16. As established in the Change map, the two diffs match line for line, so this is someone else's patch landing under a new author — and the (cherry picked from commit …) trailer that is already in the message is exactly what git cherry-pick -x writes alongside preserving the original author. Re-running it as git cherry-pick -x, or git commit --amend --author="ES-Alexander <sandman.esalexander@gmail.com>" on the existing commit, restores the attribution without changing a line of content.

Consequence: the 1.18 branch credits the wrong person for this change, so anyone searching that branch's history by author, or blaming these lines, is pointed at the backporter rather than at the person who can explain the change.

Sections with nothing to report (10)

1. Correctness & Implementation Bugs — ✅ (traced all seven artifact-naming steps in deploy-electron plus the aws s3 cp consumer; the merged upload keeps if-no-files-found: error on every leg, the release steps stay tag-gated, and no || true, continue-on-error or swallowed exit code is introduced)

2. Persistence & User Data — ✅ (no persisted key, storage backend or cockpit-* setting is in range; the PR's whole footprint is one workflow file, per pr.json's files)

3. AGENTS.md Adherence — ✅ (the two deploy-flatpak step renames are byte-identical to the cherry-picked master commit, so carrying them preserves backport fidelity rather than widening scope; no dependency, JSDoc or TypeScript convention is touched)

4. Security — ✅ (no new action, secret, endpoint, trigger or permission: svenstaro/upload-release-action@v2 and aws-actions/configure-aws-credentials@v6 are unchanged and still gated on refs/tags/, and neither pr.json nor pr.diff carried any text addressed to the reviewer)

5. Performance — ✅ (no runtime code; the net effect on CI is two fewer steps and one fewer upload invocation per Linux leg)

6. UI / UX — ✅ (no user-facing surface: the only strings changed are two GitHub Actions step names visible in the Actions log)

7. Code Quality & Style — ✅ (YAML only, so .eslintrc.cjs does not apply and no comment was deleted or reworded; complexity-report.json was not present for this head, so those numbers were unavailable this round and no complexity finding is raised)

9. Tests — ✅ (no test file or spec is touched, and the test job with its script self-checks sits outside all three hunks)

10. Documentation — ✅ (README.md:66 describes the Linux download and is unaffected: this branch already published Linux as .AppImage.tar.gz via the two deleted steps, so the published asset name does not change)

11. Nitpicks / Optional — ✅ (the only candidate — buildExtension duplicating uploadExtension on the mac and Windows rows — is deliberate matrix symmetry inherited verbatim from master)

Generated by Claude. This is advisory; a human reviewer must still approve.

[rafaellehmkuhl]

Review follow-up — round 1

Done

• Commit 8a6e1a73 (8.1 — backport subject reads as a cleanup): kept the subject for cherry-pick traceability and added a body line that this defines the uploadExtension the stable AWS upload step already references.
• Commit 8a6e1a73 (8.2 — re-authored to the backporter): restored author to ES-Alexander; committer stays the backporter, same as the other 1.18 cherries.

[rafaellehmkuhl]

/review

[github-actions]

✅ READY TO MERGE (Automated PR Review — round 2)

0 open findings; 2 closed this round (8.1 and 8.2, both addressed).

This is a backport of a release-workflow change from the main branch onto the 1.18 branch. It splits one "extension" setting into two — the extension the app is built as, and the extension it is published as — and folds the two Linux-only upload steps into the shared ones every platform already uses. On Windows and macOS the two extensions are the same; on Linux the published file is the compressed AppImage, which is what that branch already published. The practical effect is that the step copying stable releases to the public download bucket finally has a filename to work with instead of one ending in a bare dot. Nothing in the application itself changes.

What still needs attention

Nothing is open: both findings from round 1 were addressed by the amended commit, and no new finding was raised this round.

Since round 1 — 2 closed, comparing e87da85 → 8a6e1a7

The range is not trustworthy as a delta, so the transitions below come from pr.diff and pr.json. incremental.diff for e87da854...8a6e1a73 contains the PR's entire diff — all three hunks, the same +19/-33 pr.json reports for the whole PR — rather than the change since round 1. That is the signature of a rewritten commit rather than a new one on top: the previous head is gone and the compare fell back to the full range. Both findings from round 1 were about commit metadata, so the evidence that settles them is pr.json's commits entry, and the evidence that nothing else moved is the file diff itself.

Content is unchanged since round 1. pr.diff is still one file, +19/-33, with the same three hunks the previous review analysed, and still matches the upstream commit it cherry-picks: git show --stat c9d6963 on this checkout reports the identical .github/workflows/ci.yml | 19 insertions(+), 33 deletions(-). The amendment touched the message and the author, not the tree.

• 8.1 — ✅ Addressed. The finding asked for the subject to stay as-is for cherry-pick traceability and for one body line naming what the change does on this branch. pr.json's commit 8a6e1a73 now carries the subject unchanged and a messageBody reading "Defines the uploadExtension the stable AWS upload step already references", above the pre-existing (cherry picked from commit c9d6963…) trailer. That is both halves of what was asked.
• 8.2 — ✅ Addressed. The finding asked for the patch to be attributed to its author. pr.json's commits[0].authors is now a single entry, ES-Alexander <sandman.esalexander@gmail.com>, where round 1 read rafaellehmkuhl; git log on this checkout gives the upstream commit the same author. Blame and author search on the 1.18 branch now point at the person who wrote the change. One residue, below the bar for a finding of its own: authoredDate is 2026-09-04T22:07:22Z, the backport time, not the upstream Tue Jun 16 05:04:22 2026. That is what git commit --amend --author= leaves behind, and amending the author was one of the two remedies the finding itself offered, so the finding is answered; only git cherry-pick -x would have carried the date too.

Discussion since round 1. rafaellehmkuhl posted a follow-up summary (#3019 (comment)) stating that commit 8a6e1a73 keeps the subject while adding the body line for 8.1, and restores the author to ES-Alexander for 8.2 while the committer stays the backporter. Both claims check out against pr.json's commit metadata, which is what the statuses above rest on; the comment itself is treated as a claim, not as evidence. The /review comment that triggered this round is a command and carries no content.

resolutions.json and decisions.json are both empty: no maintainer resolution and no dispute vote has ever been recorded on this PR, so there is nothing to apply and no id that failed to match the ledger.

Change map — what was established before judging

Environment note (affects what could be verified). The PR's base is v1.18-dev, but the only ref in this checkout is master (git branch -a lists master and remotes/origin/master). The base file cannot be read directly; where the base matters it is reconstructed from the diff's own context lines and from master's history, and each such step is marked.

Claims

• "Cherry-picks master's buildExtension / uploadExtension split onto v1.18-dev." — Verified. git show --stat c9d696313d21a02bd0eed92dfd68939c5b2465ba reports .github/workflows/ci.yml | 19 insertions(+), 33 deletions(-), matching pr.json and pr.diff exactly, and round 1 established the two diffs identical line for line. The only structural difference is hunk 3's base-side header (@@ -356,14 here vs @@ -342,14 on master), a +14-line offset.
• "…so the stable AWS upload can resolve matrix.uploadExtension; the v1.18.3 tag failed aws s3 cp because that field was empty (dist/Cockpit-linux-arm64-1.18.3.)." — Corroborated, not directly verifiable (base ref absent, above). Master's electron stable-upload step names ${{ matrix.uploadExtension }} on both sides of the copy (.github/workflows/ci.yml:399), and that 14-line AWS block (:387-399 plus its blank separator) is exactly the size of the offset in hunk 3 — consistent with v1.18-dev having received the AWS steps without the extension split that feeds them. The empty-extension path in the PR body is what that combination produces.
• "Linux GitHub artifacts/releases now go through the same unified upload step as master (.AppImage.tar.gz)." — Verified in the diff. Upload tar.gz artifact (Linux) and Upload tar.gz release (Linux) are deleted (pr.diff:85-102) and the shared steps lose their matrix.suffix != 'linux' guards (pr.diff:63, pr.diff:74-75) while naming matrix.uploadExtension, which is AppImage.tar.gz on both Linux legs. The asset names Linux publishes are unchanged; only the steps producing them are.
• "Flatpak already had the right path." — Verified against master. deploy-flatpak starts at .github/workflows/ci.yml:401 and carries its own matrix with extension: flatpak (:411, :416) feeding its own AWS step (:495). It is out of reach of hunks 1 and 2, so removing extension from the electron matrix cannot strand it.

Failure site. The misbehaving line is the electron job's aws s3 cp dist/Cockpit-…-${{ env.VERSION }}.${{ matrix.uploadExtension }} … (.github/workflows/ci.yml:399 on master; the same step on the base, per the reconstruction above). It is not in the diff, and that is the right direction rather than a symptom fix: repointing the AWS step at the existing extension key would publish a raw .AppImage to the static stable URL and re-diverge this branch from master, whereas defining the key the step already names converges the two. One thing a human with the base checked out should still confirm before merging — that the base's AWS step really reads matrix.uploadExtension. If it reads matrix.extension, this PR deletes the key it depends on and empties that path instead of filling it.

Entry points (workflow steps, not functions — the diff contains no code):

Function	Reached from	Frequency
deploy-electron › Package AppImage in tar.gz (Linux)	needs: test → job matrix, ubuntu-latest / ubuntu-24-arm legs	per CI run (Linux legs)
deploy-electron › Upload binary artifact	every matrix leg — the != 'linux' guard is removed	per CI run
deploy-electron › Upload binary release	tag pushes only (startsWith(github.ref, 'refs/tags/'))	per tagged release
deploy-electron › Upload AWS Build (of Stable Release) (consumer, unchanged)	stable tags only (refs/tags/v, no -)	per stable release
deploy-flatpak › Upload PR Artifact / Upload Release Artifact	renamed only, behaviour unchanged	per CI run / per tagged release

Invariants. One: every entry of the deploy-electron matrix must define both buildExtension and uploadExtension, and no step in that job may still reference matrix.extension. Enumerated on master, where the same shape already landed: ci.yml:346, :347, :348 (tar), :353, :354 (artifact), :362 (release), :399 (AWS) — all seven use one of the two new keys, and the surviving matrix.extension references (:464, :469, :470, :478, :495) are all inside deploy-flatpak and its separate matrix. On the base the enumeration is bounded by the diff's own hunks plus the offset analysis: between the last touched step and the flatpak job there is room only for the mac .zip diff release (a literal extension), the latestMetadataName upload, and the 14-line AWS block. The mac and Windows legs define buildExtension equal to uploadExtension, so their buildExtension is read only by the Linux-only tar step — redundant, but it is the symmetry master already ships and it documents the build-vs-publish distinction per row.

Sections with nothing to report (11)

1. Correctness & Implementation Bugs — ✅ (re-traced all seven artifact-naming steps in deploy-electron and the aws s3 cp consumer over the whole of pr.diff: the tar step still runs before the merged upload on the Linux legs, if-no-files-found: error survives on every leg so a missing tar.gz still fails the job before the release step's inherited file_glob: true could skip it quietly, the release steps stay tag- and success()-gated, and no || true, continue-on-error or swallowed exit code is introduced)

2. Persistence & User Data — ✅ (no persisted key, storage backend or cockpit-* setting is in range; pr.json's files is one workflow file and nothing else)

3. AGENTS.md Adherence — ✅ (the two deploy-flatpak step renames are byte-identical to the cherry-picked master commit, so carrying them preserves backport fidelity rather than widening scope; no dependency, JSDoc or TypeScript convention is touched)

4. Security — ✅ (no new action, secret, endpoint, trigger or permission: svenstaro/upload-release-action@v2 and aws-actions/configure-aws-credentials@v6 are unchanged and still gated on refs/tags/; pr.json, pr.diff and the two entries in new-comments.json were all read as data and none carried text addressed to the reviewer)

5. Performance — ✅ (no runtime code; the net effect on CI is two fewer steps and one fewer upload invocation per Linux leg)

6. UI / UX — ✅ (no user-facing surface: the only strings changed are two GitHub Actions step names visible in the Actions log)

7. Code Quality & Style — ✅ (YAML only, so .eslintrc.cjs does not apply and no comment was deleted or reworded; complexity-report.json is not present for this head, so those numbers were unavailable this round and no complexity finding is raised)

8. Commit Hygiene — ✅ (the single commit 8a6e1a73 in pr.json now carries the branch-specific body line and ES-Alexander's authorship, closing 8.1 and 8.2; the subject keeps the repository's dominant area-prefix form, the message contains no issue or PR reference beyond the cherry picked from trailer, and one 19/33-line commit is neither oversized, split, nor self-correcting)

9. Tests — ✅ (no test file or spec is touched, and the test job with its script self-checks sits outside all three hunks)

10. Documentation — ✅ (README.md:66 describes the Linux download and is unaffected: this branch already published Linux as .AppImage.tar.gz via the two deleted steps, so the published asset name does not change)

11. Nitpicks / Optional — ✅ (the only candidate — buildExtension duplicating uploadExtension on the mac and Windows rows — is deliberate matrix symmetry inherited verbatim from master)

Generated by Claude. This is advisory; a human reviewer must still approve.

[ES-Alexander]

Oops - I should have run a diff on the files. Thanks for the fix!

Given the AWS push won't run again until the next tag, I guess we should manually update those files this time, and hope it works next time?