Repository navigation
fix(cli): pass dev auth profiles through child environments - #453
petebacondarwin wants to merge 1 commit into
Conversation
|
LGTM |
commit: |
There was a problem hiding this comment.
💬 Approved with comments — The change is small and correct: an explicit --profile is passed as CLOUDFLARE_AUTH_PROFILE to both the delegate and framework dev-command paths, and tests cover both. The only finding is an INFO about the README, which does not describe the new behaviour.
Findings: 1 info
The decision is advisory: this review is a comment and never approves, blocks or requests changes on the pull request.
📝 Documentation: 💬 1 info
The README does not describe how cf dev --profile passes the profile to child processes.
Recommendation: 💬 Optional Nits
- [INFO] Consider documenting in the README's authentication section that
cf dev --profile <name>passes the selection to development servers throughCLOUDFLARE_AUTH_PROFILE, and that this needs a compatible@cloudflare/workers-authversion in the development server. (packages/cli/README.md:34)
View full review
The changeset and packages/cli/AGENTS.md describe the new behaviour accurately. The user-facing README section on profile selection (packages/cli/README.md:34-38) does not yet mention that cf dev --profile sets CLOUDFLARE_AUTH_PROFILE for development servers, or that a development server only uses the selected credentials if it runs a compatible @cloudflare/workers-auth.
✅ All other checks passed
📋 Code Quality: ✅ LGTM
profileEnv is set only when --profile is given explicitly. On the framework path it is merged after details.env, and on the delegate path it goes through implEnvironment(), so the flag takes precedence over an inherited CLOUDFLARE_AUTH_PROFILE and the rest of the environment is still inherited. Tests cover both spawn paths.
Reviewed commit: e287d924
|
| mean | stddev | min | max | |
|---|---|---|---|---|
| PR | 230.9 ms | 4.0 ms | 222.1 ms | 238.9 ms |
| base | 233.1 ms | 6.3 ms | 226.5 ms | 255.1 ms |
Δ (PR vs base): ↓ -2.2 ms (-1.0%)
Measured with hyperfine on ubuntu-latest (3 warmup runs, 20 timed runs). CI numbers carry ±a few ms of runner jitter; treat small deltas as noise.
There was a problem hiding this comment.
Digging into our conversation earlier and this solution, Claude is suggesting a version range update which I think makes sense to avoid a delegation to a wrangler account that isn't explicitly part of the profile. See below:
The environment variable approach looks right to me. One gap I noticed is version mismatch with older delegates.
cf dev checks delegate versions against the versionConstraint floors in known-impls.ts. These are currently wrangler >=4.136.0 and @cloudflare/vite-plugin >=2.0.0-0 <3.0.0-0. Both ranges allow releases from before cloudflare/workers-sdk#16599, and those releases ignore CLOUDFLARE_AUTH_PROFILE:
- An older
cf-vitefalls back to the directory binding or the default profile. - An older
cf-wranglerreads Wrangler's credential store rather than cf's.
Either way, cf dev --profile work runs against a different account and no error appears.
Could we raise both floors to the first releases that include cloudflare/workers-sdk#16599? The existing check in resolveProjectImpl would then fail early with the install hint, rather than starting the server with other credentials. This would also block projects on older delegates that don't use profiles. Since an older cf-wrangler doesn't read cf's credentials anyway, I think that's acceptable.
The versions aren't known until cloudflare/workers-sdk#16599 is released, so this would mean landing this PR after that release. The fixture versions in the new auth profile tests (4.136.0 and 2.0.0-beta.sha-805ec1ff3) would also need updating to pass the new floors.
cf dev --profile workselects a profile in cf, but the development server runs in a child process and loses that explicit selection. Pass it asCLOUDFLARE_AUTH_PROFILEto both Cloudflare delegates and detected framework dev commands. An explicit flag overrides the inherited value, and framework-specific environment overrides are preserved.Companion to cloudflare/workers-sdk#16599, which adds shared profile resolution in
@cloudflare/workers-authand initializes cf authentication incf-wrangler. The selected development server needs a release containing that support. This provides the environment-based approach discussed for #308 without adding a--profileargument to the delegate contract.Validation:
cf-vite,cf-wrangler, and a framework dev command.pnpm check: passed (types, build, lint, and formatting).