Skip to content

fix(workers): standardize Worker selection on --worker - #387

Draft
petebacondarwin wants to merge 1 commit into
mainfrom
codex/fix-345-worker-identifiers
Draft

petebacondarwin wants to merge 1 commit into
mainfrom
codex/fix-345-worker-identifiers

Conversation

@petebacondarwin

Copy link
Copy Markdown
Collaborator

Fixes #345.

Generated Workers commands now accept --worker consistently, including cf workers get and cf workers versions list. Keep --worker-id and --script-name as aliases, and preserve positional input for get and delete.

Regenerate the commands and metadata so help, completion, and tools expose the canonical flag. Cover the secret → versions → deployment sequence, compatibility aliases, missing Worker input, and URL encoding in API and dry-run requests.

Validation:

  • pnpm check
  • pnpm --dir packages/cli test — 1,988 passed, 6 skipped
@pkg-pr-new

pkg-pr-new Bot commented Oct 22, 2026

Copy link
Copy Markdown
npm i https://pkg.pr.new/cf@387

commit: 622cecc

@clanky-the-clanker

Copy link
Copy Markdown

💬 Approved with comments — The generator change, regenerated output and tests are consistent, and I found no correctness or security problems. The only item is the changeset bump type: it is patch, but the repository's rules call for minor when a change adds flags.

Findings: 1 warning

The decision is advisory: this comment never approves, blocks or requests changes on the pull request.

🔧 Fix with your agent

Paste the following into your coding agent to investigate and fix these issues.

Fix the following code review issues found in pull request #387.

Before making changes, review each issue and present a brief summary table:
- For each finding, state whether you agree, disagree, or need clarification
- If you disagree (e.g., the fix would require disproportionate changes
  for minimal ROI, or the finding is factually incorrect), explain why
- If you need to clarify something before deciding, ask those questions
- Then share your plan for which issues to tackle and in what order

## WARNING (1)

### [Release] The changeset is marked `patch`, but this change adds the `--worker` flag to commands such as `cf workers get` and `cf workers versions list`. The repository's changeset rules give new flags a `minor` bump. This affects release versioning only.
- **File:** .changeset/consistent-worker-identifiers.md:2

🚀 Release: ⚠️ 1 warning

The changeset bump type may be too low under the repository's rules

Recommendation: 📝 Minor Comments

  • [WARNING] The changeset is marked patch, but this change adds the --worker flag to commands such as cf workers get and cf workers versions list. The repository's changeset rules give new flags a minor bump. This affects release versioning only. (.changeset/consistent-worker-identifiers.md:2)
View full review

Risk: Low

  • .changeset/consistent-worker-identifiers.md:2 is marked patch. This change adds a --worker option to generated commands that didn't accept it before, such as cf workers get and cf workers versions list. The repository's changeset rules give new flags a minor bump. Consider changing it to minor. This affects release versioning only, not runtime behaviour.

✅ All other checks passed

🔒 Security: ✅ LGTM

The Worker name is now URL-encoded in API paths and in dry-run URLs. This is a hardening change, and I found nothing exploitable.


📖 AGENTS.md: ✅ LGTM

This pull request does not change any agent instruction files. The existing guidance already names --worker as the canonical Worker flag for the workers resource, and this change matches it.


📋 Code Quality: ✅ LGTM

I found no problems in the generator, the regenerated commands, the metadata or the tests.

  • Generator: args.ts, builder.ts, files.ts, index.ts, sdk.ts and handler.ts agree with each other. --worker is canonical, script-name and worker-id are aliases, and get/delete keep their positional argument.
  • Generated files: the generated workers/** files match that output.
  • Scope: Worker-name handling only applies when the resource is workers. Tests confirm that Worker identifiers outside that root, and worker_id body fields, are left unchanged.
  • Missing Worker: the Worker name is resolved before the delete confirmation, so a missing Worker fails early. Tests cover this.

⚡ Performance: ✅ LGTM

The changes only affect how generated commands parse their arguments, plus one encodeURIComponent call per request. No hot paths are affected.


📝 Documentation: ✅ LGTM

Help text and command metadata are regenerated and show --worker as the canonical flag.


Reviewed commit: 622cecc8

@github-actions

Copy link
Copy Markdown
Contributor

cf --help startup

mean stddev min max
PR 171.0 ms 3.7 ms 161.4 ms 178.9 ms
base 176.1 ms 3.5 ms 170.2 ms 180.9 ms

Δ (PR vs base): ↓ -5.1 ms (-2.9%)

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.

Comment on lines +164 to +165
scriptName:
'${argv["worker"] == null ? \'<worker>\' : encodeURIComponent(String(argv["worker"]))}',

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Worker-required guard only runs after the early argv.dryRun return. This changes cf workers get --dry-run and cf workers versions list --dry-run from a missing-identifier error into a successful preview with <worker>, even though Worker selection is required. Validate the supplied value while producing the preview as well.

Suggested change
scriptName:
'${argv["worker"] == null ? \'<worker>\' : encodeURIComponent(String(argv["worker"]))}',
scriptName:
'${encodeURIComponent(getWorkerName({ scriptName: argv["worker"] }))}',
@ask-bonk

ask-bonk Bot commented Oct 22, 2026

Copy link
Copy Markdown
Contributor

Review: 1 findings.

Posted 1 inline suggestion.

github run

@petebacondarwin
petebacondarwin marked this pull request as draft October 23, 2026 06:26
@petebacondarwin
petebacondarwin requested review from a team and removed request for a team, dimitropoulos and srikrsna October 23, 2026 12:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

1 participant