Retry CLI release asset uploads and verify the release has every platform

[igorlukanin]

The v1.7.14 release shipped without cube-aarch64-apple-darwin.tar.gz, so the documented install command

curl -fsSL https://raw.githubusercontent.com/cube-js/cube/master/install-cli.sh | sh

404'd on every Apple Silicon Mac. The binary built fine — only the upload step failed, with a one-off Connect Timeout Error (attempted address: api.github.com:443, timeout: 10000ms). Because cube-cli runs fail-fast: false and nothing depends on it, the rest of the release published normally and the gap was invisible until someone tried to install.

Two changes:

• The upload is wrapped in nick-fields/retry@v3 (the convention already used elsewhere in this workflow), 5 attempts with a 30s wait, so a single API blip no longer drops a platform. It now shells out to gh release upload --clobber rather than svenstaro/upload-release-action, since that action has no retry input of its own. shell: bash is set explicitly — the retry action defaults to powershell on Windows, where ${GITHUB_REF_NAME} would not expand.
• A new cube-cli-assets-complete job runs after the matrix and fails the release if any of the five targets is missing from the release. A release that is missing a platform is now a red run instead of a silent 404.

The missing v1.7.14 asset has been re-uploaded separately by re-running the failed job, so the install works on Apple Silicon again.

Only the cube-cli upload path is touched here; the other five svenstaro/upload-release-action call sites (cubestored, native) have the same exposure but are left alone to keep this reviewable.

CORE-694

[claude]

Claude finished @igorlukanin's task in 2m 45s —— View job

---

Review complete — sound fix, worth landing. 2 medium, 3 low; the notable one is that the new guard job is skipped in exactly the failure mode it was written for. 5 inline comments posted.

Full review

Tasks

• Gather context (diff, surrounding cube-cli job, workflow triggers, other upload sites)
• Understand the request (code review)
• Review correctness, CI/release semantics, cross-platform behavior
• Check for prior Claude review threads to dedupe/resolve (none exist)
• Post inline comments and summary

What's good

• --clobber is the right call. It's what makes the retry actually idempotent — plain gh release upload fails with "asset already exists" if attempt 1 uploaded the asset but died before reporting success, which would have turned a transient blip into a permanent red.
• The shell: bash comment is a genuinely non-obvious catch. nick-fields/retry defaults to powershell on Windows, where ${GITHUB_REF_NAME} silently doesn't expand. Documenting the why inline saves the next person the debugging session.
• Correct blast radius. Scoping to the cube-cli path and explicitly naming the other five untouched svenstaro call sites in the PR body is the right reviewability trade-off.
• permissions: contents: read on the guard job, and --repo passed explicitly since that job has no checkout — both correct.
• The comment above the upload step records the v1.7.14 incident where the code lives, not just in the PR description. Good.

Findings

#	Severity	Location	Issue
1	Medium	L855	Guard job is skipped, not run, when a matrix leg fails — i.e. in the exact v1.7.14 scenario. if: ${{ !cancelled() }} makes it the authoritative signal and names the missing platform.
2	Medium	L848	gh release upload does not create the release; svenstaro/upload-release-action did. No gh release create exists anywhere in .github/workflows/, and cube-cli has no needs: — needs confirmation that the release object exists before this job runs.
3	Low–Med	L866–872	Target list is a hand-copy of the matrix at L787–800; adding a sixth target leaves the guard silently passing without covering it.
4	Low	L873	Name-only check accepts an asset stuck in state: starting or a 0-byte tarball — the exact residue an interrupted upload leaves. Filter on state/size.
5	Low	L843	retry_on: error doesn't retry on timeout_minutes expiry. The original failure is covered (non-zero exit), but a hung upload gets no retries. Matches existing file convention, so non-blocking.

Finding #2 is the only one I'd want answered before merge — it's a plausible regression rather than a style point, and I couldn't resolve it from the repo alone (I don't have API access to inspect who authored the v1.7.14 release object).

Notes on the other review dimensions

• Security — nothing to flag. ${{ matrix.target }} is the only expression interpolated into a shell command and it's workflow-authored, not attacker-controlled. GITHUB_TOKEN is passed via env: rather than an argv arg, so it won't leak into process listings or logs. The guard job correctly narrows to contents: read.
• Performance — negligible; one extra short-lived ubuntu-24.04 job per release, gated behind the matrix.
• Testing — not applicable in the usual sense: workflow changes on a push: tags trigger can't be exercised by CI on this PR. The real verification is the next tagged release, which is worth keeping in mind — findings ReferenceError: self is not defined - Issue with whatwg-fetch when using the core lib with Node.js #1 and MUTEX for requests #2 are both failure modes that will only surface at that point. A dry run against a throwaway tag on a fork would de-risk MUTEX for requests #2 if you'd rather not find out during a real release.
• Documentation — the inline comments are sufficient; no user-facing docs change is warranted since install-cli.sh behavior is unchanged.

• branch `igor/cli-release-upload-retry`

[claude]

Claude finished @igorlukanin's task in 2m 7s —— View job

---

