feat(#5632): support --pem-dir for CF mint deploy (PEM bootstrap parity) - #5950
feat(#5632): support --pem-dir for CF mint deploy (PEM bootstrap parity)#5950fullsend-ai-coder[bot] wants to merge 6 commits into
Conversation
CF deploy (--platform=cloudflare) can now bootstrap role PEMs and ROLE_APP_IDS using the same --pem-dir flag that GCP already supports. When --pem-dir is provided: 1. PEM files are validated and each key is verified against the GitHub App API (same loadAppSetPEMs path GCP uses). 2. ROLE_APP_IDS is set as a Worker env var via --var during deploy. 3. Each role PEM is stored as a Worker secret (e.g. CODER_APP_PEM) via wrangler secret put after the Worker is deployed. This works for both durable and preview deploys. Preview deploys pass ROLE_APP_IDS via --var on wrangler versions upload (version- scoped), while PEM secrets are stored on the durable Worker script (shared with production, as wrangler secret put targets the script). The --pem-dir flag description drops its "(GCP only)" suffix and is removed from the CF irrelevant-flags warning list. The deploy command long help now documents --pem-dir for both platforms. Note: golangci-lint could not run (not installed in sandbox). go vet and gofmt passed. Pre-commit could not run (network error in sandbox); post-script runs it authoritatively. Closes #5632
|
🤖 Finished Review · ✅ Success · Started 7:10 PM UTC · Completed 7:25 PM UTC |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
ReviewFindingsHigh
Medium
Low
Next steps:
Previous runReviewFindingsHigh
Medium
Low
Labels: PR modifies mint CLI deploy code and CF dispatch provisioner Next steps:
Previous run (2)ReviewFindingsHigh
Medium
Low
Next steps:
Previous run (3)ReviewFindingsHigh
Medium
Low
Next steps:
Previous run (4)ReviewFindingsHigh
Medium
Low
Next steps:
Previous run (5)ReviewFindingsHigh
Medium
Low
Labels: PR modifies mint CLI deploy code (internal/cli/mint.go) Next steps:
|
![fullsend-ai-review[bot]](https://avatars.githubusercontent.com/in/3478501?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
fullsend-ai-review
Bot
added
the
component/mint
|
/fs-fix |
|
🤖 Finished Fix · ✅ Success · Started 8:53 PM UTC · Completed 9:02 PM UTC |
- Move --pem-dir flag from GCP-specific to Common flags block since it is now used by both GCP and Cloudflare platforms - Add test for PEM secret storage failure path (exercises secretPutErr) - Improve error message on partial PEM secret storage to report progress (N/M already stored) and note that re-run is safe - Reorder runMintDeployCloudflare parameters to group string params before bool, matching runMintDeployGCP pattern Addresses review feedback on #5950
🔧 Fix agent — iteration 1 (human-triggered)Addressed all 4 review findings: moved --pem-dir to common flags, added PEM secret failure test, improved partial-failure error messaging, and reordered function parameters for cross-platform consistency. Fixed (4):
Tests: passedNext steps:
|
|
🤖 Finished Review · ✅ Success · Started 9:03 PM UTC · Completed 9:21 PM UTC |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
/fs-fix putting the secrets on the durable deploy for a preview deployment will not work. The LiveWranglerRunner PutSecret method should instead accept a previewAlias parameter like other commands and pass the relevant arguments to wrangler as needed. The provisioner StoreAgentPEM method should then call it passing the parameter as needed. Also deal with other review agent comments |
|
🤖 Finished Fix · ✅ Success · Started 5:32 AM UTC · Completed 5:56 AM UTC |
PutSecret on the WranglerRunner interface now accepts a previewAlias parameter so preview deploys scope secrets to the preview version via --preview-alias instead of targeting the durable Worker script. StoreAgentPEM forwards the provisioner's PreviewAlias to PutSecret. Also reorder runMintDeployCloudflare parameters to group strings before bools (addresses review feedback on parameter ordering). Addresses review feedback on #5950
🔧 Fix agent — iteration 2 (human-triggered)Addressed 2 findings (human instruction + parameter ordering). Disagreed with 3 review agent findings that were factually incorrect: pemDir flag was already in Common block, PEM failure test already exists, and error messages already include stored-count information. Key change: WranglerRunner.PutSecret now accepts previewAlias so preview deploys scope secrets correctly instead of targeting the durable Worker. Fixed (2):
Disagreed (3):
Tests: passed Decision points
Next steps:
|
|
🤖 Finished Review · ✅ Success · Started 5:58 AM UTC · Completed 6:16 AM UTC |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
/fs-fix --preview-alias cannot be passed to "wrangler secret put", to attach secrets to a particular preview version, the secrets must all be passed via a file specified to the --secrtes-file parameter of the "wrangler version upload" command, which can also accept --preview-alias to upload a preview version. Consider all changes needed for a deployment process that would be based on a single invocation of "wrangler version upload". The wrngler version comamnd required wrangler v3.73.0 or newer so upgrade what we use if we currently use an older version. |
|
🤖 Finished Fix · ✅ Success · Started 6:18 AM UTC · Completed 6:37 AM UTC |
…ngler secret put wrangler secret put does not support --preview-alias, so preview deploys cannot scope PEM secrets to a preview version via separate PutSecret calls. Instead, pass secrets through Deploy via --secrets-file on wrangler versions upload. Durable deploys continue using PutSecret (which works without --preview-alias). Changes: - Add secrets parameter to WranglerRunner.Deploy interface - Add Config.Secrets field for passing secrets through Provision - Remove previewAlias from WranglerRunner.PutSecret (not supported by CLI) - Write secrets to temp JSON file and pass --secrets-file in deployPreview - Add PEMSecretsFromRoles helper for role→secret-name conversion - In mint.go: preview deploys pass PEMs via Config.Secrets; durable deploys keep post-deploy StoreAgentPEM loop Wrangler v3.73.0+ is required for --secrets-file support; package.json already specifies wrangler ^4.0.0 so no version upgrade was needed. Addresses review feedback on #5950
🔧 Fix agent — iteration 3 (human-triggered)Addressed all human findings. Removed --preview-alias from PutSecret (not supported by wrangler secret put). Preview PEM secrets now go through wrangler versions upload via --secrets-file. Durable deploys unchanged. Wrangler v4.0.0 already in use (above v3.73.0 requirement). All tests pass with race detection. Fixed (4):
Disagreed (1):
Tests: passed
Decision points
Next steps:
|
|
🤖 Finished Review · ✅ Success · Started 6:38 AM UTC · Completed 6:55 AM UTC |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
/fs-fix We need to adjust the design because ot the linearity of wrangler worker deployment versions. For preview deploys we will only support the Lets try to unify the code path for preview and non-preview deployments as much as possible Non-preview deployments will also support configuration commands such as "add-role" and "enroll", but those would setup new deployment versions based on the last deployed version, so not applicable to a multiple previews situation where the latest deployed version might refer to the wrong code version (but the other commands are out of scope for this PR) |
|
🤖 Finished Fix · ✅ Success · Started 7:38 AM UTC · Completed 7:59 AM UTC |
Add --allowed-orgs, --per-repo-wif-repos, --workflow-host-repos flags to `mint deploy --platform=cloudflare` so preview deploys can receive all mint configuration in a single command. Make --public work for CF as an alias for --per-repo-wif-repos="*". These flags set Worker env vars via the unified cfEnvVars → Config.EnvVars → wrangler --var path, which works identically for both preview and durable deploys. Addresses review feedback on #5950
🔧 Fix agent — iteration 4 (human-triggered)Added --allowed-orgs, --per-repo-wif-repos, --workflow-host-repos flags and made --public work for CF deploys. All config env vars use the unified cfEnvVars path for both preview and durable deploys. 10 new tests added. Fixed (5):
Tests: passed
Decision points
Next steps:
|
|
🤖 Finished Review · ✅ Success · Started 8:01 AM UTC · Completed 8:17 AM UTC |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
fullsend-ai-review
Bot
added
the
component/dispatch
|
/fs-fix Add Why
What to implement
Out of scopeDo not change enrollment/workflow-host CF support in this fix unless required to compile/pass tests for the flag wiring. |
|
🤖 Fix · |
|
/fs-fix Cancel/supersede the previous 1.
|
|
🤖 Finished Fix · ✅ Success · Started 10:14 AM UTC · Completed 10:40 AM UTC |
1. Add --app-set flag to `mint deploy` for both GCP and Cloudflare
platforms, allowing PEM bootstrap against a non-default app set
(e.g. fullsand-ai). Defaults to fullsend-ai when omitted.
Validates via appsetup.ValidateAppSet and threads the chosen
app set through loadAppSetPEMs, dry-run messaging, and deploy
summary for both platforms.
2. Accept Wrangler OAuth login session as an alternative to
CLOUDFLARE_API_TOKEN for CF deploy. ResolveCloudflareAuth()
replaces the hard ValidateCloudflareEnv() requirement:
- Prefers CLOUDFLARE_API_TOKEN + CLOUDFLARE_ACCOUNT_ID when set
- Falls back to wrangler login session when token is absent
- Auto-discovers account ID from wrangler whoami output when
CLOUDFLARE_ACCOUNT_ID is unset and exactly one account exists
Addresses review feedback on #5950
🔧 Fix agent — iteration 5 (human-triggered)Implemented both features from the /fs-fix instruction: (1) --app-set flag on mint deploy for both GCP and CF platforms, threading custom app set through PEM bootstrap, dry-run messages, and deploy summaries; (2) Wrangler OAuth session support as an alternative to CLOUDFLARE_API_TOKEN, with automatic account ID discovery from wrangler whoami. All existing tests pass; 11 new tests added covering both features. Fixed (5):
Tests: passedNext steps:
|
|
🤖 Finished Review · ✅ Success · Started 10:41 AM UTC · Completed 10:57 AM UTC |
| // caller (Provisioner or CLI) after deploy completes — wrangler deploy | ||
| // does not support --secrets-file. The secrets parameter is accepted | ||
| // for interface consistency but not used in the deploy command. | ||
| func (r *LiveWranglerRunner) deployDurable(ctx context.Context, sourceDir, workerName string, envVars map[string]string, _ map[string][]byte) (string, error) { |
There was a problem hiding this comment.
[low] edge-case
deployDurable accepts the secrets parameter but ignores it (named _). The doc comment explains the design (wrangler deploy does not support --secrets-file), and the CLI correctly gates Config.Secrets to preview deploys only. However, the interface allows constructing a Config with DeployMode=DeployDurable and non-nil Secrets, which would silently drop them.
| if err != nil { | ||
| return "", nil, fmt.Errorf("marshaling secrets: %w", err) | ||
| } | ||
| f, err := os.CreateTemp("", "wrangler-secrets-*.json") |
There was a problem hiding this comment.
[low] secrets-handling
writeSecretsFile creates a temporary file for PEM keys via os.CreateTemp, which uses 0600 permissions per Go's standard library. The doc comment claims restrictive permissions but relies on implicit Go behavior rather than an explicit os.Chmod call.
|
|
||
| accountID := os.Getenv("CLOUDFLARE_ACCOUNT_ID") | ||
| // Handle --public as an alias for --per-repo-wif-repos="*". | ||
| if public { |
There was a problem hiding this comment.
[low] authorization
--public silently overrides an explicit --per-repo-wif-repos value to *, widening authorization. A user who passes both flags may not realize the explicit restriction is discarded. The override behavior is documented in the flag help text and tested intentionally.
| cmd.Flags().BoolVar(&dryRun, "dry-run", false, "preview changes without making them") | ||
| cmd.Flags().StringVar(&pemDir, "pem-dir", "", "optional: directory containing {role}.pem files for PEM bootstrap") | ||
| cmd.Flags().StringVar(&appSet, "app-set", "", "app set name for PEM bootstrap (default: fullsend-ai)") | ||
| cmd.Flags().BoolVar(&public, "public", false, "deploy public mint (GCP: ALLOWED_ORGS=*; Cloudflare: PER_REPO_WIF_REPOS=*)") |
There was a problem hiding this comment.
[low] scope-creep
The PR adds --public flag support for Cloudflare, but issue #5632's suggested serial order says 'CF --public can follow this issue.' The implementation is minimal (one line: perRepoWIFRepos = "*") and falls naturally from --per-repo-wif-repos support.
| cmd.Flags().BoolVar(&dryRun, "dry-run", false, "preview changes without making them") | ||
| cmd.Flags().StringVar(&pemDir, "pem-dir", "", "optional: directory containing {role}.pem files for PEM bootstrap") | ||
| cmd.Flags().StringVar(&appSet, "app-set", "", "app set name for PEM bootstrap (default: fullsend-ai)") | ||
| cmd.Flags().BoolVar(&public, "public", false, "deploy public mint (GCP: ALLOWED_ORGS=*; Cloudflare: PER_REPO_WIF_REPOS=*)") |
There was a problem hiding this comment.
[low] architectural-alignment
The --public flag has divergent semantics across platforms: GCP sets ALLOWED_ORGS=* while CF sets PER_REPO_WIF_REPOS=*. The divergence is explicitly documented in the flag help text and reflects the different authorization stacks (GCP uses WIF pools; CF uses Worker env vars).
1 participant
Summary
--pem-dirfor bootstrapping role PEMs andROLE_APP_IDSon Cloudflare Workers, matching the existing GCP capabilityCODER_APP_PEM);ROLE_APP_IDSis set as a Worker env var--varforROLE_APP_IDSwhile PEM secrets go on the shared durable Worker scriptChanges
internal/cli/mint.go: AddedpemDirparameter torunMintDeployCloudflare(), PEM loading/verification vialoadAppSetPEMs(), ROLE_APP_IDS env var injection, PEM secret storage viaStoreAgentPEM(), dry-run output, and updated help textinternal/cli/mint_test.go: EnhancedfakeCFWranglerRunnerto capture envVars and secret calls; added 5 new tests covering dry-run with PEMs, full deploy with PEMs, preview deploy with PEMs, bad PEM dir validation, and no-warning verificationTesting
go test -race)go vetpasses on changed packagesgofmtreports no formatting issuesCloses #5632
Post-script verification
agent/5632-cf-deploy-pem-bootstrap)8549ba72c380c8d969a6620c0a94f2abd30898d5..HEAD)