Repository navigation
chore: update OpenAPI to 5434cd8fdfdb - #477
ant-release-bot wants to merge 1 commit into
Conversation
commit: |
![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
bdca3e44.
This review, of e5f85a7a
✅ Approved — Routine pin bump with a matching changeset. No findings.
The decision is advisory: this review is a comment and never approves, blocks or requests changes on the pull request.
✅ All checks passed
📋 Code Quality: ✅ LGTM
Only the pinned OpenAPI revision and a changeset changed. The changeset follows the same format as the previous bump.
Reviewed commit: e5f85a7a
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
|
🤖 I've done a thorough review of your PR. Pins the Forge OpenAPI release; generated command artifacts remain unchanged.
|
|
| mean | stddev | min | max | |
|---|---|---|---|---|
| PR | 178.1 ms | 5.6 ms | 169.7 ms | 193.6 ms |
| base | 171.5 ms | 3.7 ms | 165.9 ms | 179.6 ms |
Δ (PR vs base): ↑ +6.6 ms (+3.8%)
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.
ant-release-bot
changed the title
e5f85a7 to
bdca3e4
Compare
|
LGTM! |
There was a problem hiding this comment.
Superseded by the review of
37f03ffb.
This review, of bdca3e44
✅ Approved — Routine OpenAPI pin bump. The regenerated Radar tld-type option is consistent across files, and the changeset is present. No findings.
The decision is advisory: this review is a comment and never approves, blocks or requests changes on the pull request.
✅ All checks passed
📋 Code Quality: ✅ LGTM
The pinned OpenAPI revision in packages/cli/generate.ts moves to c312f753550b. This regenerates the Radar tld-type option: it gains a SPECIAL_USE choice and a longer description. radar/tlds/list.ts and _meta/commands.json carry the same new value and the same description. The minor changeset in .changeset/update-openapi-c312f753550b.md fits a new option value, since cf is pre-1.0.
📝 Documentation: ✅ LGTM
No documentation concerns. The changeset matches the format of earlier OpenAPI bumps.
Reviewed commit: bdca3e44
ant-release-bot
changed the title
bdca3e4 to
37f03ff
Compare
|
LGTM! |
There was a problem hiding this comment.
Superseded by the review of
eb1d9a3b.
This review, of 37f03ffb
✅ Approved — Routine OpenAPI pin bump. The generated output and the changeset are consistent, and there are no findings.
The decision is advisory: this review is a comment and never approves, blocks or requests changes on the pull request.
✅ All checks passed
📋 Code Quality: ✅ LGTM
packages/cli/generate.ts now pins ca625b491eef52eefded1a5f123d8024307c41b4. In the regenerated tld-type option, the SPECIAL_USE choice and the updated description are the same in radar/tlds/list.ts and _meta/commands.json.
📝 Documentation: ✅ LGTM
The changeset .changeset/update-openapi-ca625b491eef.md follows the format of earlier OpenAPI bumps. Its minor level fits a new option value, since cf is pre-1.0.
Reviewed commit: 37f03ffb
37f03ff to
eb1d9a3
Compare
There was a problem hiding this comment.
Superseded by the review of
bf7aa4ac.
This review, of eb1d9a3b
✅ Approved — Routine OpenAPI pin bump. The generated output and the changeset are consistent, and there are no findings.
The decision is advisory: this review is a comment and never approves, blocks or requests changes on the pull request.
✅ All checks passed
📋 Code Quality: ✅ LGTM
packages/cli/generate.ts now pins ca625b491eef52eefded1a5f123d8024307c41b4. In the regenerated tld-type option, the SPECIAL_USE choice and the updated description are the same in radar/tlds/list.ts and _meta/commands.json.
📝 Documentation: ✅ LGTM
The changeset .changeset/update-openapi-ca625b491eef.md follows the format of earlier OpenAPI bumps. Its minor level fits a new option value, since cf is pre-1.0.
Reviewed commit: eb1d9a3b — same changes as 37f03ffb, so this is that commit's review (no new review was run; line numbers are as of 37f03ffb)
ant-release-bot
changed the title
eb1d9a3 to
bf7aa4a
Compare
|
LGTM! |
There was a problem hiding this comment.
Superseded by the review of
b5e10586.
This review, of bf7aa4ac
✅ Approved — This is a routine OpenAPI pin bump. The pinned SHA, the regenerated tld-type option and the changeset all agree, and there are no findings.
The decision is advisory: this review is a comment and never approves, blocks or requests changes on the pull request.
✅ All checks passed
📋 Code Quality: ✅ LGTM
packages/cli/generate.ts now pins da85f0061c0b7cb297b7c98ecafa9007cd671c32, which is the revision the description names. In the regenerated tld-type option, radar/tlds/list.ts and _meta/commands.json agree: both add the new SPECIAL_USE choice and both carry the same updated description of the deprecated values.
📝 Documentation: ✅ LGTM
The changeset .changeset/update-openapi-da85f0061c0b.md follows the format of earlier OpenAPI bumps. Its minor level is right for a new option value, because cf is still pre-1.0.
Reviewed commit: bf7aa4ac
ant-release-bot
changed the title
bf7aa4a to
b5e1058
Compare
|
Review: 1 finding. Posted 1 inline finding. |
There was a problem hiding this comment.
Superseded by the review of
a9349ef9.
This review, of b5e10586
🛑 Significant concerns — 1 BLOCKING: the regenerated resource-sharing recipients update and excluded-recipients update commands split a full-replace PUT body into batches. Each batch replaces the one before it, so earlier recipients are disassociated, or earlier exclusions are dropped and those accounts receive the share.
Findings: 1 blocking · 1 info (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 #477.
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
## BLOCKING (1)
### [Code Quality] `resource-sharing recipients update` (more than 100 entries) and `excluded-recipients update` (more than 1000 entries) split a full-replace PUT body into separate PUTs. Only the last batch survives: earlier recipients are disassociated, or earlier exclusions are dropped and those accounts receive the share. The success label also says "Deleted" for what is an update.
- **File:** packages/cli/src/commands/_generated/resource-sharing/recipients/update.ts:93
## INFO (1)
### [Code Quality] `cf organization share list` is removed in favour of `cf organizations shares list`, and its `--kind`/`--target-type`/`--resource-types` filters are dropped. The changeset body does not mention this, so users migrating scripts have no pointer to the new path.
- **File:** .changeset/update-openapi-a79c2e7d445a.md:5
📋 Code Quality: 🛑 1 blocking · 1 info
Batching of full-replace PUTs in two generated resource-sharing commands loses data. The removal of organization share list is missing from the changeset.
Recommendation: 🔴 Serious Concerns
- [BLOCKING]
resource-sharing recipients update(more than 100 entries) andexcluded-recipients update(more than 1000 entries) split a full-replace PUT body into separate PUTs. Only the last batch survives: earlier recipients are disassociated, or earlier exclusions are dropped and those accounts receive the share. The success label also says "Deleted" for what is an update. (packages/cli/src/commands/_generated/resource-sharing/recipients/update.ts:93) - [INFO]
cf organization share listis removed in favour ofcf organizations shares list, and its--kind/--target-type/--resource-typesfilters are dropped. The changeset body does not mention this, so users migrating scripts have no pointer to the new path. (.changeset/update-openapi-a79c2e7d445a.md:5)
View full review
Most of this pull request is a routine OpenAPI pin bump. The changeset, the pinned SHA in generate.ts and the regenerated output agree with each other. One regenerated pattern is unsafe, though.
Full-replace PUTs are split into independent batches
The affected files are resource-sharing/recipients/update.ts:93-111 and resource-sharing/excluded-recipients/update.ts:93-111.
- Both commands previously sent one
client.send(operation, …). Now, when--bodyis an array, they split it into chunks:- more than 100 entries for recipients
- more than 1000 entries for excluded-recipients
- Each chunk goes out as its own
PUT …/recipientsorPUT …/excluded-recipients. - The command's own usage text says these endpoints replace the full list and reconcile against it. So each chunk overwrites the previous one, and only the last chunk survives:
- For recipients, every account in earlier chunks is disassociated from the share.
- For excluded-recipients, every account in earlier chunks stops being excluded. Those accounts then gain access to the shared resources, which the confirmation prompt itself warns about.
- The progress and success text also changed from
Updating/UpdatedtoDeleting/Deleted. Together with the batching, this suggests the generator applied its batched-delete template to these PUT operations.
These files are generated, so the fix belongs in the generator plugin, Forge, or the OpenAPI annotation for these two operations. Then regenerate. Full-replace PUTs should send one complete payload, or reject lists that are too large. The new confirmDelete / --force guard is reasonable and can stay.
Command removal not called out in the changeset
cf organization share listis no longer registered (organization/index.ts). It is replaced bycf organizations shares list, and its--kind,--target-typeand--resource-typesfilters were dropped.resource-sharing createno longer requires--recipients/--resources.- The changeset only says "Update the generated command surface". A one-line migration note would help users whose scripts call the old path.
minoris the right level for pre-1.0.
✅ All other checks passed
🚀 Release: ✅ LGTM
Risk: High until the batching of full-replace PUTs in resource-sharing recipients update / excluded-recipients update is fixed upstream and regenerated. See the BLOCKING item in the code quality section.
The changeset does not mention that cf organization share list was removed and that cf organizations shares list replaces it. That note is also listed in the code quality section.
🔒 Security: ✅ LGTM
Most of the changes are regenerated CLI flags, descriptions and operation definitions with no security impact. The batching of excluded-recipients update can widen who receives a share. That issue is reported once, as a BLOCKING item, in the code quality section.
📖 AGENTS.md: ✅ LGTM
No agent instruction files are affected. The generated files come from regeneration, not hand edits.
⚡ Performance: ✅ LGTM
No performance concerns in the regenerated command surface.
📝 Documentation: ✅ LGTM
The changeset follows the format of earlier OpenAPI bumps and uses the minor level, as the repository's changeset rules require for pre-1.0 behaviour changes.
Reviewed commit: b5e10586
ant-release-bot
changed the title
b5e1058 to
a9349ef
Compare
|
Review: 1 finding. Pins Forge OpenAPI and regenerates the CLI command surface.
|
There was a problem hiding this comment.
Superseded by the review of
b025d2a6.
This review, of 2f0f10dd
💬 Approved with comments — One WARNING: the shell completion metadata still lists camel-case flag names that the runtime rejects, and the fix depends on #496 landing and a regeneration. The other notes are INFO.
Findings: 1 warning · 2 info (2 are inline comments 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 #477.
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] commands.json records camel-case option names (`account-pubname-startsWith`, `name-startsWith`, `user-email-startsWith`, …) that the runtime does not register, so shell completion suggests flags yargs rejects.
- **File:** packages/cli/src/commands/_generated/_meta/commands.json:123799
## INFO (2)
### [Code Quality] The `Deleting`/`Deleted` progress labels on a PUT that replaces the recipient list are misleading.
- **File:** packages/cli/src/commands/_generated/resource-sharing/recipients/update.ts:96
### [Documentation] The changeset has no migration note for the renamed and removed commands.
- **File:** .changeset/update-openapi-45502a232af3.md:5
📋 Code Quality: ⚠️ 1 warning · 1 info
The completion metadata still records camel-case flag names. The progress labels on the list-replace updates say "Deleting".
Recommendation: 📝 Minor Comments
- [WARNING] commands.json records camel-case option names (
account-pubname-startsWith,name-startsWith,user-email-startsWith, …) that the runtime does not register, so shell completion suggests flags yargs rejects. (packages/cli/src/commands/_generated/_meta/commands.json:123799) - [INFO] The
Deleting/Deletedprogress labels on a PUT that replaces the recipient list are misleading. (packages/cli/src/commands/_generated/resource-sharing/recipients/update.ts:96)
View full review
- WARNING: For
organizations members list,organizations accounts listand the newtenants accounts list,commands.jsonrecords option names likename-startsWithandaccount-pubname-endsWith. The runtime registersname-starts-with,account-pubname-ends-withand so on (organizations/accounts/list.ts:30-55,organizations/members/list.ts:40-45). As a result,cf completesuggests flags that strict yargs rejects. The generator fix is in #496, and this pull request needs regenerating after it merges. - INFO:
resource-sharing recipients updateandexcluded-recipients updatenow confirm before running and showDeleting/Deletedprogress labels, even though the request is a PUT that replaces the list. The wording comes from the generator, so the fix belongs there.
📝 Documentation: 💬 1 info
The changeset has no migration note for the renamed and removed commands.
Recommendation: 💬 Optional Nits
- [INFO] The changeset has no migration note for the renamed and removed commands. (
.changeset/update-openapi-45502a232af3.md:5)
View full review
- INFO:
.changeset/update-openapi-45502a232af3.mddoes not mention theorganization→organizationsrename, the removal oforigin-post-quantum-encryption, or the replacement of theuser membershipsmembership-details command withuser tenants list.
✅ All other checks passed
🚀 Release: ✅ LGTM
Risk: Low. Under the repository's changeset rules, minor is correct for pre-1.0 command renames and removals. The missing migration note is listed in the Documentation section.
🔒 Security: ✅ LGTM
The pin bump keeps the same full-SHA pin. The regenerated commands, including tenants accounts list, use the shared API request path.
📖 AGENTS.md: ✅ LGTM
No agent instruction files changed. The generated output comes from regeneration, not from hand edits.
⚡ Performance: ✅ LGTM
The change adds no hot-path loops, unbounded fetches or blocking I/O.
Reviewed commit: 2f0f10dd
|
Review: 1 finding. 🤖 I've done a thorough review of your PR. Updates the OpenAPI pin and regenerates CLI commands.
|
2f0f10d to
b025d2a
Compare
|
No new concerns since the last review |
There was a problem hiding this comment.
Superseded by the review of
86a74a7a.
This review, of b025d2a6
💬 Approved with comments — The camel-case completion-metadata warning is fixed in this regeneration. The two remaining notes are INFO: misleading Deleting/Deleted labels on PUT list-replacement commands, and a changeset with no migration note.
Findings: 2 info (2 are inline comments on the diff)
The decision is advisory: this review is a comment and never approves, blocks or requests changes on the pull request.
📋 Code Quality: 💬 1 info
The earlier camel-case completion warning is fixed. One INFO remains: the progress labels on the PUT replace commands.
Recommendation: 💬 Optional Nits
- [INFO] PUT list-replacement commands show
Deleting/Deletedlabels and ask for confirmation before validating--body. (packages/cli/src/commands/_generated/resource-sharing/recipients/update.ts:96)
View full review
- Fixed:
commands.jsonnow records kebab-case option names that match the flags the runtime registers inorganizations/accounts/list.ts,organizations/members/list.tsandtenants/accounts/list.ts. - INFO:
resource-sharing recipients updateandexcluded-recipients updatereplace the whole list with a PUT, but showDeleting/Deletedprogress labels. They also ask for delete confirmation before checking that--bodywas given. Both come from the generator.
📝 Documentation: 💬 1 info
The changeset has no migration note for the renamed and removed commands.
Recommendation: 💬 Optional Nits
- [INFO] The changeset does not tell users about the renamed and removed commands. (
.changeset/update-openapi-45502a232af3.md:5)
View full review
- INFO: The changeset does not mention the
organization→organizationsrename, the removal oforigin-post-quantum-encryption, or the replacement of theuser membershipsmembership-details command withuser tenants list.
✅ All other checks passed
🚀 Release: ✅ LGTM
Risk: Low. The missing migration note is covered in the Documentation section.
🔒 Security: ✅ LGTM
The pin keeps its full-SHA format. The regenerated commands, including the new tenants accounts list, use the shared API request path.
📖 AGENTS.md: ✅ LGTM
No agent instruction files changed. The generated output comes from regeneration, not from hand edits.
⚡ Performance: ✅ LGTM
The change adds no hot-path loops, unbounded fetches or blocking I/O.
Reviewed commit: b025d2a6
b025d2a to
86a74a7
Compare
There was a problem hiding this comment.
Superseded by the review of
a99cdb00.
This review, of 86a74a7a
💬 Approved with comments — Generated output is consistent. Only two INFO notes remain: Deleting/Deleted labels on PUT list-replacement commands, and a changeset with no migration note for renamed or removed commands.
Findings: 2 info
The decision is advisory: this review is a comment and never approves, blocks or requests changes on the pull request.
📋 Code Quality: 💬 1 info
The generated output is consistent. One INFO remains: PUT list-replacement commands use delete labels and ask for confirmation before checking --body.
Recommendation: 💬 Optional Nits
- [INFO] PUT list-replacement commands show Deleting/Deleted labels and ask for confirmation before checking that --body was given. (
packages/cli/src/commands/_generated/resource-sharing/recipients/update.ts:96)
View full review
- INFO:
resource-sharing recipients updateandexcluded-recipients updatereplace the whole list with a PUT, but they showDeleting/Deletedprogress labels. They also ask for delete confirmation before checking that--bodywas given. Both come from the generator, so the fix belongs there and not in the generated files.
The rest is consistent: the organizations rename, the new tenants and user tenants commands, the ssl mtls-certificates associations change from get to list, the abuse-reports list --pending-notice flag, and the --tags/--excluded-recipients options on resource-sharing.
📝 Documentation: 💬 1 info
The changeset does not tell users about the renamed and removed commands.
Recommendation: 💬 Optional Nits
- [INFO] The changeset does not mention the renamed and removed commands. (
.changeset/update-openapi-45502a232af3.md:5)
View full review
- INFO: The changeset does not mention the
organization→organizationsrename, the removal oforigin-post-quantum-encryption, the change ofssl mtls-certificates associationsfrom get to list, or the replacement of the membership-details command withuser tenants list.
✅ All other checks passed
🚀 Release: ✅ LGTM
Risk: Low. The missing migration note is covered in the Documentation section. The minor bump fits pre-1.0 behaviour changes.
🔒 Security: ✅ LGTM
The pin keeps its full-SHA format. The regenerated commands use the shared API request path.
📖 AGENTS.md: ✅ LGTM
No agent instruction files changed. The generated output comes from regeneration, not from hand edits.
⚡ Performance: ✅ LGTM
The change adds no hot-path loops, unbounded fetches or blocking I/O.
Reviewed commit: 86a74a7a
|
No new concerns since the last review |
ant-release-bot
changed the title
86a74a7 to
a99cdb0
Compare
There was a problem hiding this comment.
Superseded by the review of
0d7bfd77.
This review, of a99cdb00
💬 Approved with comments — The regenerated output is consistent and the earlier PUT-label issue is fixed. One WARNING: zaraz workflow update now sends a PUT with no body when neither --body nor --file is given. There is also one SUGGESTION about an unused import.
Findings: 1 warning · 1 suggestion (2 are inline comments 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 #477.
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] `cf zaraz workflow update` sends a PUT with no body when neither `--body` nor `--file` is given. The local required-input check was removed.
- **File:** packages/cli/src/commands/_generated/zaraz/workflow/update.ts:101
## SUGGESTION (1)
### [Code Quality] Unused `parseBody` import in the generated `zaraz/workflow/update.ts`.
- **File:** packages/cli/src/commands/_generated/zaraz/workflow/update.ts:10
📋 Code Quality: ⚠️ 1 warning · 1 suggestion
The regenerated zaraz workflow update no longer requires input and keeps an unused import. The earlier PUT-label note is fixed.
Recommendation: 📝 Minor Comments
- [WARNING]
cf zaraz workflow updatesends a PUT with no body when neither--bodynor--fileis given. The local required-input check was removed. (packages/cli/src/commands/_generated/zaraz/workflow/update.ts:101) - [SUGGESTION] Unused
parseBodyimport in the generatedzaraz/workflow/update.ts. (packages/cli/src/commands/_generated/zaraz/workflow/update.ts:10)
View full review
zaraz workflow updatewith no input (packages/cli/src/commands/_generated/zaraz/workflow/update.ts:101-104): The old--body is requiredcheck was removed. With neither--bodynor--file, the handler falls through toclient.send(operation, { zone_id: zoneId }), which sends a PUT with no body. The user gets only a server-side error, and dry-run shows a bodyless request instead of failing. Fix this in the generator.- Unused import (
update.ts:10):parseBodyis no longer used in this file.
The earlier note on the Deleting/Deleted labels for PUT list-replacement commands is fixed. Both resource-sharing update commands now use Updating/Updated and check --body before confirmDelete. The organization → organizations rename, the new tenants commands and the new flags are consistent across the builders, the telemetry safeFlags and the query parameters.
✅ All other checks passed
🚀 Release: ✅ LGTM
Risk: Low. The changeset uses minor, which fits command renames and removals before 1.0. One earlier note remains open from previous reviews: the changeset does not list the renamed and removed commands.
🔒 Security: ✅ LGTM
The regenerated commands use the shared API request path, and path parameters are URI-encoded. The OpenAPI pin keeps its full-SHA format.
📖 AGENTS.md: ✅ LGTM
No agent instruction files changed. The generated output comes from regeneration.
⚡ Performance: ✅ LGTM
The change is regenerated command scaffolding and a pin bump. It adds no hot-path work.
📝 Documentation: ✅ LGTM
No documentation issues on the changed lines.
Reviewed commit: a99cdb00
| return; | ||
| } | ||
|
|
||
| const result = await withProgress(`Updating`, async () => |
There was a problem hiding this comment.
cf zaraz workflow update sends a PUT with no body when neither --body nor --file is given. The local required-input check was removed.
The old --body is required check was removed. When neither --body nor --file is passed, this fallback calls client.send(operation, { zone_id: zoneId }). That sends a PUT with no workflow value, so the user sees only a server error instead of a clear local message. Dry-run has the same gap and prints a bodyless request. Make the generator fail locally when neither input is given, for example by throwing "--body or --file is required", and then regenerate. Do not edit this file by hand.
| import type { Argv, CommandModule } from "yargs"; | ||
| import { createCommandClient, getZoneId } from "#lib/auth.js"; | ||
| import { createCommandClient, getZoneId, requestApi } from "#lib/auth.js"; | ||
| import { parseBody } from "#lib/body-parser.js"; |
There was a problem hiding this comment.
💬 SUGGESTION · Code Quality
Unused parseBody import in the generated zaraz/workflow/update.ts.
parseBody is still imported, but nothing in this file uses it any more. Dry-run passes argv.body as it is, and the real request uses resolveFileToken. Have the generator drop this import when the body is not JSON so lint does not flag the generated file.
| return; | ||
| } | ||
|
|
||
| const result = await withProgress(`Updating`, async () => |
There was a problem hiding this comment.
The raw text/plain workflow request used to reject calls without --body; this fallback now sends a bodyless PUT and prints Updated. Since --file is the alternate body source, fail locally instead of treating an omitted workflow value as a successful update. Regenerate this from the Forge/OpenAPI source rather than hand-editing the leaf.
| const result = await withProgress(`Updating`, async () => | |
| throw new Error( | |
| "--body or --file is required for this command. Pass --body '<workflow>' or --file path/to/file." | |
| ); |
|
Review: 1 finding. 🤖 I've done a thorough review of your PR. Regenerates the CLI surface for the pinned Forge OpenAPI release. Posted 1 inline suggestion. |
ant-release-bot
changed the title
a99cdb0 to
0d7bfd7
Compare
|
No new concerns since the last review |
There was a problem hiding this comment.
Superseded by the review of
da6deb1d.
This review, of 0d7bfd77
💬 Approved with comments — The regenerated output is consistent. One WARNING is still open: cf zaraz workflow update sends a PUT with no body when given no input. There is also one SUGGESTION about an unused import.
Findings: 1 warning · 1 suggestion
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 #477.
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] `cf zaraz workflow update` sends a PUT with no body when neither `--body` nor `--file` is given, because the required-input check was removed.
- **File:** packages/cli/src/commands/_generated/zaraz/workflow/update.ts:101
## SUGGESTION (1)
### [Code Quality] `parseBody` is imported but no longer used in the generated `zaraz/workflow/update.ts`.
- **File:** packages/cli/src/commands/_generated/zaraz/workflow/update.ts:10
📋 Code Quality: ⚠️ 1 warning · 1 suggestion
Both earlier findings are still there: zaraz workflow update can send a PUT with no body, and parseBody is an unused import.
Recommendation: 📝 Minor Comments
- [WARNING]
cf zaraz workflow updatesends a PUT with no body when neither--bodynor--fileis given, because the required-input check was removed. (packages/cli/src/commands/_generated/zaraz/workflow/update.ts:101) - [SUGGESTION]
parseBodyis imported but no longer used in the generatedzaraz/workflow/update.ts. (packages/cli/src/commands/_generated/zaraz/workflow/update.ts:10)
View full review
zaraz workflow update with no input (packages/cli/src/commands/_generated/zaraz/workflow/update.ts:101-104)
- The old
--body is requiredcheck is gone, and the operation droppedbody: "json". - With neither
--bodynor--file(or with--body "", because the check only tests truthiness), the handler falls through toclient.send(operation, { zone_id: zoneId }). That sends a PUT with no body. - The user gets only a server-side error after a network round trip.
--dry-runshows a bodyless request instead of failing. - Fix this in the generator or the overlay so it emits a required-input check that accepts either
--bodyor--file.
Unused import (update.ts:10)
parseBodyis no longer used anywhere in the file. The generator should stop emitting it when the body kind is not JSON.
The rest of the regenerated output is consistent: the organizations rename, the tenants accounts list command, and the resource-sharing update commands with their Updating labels and --force confirmation.
✅ All other checks passed
🚀 Release: ✅ LGTM
Risk: Low. The changeset uses minor, which fits command renames and removals before 1.0. One earlier note is still open: the changeset does not list the renamed (organization → organizations) or removed (origin-post-quantum-encryption) commands.
🔒 Security: ✅ LGTM
The regenerated commands use the shared API request path, and path parameters are URI-encoded. zaraz workflow update reads --file and @file references through the existing input-validation helpers. The resource-sharing PUT commands now ask for confirmation or --force before replacing a list. The OpenAPI pin keeps its full-SHA format.
📖 AGENTS.md: ✅ LGTM
No agent instruction files changed.
⚡ Performance: ✅ LGTM
The change is regenerated command scaffolding plus a pin bump. It adds no work on hot paths.
📝 Documentation: ✅ LGTM
No new documentation issues on the changed lines.
Reviewed commit: 0d7bfd77
ant-release-bot
changed the title
0d7bfd7 to
da6deb1
Compare
There was a problem hiding this comment.
Superseded by the review of
4ea0cb08.
This review, of da6deb1d
💬 Approved with comments — One WARNING is still open: cf zaraz workflow update sends a PUT with no body when neither --body nor --file is given. There is also a SUGGESTION about an unused import and an INFO about the changeset wording.
Findings: 1 warning · 1 info · 1 suggestion (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 #477.
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] `cf zaraz workflow update` sends a PUT with no body when neither `--body` nor `--file` is given.
- **File:** packages/cli/src/commands/_generated/zaraz/workflow/update.ts:101
## INFO (1)
### [Release] The changeset does not call out the `organization` → `organizations` rename or the removal of `origin-post-quantum-encryption`.
- **File:** .changeset/update-openapi-dcba72008b22.md:5
## SUGGESTION (1)
### [Code Quality] `parseBody` is imported but unused in the generated `zaraz/workflow/update.ts`.
- **File:** packages/cli/src/commands/_generated/zaraz/workflow/update.ts:10
📋 Code Quality: ⚠️ 1 warning · 1 suggestion
The bodyless PUT in zaraz workflow update and the unused parseBody import are still present.
Recommendation: 📝 Minor Comments
- [WARNING]
cf zaraz workflow updatesends a PUT with no body when neither--bodynor--fileis given. (packages/cli/src/commands/_generated/zaraz/workflow/update.ts:101) - [SUGGESTION]
parseBodyis imported but unused in the generatedzaraz/workflow/update.ts. (packages/cli/src/commands/_generated/zaraz/workflow/update.ts:10)
View full review
zaraz workflow update with no input (packages/cli/src/commands/_generated/zaraz/workflow/update.ts:101-104)
- If neither
--bodynor--fileis given, the handler falls through toclient.send(operation, { zone_id: zoneId })and sends a PUT with no payload. --body ""does the same, because the check only tests truthiness.--dry-runprints the bodyless request instead of failing.- Fix this in the Forge overlay or the generator so the command emits a required-input check that accepts either
--bodyor--file.
Unused import (update.ts:10)
parseBodyis not used anywhere in the file.
The rest of the regenerated output is consistent.
🚀 Release: 💬 1 info
The changeset does not mention the renamed and removed commands.
Recommendation: 💬 Optional Nits
- [INFO] The changeset does not call out the
organization→organizationsrename or the removal oforigin-post-quantum-encryption. (.changeset/update-openapi-dcba72008b22.md:5)
View full review
Risk: Low. minor fits command renames and removals before 1.0. The changeset body does not mention that cf organization became cf organizations or that cf origin-post-quantum-encryption was removed.
✅ All other checks passed
🔒 Security: ✅ LGTM
The OpenAPI pin is still a full 40-character SHA. No secrets, injection paths or authorization changes were found.
📖 AGENTS.md: ✅ LGTM
No agent instruction files changed.
⚡ Performance: ✅ LGTM
The change is regenerated command scaffolding plus a pin bump. It adds no work on hot paths.
📝 Documentation: ✅ LGTM
No documentation issues found.
Reviewed commit: da6deb1d
| "cf": minor | ||
| --- | ||
|
|
||
| Update the generated command surface for Forge OpenAPI release |
There was a problem hiding this comment.
ℹ️ INFO · Release
The changeset does not call out the organization → organizations rename or the removal of origin-post-quantum-encryption.
This release replaces cf organization with cf organizations and removes cf origin-post-quantum-encryption, but the changeset only says the generated command surface was updated. Naming the rename and the removal in the release notes would help users whose scripts stop working.
|
No new concerns since the last review |
ant-release-bot
changed the title
da6deb1 to
4ea0cb0
Compare
There was a problem hiding this comment.
💬 Approved with comments — One WARNING is still open: cf zaraz workflow update can send a bodyless PUT when no input is given. There is also one INFO about the changeset wording.
Findings: 1 warning · 1 info (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 #477.
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] `cf zaraz workflow update` sends a PUT with no body when neither `--body` nor `--file` is given, or when `--body ""` is passed.
- **File:** packages/cli/src/commands/_generated/zaraz/workflow/update.ts:102
## INFO (1)
### [Release] The changeset does not mention the `organization` → `organizations` rename, the `origin-post-quantum-encryption` removal, or the new `tenants accounts list`.
- **File:** .changeset/update-openapi-5434cd8fdfdb.md:5
🚀 Release: 💬 1 info
The changeset does not list the user-visible command changes.
Recommendation: 💬 Optional Nits
- [INFO] The changeset does not mention the
organization→organizationsrename, theorigin-post-quantum-encryptionremoval, or the newtenants accounts list. (.changeset/update-openapi-5434cd8fdfdb.md:5)
View full review
Risk: Low. minor is the right bump for command renames and removals before 1.0. The changeset body only says the generated surface was updated. It does not mention:
cf organization→cf organizations- the removal of
cf origin-post-quantum-encryption - the new
cf tenants accounts list
📋 Code Quality: ⚠️ 1 warning
cf zaraz workflow update can still send a PUT with no body.
Recommendation: 📝 Minor Comments
- [WARNING]
cf zaraz workflow updatesends a PUT with no body when neither--bodynor--fileis given, or when--body ""is passed. (packages/cli/src/commands/_generated/zaraz/workflow/update.ts:102)
View full review
zaraz workflow update with no input (packages/cli/src/commands/_generated/zaraz/workflow/update.ts:85-104)
- The old
--body is requiredguard was removed. - If neither
--filenor--bodyis set, the handler falls through toclient.send(operation, { zone_id: zoneId }). That sends a PUT with no payload to the zone's Zaraz workflow setting. --body ""takes the same path, because the check at line 85 only tests truthiness.--dry-runprints the bodyless request and does not fail. It also never shows--filecontent.- Fix this in the Forge overlay or the generator, not in the generated file. Emit a required-input check that accepts either
--bodyor--file.
The rest of the regenerated output is consistent: the organization → organizations rename, the origin-post-quantum-encryption removal, the new tenants accounts list, the resource-sharing flag changes, and the command renames under user and ssl.
✅ All other checks passed
🔒 Security: ✅ LGTM
The OpenAPI pin is a full 40-character SHA. Path parameters are encoded in dry-run URLs, and --force remains the only way to skip a confirmation. No secrets, injection paths or authorization changes were found.
📖 AGENTS.md: ✅ LGTM
No agent instruction files changed.
⚡ Performance: ✅ LGTM
The change is regenerated command scaffolding plus a pin bump. It adds no work on hot paths.
📝 Documentation: ✅ LGTM
No documentation issues found.
Reviewed commit: 4ea0cb08
| } | ||
|
|
||
| const result = await withProgress(`Updating`, async () => | ||
| client.send(operation, { zone_id: zoneId }) |
There was a problem hiding this comment.
cf zaraz workflow update sends a PUT with no body when neither --body nor --file is given, or when --body "" is passed.
This change removed the --body is required guard. If neither --file nor --body is set, the handler now reaches this fallback and sends a PUT with no payload to the zone's Zaraz workflow setting. --body "" does the same, because the check at line 85 only tests truthiness, and --dry-run prints the bodyless request without failing. Have the generator or Forge overlay emit a check that requires either --body or --file (rather than hand-editing this file).
|
LGTM! |
Updates cf to
openapi@5434cd8fdfdb82334910d1e2e2f5edee5ad4b41b.5434cd8fdfdb82334910d1e2e2f5edee5ad4b41bThis PR is maintained automatically by the Update OpenAPI workflow.