Re-review of ad5994b — the three substantive points from the first pass are properly addressed. 1 medium, 2 low; the medium is that the new marker artifact breaks the re-run failed jobs recovery path. 3 inline comments posted, 4 prior threads resolved.

Full review

Tasks

• Gather context (diff of ad5994b vs e8b7d13, workflow triggers, cube-cli matrix)
• Review the marker-artifact approach replacing the hand-copied target list
• Re-check the four prior findings for resolution
• Resolve stale Claude threads, dedupe against existing ones
• Post inline comments and summary

How the prior findings landed

Prior	Status
#1 guard skipped on a failed leg	Fixed — if: ${{ !cancelled() }} at L882, with a comment recording why. Resolved.
#2 gh release upload doesn't create the release	Answered in-code at L843–845: the release is published before the tag push that triggers this workflow, so it always exists. Taking that as confirmed; resolved.
#3 hand-copied target list	Fixed, and better than what I suggested — each leg writes its own marker artifact, so the guard checks the matrix as it actually ran. !cancelled() on the marker steps is the right call: a leg that failed its upload must stay in the expected set. Resolved.
#4 name-only asset check	Fixed — select(.state == "uploaded" and .size > 0). Resolved.
#5 retry_on: error vs any	Unchanged and still applies (thread left open). Non-blocking, matches file convention.

New findings

