[petebacondarwin]

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]

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

commit: c7ed823

[github-actions]

cf --help startup

	mean	stddev	min	max
PR	177.1 ms	7.7 ms	163.8 ms	202.4 ms
base	172.3 ms	4.2 ms	167.0 ms	183.2 ms

Δ (PR vs base): ↑ +4.8 ms (+2.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.

[clanky-the-clanker]

💬 Approved with comments — No correctness, security or performance problems found. Only an optional test-coverage suggestion for branch detection and a small documentation clarification remain.

Findings: 1 info · 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

## INFO (1)

### [Documentation] docs/architecture.md says missing required body fields are prompted for on a TTY. Branch-specific fields required by a nested union are rejected without a prompt instead, and the guide should say so.
- **File:** docs/architecture.md:630

## SUGGESTION (1)

### [Code Quality] Tests cover only string-enum fixtures. Negative cases for branch detection (overlapping or untagged unions) and boolean selectors (used by k2/streams) are not tested.
- **File:** packages/cli/generator/plugin/body-unions.ts:15

---

📋 Code Quality: 💬 1 suggestion

No correctness bugs in the generator or the runtime validator. One optional gap in test coverage for branch detection.

Recommendation: 💬 Optional Nits

• [SUGGESTION] Tests cover only string-enum fixtures. Negative cases for branch detection (overlapping or untagged unions) and boolean selectors (used by k2/streams) are not tested. (packages/cli/generator/plugin/body-unions.ts:15)

View full review

Test coverage of branch detection (suggestion)

• packages/cli/generator/plugin/body-unions.ts:15-44 (taggedMembers) has several decision points:
  • enum values that overlap across members
  • a selector that is not required in every member
  • a union whose members are not mutually exclusive
  • a member that is not an object
  • the members.length < 2 guard
• nested-unions.test.ts has only positive string-enum fixtures, with and without an explicit discriminator.
• No negative fixture checks that an untagged or overlapping union is left alone.
• No fixture covers a boolean selector. The regenerated k2/streams create and update commands depend on one (values: [false] / [true]).
• If the detector breaks, the only sign would be a change in the generated output. No test would fail. A small unit test on bodyUnions() for these cases would catch that.

What was checked

• The import, the bodyUnions declaration, the dry-run path and the real-send path are emitted consistently. All of them are gated on bodyUnions.length > 0 and hasBodyParams.
• validateBodyUnions depends on validateJsonSchema handling oneOf, anyOf, enum, required, additionalProperties: false and maxItems. packages/cli/src/lib/schema-flags.ts handles all of them, so what the generator emits matches what the runtime enforces.
• A complete --body input still skips this validation, as the description says.
• packages/cli/src/lib/body-unions.ts stays product-agnostic, which matches the repository's design boundaries.

---

📝 Documentation: 💬 1 info

The architecture guide does not say how missing branch-specific fields in nested unions are handled.

Recommendation: 💬 Optional Nits

• [INFO] docs/architecture.md says missing required body fields are prompted for on a TTY. Branch-specific fields required by a nested union are rejected without a prompt instead, and the guide should say so. (docs/architecture.md:630)

View full review

• docs/architecture.md:630-642 says missing required body fields are prompted for when stdin is a TTY.
• The new nested-union validation rejects a missing branch-specific field whether or not stdin is a TTY. For example, it rejects the datasets scope without dataset_ids in cloudforce-one/priority-intelligence/create.ts.
• Consider saying in the guide that these branch-specific requirements fail validation instead of prompting.

---

✅ All other checks passed

🚀 Release: ✅ LGTM

Risk: Low. The patch changeset is appropriate for a bug fix.

---

🔒 Security: ✅ LGTM

This change adds validation on the CLI side. It crosses no new trust boundary, and the server is still the final check on request bodies.

• The path walk in packages/cli/src/lib/body-unions.ts uses Object.hasOwn, so it does not follow inherited or __proto__ keys.
• The generator emits only data taken from the schema, never user input.

This review read the two new body-unions.ts files and the handler change for security. It did not cover the regenerated command files or the tests.

---

📖 AGENTS.md: ✅ LGTM

The change follows the repository conventions:

• Generic emission rules are in generator/plugin/.
• The runtime helper in src/lib/ is product-agnostic.
• The generated output was regenerated rather than edited by hand.
• A patch changeset is included.

---

⚡ Performance: ✅ LGTM

No performance concerns. Validation runs once per request over small schema fragments.

---

_{Reviewed commit: c7ed823d}

[ask-bonk]

--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]

🤖 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]

Seems to be some file conflicts that need fixing @petebacondarwin

[ask-bonk]

LGTM

github run

[petebacondarwin]

@NuroDev - ready for another review.

[ask-bonk]

An untagged outer oneOf can still contain a tagged union that is flattened into flags, but this walk only descends through objects and intersections. For example, if one branch contains scope: oneOf[{ type: "default" }, { type: "datasets", dataset_ids: ... }] and the outer union has no disjoint required selector, bodyUnions() returns no entry, so --scope-type datasets without IDs again reaches the API. Traverse union members too (and add this nesting shape to the fixture).

Suggested change

	} else if (t.kind === "intersection") {
	} else if (t.kind === "intersection" || t.kind === "union") {

[ask-bonk]

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

Adds generated validation for nested tagged-union flag bodies.

1. Warning: Posted 1 inline suggestion.

github run