Skip to content

fix: validate nested tagged union request bodies - #336

Open
petebacondarwin wants to merge 1 commit into
mainfrom
codex/fix-nested-union-validation
Open

petebacondarwin wants to merge 1 commit into
mainfrom
codex/fix-nested-union-validation

Conversation

@petebacondarwin

Copy link
Copy Markdown
Collaborator

What changes

Nested oneOf flags can currently assemble bodies that violate the selected branch. For example, priority-intelligence create/update accept --dataset-scope-type datasets without IDs, or --dataset-scope-type default with 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 --body input 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.
  • Final targeted regression run — 24 passed. Direct priority-intelligence create/update dry-run smoke checks confirmed valid cases and rejection of missing IDs, forbidden IDs, and 51 IDs.

Addresses the cf portion of cloudflare/forge#33. Missing minItems and UUID-format metadata remains tracked separately in cloudflare/forge#47.

@pkg-pr-new

pkg-pr-new Bot commented Oct 16, 2026 •

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

commit: db9821d

@github-actions

github-actions Bot commented Oct 16, 2026 •

Copy link
Copy Markdown
Contributor

cf --help startup

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.

@clanky-the-clanker

clanky-the-clanker Bot commented Oct 16, 2026 •

Copy link
Copy Markdown

💬 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 agent

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

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

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

## SUGGESTION (1)

### [AGENTS.md] Add a short note to the AGENTS.md generator conventions: generated commands validate nested tagged-union bodies built from flags before a dry-run preview or send, and a complete `--body` stays available for shapes that can't be built from flags.

📖 AGENTS.md: 💬 1 suggestion

The generator guidance in AGENTS.md doesn't yet describe validation of nested tagged unions.

Recommendation: 💬 Optional Nits

  • [SUGGESTION] Add a short note to the AGENTS.md generator conventions: generated commands validate nested tagged-union bodies built from flags before a dry-run preview or send, and a complete --body stays available for shapes that can't be built from flags.
View full review

The "Subject = positional + arg shape" generator conventions in AGENTS.md explain how body flags are emitted and when demandOption is suppressed. They don't mention that generated commands now project nested tagged-union branches into a JSON Schema and check the body assembled from flags before a dry-run preview or send (packages/cli/src/lib/body-unions.ts). They also don't say that a complete --body skips that check. A short bullet there would help future generator changes keep this behaviour.


✅ All other checks passed

🚀 Release: ✅ LGTM

Risk: Low. The .changeset/nested-union-validation.md patch changeset fits a fix: change to cf, as the repository's changeset rules require.


🔒 Security: ✅ LGTM

This 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: ✅ LGTM

No problems found.

  • Generator and runtime match. The emitted validateBodyUnions import, the bodyUnions constant, and the dry-run and send call sites in generator/plugin/emit/handler.ts and generator/plugin/files.ts all line up with src/lib/body-unions.ts. The import is added only when hasBodyParams is true. bodyAssembly itself only runs on the path that builds the body from flags.
  • --body is unchanged. A complete --body still goes through parseBody without validation (see the regenerated priority-intelligence/create.ts, around lines 310–312). So the escape hatch for shapes that can't be built from flags still works.
  • Schema projection. The keywords the projection emits (maxItems, required, additionalProperties, anyOf, allOf) are all supported by validateJsonSchema in lib/schema-flags.ts.
  • Nested unions use anyOf. The projection drops some constraints, so using anyOf rather than oneOf avoids rejecting valid input. The code comment explains this.
  • Missing selector. If the selector field is absent, validation falls back to a oneOf across all branches. That choice is reasonable.
  • Tests. The tests run freshly generated builders and handlers. They cover omitting the optional field, valid branches, missing and forbidden fields, the 50/51-item boundary, renamed flags, referenced schemas, and explicit and implicit selectors.
  • Regenerated files. The workers create diff is mostly re-indentation from wrapping the body in the new validation call.

⚡ Performance: ✅ LGTM

Validation 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: ✅ LGTM

The changeset and code comments explain the new behaviour well.


Reviewed commit: db9821df

@@ -278,13 +278,17 @@ export function dryRun(ctx: Ctx): string[] {
}
lines.push(` ? ${parseBodyExpression(ctx)}`);

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.

--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()).

Suggested change
lines.push(` ? ${parseBodyExpression(ctx)}`);
lines.push(
` ? ${(op.bodyUnions?.length ?? 0) > 0 && d.hasBodyParams ? `validateBodyUnions(${parseBodyExpression(ctx)}, bodyUnions)` : parseBodyExpression(ctx)}`
);

@ask-bonk

ask-bonk Bot commented Oct 16, 2026

Copy link
Copy Markdown
Contributor

🤖 I've done a thorough review of your PR.

Adds generated nested tagged-union validation for flagged request bodies.

  1. Warning: --body bypasses the new validation in dry-run and request paths. Posted a suggestion on packages/cli/generator/plugin/emit/handler.ts:279.

github run

@NuroDev NuroDev left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Seems to be some file conflicts that need fixing @petebacondarwin

@petebacondarwin
petebacondarwin force-pushed the codex/fix-nested-union-validation branch from 6ca897b to db9821d Compare October 20, 2026 14:31
@ask-bonk

ask-bonk Bot commented Oct 20, 2026

Copy link
Copy Markdown
Contributor

LGTM

github run

@petebacondarwin

Copy link
Copy Markdown
Collaborator Author

@NuroDev - ready for another review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants