Conversation
🦋 Changeset detectedLatest commit: fe70a72 The changes in this PR will be included in the next version bump. Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Codeowners approval required for this PR:
Show detailed file reviewers
|
There was a problem hiding this comment.
🔴 String coercion of undefined name produces literal "undefined" in KV namespace name
The type of props.name was changed from string to string | undefined at packages/wrangler/src/dev/remote.ts:97, but the expression props.name + (props.useServiceEnvironments && props.env ? \-${props.env}` : "")at line 131 still uses string concatenation. In JavaScript,undefined + ""evaluates to the literal string"undefined". When a worker has no configured name (e.g. running wrangler devwithout a name in config), this will pass"undefined"(or"undefined-envName") as the scriptNametosyncWorkersSite`, potentially creating/syncing a KV namespace with that bogus name.
Previously this was not possible because the old code in RemoteRuntimeController computed a scriptId fallback (packages/wrangler/src/api/startDevWorker/RemoteRuntimeController.ts old lines 131-135) that derived a name from the session when props.name was falsy. That fallback was removed in this PR.
(Refers to lines 131-132)
Was this helpful? React with 👍 or 👎 to provide feedback.
| modules: props.modules, | ||
| accountId: props.accountId, | ||
| name: scriptId, | ||
| name: props.name, |
There was a problem hiding this comment.
🔴 Removed scriptId fallback causes undefined to be interpolated into API URL paths
The old code in RemoteRuntimeController.#previewToken had a fallback: const scriptId = props.name || (workerContext.zone ? this.#session.id : this.#session.host.split(".")[0]) which ensured a valid script identifier was always used even when config.name was undefined. This PR removes that fallback and passes props.name (now string | undefined) directly through createRemoteWorkerInit to createPreviewToken (packages/wrangler/src/dev/create-worker-preview.ts:224-225), where worker.name is interpolated into API URL paths like /accounts/${accountId}/workers/scripts/${worker.name}/edge-preview. When name is undefined, this produces the literal URL segment "undefined", causing the API call to target a non-existent script named "undefined" and fail.
Prompt for agents
In packages/wrangler/src/api/startDevWorker/RemoteRuntimeController.ts, the #previewToken method (around line 136-151) now passes props.name directly to createRemoteWorkerInit. Previously (before this PR), there was a scriptId fallback computed as: const scriptId = props.name || (workerContext.zone ? this.#session.id : this.#session.host.split('.')[0]). This ensured a valid name was always used.
Since the PR removed session.id (from CfPreviewSession), a new fallback strategy is needed. Consider using the session host as a fallback, e.g.:
const scriptName = props.name ?? this.#session.host.split('.')[0];
and pass scriptName instead of props.name to createRemoteWorkerInit at line 141.
Similarly, in packages/wrangler/src/dev/remote.ts line 131, the syncWorkersSite call uses props.name in string concatenation. If props.name can be undefined, this must be guarded (e.g. skip the call or use a fallback name).
Was this helpful? React with 👍 or 👎 to provide feedback.
create-cloudflare
@cloudflare/kv-asset-handler
miniflare
@cloudflare/pages-shared
@cloudflare/unenv-preset
@cloudflare/vite-plugin
@cloudflare/vitest-pool-workers
@cloudflare/workers-editor-shared
wrangler
commit: |
…workers - Fix test mocks to match new CfPreviewSession/CfPreviewToken shapes - playground-preview-worker: make exchange_url optional, use initial token as fallback, remove inspector URL construction and prewarm - playground frontend: remove inspector from response codec - edge-preview-authenticated-proxy: remove /exchange CORS proxy endpoint (dead since DEVX-979), remove prewarm from .update-preview-token flow
|
✅ All changesets look good |
There was a problem hiding this comment.
Devin Review found 1 potential issue.
🐛 1 issue in files not directly in the diff
🐛 Incomplete tailLogs removal enables inspector proxy in remote mode without an inspector URL (packages/wrangler/src/api/startDevWorker/ProxyController.ts:414-418)
The PR removed the --experimental-tail-logs CLI flag (which had default: true), and stopped setting input.experimental.tailLogs in start-dev.ts. However, the check at ProxyController.ts:417 still gates inspector behavior on !this.latestConfig?.experimental?.tailLogs. Since tailLogs is now always false (because ConfigController.ts:375 computes !!input.experimental?.tailLogs from the now-absent input), the inspectorEnabled getter returns true for remote mode — the opposite of the previous default behavior.
Before this PR: tailLogs defaulted to true → !true = false → inspector disabled in remote mode.
After this PR: tailLogs is always false → !false = true → inspector enabled in remote mode.
But the PR also removed userWorkerInspectorUrl from the proxyData emitted by RemoteRuntimeController (RemoteRuntimeController.ts:307). So the InspectorProxyWorker is now started (ProxyController.ts:139) and receives reloadComplete messages (ProxyController.ts:457-461) with no inspector URL to connect to, which is a regression from the previous default behavior.
View 6 additional findings in Devin Review.
| const subdomain = await getWorkersDevSubdomain( | ||
| complianceConfig, | ||
| account.accountId, | ||
| undefined | ||
| ); |
There was a problem hiding this comment.
🔴 getWorkersDevSubdomain called without explicit API token, inconsistent with surrounding auth
In the new createPreviewSession, when ctx.host is not set (workers.dev preview), getWorkersDevSubdomain is called at line 182 to fetch the account's subdomain. However, this function calls fetchResult internally without an explicit apiToken parameter, relying on global auth context (env vars or ~/.wrangler/config). This is inconsistent with the preceding fetchResult call at create-worker-preview.ts:169 which explicitly passes apiToken from the account parameter.
In the old code, the host was derived from the exchange response, so no additional API call was needed. The new code introduces this dependency on global auth which can fail for programmatic API users (e.g., startRemoteProxySession) who provide auth explicitly via config.dev.auth without having global auth configured.
Prompt for agents
In packages/wrangler/src/dev/create-worker-preview.ts, the getWorkersDevSubdomain function at line 182 is called without the explicit apiToken that is available in the account parameter (line 161). Either:
1. Modify getWorkersDevSubdomain (defined in packages/wrangler/src/routes.ts at line 15) to accept an optional apiToken parameter and pass it through to its internal fetchResult call, then pass account.apiToken from createPreviewSession.
2. Or, create a local variant that accepts and passes the apiToken, to keep getWorkersDevSubdomain's existing signature for other callers.
The key fix is ensuring the fetchResult call inside getWorkersDevSubdomain uses the same explicit apiToken as the fetchResult call at line 166-169 of create-worker-preview.ts.
Was this helpful? React with 👍 or 👎 to provide feedback.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| async function tryExpandToken(exchangeUrl: string): Promise<string> { | ||
| const response = await fetch(exchangeUrl); | ||
| const json = (await response.json()) as { token: string }; | ||
| return json.token; | ||
| } |
There was a problem hiding this comment.
🟡 tryExpandToken silently returns undefined on malformed exchange response, overwriting valid initial token
In packages/playground-preview-worker/src/realish.ts, the new tryExpandToken function doesn't validate the exchange response shape. If the exchange endpoint returns valid JSON that lacks a token property, json.token is undefined, which is silently returned without throwing. The caller at line 92 then overwrites the valid uploadConfigToken (from previewSession.token) with undefined. The try/catch only catches thrown errors, not undefined returns, so the fallback doesn't engage.
The old code used UploadToken.parse(json) (Zod validation at packages/playground-preview-worker/src/realish.ts:31-36 before this PR) which would throw a ZodError if token was missing, correctly triggering the error path. The new code removes this validation.
Example of the unsafe cast
async function tryExpandToken(exchangeUrl: string): Promise<string> {
const response = await fetch(exchangeUrl);
const json = (await response.json()) as { token: string }; // unsafe cast
return json.token; // undefined if token missing, but typed as string
}| async function tryExpandToken(exchangeUrl: string): Promise<string> { | |
| const response = await fetch(exchangeUrl); | |
| const json = (await response.json()) as { token: string }; | |
| return json.token; | |
| } | |
| async function tryExpandToken(exchangeUrl: string): Promise<string> { | |
| const response = await fetch(exchangeUrl); | |
| if (!response.ok) { | |
| throw new Error(`Exchange failed with status ${response.status}`); | |
| } | |
| const json = (await response.json()) as { token?: string }; | |
| if (typeof json.token !== "string") { | |
| throw new Error("Exchange response missing token"); | |
| } | |
| return json.token; | |
| } |
Was this helpful? React with 👍 or 👎 to provide feedback.
| const { token } = parseJSON(bodyText) as { | ||
| token: string; | ||
| }; | ||
| return token; |
There was a problem hiding this comment.
🟡 Wrangler tryExpandToken can silently return undefined, overwriting the valid initial preview session token
In packages/wrangler/src/dev/create-worker-preview.ts, tryExpandToken uses parseJSON(bodyText) as { token: string } which is an unsafe type assertion. If the exchange response is valid JSON but doesn't contain a token field, token is undefined and is returned without throwing. The caller at lines 174-175 then sets previewSessionToken = undefined, overwriting the valid initial token from the edge-preview API. Since the function returned without throwing, the catch block (line 180) doesn't engage, and createPreviewSession proceeds with value: undefined in the returned session object.
| const { token } = parseJSON(bodyText) as { | |
| token: string; | |
| }; | |
| return token; | |
| const { token } = parseJSON(bodyText) as { | |
| token: string; | |
| }; | |
| if (typeof token !== "string") { | |
| throw new Error("Exchange response missing token"); | |
| } | |
| return token; |
Was this helpful? React with 👍 or 👎 to provide feedback.
Depending on the zone settings, a fetch to
exchange_urlcan sometimes fail. This PR makes that fetch optional, falling back to the raw token provided by the control plane. Additionally, it removes the experimental tail logs flag, which is now always on.A picture of a cute animal (not mandatory, but encouraged)