Repository navigation
fix(workers): standardize Worker selection on --worker - #387
Conversation
commit: |
|
This pull request's reviews are posted as pull request reviews: see the review of |
|
| mean | stddev | min | max | |
|---|---|---|---|---|
| PR | 278.0 ms | 3.8 ms | 273.8 ms | 288.9 ms |
| base | 278.2 ms | 3.2 ms | 274.1 ms | 284.6 ms |
Δ (PR vs base): — -0.3 ms (-0.1%)
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.
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
|
Review: 1 findings. Posted 1 inline suggestion. |
|
LGTM |
![clanky-the-clanker[bot]](https://avatars.githubusercontent.com/in/5306670?s=60&v=4){kind=small}
There was a problem hiding this comment.
Superseded by the review of
27717e10.
This review, of 945e58de
✅ Approved — I found no problems in the generator changes, the regenerated commands or the tests. The changeset bump flagged in the last review is now minor, as the repository's rules require for new flags.
The decision is advisory: this review is a comment and never approves, blocks or requests changes on the pull request.
✅ All checks passed
🚀 Release: ✅ LGTM
.changeset/consistent-worker-identifiers.md now uses minor, which matches the repository's rule for changes that add flags.
🔒 Security: ✅ LGTM
The Worker name is URL-encoded in API and dry-run URLs, and it is validated before authentication and the destructive confirmation.
📖 AGENTS.md: ✅ LGTM
This pull request does not change any agent instruction files.
📋 Code Quality: ✅ LGTM
The generator, the regenerated workers/** commands and the tests agree with each other. The Worker is validated before dry runs and the delete confirmation.
⚡ Performance: ✅ LGTM
The changes only affect argument parsing, plus one encodeURIComponent call per request. No hot paths are affected.
📝 Documentation: ✅ LGTM
The architecture notes and the regenerated help text describe --worker as the canonical flag, with its aliases.
Reviewed commit: 945e58de
945e58d to
27717e1
Compare
There was a problem hiding this comment.
Superseded by the review of
aae9c95b.
This review, of 27717e10
💬 Approved with comments — One WARNING: cf workers get --help and cf workers delete --help show a duplicate, unstyled Options: section. Worker selection, validation and URL encoding are correct.
Findings: 1 warning (1 is an inline comment on the diff)
The decision is advisory: this review is a comment and 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)
### [Code Quality] The hard-coded `"Options:"` group gives `cf workers get --help` and `cf workers delete --help` a second, unstyled `Options:` section, separate from the branded one.
- **File:** packages/cli/generator/plugin/emit/builder.ts:92
📋 Code Quality: ⚠️ 1 warning
The generator and regenerated commands are consistent. The help for workers get and workers delete now has a duplicate Options: group.
Recommendation: 📝 Minor Comments
- [WARNING] The hard-coded
"Options:"group givescf workers get --helpandcf workers delete --helpa second, unstyledOptions:section, separate from the branded one. (packages/cli/generator/plugin/emit/builder.ts:92)
View full review
The changes since the last review keep Worker validation ahead of dry runs and authentication, and keep the URL encoding.
Duplicate Options: help group. builder.ts:92 emits group: "Options:" whenever the Worker option is also a positional. That applies to get.ts:31 and delete.ts:34. src/index.ts passes { "Options:": orange("Options") } to updateStrings, so yargs's default group gets the branded name. The literal "Options:" then forms a separate group with a plain heading, as @edmundhung's help output shows. The help test only checks that --worker appears in the output, so it misses this.
✅ All other checks passed
🚀 Release: ✅ LGTM
.changeset/consistent-worker-identifiers.md uses minor. That matches the repository's rule for changes that add flags.
🔒 Security: ✅ LGTM
The Worker name is URL-encoded in API and dry-run URLs. It is validated before authentication and before the destructive confirmation.
📖 AGENTS.md: ✅ LGTM
This pull request does not change any agent instruction files.
⚡ Performance: ✅ LGTM
The changes only affect argument parsing and add one encodeURIComponent call per request.
📝 Documentation: ✅ LGTM
The updated architecture notes describe --worker as the canonical flag, with --script-name and --worker-id as aliases.
Reviewed commit: 27717e10
|
LGTM |
|
LGTM |
There was a problem hiding this comment.
✅ Approved — The duplicate Options: help section from the last review is fixed, with a test covering it. No other issues found.
The decision is advisory: this review is a comment and never approves, blocks or requests changes on the pull request.
✅ All checks passed
🚀 Release: ✅ LGTM
The changeset uses minor. That matches the repository's rule for changes that add flags.
🔒 Security: ✅ LGTM
The Worker name is checked before dry-run, authentication and the destructive confirmation, and URL-encoded in API and dry-run URLs. The canonical flag and its aliases are reserved against collisions with API fields.
📖 AGENTS.md: ✅ LGTM
This pull request does not change any agent instruction files.
📋 Code Quality: ✅ LGTM
builder.ts now puts worker in the group theme.brand("Options"), which matches the branded default group set in src/index.ts. The generated workers/get.ts and workers/delete.ts use it. The new help.test.ts cases check that --worker, --dry-run and --force share one Options section at colour levels 0 and 3. The Worker is still checked before dry runs, authentication and confirmation, and URL encoding is unchanged.
⚡ Performance: ✅ LGTM
The changes only affect argument parsing and code generation.
📝 Documentation: ✅ LGTM
The architecture notes describe --worker as the canonical flag, with --script-name and --worker-id as aliases.
Reviewed commit: aae9c95b
Fixes #345.
Generated
cf workerscommands now accept--workerconsistently, includingcf workers getandcf workers versions list. Keep--worker-idand--script-nameas aliases, and preserve positional input forgetanddelete.Require Worker selection before dry-run previews, authentication, and destructive confirmation. Encode the Worker in API and dry-run URLs, and reserve the canonical flag and aliases against API-field collisions. Regenerate help, completion, tools, and schema metadata to expose the canonical flag.
Cover the secret → versions → deployment sequence, compatibility aliases, missing Worker input in API and dry-run requests, URL encoding, and flag collisions. Update the Wrangler compatibility expectations and use a minor changeset for the added flags.
Validation:
pnpm checkpnpm --dir packages/cli test— 2,513 passed, 6 skippedpnpm --dir packages/wrangler-tests test— 659 passed, 3,409 skipped, 1,505 todo