Repository navigation
Conversation
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
|
Review: 1 findings. Posted 1 inline suggestion. |
![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
06d4fbe0.
This review, of b06b1c82
💬 Approved with comments — Nothing found makes this unsafe to merge. One WARNING: the new account/zone route merge strips shared path-item keys from zone paths that still have operations. There are also open questions about the pinned SHA and the Forge dependency, plus stale developer docs.
Findings: 2 warnings · 1 question · 1 info · 1 suggestion (3 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 #490.
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 (2)
### [Code Quality] Merging account/zone routes deletes shared path-item keys (e.g. path-level `parameters`) from zone paths that are kept because they have extra methods, which can leave those operations without their `{zone_id}` parameter.
- **File:** packages/cli/generate.ts:257
### [Documentation] `scripts/README.md`, `docs/architecture.md` and `packages/cli/AGENTS.md` still describe fetching the Forge `openapi.forge.json` release asset. Update them for the `cloudflare/api-schemas` source.
## INFO (1)
### [Code Quality] The new `combineAccountAndZoneRoutes` and legacy cloudforce-one handling have no tests.
- **File:** packages/cli/generate.ts:154
## SUGGESTION (1)
### [Code Quality] The JSDoc for `prepareAccountOrZoneOperations` now documents `rewriteAccountZoneDocumentation`.
- **File:** packages/cli/generate.ts:145
## QUESTIONS (1)
### [Code Quality] The pinned `FORGE_OPENAPI_VERSION` (`ec5b5772…`) was a Forge release identifier and now resolves to `raw.githubusercontent.com/cloudflare/api-schemas/<sha>/openapi.json`. Is this SHA a real `cloudflare/api-schemas` commit? Also, should the pinned `@cloudflare/forge` build be bumped here, given the stated dependency on cloudflare/forge#89?
- **File:** packages/cli/generate.ts:20
📋 Code Quality: ⚠️ 1 warning · 1 question · 1 info · 1 suggestion
The route-combining logic can leave zone-only operations without their path-level keys. The pinned SHA and the Forge dependency need confirming, and the new logic has no tests.
Recommendation: 📝 Minor Comments
- [WARNING] Merging account/zone routes deletes shared path-item keys (e.g. path-level
parameters) from zone paths that are kept because they have extra methods, which can leave those operations without their{zone_id}parameter. (packages/cli/generate.ts:257) - [QUESTION] The pinned
FORGE_OPENAPI_VERSION(ec5b5772…) was a Forge release identifier and now resolves toraw.githubusercontent.com/cloudflare/api-schemas/<sha>/openapi.json. Is this SHA a realcloudflare/api-schemascommit? Also, should the pinned@cloudflare/forgebuild be bumped here, given the stated dependency on cloudflare/forge#89? (packages/cli/generate.ts:20) - [INFO] The new
combineAccountAndZoneRoutesand legacy cloudforce-one handling have no tests. (packages/cli/generate.ts:154) - [SUGGESTION] The JSDoc for
prepareAccountOrZoneOperationsnow documentsrewriteAccountZoneDocumentation. (packages/cli/generate.ts:145)
View full review
- Partial merge strips zone path-item keys (
packages/cli/generate.ts:255-258).combineAccountAndZoneRoutesdeletes every key of the cloned account item from both source path items, including path-levelparameters,summary,descriptionandservers. If the zone path has a method the account path lacks, the zone path is kept, but it can lose the path-levelparametersthat declare{zone_id}. That method's spec is then broken. The zone variant's own request and response schemas are also dropped in favour of the account variant's. - Pinned SHA and Forge dependency (
packages/cli/generate.ts:17).FORGE_OPENAPI_VERSIONis unchanged, but it now resolves againstcloudflare/api-schemasinstead of a Forge release tag. The description also says this needscloudflare/forge#89, and the pinned Forge build is not bumped here. - No tests for
combineAccountAndZoneRoutesor the legacy cloudforce-one handling. - Misplaced JSDoc (
packages/cli/generate.ts:145). The doc block forprepareAccountOrZoneOperationsnow sits aboverewriteAccountZoneDocumentation.
📝 Documentation: ⚠️ 1 warning
Developer docs still describe the Forge release asset flow.
Recommendation: 📝 Minor Comments
- [WARNING]
scripts/README.md,docs/architecture.mdandpackages/cli/AGENTS.mdstill describe fetching the Forgeopenapi.forge.jsonrelease asset. Update them for thecloudflare/api-schemassource.
View full review
Three developer docs still say the OpenAPI pin comes from the latest Forge release and its openapi.forge.json asset: scripts/README.md:16-19, docs/architecture.md (around lines 147 and 787) and packages/cli/AGENTS.md:73-75. Update them to describe the cloudflare/api-schemas main-commit lookup and the openapi.json download.
✅ All other checks passed
🔒 Security: ✅ LGTM
The new latest-commit lookup accepts only a 40-character hex SHA before it is interpolated into the URL, the version pin and the PR body. The route rewriting runs only at build time, on the spec document.
📖 AGENTS.md: ✅ LGTM
The packages/cli/AGENTS.md:73-75 description of the OpenAPI source is covered in the documentation section.
⚡ Performance: ✅ LGTM
No performance concerns.
Reviewed commit: b06b1c82
b06b1c8 to
06d4fbe
Compare
commit: |
|
| mean | stddev | min | max | |
|---|---|---|---|---|
| PR | 298.0 ms | 6.6 ms | 289.3 ms | 317.2 ms |
| base | 301.1 ms | 11.1 ms | 291.1 ms | 330.9 ms |
Δ (PR vs base): ↓ -3.1 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.
|
Review: 0 findings. No new concerns since the last review. |
There was a problem hiding this comment.
Superseded by the review of
307f323a.
This review, of 06d4fbe0
💬 Approved with comments — One correctness WARNING: the account/zone route merge removes path-level keys from zone paths that are kept. One documentation WARNING: the developer docs are stale. There is also an open question about the pinned SHA. Nothing found is a blocking production risk.
Findings: 2 warnings · 1 question · 2 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 #490.
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 (2)
### [Code Quality] combineAccountAndZoneRoutes deletes every key of the account item, including path-level `parameters`, from a zone path that is kept because it has extra methods. Those operations can lose their `{zone_id}` path parameter.
- **File:** packages/cli/generate.ts:257
### [Documentation] scripts/README.md, docs/architecture.md and packages/cli/AGENTS.md still say the OpenAPI pin comes from the latest Forge release and its `openapi.forge.json` asset. Update them to describe the cloudflare/api-schemas commit lookup and the `openapi.json` download.
## INFO (2)
### [Code Quality] The new combineAccountAndZoneRoutes logic and the handling of the legacy cloudforce-one v1 operations have no tests.
- **File:** packages/cli/generate.ts:153
### [Documentation] The comment on the pin still calls the SHA a release, but it now identifies a cloudflare/api-schemas commit.
- **File:** packages/cli/generate.ts:16
## SUGGESTION (1)
### [Code Quality] The JSDoc describing prepareAccountOrZoneOperations now sits above rewriteAccountZoneDocumentation.
- **File:** packages/cli/generate.ts:145
## QUESTIONS (1)
### [Code Quality] FORGE_OPENAPI_VERSION is unchanged but now resolves to raw.githubusercontent.com/cloudflare/api-schemas/<sha>/openapi.json. Is `ec5b5772…` a commit in cloudflare/api-schemas? If not, `pnpm generate` will fail with a 404.
- **File:** packages/cli/generate.ts:20
📋 Code Quality: ⚠️ 1 warning · 1 question · 1 info · 1 suggestion
The route merge removes shared path-item keys from zone paths that are kept, the pinned SHA has not been confirmed, and the new logic has no tests.
Recommendation: 📝 Minor Comments
- [WARNING] combineAccountAndZoneRoutes deletes every key of the account item, including path-level
parameters, from a zone path that is kept because it has extra methods. Those operations can lose their{zone_id}path parameter. (packages/cli/generate.ts:257) - [QUESTION] FORGE_OPENAPI_VERSION is unchanged but now resolves to raw.githubusercontent.com/cloudflare/api-schemas/<sha>/openapi.json. Is
ec5b5772…a commit in cloudflare/api-schemas? If not,pnpm generatewill fail with a 404. (packages/cli/generate.ts:20) - [INFO] The new combineAccountAndZoneRoutes logic and the handling of the legacy cloudforce-one v1 operations have no tests. (
packages/cli/generate.ts:153) - [SUGGESTION] The JSDoc describing prepareAccountOrZoneOperations now sits above rewriteAccountZoneDocumentation. (
packages/cli/generate.ts:145)
View full review
- Shared keys removed from zone paths that are kept (
packages/cli/generate.ts:255-258). The loop deletes every key of the cloned account item from both source items, including path-levelparameters,summary,descriptionandservers. When the zone path has a method the account path lacks, that zone path is kept, but it can lose the path-level parameter that declares{zone_id}. - Pinned SHA (
packages/cli/generate.ts:17-20). The same SHA is now used forcloudflare/api-schemas. Please confirm it is a commit in that repository. - No tests for
combineAccountAndZoneRoutesor the handling of the legacy cloudforce-one v1 operations. - Misplaced JSDoc (
packages/cli/generate.ts:130-145). The doc block forprepareAccountOrZoneOperationsnow sits aboverewriteAccountZoneDocumentation.
The @cloudflare/forge bump in package.json and the lockfile is consistent.
📝 Documentation: ⚠️ 1 warning · 1 info
The developer docs and one comment in the generator still describe the retired Forge release-asset source.
Recommendation: 📝 Minor Comments
- [WARNING] scripts/README.md, docs/architecture.md and packages/cli/AGENTS.md still say the OpenAPI pin comes from the latest Forge release and its
openapi.forge.jsonasset. Update them to describe the cloudflare/api-schemas commit lookup and theopenapi.jsondownload. - [INFO] The comment on the pin still calls the SHA a release, but it now identifies a cloudflare/api-schemas commit. (
packages/cli/generate.ts:16)
View full review
-
These docs still describe picking the latest Forge release and downloading its
openapi.forge.jsonasset:scripts/README.md:16-19,29-30docs/architecture.md:147,787-788packages/cli/AGENTS.md:73-75
Update them for the
cloudflare/api-schemasmain-commit lookup and theopenapi.jsondownload, including the note about anonymous requests. -
packages/cli/generate.ts:16still says to bump the SHA for a "new release". The SHA now identifies anapi-schemascommit.
✅ All other checks passed
🔒 Security: ✅ LGTM
The latest-commit lookup checks the SHA against ^[0-9a-f]{40}$ before it goes into the URL, the version pin and the pull request body. The route rewriting runs only at build time on the spec document. The @cloudflare/forge pin moves to another commit-pinned build, with a matching integrity hash in the lockfile.
📖 AGENTS.md: ✅ LGTM
The packages/cli/AGENTS.md:73-75 description of the OpenAPI source is covered in the documentation section.
⚡ Performance: ✅ LGTM
No performance concerns.
Reviewed commit: 06d4fbe0
06d4fbe to
307f323
Compare
|
No new concerns since the last review |
There was a problem hiding this comment.
💬 Approved with comments — The earlier correctness and documentation warnings are fixed. What remains is a question about whether the pinned SHA exists in cloudflare/api-schemas, and a note about missing tests.
Findings: 1 question · 1 info
The decision is advisory: this review is a comment and never approves, blocks or requests changes on the pull request.
📋 Code Quality: ❓ 1 question · 1 info
The earlier issues are fixed. A question about the pinned SHA and a note about missing tests remain.
Recommendation: 💬 Optional Nits
- [QUESTION] FORGE_OPENAPI_VERSION is unchanged but now resolves to raw.githubusercontent.com/cloudflare/api-schemas/<sha>/openapi.json. Please confirm
ec5b5772…is a commit in cloudflare/api-schemas; otherwisepnpm generatefails with a 404. (packages/cli/generate.ts:17) - [INFO] combineAccountAndZoneRoutes and the legacy cloudforce-one v1 handling have no tests. (
packages/cli/generate.ts:138)
View full review
- Pinned SHA (
packages/cli/generate.ts:17-20): the value ofFORGE_OPENAPI_VERSIONhas not changed, but it is now used as a commit incloudflare/api-schemas. If that repository has no such commit, the raw.githubusercontent.com download returns a 404 andpnpm generatefails. - No tests for
combineAccountAndZoneRoutesor for the handling of the legacy cloudforce-one v1 operations.
Fixed since the last review:
- Only the combined HTTP methods are deleted now, so a zone path that is kept still has its path-level keys.
- The JSDoc is back above
prepareAccountOrZoneOperations.
The changes to scripts/update-openapi.ts and its tests match each other.
✅ All other checks passed
🔒 Security: ✅ LGTM
The update script checks the commit SHA against ^[0-9a-f]{40}$ before it goes into the URL or the version pin. The spec rewriting runs only at build time. The @cloudflare/forge pin moves to another commit-pinned build, with a matching integrity hash in the lockfile.
📖 AGENTS.md: ✅ LGTM
packages/cli/AGENTS.md now describes the OpenAPI source correctly.
⚡ Performance: ✅ LGTM
No performance concerns. The changes run only at build time.
📝 Documentation: ✅ LGTM
scripts/README.md, docs/architecture.md and packages/cli/AGENTS.md now describe the cloudflare/api-schemas commit lookup and the openapi.json download. The comment on the pin in generate.ts now refers to an api-schemas commit.
Reviewed commit: 307f323a
84ed843 to
317fb4b
Compare
|
Review: 1 findings. 🤖 I've done a thorough review of your PR. Fetches OpenAPI directly from
|
317fb4b to
6d2dcc8
Compare
|
No new concerns since the last review |
6d2dcc8 to
8d2a05b
Compare
|
LGTM |
8d2a05b to
56e9fd3
Compare
| // Bump this SHA to generate the commands from a new release. | ||
| const FORGE_OPENAPI_VERSION = "ec5b5772f5cf7dfb16c49f3527671edfa37bb666"; | ||
| // Bump this SHA to generate the commands from a newer cloudflare/api-schemas commit. | ||
| const FORGE_OPENAPI_VERSION = "193e683baba9c46b675d91c959c3b9a7926b2d84"; |
There was a problem hiding this comment.
193e683baba9c46b675d91c959c3b9a7926b2d84 is not a commit in cloudflare/api-schemas: GitHub's commit API returns 422 and the raw openapi.json URL returns 404. Consequently pnpm generate fails before Forge runs. Pin a real api-schemas commit (the commit below existed before this PR was opened and serves openapi.json) and regenerate the committed output.
| const FORGE_OPENAPI_VERSION = "193e683baba9c46b675d91c959c3b9a7926b2d84"; | |
| const FORGE_OPENAPI_VERSION = "bdea3bc5b21212e02c8b54dd8f0c5929a4f7b8cf"; |
|
Review: 1 findings. 🤖 I've done a thorough review of your PR. Fetches OpenAPI directly from
|
56e9fd3 to
2e3387e
Compare
|
LGTM! |
Fetch
openapi.jsondirectly fromcloudflare/api-schemasinstead of theopenapi.forge.jsonGitHub release artifact oncloudflare/forge.packages/cli/generate.ts: Combine matching/accounts/{account_id}/<subpath>and/zones/{zone_id}/<subpath>route pairs into/{account_or_zone}/{account_or_zone_id}/<subpath>at the start ofprepareAccountOrZoneOperations, and ignore legacy v1/accounts/{account_id}/cloudforce-one/requestsoperations that still carryx-forge-hidden: trueupstream.scripts/update-openapi.ts: Resolve the latest commit SHA fromrepos/cloudflare/api-schemas/commits/maininstead ofrepos/cloudflare/forge/releases/latest.Depends on cloudflare/forge#89 for the
@cloudflare/forgelinker updates.Before:
After: