feat(cloudflare-client): add zone account filter and overwrite guard on deploy#1728
Conversation
… guard for deployWorkerScript Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Hey @nit23uec,
⚠ Degraded review - no spec document was found for this change (searched the PR links, the touched repos' docs, the architecture/guidelines docs, and linked Jira). This review covers code-level quality but could not validate the change against an agreed design, so confidence is reduced. Add a spec link (PR template section 4) and re-request review for a full-confidence pass.
Verdict: Request changes - one blocking finding on internal consistency.
Complexity: LOW - small diff, single-service.
Changes: Adds an overwrite safety guard to deployWorkerScript and an accountId filter to listZones (3 files).
Must fix before merge
- [Important]
#workerExistsduplicates#cfFetchlogic instead of reusing the established helper -cloudflare-client.js:175-193(details inline)
Non-blocking (3): minor issues and suggestions
- nit: TOCTOU race between existence check and PUT is inherent; add a JSDoc note that the guard is best-effort, not atomic -
cloudflare-client.js:126 - suggestion: Use HTTP HEAD instead of GET in
#workerExiststo avoid fetching the full script body on large workers -cloudflare-client.js:181 - suggestion: URL-encode
accountIdinlistZonesquery string for defense-in-depth (encodeURIComponent(accountId)) -cloudflare-client.js:207
Skill: pr-review | Model: us.anthropic.claude-opus-4-6-v1[1m] | Duration: 2m 8s | Cost: $4.43 | Commit: 0a8b324bd0c10eab51cd4307840461a7686375a8
If this code review was useful, please react with 👍. Otherwise, react with 👎.
| if (!res.ok) { | ||
| const text = await res.text().catch(() => ''); | ||
| throw new Error(`Cloudflare API returned ${res.status} on ${path}: ${text.slice(0, 200)}`); | ||
| } |
There was a problem hiding this comment.
issue (blocking): #workerExists reimplements the HTTP fetch, auth-header injection, and error handling that #cfFetch already encapsulates. Every other method in this class routes through #cfFetch; this is the only one that bypasses it.
Why it matters: If #cfFetch gains retry logic, updated tracing headers, rate-limit handling, or modified error formatting, #workerExists will silently diverge. The pattern also invites copy-paste for future "exists?" checks (zones, routes, secrets).
Suggested fix: Extend #cfFetch to accept an option like { allowNotFound: true } that returns null on 404 instead of throwing:
async #workerExists(accountId, scriptName) {
const result = await this.#cfFetch(
`/accounts/${accountId}/workers/scripts/${scriptName}`,
{ allowNotFound: true },
);
return result !== null;
}This keeps auth and error handling in one place and establishes a reusable pattern.
…D; encode accountId Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
This PR will trigger a minor release when merged. |
There was a problem hiding this comment.
Hey @nit23uec,
⚠ Degraded review - no spec document was found for this change (searched the PR links, the touched repos' docs, the architecture/guidelines docs, and linked Jira). This review covers code-level quality but could not validate the change against an agreed design, so confidence is reduced. Add a spec link (PR template section 4) and re-request review for a full-confidence pass.
Verdict: Approve - prior findings cleanly addressed; no blocking issues remain.
Complexity: LOW - small diff, single-service.
Changes: Adds an overwrite safety guard to deployWorkerScript and an accountId filter to listZones, reusing #cfFetch via new allowNotFound and HEAD support (3 files).
Previously flagged, now resolved
#workerExistsnow delegates to#cfFetchwithallowNotFound: trueinstead of duplicating fetch logic- TOCTOU race documented in JSDoc as best-effort, not atomic
- HEAD method used for existence check (avoids fetching full script body)
encodeURIComponentapplied toaccountIdinlistZonesquery string
Non-blocking (2): minor issues and suggestions
- suggestion: Apply
encodeURIComponenttoscriptName(andaccountId) in the URL path inside#workerExistsanddeployWorkerScriptfor defensive-in-depth consistency with thelistZonesfix. Cloudflare constrains names to[a-z0-9-]so this is safe today, but the encoding is cheap insurance -cloudflare-client.js:187 - nit:
#cfFetchnow returns three distinct types (object | null | true). A one-line comment documenting the return contract (null = allowNotFound + 404, true = HEAD success, object = normal) would help the next reader reason about callers -cloudflare-client.js:48
Skill: pr-review | Model: us.anthropic.claude-opus-4-6-v1[1m] | Duration: 8m 31s | Cost: $3.63 | Commit: b955fcb60a25d782402aaa04a558284c671ff51c
If this code review was useful, please react with 👍. Otherwise, react with 👎.
## [@adobe/spacecat-shared-cloudflare-client-v1.1.0](https://github.com/adobe/spacecat-shared/compare/@adobe/spacecat-shared-cloudflare-client-v1.0.0...@adobe/spacecat-shared-cloudflare-client-v1.1.0) (2026-06-26) ### Features * **cloudflare-client:** add zone account filter and overwrite guard on deploy ([#1728](#1728)) ([6f8cbe6](6f8cbe6))
|
🎉 This PR is included in version @adobe/spacecat-shared-cloudflare-client-v1.1.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Summary
listZonesnow accepts an optionalaccountIdto scope results to a specific account (appendsaccount.idquery param to the Cloudflare API call)deployWorkerScriptdefaults tooverwrite: false— checks whether a script with the same name already exists before uploading and throws if it does, preventing accidental overwrites; callers must explicitly passoverwrite: trueto replace an existing scriptTest plan
listZones— new test: filters byaccountIdwhen provideddeployWorkerScript— new tests: throws by default when script exists, deploys when script is absent, skips check whenoverwrite: true, handles non-404 error and network failure from existence check🤖 Generated with Claude Code