Skip to content

fix(#4861): restore explicit npx wrangler@ pin in preCommands - #4864

Merged
waynesun09 merged 2 commits into
mainfrom
fix-4861-precommands-wrangler-path
Jul 15, 2026
Merged

fix(#4861): restore explicit npx wrangler@ pin in preCommands#4864
waynesun09 merged 2 commits into
mainfrom
fix-4861-precommands-wrangler-path

Conversation

@waynesun09

Copy link
Copy Markdown
Member

Summary

Restores the explicit npx wrangler@4.110.0 invocation inside both preCommands blocks in .github/workflows/site-deploy.yml, fixing the wrangler: not found failure that has broken every production and PR-preview deploy since PR #4102 merged.

Related Issue

Fixes #4861

Changes

  • .github/workflows/site-deploy.yml: revert wrangler versions secret bulk ... back to npx wrangler@4.110.0 versions secret bulk ... in both the production-deploy and PR-preview preCommands blocks.
  • Add comments explaining why the explicit npx wrangler@ pin must stay, to prevent this regressing again.

Root cause

PR #4102's second commit changed both preCommands blocks from an explicit npx wrangler@<version> ... invocation to bare wrangler ..., based on a review finding that cloudflare/wrangler-action already installs the pinned version and rewrites wrangler-prefixed commands to reuse it.

That rewrite ( dist/index.mjs, execCommands() in cloudflare/wrangler-action@9acf94a / v3.15.0) only fires when a preCommands line's entire string starts with the literal "wrangler":

const cmd = command.startsWith("wrangler")
    ? `${packageManager.exec} ${command}`
    : command;

Both preCommands here are single compound shell lines (set -eu; secrets_file=...; wrangler versions secret bulk ...; rm -f ...) — by design, per the existing comments, since preCommands runs each newline as a separate /bin/sh invocation and the mktemp/write/cleanup sequence needs to share one temp file path. That line starts with set, not wrangler, so the rewrite never triggers, and bare wrangler isn't on $PATH — every prod/preview deploy has failed with wrangler: not found (exit 127) since #4102 merged.

Testing

  • make lint passes (staged changes first)
  • YAML syntax validated (python3 -c "import yaml; yaml.safe_load(...)")
  • Tests added/updated for new or modified logic — not applicable, workflow-only fix; verified by confirming the next Deploy Site run against main succeeds.

Checklist

  • PR title follows Conventional Commits (correct type, ! for breaking changes)
  • Commits are signed off (DCO) — human and human-directed agent sessions only
  • I wrote this contribution myself and can explain all changes in it

PR #4102 changed both site-deploy.yml preCommands blocks from
`npx wrangler@<version> ...` to bare `wrangler ...`, based on a
review finding that wrangler-action already installs the pinned
version and rewrites `wrangler`-prefixed commands to use it.

That rewrite only fires when a preCommands line's full string starts
with the literal "wrangler" (wrangler-action's execCommands checks
command.startsWith("wrangler")). Both preCommands here are compound
shell lines starting with `set -eu; ...`, so the rewrite never
triggers and bare `wrangler` isn't on $PATH, failing every prod and
PR-preview deploy since #4102 merged with "wrangler: not found"
(exit 127).

Restore the explicit npx invocation in both blocks and document why
it must stay explicit, so this doesn't regress again.

Assisted-by: Claude
Signed-off-by: Wayne Sun <gsun@redhat.com>
@waynesun09
waynesun09 requested a review from a team as a code owner July 14, 2026 18:13
@fullsend-ai-review

fullsend-ai-review Bot commented Jul 14, 2026

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 6:14 PM UTC · Completed 6:22 PM UTC
Commit: a737637 · View workflow run →

@qodo-code-review

Copy link
Copy Markdown

PR Summary by Qodo

Fix deploy workflow by restoring explicit npx wrangler@ pin in preCommands

🐞 Bug fix ⚙️ Configuration changes 🕐 10-20 Minutes

Grey Divider

AI Description

• Restore explicit npx wrangler@4.110.0 in production and PR-preview preCommands.
• Prevent wrangler: not found failures caused by wrangler-action’s non-matching rewrite logic.
• Add comments documenting why preCommands must keep the explicit npx pin.
Diagram

graph TD
  A["site-deploy.yml"] --> B["wrangler-action step"] --> C["preCommands (single sh line)"] --> D["npx wrangler@4.110.0"] --> E[["Cloudflare API"]]
  C --> F[("temp secrets file")] --> D
  subgraph Legend
    direction LR
    _comp["Component"] ~~~ _tmp[("Temp file")] ~~~ _ext[["External"]]
  end
Loading
High-Level Assessment

The following are alternative approaches to this PR:

1. Refactor preCommands to start with `wrangler`
  • ➕ Would allow wrangler-action’s command.startsWith(&quot;wrangler&quot;) rewrite to trigger
  • ➕ Avoids explicit npx wrangler@... duplication if action-managed versioning is trusted
  • ➖ Conflicts with the single-line requirement (mktemp/write/use/cleanup must share state) unless moved into a script
  • ➖ More fragile: depends on action internals and exact string prefix matching
2. Move the secret-bulk logic into a checked-in script
  • ➕ Keeps workflow YAML simpler and easier to audit
  • ➕ Script can enforce strict shell options and be unit-tested/linted separately
  • ➖ Adds a new repo artifact to maintain
  • ➖ Still must decide how wrangler is provided (npx pin, bundled, or installed)
3. Install wrangler explicitly as a separate step (npm/pnpm)
  • ➕ Makes wrangler reliably available on PATH for compound shell lines
  • ➕ Decouples from wrangler-action’s rewriting behavior
  • ➖ Longer CI time and more moving parts
  • ➖ Version management can drift unless pinned and kept in sync

Recommendation: Keep the PR’s approach (explicit npx wrangler@4.110.0) because it is the smallest and most reliable fix: it avoids depending on wrangler-action’s prefix-based rewrite behavior, which cannot work with the required single-line preCommands. If maintainability becomes a concern later, consider moving the compound secret-bulk logic into a repository script while keeping an explicit wrangler provisioning strategy.

Files changed (1) +8 / -2

Bug fix (1) +8 / -2
site-deploy.ymlRestore pinned 'npx wrangler@4.110.0' in prod/preview preCommands +8/-2

Restore pinned 'npx wrangler@4.110.0' in prod/preview preCommands

• Replaces bare 'wrangler ...' calls with explicit 'npx wrangler@4.110.0 ...' in both production deploy and PR-preview upload preCommands. Adds comments explaining why the explicit 'npx wrangler@' pin is required (compound shell line doesn’t trigger wrangler-action’s rewrite).

.github/workflows/site-deploy.yml

@qodo-code-review

qodo-code-review Bot commented Jul 14, 2026

Copy link
Copy Markdown

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0) 📜 Skill insights (0)

Context used
✅ Compliance rules (platform): 54 rules

Grey Divider


Remediation recommended

1. Wrangler version duplicated ✓ Resolved 🐞 Bug ⚙ Maintainability
Description
The workflow now pins Wrangler in two places (wranglerVersion and npx wrangler@4.110.0 in
preCommands), so a future version bump can easily update only one and run secret upload with a
different Wrangler than the main deploy/upload command. This is a latent correctness/operability
risk (not a current mismatch) because the versions currently match but can silently diverge later.
Code

.github/workflows/site-deploy.yml[144]

+          preCommands: set -eu; secrets_file="$(mktemp)"; printf '%s\n' "GITHUB_APP_CLIENT_SECRET=${GITHUB_APP_CLIENT_SECRET}" "TURNSTILE_SECRET_KEY=${TURNSTILE_SECRET_KEY}" >"$secrets_file"; npx wrangler@4.110.0 versions secret bulk "$secrets_file" --message "Production secrets (workflow-run ${{ github.event.workflow_run.id }})"; rm -f "$secrets_file"
Relevance

⭐⭐⭐ High

Same “dedupe wranglerVersion vs npx wrangler@” suggestion was definitely accepted in PR #4102.

PR-#4102

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The workflow sets wranglerVersion: "4.110.0" but the modified preCommands lines also hard-code
npx wrangler@4.110.0, creating two independent version pins that must be kept in sync manually.

.github/workflows/site-deploy.yml[138-145]
.github/workflows/site-deploy.yml[166-173]
PR-#4102

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
The workflow specifies the Wrangler version twice: once via `wranglerVersion:` for `cloudflare/wrangler-action`, and again by hard-coding `npx wrangler@4.110.0` inside `preCommands`. This duplication makes it easy to update only one location later, creating inconsistent behavior between secret upload and the main action command.

### Issue Context
You still need an explicit `npx wrangler@...` invocation for the compound `preCommands` line to work, but the version should be defined once and referenced in both places.

### Fix Focus Areas
- .github/workflows/site-deploy.yml[25-37]
- .github/workflows/site-deploy.yml[138-145]
- .github/workflows/site-deploy.yml[166-173]

### Suggested fix
- Define a single version variable (e.g. `env: WRANGLER_VERSION: "4.110.0"` at the job level).
- Use it in both steps:
 - `wranglerVersion: ${{ env.WRANGLER_VERSION }}`
 - `preCommands: ...; npx wrangler@${{ env.WRANGLER_VERSION }} versions secret bulk ...`

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


2. Secrets temp file not trapped ✓ Resolved 🐞 Bug ⛨ Security
Description
preCommands writes secrets into a mktemp file and relies on a trailing rm -f, but set -e
will exit early on any failure (including the Wrangler call) and skip the cleanup. This can leave a
secrets-containing file on disk for the remainder of the job (and is worse on persistent/self-hosted
runners).
Code

.github/workflows/site-deploy.yml[172]

+          preCommands: set -eu; secrets_file="$(mktemp)"; printf '%s\n' "GITHUB_APP_CLIENT_SECRET=${GITHUB_APP_CLIENT_SECRET}" "TURNSTILE_SECRET_KEY=${TURNSTILE_SECRET_KEY}" >"$secrets_file"; npx wrangler@4.110.0 versions secret bulk "$secrets_file" --message "PR preview secrets (workflow-run ${{ github.event.workflow_run.id }})"; rm -f "$secrets_file"
Relevance

⭐⭐ Medium

No historical suggestions found about trapping mktemp secret-file cleanup in preCommands; acceptance
unclear.

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
Both modified preCommands lines explicitly use set -eu, create a mktemp secrets file, invoke
Wrangler, and only then run rm -f; with -e, any non-zero exit before the final segment prevents
cleanup from running.

.github/workflows/site-deploy.yml[140-145]
.github/workflows/site-deploy.yml[168-172]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
The compound `preCommands` line creates a temporary file containing secrets and only removes it at the end. Because the line starts with `set -e`, any failure (e.g., the `npx wrangler ...` invocation) will terminate the shell before reaching `rm -f`, leaving the secrets file behind.

### Issue Context
`preCommands` must remain a single line, but you can still use `trap` in that same line to guarantee cleanup.

### Fix Focus Areas
- .github/workflows/site-deploy.yml[144-144]
- .github/workflows/site-deploy.yml[172-172]

### Suggested fix
Insert an EXIT trap immediately after `mktemp`, e.g.:
- `preCommands: set -eu; secrets_file="$(mktemp)"; trap 'rm -f "$secrets_file"' EXIT; printf ... >"$secrets_file"; npx wrangler@... versions secret bulk ...`
Optionally remove the trailing `rm -f` since the trap will handle cleanup on both success and failure.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

Qodo Logo

Comment thread .github/workflows/site-deploy.yml Outdated
Comment thread .github/workflows/site-deploy.yml Outdated
@codecov

codecov Bot commented Jul 14, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@fullsend-ai-review

fullsend-ai-review Bot commented Jul 14, 2026

Copy link
Copy Markdown

Review

Verdict: comment

Clean, well-scoped fix for a critical production deployment outage. The root cause analysis is accurate and the implementation is correct. No blocking code issues found — the only required action is human approval for the protected path.

Correctness

  • npx --no-install wrangler correctly resolves to the version installed by wranglerVersion: "4.110.0". The --no-install flag ensures npx will fail rather than silently fetch a different version from the registry — this is safe. Verified that the packageManager input exists in cloudflare/wrangler-action@v3.15.0 (action.yml at the pinned commit @9acf94a).
  • trap 'rm -f "$secrets_file"' EXIT is POSIX-compliant and works correctly under /bin/sh (dash on Ubuntu). This is a strict improvement: under set -eu, the old trailing rm -f was unreachable if the wrangler command failed, leaving the secrets file on disk until job cleanup.
  • Both preCommands blocks (production deploy and PR preview) are fixed identically — no inconsistency.
  • Adding packageManager: npm is not strictly necessary (auto-detection defaults to npm when no lockfile exists in cloudflare_site/), but it makes the behavior explicit and prevents future detection issues.

Security

No concerns. The change is a net security improvement:

  • Secrets cleanup: The trap ... EXIT pattern ensures the temp file containing GITHUB_APP_CLIENT_SECRET and TURNSTILE_SECRET_KEY is removed even when wrangler fails under set -eu. The old pattern left the secrets file persisting on the runner's tmpfs until job end.
  • Supply-chain: npx --no-install refuses to fetch from the registry and only resolves locally-installed packages, eliminating any risk of resolving to an unintended package.
  • No new injection surfaces. All interpolated expressions (${{ github.event.workflow_run.id }}, ${{ vars.CLOUDFLARE_PROJECT_NAME }}) are unchanged. Workflow trigger, permissions, and fork-safety guards are unmodified.

Intent & Coherence

  • Traces directly to issue Deploy Site failing: wrangler: not found in preCommands after #4102 #4861 (priority/critical, type/bug, component/ci).
  • PR scope matches the issue's authorized fix: restoring the explicit wrangler invocation plus preventive comments.
  • The additions beyond the minimal fix (packageManager: npm, trap-based cleanup) are justified defensive hardening in the context of a critical production outage.
  • PR title fix(#4861) is appropriate — the broken deployment is user-visible (public documentation site stops updating), so fix type is correct per COMMITS.md. The scope uses the recommended issue-number pattern.

Style

Added comments follow the existing YAML comment style, indentation, and technical phrasing. The cross-reference pattern (see #4861) varies slightly from the existing parenthetical pattern (cloudflare/wrangler-action#374), but the difference is minor and the comments are clear.

Docs Currency

No documentation was made stale by this PR. One pre-existing staleness issue noted below.


Findings

protected-path (medium) — .github/workflows/site-deploy.yml is under .github/, a protected path. The PR has a linked issue (#4861) and a clear root-cause explanation. Human approval is required for this change.

stale-reference (low, pre-existing) — docs/superpowers/specs/2026-04-09-site-cloudflare-pages-design.md references "pinned 4.30.0 in workflow" but the workflow has used 4.110.0 since before this PR. Consider updating the design spec version reference as a follow-up.


Protected paths detected — this PR modifies files under .github/, a protected path. Human approval is required.

Protected files in this PR:

  • .github/workflows/site-deploy.yml
Previous run

Review

Verdict: approve

Clean, well-scoped fix for a critical production outage. The root cause analysis is accurate and the fix is correct.

Correctness

  • The npx wrangler@4.110.0 pin matches the existing wranglerVersion: "4.110.0" in both with: blocks — version consistency verified.
  • Both preCommands blocks (production deploy at line 141 and PR preview at line 169) are fixed identically — no inconsistency.
  • The compound shell line structure (set -eu; mktemp; printf; npx wrangler@...; rm -f) is preserved correctly — the substitution is surgical.
  • The command: fields (deploy ... and versions upload ...) are unaffected, which is correct — wrangler-action handles command: rewriting via a different code path than preCommands.
  • No other workflow files in the repo use cloudflare/wrangler-action, so no other files need the same fix.

Security

No concerns. The secrets handling pattern (write to temp file → wrangler versions secret bulkrm -f) is unchanged. No new permissions, no new secrets exposure, no injection vectors introduced. The explicit npx wrangler@4.110.0 pin is slightly better from a supply-chain standpoint than relying on the action's implicit PATH rewrite.

Intent & Coherence

  • Traces directly to issue Deploy Site failing: wrangler: not found in preCommands after #4102 #4861 (priority/critical, type/bug, component/ci).
  • PR scope exactly matches the issue's authorized fix — restoring the explicit npx invocation plus preventive comments.
  • No scope creep.
  • PR title fix(#4861) follows COMMITS.md conventions (issue-number scope, fix type appropriate for user-visible outage).

Style

Added comments follow the existing YAML comment style and indentation. The second block's comment cross-references the first block's explanation with (#4861) — consistent with the file's existing pattern of referencing issue numbers in comments.

Docs

No documentation was made stale by this PR. One pre-existing staleness issue noted below.


Low-severity note (pre-existing, not introduced by this PR):

docs/superpowers/specs/2026-04-09-site-cloudflare-pages-design.md line 99 references "pinned 4.30.0 in workflow" but the workflow has been using 4.110.0 since before this PR. Consider updating the design spec version reference as a follow-up.


Protected paths detected — this PR modifies files under one or more
protected paths. The review agent cannot approve PRs that touch these paths.
A human reviewer must approve this PR.

Protected files in this PR:

  • .github/workflows/site-deploy.yml

Labels: PR fixes a bug in the CI deploy workflow for the documentation site.

@fullsend-ai-review fullsend-ai-review Bot added requires-manual-review Review requires human judgment component/ci CI pipelines and checks type/bug Confirmed defect in existing behavior labels Jul 14, 2026
- Use `npx --no-install wrangler` instead of `npx wrangler@4.110.0` so
  wranglerVersion remains the single version source of truth
- Add `trap 'rm -f "$secrets_file"' EXIT` to guarantee cleanup if
  wrangler or any earlier command fails under `set -eu`
- Add explicit `packageManager: npm` to both steps so the invocation
  doesn't silently break if cloudflare_site gains a lockfile
- Pin the wrangler-action rewrite-rule comment to the verified commit
  SHA (9acf94a) so bumping the action pin prompts re-verification

Assisted-by: Claude (review, fix)
Signed-off-by: Wayne Sun <gsun@redhat.com>
@fullsend-ai-review

fullsend-ai-review Bot commented Jul 15, 2026

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 2:53 PM UTC · Completed 3:03 PM UTC
Commit: ea4b774 · View workflow run →

@waynesun09
waynesun09 added this pull request to the merge queue Jul 15, 2026
Merged via the queue into main with commit 0979e95 Jul 15, 2026
14 checks passed
@waynesun09
waynesun09 deleted the fix-4861-precommands-wrangler-path branch July 15, 2026 15:03
@fullsend-ai-review fullsend-ai-review Bot added requires-manual-review Review requires human judgment and removed requires-manual-review Review requires human judgment labels Jul 15, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

component/ci CI pipelines and checks requires-manual-review Review requires human judgment type/bug Confirmed defect in existing behavior

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Deploy Site failing: wrangler: not found in preCommands after #4102

2 participants