From 1c551b5a06d55d7341b2d86c840065dd8b5e144d Mon Sep 17 00:00:00 2001 From: stordahl Date: Sun, 6 Sep 2026 12:13:55 +0000 Subject: [PATCH 1/2] fix(cli): run bundled wrangler binary instead of npx `npx wrangler` resolves whatever wrangler is cached in ~/.npm/_npx for the bare "wrangler" spec (or on PATH), which can be an old version that misreads the modern `assets.directory` config in wrangler.json and fails with "assets.bucket is a required field". Run the wrangler binary bundled with @counterscale/cli's own dependency tree instead, falling back to `npx wrangler` only if resolution fails. Fixes #256 --- .../cli/src/lib/__tests__/cloudflare.test.ts | 88 ++++++++++++++++++- packages/cli/src/lib/cloudflare.ts | 36 ++++++-- 2 files changed, 118 insertions(+), 6 deletions(-) diff --git a/packages/cli/src/lib/__tests__/cloudflare.test.ts b/packages/cli/src/lib/__tests__/cloudflare.test.ts index 4a9956d2..0ebea42e 100644 --- a/packages/cli/src/lib/__tests__/cloudflare.test.ts +++ b/packages/cli/src/lib/__tests__/cloudflare.test.ts @@ -1,9 +1,11 @@ import { describe, it, expect, vi, beforeEach, afterEach } from "vitest"; -import { CloudflareClient } from "../cloudflare.js"; +import { CloudflareClient, getBundledWranglerBinPath } from "../cloudflare.js"; import { ProcessOutput } from "zx"; import path from "path"; import { homedir } from "node:os"; +const nodeModuleState = vi.hoisted(() => ({ throwOnCreateRequire: false })); + // Mock the entire zx module vi.mock("zx", async (importOriginal) => { const actual = await importOriginal(); @@ -13,6 +15,19 @@ vi.mock("zx", async (importOriginal) => { }; }); +vi.mock("node:module", async (importOriginal) => { + const actual = await importOriginal(); + return { + ...actual, + createRequire: vi.fn((...args) => { + if (nodeModuleState.throwOnCreateRequire) { + throw new Error("Simulated resolution failure"); + } + return actual.createRequire(...args); + }), + }; +}); + // Import and spy on the mocked function const { $ } = await import("zx"); @@ -25,6 +40,7 @@ describe("CloudflareClient", () => { beforeEach(() => { vi.clearAllMocks(); + nodeModuleState.throwOnCreateRequire = false; client = new CloudflareClient(mockConfigPath); }); @@ -45,6 +61,76 @@ describe("CloudflareClient", () => { }); }); + describe("wrangler binary resolution", () => { + const mockAccountId = "1234567890abcdef1234567890abcdef"; + + // captured[i] holds the interpolated substitution values of the i-th + // `$` invocation. Templates like `${argv} secret list --config ${path}` + // yield multiple substitutions, so argv is itself a nested array. + function captureSubs(): unknown[][] { + const captured: unknown[][] = []; + vi.mocked($).mockImplementation(((...args: unknown[]) => { + const [strings, ...subs] = args; + if (!Array.isArray(strings)) { + return (( + templateStrings: TemplateStringsArray, + ...subs: unknown[] + ) => { + captured.push(subs); + return { + stdout: `random content ${mockAccountId} \nmore random content`, + stderr: "", + exitCode: 0, + }; + }) as any; + } + captured.push(subs); + return { + stdout: JSON.stringify([]), + stderr: "", + exitCode: 0, + } as any; + }) as any); + return captured; + } + + it("should resolve the wrangler binary bundled with the CLI", () => { + const binPath = getBundledWranglerBinPath(); + expect(binPath).toMatch(/[\\/]bin[\\/]wrangler\.js$/); + expect(binPath).toContain("node_modules"); + }); + + it("should spawn the bundled wrangler binary for whoami", async () => { + const captured = captureSubs(); + + await client.getAccountId(); + + expect(captured[0]).toEqual([ + ["node", getBundledWranglerBinPath()], + ]); + }); + + it("should spawn the bundled wrangler binary for secret list", async () => { + const captured = captureSubs(); + + await client.getCloudflareSecrets(); + + expect(captured[0]).toEqual([ + ["node", getBundledWranglerBinPath()], + mockConfigPath, + ]); + }); + + it("should fall back to npx wrangler when the bundled binary cannot be resolved", async () => { + nodeModuleState.throwOnCreateRequire = true; + const captured = captureSubs(); + + await client.getAccountId(); + + expect(captured[0]).toEqual([["npx", "wrangler"]]); + }); + }); + describe("getAccountId", () => { it("should extract account ID from whoami command", async () => { const mockAccountId = "1234567890abcdef1234567890abcdef"; diff --git a/packages/cli/src/lib/cloudflare.ts b/packages/cli/src/lib/cloudflare.ts index 06b673c0..267a6d2a 100644 --- a/packages/cli/src/lib/cloudflare.ts +++ b/packages/cli/src/lib/cloudflare.ts @@ -1,4 +1,6 @@ import { $, ProcessOutput } from "zx"; +import { createRequire } from "node:module"; +import { existsSync } from "node:fs"; import path from "path"; import { homedir } from "node:os"; @@ -7,6 +9,21 @@ interface SecretItem { type: string; } +export function getBundledWranglerBinPath(): string | null { + try { + const require = createRequire(import.meta.url); + const packageJsonPath = require.resolve("wrangler/package.json"); + const binPath = path.join( + path.dirname(packageJsonPath), + "bin", + "wrangler.js", + ); + return existsSync(binPath) ? binPath : null; + } catch { + return null; + } +} + interface AccountInfo { id: string; name: string; @@ -41,9 +58,16 @@ export class CloudflareClient { path.join(homedir(), ".counterscale", "wrangler.json"); } + private wranglerArgv(): string[] { + const binPath = getBundledWranglerBinPath(); + return binPath ? ["node", binPath] : ["npx", "wrangler"]; + } + async getAccountId(): Promise { try { - const result = await $({ quiet: true })`npx wrangler whoami`; + const result = await $({ + quiet: true, + })`${this.wranglerArgv()} whoami`; const match = result.stdout.match(/([0-9a-f]{32})/); return match ? match[0] : null; } catch (error) { @@ -56,7 +80,9 @@ export class CloudflareClient { async getAccounts(): Promise { try { - const result = await $({ quiet: true })`npx wrangler whoami`; + const result = await $({ + quiet: true, + })`${this.wranglerArgv()} whoami`; const accounts = this.parseAccountsFromTable(result.stdout); // If table parsing failed, fall back to single account @@ -104,7 +130,7 @@ export class CloudflareClient { private async fetchCloudflareSecrets(): Promise { try { const result = - await $`npx wrangler secret list --config ${this.configPath}`; + await $`${this.wranglerArgv()} secret list --config ${this.configPath}`; return result.stdout; } catch (error) { throw error instanceof ProcessOutput @@ -192,7 +218,7 @@ export class CloudflareClient { ): Promise { for (const [key, value] of Object.entries(secrets)) { try { - await $`echo ${value} | npx wrangler secret put ${key} --config ${this.configPath}`; + await $`echo ${value} | ${this.wranglerArgv()} secret put ${key} --config ${this.configPath}`; } catch { return false; } @@ -204,7 +230,7 @@ export class CloudflareClient { try { const p = $({ quiet: true, - })`npx wrangler deploy --config ${this.configPath} --var VERSION:${version}`; + })`${this.wranglerArgv()} deploy --config ${this.configPath} --var VERSION:${version}`; let output = ""; for await (const text of p) { From fb6195fc727c076b10c703f07b5409733f8e25d2 Mon Sep 17 00:00:00 2001 From: stordahl Date: Thu, 10 Sep 2026 12:21:04 +0000 Subject: [PATCH 2/2] fix(cli): warn on bundled wrangler resolution failure and memoize result Address PR review: the silent npx fallback made a future resolution failure (e.g. wrangler changing its exports map) degrade invisibly back to the bug this PR fixes. Also memoize the resolved binary path. --- .../cli/src/lib/__tests__/cloudflare.test.ts | 66 ++++++++++++++++++- packages/cli/src/lib/cloudflare.ts | 65 +++++++++++++----- 2 files changed, 112 insertions(+), 19 deletions(-) diff --git a/packages/cli/src/lib/__tests__/cloudflare.test.ts b/packages/cli/src/lib/__tests__/cloudflare.test.ts index 0ebea42e..d5e05806 100644 --- a/packages/cli/src/lib/__tests__/cloudflare.test.ts +++ b/packages/cli/src/lib/__tests__/cloudflare.test.ts @@ -1,10 +1,18 @@ import { describe, it, expect, vi, beforeEach, afterEach } from "vitest"; -import { CloudflareClient, getBundledWranglerBinPath } from "../cloudflare.js"; +import { + CloudflareClient, + getBundledWranglerBinPath, + resetWranglerBinPathCache, +} from "../cloudflare.js"; import { ProcessOutput } from "zx"; import path from "path"; +import { createRequire } from "node:module"; import { homedir } from "node:os"; -const nodeModuleState = vi.hoisted(() => ({ throwOnCreateRequire: false })); +const nodeModuleState = vi.hoisted(() => ({ + throwOnCreateRequire: false, + failExistsCheck: false, +})); // Mock the entire zx module vi.mock("zx", async (importOriginal) => { @@ -28,6 +36,19 @@ vi.mock("node:module", async (importOriginal) => { }; }); +vi.mock("node:fs", async (importOriginal) => { + const actual = await importOriginal(); + return { + ...actual, + existsSync: vi.fn((...args) => { + if (nodeModuleState.failExistsCheck) { + return false; + } + return actual.existsSync(...args); + }), + }; +}); + // Import and spy on the mocked function const { $ } = await import("zx"); @@ -41,6 +62,8 @@ describe("CloudflareClient", () => { beforeEach(() => { vi.clearAllMocks(); nodeModuleState.throwOnCreateRequire = false; + nodeModuleState.failExistsCheck = false; + resetWranglerBinPathCache(); client = new CloudflareClient(mockConfigPath); }); @@ -100,6 +123,20 @@ describe("CloudflareClient", () => { expect(binPath).toContain("node_modules"); }); + it("should memoize the resolved binary path across calls", () => { + getBundledWranglerBinPath(); + + const createRequireSpy = vi.mocked(createRequire); + const callsAfterFirstResolution = + createRequireSpy.mock.calls.length; + + getBundledWranglerBinPath(); + + expect(createRequireSpy.mock.calls.length).toBe( + callsAfterFirstResolution, + ); + }); + it("should spawn the bundled wrangler binary for whoami", async () => { const captured = captureSubs(); @@ -121,13 +158,36 @@ describe("CloudflareClient", () => { ]); }); - it("should fall back to npx wrangler when the bundled binary cannot be resolved", async () => { + it("should fall back to npx wrangler and warn when the bundled binary cannot be resolved", async () => { nodeModuleState.throwOnCreateRequire = true; + const warnSpy = vi + .spyOn(console, "warn") + .mockImplementation(() => {}); const captured = captureSubs(); await client.getAccountId(); expect(captured[0]).toEqual([["npx", "wrangler"]]); + expect(warnSpy).toHaveBeenCalledWith( + "Failed to resolve bundled wrangler binary, falling back to npx:", + expect.anything(), + ); + warnSpy.mockRestore(); + }); + + it("should warn when the resolved binary path does not exist", () => { + nodeModuleState.failExistsCheck = true; + const warnSpy = vi + .spyOn(console, "warn") + .mockImplementation(() => {}); + + expect(getBundledWranglerBinPath()).toBeNull(); + expect(warnSpy).toHaveBeenCalledWith( + expect.stringMatching( + /Bundled wrangler binary not found at .*bin[\\/]wrangler\.js, falling back to npx/, + ), + ); + warnSpy.mockRestore(); }); }); diff --git a/packages/cli/src/lib/cloudflare.ts b/packages/cli/src/lib/cloudflare.ts index 267a6d2a..7a521b92 100644 --- a/packages/cli/src/lib/cloudflare.ts +++ b/packages/cli/src/lib/cloudflare.ts @@ -9,7 +9,13 @@ interface SecretItem { type: string; } +let cachedWranglerBinPath: string | null | undefined; + export function getBundledWranglerBinPath(): string | null { + if (cachedWranglerBinPath !== undefined) { + return cachedWranglerBinPath; + } + try { const require = createRequire(import.meta.url); const packageJsonPath = require.resolve("wrangler/package.json"); @@ -18,10 +24,25 @@ export function getBundledWranglerBinPath(): string | null { "bin", "wrangler.js", ); - return existsSync(binPath) ? binPath : null; - } catch { - return null; + cachedWranglerBinPath = existsSync(binPath) ? binPath : null; + if (!cachedWranglerBinPath) { + console.warn( + `Bundled wrangler binary not found at ${binPath}, falling back to npx`, + ); + } + } catch (err) { + console.warn( + "Failed to resolve bundled wrangler binary, falling back to npx:", + err, + ); + cachedWranglerBinPath = null; } + + return cachedWranglerBinPath; +} + +export function resetWranglerBinPathCache(): void { + cachedWranglerBinPath = undefined; } interface AccountInfo { @@ -84,15 +105,20 @@ export class CloudflareClient { quiet: true, })`${this.wranglerArgv()} whoami`; const accounts = this.parseAccountsFromTable(result.stdout); - + // If table parsing failed, fall back to single account if (accounts.length === 0) { const accountId = await this.getAccountId(); if (accountId) { - return [{ id: accountId, name: `Account ${accountId.slice(-6)}` }]; + return [ + { + id: accountId, + name: `Account ${accountId.slice(-6)}`, + }, + ]; } } - + return accounts; } catch (error) { if (error instanceof ProcessOutput) { @@ -104,26 +130,33 @@ export class CloudflareClient { private parseAccountsFromTable(output: string): AccountInfo[] { const accounts: AccountInfo[] = []; - const lines = output.split('\n'); - + const lines = output.split("\n"); + for (const line of lines) { // Skip header and separator lines - if (!line.includes('│') || line.includes('Account Name') || line.includes('─')) { + if ( + !line.includes("│") || + line.includes("Account Name") || + line.includes("─") + ) { continue; } - - const parts = line.split('│').map(part => part.trim()).filter(Boolean); - + + const parts = line + .split("│") + .map((part) => part.trim()) + .filter(Boolean); + if (parts.length >= 2) { const [name, id] = parts; - + // Validate account ID format (32 hex characters) if (/^[0-9a-f]{32}$/.test(id)) { accounts.push({ id, name }); } } } - + return accounts; } @@ -173,7 +206,8 @@ export class CloudflareClient { const data: TokenValidationResponse = await response.json(); if (!data.success) { - const errorMessage = data.errors?.[0]?.message || "Token validation failed"; + const errorMessage = + data.errors?.[0]?.message || "Token validation failed"; return { valid: false, error: errorMessage }; } @@ -252,4 +286,3 @@ export class CloudflareClient { } } } -