#	Severity	Location	Issue
1	Medium	L866–872	upload-artifact@v4 409s on a duplicate artifact name, and artifacts survive across attempts — so re-running a failed leg (the v1.7.14 recovery path) now dies at the marker step before retrying the release upload. Needs overwrite: true.
2	Low	L899–903	The "matrix did not run" diagnostic is unreachable: cat expected/* fails under set -e when no markers exist, so the operator sees a cat error instead. Fails safe, just unhelpfully.
3	Low	L911	grep -qx treats the asset name as a regex; grep -qxF is the exact-match intent.

Only #1 would I fix before merge — it silently disarms the manual recovery you actually used last release. #2 and #3 are polish.

Other dimensions

• Correctness of the marker scheme — checks out. Artifact names are per-target so merge-multiple: true can't collide, each marker file holds one line, sort -u is harmless, and the <<< "$expected" loop is guarded by the preceding -z test (so no phantom empty-line iteration). shell: bash on the Windows leg is fine.
• Interaction with build failures — a leg that fails before the archive step still writes its marker, so the guard also reports missing assets for genuine build breaks. Slightly redundant with the already-red leg, but the alternative is the gap this PR closes, so this is the right trade.
• Security — unchanged from the first pass: only workflow-authored ${{ matrix.target }} reaches a shell, tokens go through env:, guard job narrowed to contents: read.
• Performance — five tiny artifact uploads plus one short ubuntu-24.04 job per release. Negligible.
• Testing — still not exercisable on a push: tags workflow from a PR. Finding ReferenceError: self is not defined - Issue with whatwg-fetch when using the core lib with Node.js #1 in particular will only surface the next time someone re-runs a failed leg, which is precisely when they're already firefighting.
• Documentation — inline comments are good and now carry the why for each non-obvious choice (shell: bash, !cancelled() in two places, the state filter, the release-pre-exists assumption). No user-facing docs change needed.

• branch `igor/cli-release-upload-retry`

[claude]

Claude finished @igorlukanin's task in 1m 52s —— View job

---

Re-review of 382dc5c — the unreachable-diagnostic fix is correct. 0 new mediums; 2 prior threads still open (1 medium, 1 nit) and 1 new low. 1 inline comment posted, 1 stale thread resolved.

Full review

Tasks

• Gather context (diff of 382dc5c vs ad5994b, current guard job, cube-cli matrix)
• Verify the empty-marker branch is now actually reachable
• Re-check the two still-open findings against the current diff
• Resolve stale Claude threads, dedupe against existing ones
• Post inline comment and summary

The fix in 382dc5c

Correct. expected=$(find expected -type f -exec cat {} + 2>/dev/null | sort -u || true) reaches the intended branch: errexit does not apply to the left operand of ||, and pipefail's non-zero pipeline status is consumed by the || true, so a missing or empty expected/ yields an empty $expected and the step reports "the matrix did not run" rather than a cat: 'expected/*' glob error. find also drops the "unexpanded glob" hazard entirely. Thread resolved.

Two details that hold up: -exec cat {} + on markers written with echo gives one clean line each (no missing trailing newline to swallow a name), and the <<< "$expected" loop is still guarded by the preceding -z test, so there's no phantom empty-line iteration.

Still open from the previous pass

Severity	Location	Issue
Medium	L872	upload-artifact@v4 409s on a duplicate artifact name, and artifacts persist across attempts — so Re-run failed jobs (the v1.7.14 recovery path) dies at the marker step before retrying the release upload. Needs overwrite: true. Unchanged in 382dc5c; re-affirmed, not re-posted.
Low	L914	grep -qx treats the asset name as a regex; -qxF is the exact-match intent.
Low	L848	retry_on: error doesn't cover timeout_minutes expiry. Answered and consciously deferred to match file convention — non-blocking.

overwrite: true is the one I'd still fix before merge; it's a one-line change and it disarms the manual recovery you actually used last release.

New finding

Severity	Location	Issue
Low	L866	The marker upload is a hard step with no retry, so an artifact-service blip on a leg whose release upload succeeded now reds an otherwise-complete release. Fails safe, but widens what can fail a release run; continue-on-error: true is the alternative if you'd rather it not gate.

Other dimensions

• Correctness — the marker scheme reads clean end to end: per-target artifact names can't collide under merge-multiple: true, sort -u is harmless, !cancelled() on both the marker steps and the guard job is the right choice in each place, and the state == "uploaded" and .size > 0 filter closes the stuck-asset hole.
• Permissions — download-artifact@v4 for the current run goes through the Actions Results API with the runtime token, not GITHUB_TOKEN, so the guard's contents: read narrowing does not break the download. (Only cross-run downloads would need actions: read.)
• Security — unchanged: ${{ matrix.target }} is the only expression reaching a shell and it's workflow-authored; tokens are passed via env:.
• Performance — five tiny artifacts plus one short ubuntu-24.04 job per release. Negligible.
• Testing — still not exercisable from a PR on a push: tags workflow. The open overwrite: finding in particular only surfaces during a re-run, i.e. when someone is already firefighting.
• Documentation — the inline comments now carry the why for every non-obvious choice (shell: bash, both !cancelled() sites, the state filter, the release-pre-exists assumption, and the empty-marker case). No user-facing docs change warranted.

• branch `igor/cli-release-upload-retry`

[claude]

Claude finished @igorlukanin's task in 1m 22s —— View job

---

Re-review of 5c06a12 — both fixes are correct; the marker step is now idempotent and non-gating. 0 mediums, 1 new low, 1 prior low still open (consciously deferred). 1 inline comment posted, 3 prior threads resolved. This looks ready to land.

Full review

Tasks

• Diff 5c06a12 vs 382dc5c
• Verify overwrite: true + continue-on-error interaction with the guard
• Re-check open threads against the current diff
• Resolve stale Claude threads, dedupe against existing ones
• Post inline comment and summary

The changes in 5c06a12

Both correct.

• overwrite: true closes the re-run hole. upload-artifact@v4 409s on a duplicate name and artifacts persist across attempts, so without this a re-run of a failed leg died at the marker step before ever retrying the release upload. Verified only the producer side needs the flag — the guard's download-artifact on a re-run picks up whatever markers exist, which is the desired behavior.
• continue-on-error: true makes the trade explicit: a dropped marker leaves that target unverified (its pre-PR status quo) rather than reddening a release whose assets are all actually present. The guard still hard-fails when no marker arrives.
• grep -qxF — exact literal line match, matching the intent.
• The comment above the step records why both flags are there, including the v1.7.14 recovery path. That's the right place for it.

Prior findings

Prior	Status
upload-artifact 409 breaks re-run	Fixed via overwrite: true. Resolved.
marker upload can redden a complete release	Fixed via continue-on-error: true. Resolved.
grep -qx treats name as regex	Fixed via -qxF. Resolved.
unreachable "matrix did not run" diagnostic	Fixed in 382dc5c (find … -exec cat). Resolved earlier.
retry_on: error vs any	Unchanged, thread left open. Answered and deferred to match file convention — non-blocking.

New finding

Severity	Location	Issue
Low	L873–878	overwrite: true is delete-then-upload; with continue-on-error swallowing a failure after the delete, that target silently drops out of the guard's expected set. Only reachable on a re-run — which is the recovery path for a failed upload, so it's the one case where losing verification reproduces the gap. Narrow; retrying the marker upload instead of swallowing it would get both properties, but not worth blocking on.

Other dimensions

• Correctness end to end — the marker scheme holds: per-target artifact names can't collide under merge-multiple: true, one clean line per marker via echo, sort -u harmless, the <<< "$expected" loop guarded by the preceding -z test, state == "uploaded" and .size > 0 closes the stuck-asset hole, and !cancelled() is the right choice at all three sites (both marker steps and the guard job).
• Permissions — download-artifact@v4 for the current run uses the Actions Results API with the runtime token, so the guard's contents: read narrowing doesn't break it. --repo passed explicitly since the guard has no checkout. Both correct.
• Security — unchanged: ${{ matrix.target }} is the only expression reaching a shell and it's workflow-authored; tokens go through env: rather than argv.
• Performance — five tiny artifacts plus one short ubuntu-24.04 job per release. Negligible.
• Testing — still not exercisable from a PR on a push: tags workflow. The remaining low only surfaces during a re-run; the main paths (retry, guard, marker) will be validated by the next tagged release.
• Documentation — inline comments now carry the why for every non-obvious choice (shell: bash, all three !cancelled() sites, the state filter, the release-pre-exists assumption, the empty-marker case, and both new flags). No user-facing docs change warranted — install-cli.sh behavior is unchanged.

• branch `igor/cli-release-upload-retry`