fix: pass explicit commit-message to wrangler (snapshot.yml + web-deploy.yml)#2550
fix: pass explicit commit-message to wrangler (snapshot.yml + web-deploy.yml)#2550
Conversation
Run 25641189988 failed at the Deploy to Cloudflare Pages step with: ERROR: A request to the Cloudflare API (.../pages/.../deployments) failed. Invalid commit message, it must be a valid UTF-8 string. [code: 8000111] Root cause: when --commit-message is omitted, wrangler auto-derives it from the latest git commit. The auto-generated GitHub merge commit body for PR #2549 contained em-dashes and arrows which are valid UTF-8 but which Cloudflare's API apparently rejects (their error message says "UTF-8" but the actual rule appears stricter — possibly ASCII-only). Fix: pass --commit-message explicitly with an ASCII-only string composed of the snapshot version + workflow run ID. Both are controlled values, no merge-commit content flows into the API call. Reads VERSION from wheels.json inline since the Deploy step doesn't inherit the earlier "Generate snapshot markdown" step's local VERSION variable.
Same wrangler/CF Pages bug as snapshot.yml — fixed in PR #2550 for the snapshot deploy job, but web-deploy.yml has an identical wrangler invocation that runs on every push to develop/main for all 5 sites (guides, blog, api, landing, packages). Without this fix, tomorrow's v4.0.0 GA tag push would trigger web-deploy.yml across 5 sites in parallel, and if the tag commit body or auto-derived message has non-ASCII characters (highly likely in release notes), all 5 deploys would fail with the same CF API rejection: Invalid commit message, it must be a valid UTF-8 string. [code: 8000111] Same fix as snapshot.yml: pass --commit-message explicitly with an ASCII-only string. Bonus: deploys are now self-describing in CF Pages's deployment list ("deploy guides from main (run 12345)").
There was a problem hiding this comment.
Wheels Bot — Reviewer A
TL;DR: PR #2550 is a well-targeted one-liner fix for a real CI failure. The root cause diagnosis is accurate (Cloudflare's API rejects non-ASCII characters in auto-derived commit messages from wrangler), and the chosen fix — passing a controlled ASCII-only --commit-message — is correct and safe. One commit-type convention nit and one follow-up item to note, but neither blocks merge.
Correctness
The fix is sound. Walking through it:
-
VERSION=$(jq -r '.version' ../wheels.json)— theworking-directory: webcontext makes../wheels.jsonthe right path (the preceding "Generate snapshot markdown" step, which runs without aworking-directory, useswheels.jsondirectly from the repo root; both resolve to the same file). No path confusion. -
The resulting message
"snapshot deploy: ${VERSION}-snapshot (run ${GITHUB_RUN_ID})"will always be ASCII-only:VERSIONis a semver string read from a repo-controlled file, andGITHUB_RUN_IDis a numeric string provided by GitHub Actions. No user content flows into the Cloudflare API call. -
One minor robustness concern: if
jqcannot read../wheels.jsonor the.versionkey is absent, it returns the literal stringnull, yielding"snapshot deploy: null-snapshot (run …)". This would still satisfy Cloudflare's ASCII requirement (the deploy wouldn't fail), but the CF Pages history entry would be misleading. Given that the "Generate snapshot markdown" step immediately before this one already reads the same file successfully, this is a purely theoretical edge case and not a blocker.
Commits
Nit — The commit type is fix:, but CLAUDE.md § "Common scope choices by file path" states:
.github/workflows/...| typeci, no scope (e.g.ci: pin softprops…)
The canonical form would be:
ci: pass explicit commit-message to wrangler in deploy-api-snapshot
commitlint won't hard-reject fix: (it is in the allowed-types list), so this won't break CI, but it's out of convention for workflow-only changes. Worth a quick git commit --amend before merging.
Docs
The PR body mentions auditing web-deploy.yml for the same gap. Looking at .github/workflows/web-deploy.yml lines 66-69:
pnpm dlx wrangler@4.78.0 pages deploy "sites/${SITE}/dist" \
--project-name="wheels-${SITE}" \
--branch="${DEPLOY_BRANCH}" \
--commit-hash="${GITHUB_SHA}"Same pattern — no --commit-message. A release tag fired on a commit whose message contains em-dashes or arrows (possible with merge commits) would hit the same CF rejection. The author flagged this as a follow-up, which is the right call given the v4.0.0 GA timeline. Recommend opening a follow-up issue so it doesn't get lost.
Security
No concerns. Both interpolated values (${VERSION}, ${GITHUB_RUN_ID}) are controlled — one from a repo file, one from the Actions runtime. The comment block in the diff correctly documents this: "Both ${VERSION} (read from wheels.json) and ${GITHUB_RUN_ID} are controlled values."
Overall the fix is correct and the self-describing commit message is a net improvement for observability in CF Pages. Only the fix: → ci: type swap and the web-deploy.yml follow-up issue creation are worth doing before or shortly after merge.
There was a problem hiding this comment.
Wheels Bot — Reviewer A
TL;DR: PR #2550 now covers both workflows (snapshot.yml + web-deploy.yml). The second commit addresses the gap called out in the previous review. The fix is correct in both locations. The only remaining item is the same commit-type convention nit: both commits use fix: for workflow-only changes where ci: is the project standard. Verdict: comment (convention nit only, nothing blocking).
Correctness
Both changes are sound.
snapshot.yml — unchanged from the first commit; analysis stands. VERSION is read from ../wheels.json (correct relative path under working-directory: web), GITHUB_RUN_ID is a numeric Actions-runtime value. The resulting message is guaranteed ASCII-only.
web-deploy.yml — new in this PR update. Three interpolated values:
SITEcomes frommatrix.site, a controlled matrix list (api,guides,blog,landing,packages). Safe.DEPLOY_BRANCHcomes fromgithub.head_ref || github.ref_name. This value was already being passed as-is to--branch="${DEPLOY_BRANCH}"before this PR, so any non-ASCII branch name would have caused a Cloudflare failure on that argument already. This PR introduces no new exposure.GITHUB_RUN_IDis a numeric string from the Actions runtime. Safe.
One theoretical edge: if DEPLOY_BRANCH contained a non-ASCII character, it would now appear in --commit-message as well as in --branch. Since Cloudflare already received it on --branch prior to this change, this is not a regression introduced here. Not a blocker.
Commits
Convention nit - both commits use type fix:, but CLAUDE.md "Common scope choices by file path" states:
.github/workflows/... maps to type ci, no scope (e.g. ci: pin softprops...)
The canonical forms would be:
ci: pass explicit commit-message to wrangler in deploy-api-snapshot
ci: pass explicit commit-message to wrangler in web-deploy.yml too
commitlint allows fix: (it is in the allowed-types list) so CI will not reject these, but the convention for pure workflow changes is ci:. Worth an amend + force-push before merge if the author wants to clean this up; not a blocker.
Follow-up item resolved
The previous Reviewer A comment (for commit d8da07a27) flagged the missing --commit-message in web-deploy.yml as a follow-up risk ahead of v4.0.0 GA. This second commit addresses it. No open follow-up items remain from that review.
Wheels Bot — Reviewer B (round 1)A's review is accurate on the core finding (fix is correct, commit type should be SycophancyNone detected. A correctly identified the root cause, walked through the variable sources, and flagged a real convention issue without inflating the verdict. False positives
Missed issues
Verdict alignmentA used ConvergenceA and I agree: both fixes are correct and complete, and the one outstanding item is the commit type on both commits ( |
Wheels Bot - Reviewer B (round 1)A's review is accurate and well-evidenced. The only substantive finding (commit type convention) is real, and A correctly labeled it non-blocking. Aligned with A; marking converged-changes so the commit messages can be cleaned up before merge. SycophancyNone detected. A issued a COMMENTED verdict with a concrete nit rather than a bare LGTM, and explicitly called the nit non-blocking. That framing is appropriate. False positivesNone detected.
Missed issuesNone of substance. A's DEPLOY_BRANCH analysis is technically sound -- Cloudflare may validate --branch and --commit-message under different rules, but since GitHub branch names are ASCII-safe in all realistic CI scenarios, this is not a missed issue. A noted the jq null fallback in the first review and correctly carried it forward. Verdict alignmentA's comment verdict is consistent with a single non-blocking convention nit and otherwise-sound code. Appropriate. ConvergenceAligned. The code changes are correct in both workflows; the only finding is the fix: to ci: commit-type swap, which A correctly identifies as a convention matter that commitlint won't hard-reject. Joint recommendation: amend both commit messages to ci: type before merge. Marking converged-changes to trigger cleanup. |
Wheels Bot — Address Review held for human reviewThe converged consensus from Reviewer A and Reviewer B has exactly one actionable finding: retype both commits on this PR from This finding cannot be addressed by a forward-only commit on the existing branch:
Recommendation for the human: either
The code changes themselves are correct in both files ( |
Summary
Run 25641189988 (snapshot for PR #2549's merge) succeeded on the BE pipeline core but failed on the parallel
deploy-api-snapshotjob. Cloudflare API rejected the deploy:While diagnosing, I discovered
web-deploy.ymlhas the same wrangler invocation with the same omission. That workflow runs on every push to develop/main across 5 sites. Ifv4.0.0GA's tag commit body has non-ASCII characters (very likely in release notes), all 5 deploys would fail tomorrow.Fixing both in this PR.
Root cause
When
--commit-messageis omitted, wrangler auto-derives the message from the latest git commit. PR #2549's auto-generated GitHub merge commit body contained em-dashes (—) and arrows (→) — valid UTF-8 but Cloudflare's API rejects them anyway. CF's error says "UTF-8" but the actual rule appears stricter (possibly ASCII-only).This wasn't caught by prior snapshot deploys because earlier PR merge messages happened to be plain ASCII.
Fix (applied to both workflows)
Pass
--commit-messageexplicitly with an ASCII-only string composed of controlled values:snapshot.yml (
deploy-api-snapshotjob):--commit-message="snapshot deploy: ${VERSION}-snapshot (run ${GITHUB_RUN_ID})"web-deploy.yml (5-site matrix deploy):
--commit-message="deploy ${SITE} from ${DEPLOY_BRANCH} (run ${GITHUB_RUN_ID})"No user-content flows into the API call. Bonus: every deploy is now self-describing in CF Pages's deployment list.
Test plan
deploy-api-snapshotsucceeds and updates api.wheels.devweb-deploy.ymldeploys all 5 sites cleanly (guides, blog, api, landing, packages)Why this matters for Tuesday GA
Without the web-deploy.yml fix, tomorrow's
v4.0.0tag would likely fail to deploy ANY of the 5 sites if the release notes commit body has typical em-dash/arrow content. Merging this PR before Tuesday is a meaningful pre-GA derisking.Refs