Repository navigation
fix: validate nested tagged union request bodies - #336
petebacondarwin wants to merge 1 commit into
Conversation
commit: |
|
| mean | stddev | min | max | |
|---|---|---|---|---|
| PR | 290.1 ms | 5.5 ms | 282.3 ms | 305.0 ms |
| base | 290.4 ms | 5.5 ms | 278.4 ms | 302.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.
|
💬 Approved with comments — The generator, runtime validator and regenerated commands match each other, and the tests cover both valid and invalid branches. The only finding is an optional suggestion to add a note to AGENTS.md. Findings: 1 suggestion The decision is advisory: this comment never approves, blocks or requests changes on the pull request. 🔧 Fix with your agentPaste the following into your coding agent to investigate and fix these issues. 📖 AGENTS.md: 💬 1 suggestionThe generator guidance in AGENTS.md doesn't yet describe validation of nested tagged unions. Recommendation: 💬 Optional Nits
View full reviewThe "Subject = positional + arg shape" generator conventions in AGENTS.md explain how body flags are emitted and when ✅ All other checks passed🚀 Release: ✅ LGTMRisk: Low. The 🔒 Security: ✅ LGTMThis change adds client-side request-body validation only. The bodies sent to the API and the trust boundaries don't change, and nothing here raises a security concern. 📋 Code Quality: ✅ LGTMNo problems found.
⚡ Performance: ✅ LGTMValidation runs once per command call, on a small request body, using a precomputed schema. The recursion during generation is capped by depth and cycle limits. No performance concerns. 📝 Documentation: ✅ LGTMThe changeset and code comments explain the new behaviour well. Reviewed commit: |
| @@ -278,13 +278,17 @@ export function dryRun(ctx: Ctx): string[] { | |||
| } | |||
| lines.push(` ? ${parseBodyExpression(ctx)}`); | |||
There was a problem hiding this comment.
--body takes this raw parse path without calling validateBodyUnions; the request path has the same bypass in bodyBypass(). Consequently a body such as {"dataset_scope": {"type": "datasets"}} is still previewed and sent despite missing the selected branch's required IDs. Validate parsed raw bodies too (and apply the same wrapper after parsing in bodyBypass()).
| lines.push(` ? ${parseBodyExpression(ctx)}`); | |
| lines.push( | |
| ` ? ${(op.bodyUnions?.length ?? 0) > 0 && d.hasBodyParams ? `validateBodyUnions(${parseBodyExpression(ctx)}, bodyUnions)` : parseBodyExpression(ctx)}` | |
| ); |
|
🤖 I've done a thorough review of your PR. Adds generated nested tagged-union validation for flagged request bodies.
|
NuroDev
left a comment
There was a problem hiding this comment.
Seems to be some file conflicts that need fixing @petebacondarwin
6ca897b to
db9821d
Compare
|
LGTM |
|
@NuroDev - ready for another review. |
What changes
Nested
oneOfflags can currently assemble bodies that violate the selected branch. For example, priority-intelligence create/update accept--dataset-scope-type datasetswithout IDs, or--dataset-scope-type defaultwith IDs.Preserve nested union branches selected by disjoint required enum values, including schemas without an explicit discriminator, and validate the assembled wire body with cf's existing JSON Schema validator before sending it or showing a dry-run preview. Enforce branch-specific required fields, closed-object rules, and available array
maxItems. Keep complete--bodyinput available for shapes that cannot be supplied as flags.Regenerate the nine affected commands and add a patch changeset. Generic fixtures execute freshly generated create/update builders and handlers, covering optional omission, valid branches, missing/forbidden fields, the 50/51-item boundary, renamed flags, referenced schemas, and explicit/implicit selectors.
Validation
pnpm --filter cf test— 1,700 passed, 6 skipped.pnpm check— generation, dependency build, workspace type checking, lint, and formatting passed.Addresses the cf portion of cloudflare/forge#33. Missing
minItemsand UUID-format metadata remains tracked separately in cloudflare/forge#47.