Skip to content

fix(openapi-generator): propagate comment from union requestBody to body schema#1149

Open
bitgo-ai-agent-dev[bot] wants to merge 1 commit into
masterfrom
fix/union-requestbody-description
Open

fix(openapi-generator): propagate comment from union requestBody to body schema#1149
bitgo-ai-agent-dev[bot] wants to merge 1 commit into
masterfrom
fix/union-requestbody-description

Conversation

@bitgo-ai-agent-dev

Copy link
Copy Markdown

What

  • In derefRequestSchema, preserve the Block comment returned as the third element of
    findSymbolInitializer's tuple instead of discarding it. The comment is now attached to
    the resolved schema when no comment is already present.
  • In parseRequestUnion, propagate schema.comment onto the newly created body union node
    ({ type: 'union', schemas: [] }) so the description survives into the schema that the
    OpenAPI generator and lint checks inspect.

Why

  • api-ts's OpenAPI generator silently dropped JSDoc descriptions on request bodies built as
    a t.union([httpRequest(...), httpRequest(...)]) pattern (common for endpoints that accept
    either a userId or email). The generated spec had no description on
    requestBody.content['application/json'].schema, causing the missing-request-body-description
    quality check to fail even when JSDoc was present in the source.
  • This blocked POST /api/v2/enterprise/{enterpriseId}/safes/{safeId}/members (BitGo/bitgo-microservices#61023)
    and is the root cause identified in TW-295.

Test plan

  • Added route.test.ts case: verifies body.comment is populated at IR level when the
    request is a named-const union with a JSDoc comment.
  • Added openapi/union.test.ts case: verifies end-to-end that the description surfaces
    in requestBody.content['application/json'].schema.description in the OpenAPI output.
  • All 203 existing tests continue to pass.

Ticket: TW-295

@bitgo-ai-agent-dev
bitgo-ai-agent-dev Bot force-pushed the fix/union-requestbody-description branch from 543782d to 768e4f6 Compare July 16, 2026 21:26

Copy link
Copy Markdown
Contributor

Review — solves TW-295 ✅

Reviewed against the Slack thread / TW-295. The fix is correct, minimal, and matches the root cause diagnosed there.

Verified end-to-end by reproducing the exact endpoint shape from bitgo-microservices#61023 (POST /api/v2/enterprise/{enterpriseId}/safes/{safeId}/members, path params + userId-or-email union body with a JSDoc description on the named const):

requestBody.content['application/json'].schema.description
master undefined ← the bug
this PR "Add a member to a safe by userId or email"

That's exactly the node the missing-request-body-description quality check inspects, so this unblocks the failing spec-check. All 203 openapi-generator tests pass, including the two added cases.

On correctness:

  • The two-part fix is right — derefRequestSchema was discarding the third (comment) element of findSymbolInitializer's tuple, and parseRequestUnion was building a fresh { type: 'union', schemas: [] } node without carrying the comment over. Both are now handled.
  • Mirrors the established sibling pattern in codec.ts (L555–559), so it's consistent with how comments are already propagated on deref.
  • No shared-reference risk: parseCodecInitializer re-parses from the AST on each call rather than returning a cached object, so mutating .comment can't leak into another schema.
  • The resolvedE.right.comment === undefined guard correctly gives precedence to a comment already present on the union expression.
  • Commit is a conventional fix(openapi-generator):, so multi-semantic-release will cut a patch release — no changeset needed.

One thing worth calling out (not blocking): this propagates the description from the union-level JSDoc (on the named const or the t.union(...) expression). It does not synthesize a wrapper description from per-variant comments on the individual httpRequest(...) members — which was the other thing Sibi tried ("inline descriptions in the union type with no success"). That's the correct behavior for this lint (it wants a description on the union wrapper, which per-variant comments can't provide), but it's worth documenting the guidance: put the JSDoc on the union codec, not on the individual variants. Consider a note in the openapi-generator README's union section so the next person doesn't hit the same dead end.

Ready to come out of draft from my side.


Generated by Claude Code

@bitgo-ai-agent-dev

Copy link
Copy Markdown
Author

Added a note in section 6.2.3.1 of the README (new sub-section under Custom Tags) documenting exactly this: the JSDoc must go on the union codec itself, not on the individual httpRequest members, with a working example and an explicit warning about the wrong placement. Commit: 6e6342c.

@linear-code

linear-code Bot commented Jul 16, 2026

Copy link
Copy Markdown

TW-295

@bitgo-ai-agent-dev
bitgo-ai-agent-dev Bot force-pushed the fix/union-requestbody-description branch from 6e6342c to 3282654 Compare July 16, 2026 22:03
@claude

claude Bot commented Jul 16, 2026

Copy link
Copy Markdown

The Nix Flake Check failure was a Prettier prose-wrap issue in the new README section 6.2.3.1 (added in 3282654d) — the added paragraphs were wrapped wider than this repo's proseWrap: always config. I pushed a fix (7c3568ca, npx prettier --write packages/openapi-generator/README.md) that resolves it — Nix Flake Check now passes on that commit.

However, 7c3568ca is now the sole blocker on Validate Humans In The Loop: it's signed with an SSH key rather than GPG, so it's rejected even though the diff itself is correct. I don't have a way to produce a GPG-signed commit here, and I can't force-push to remove it from this branch. Whoever has commit access with a GPG key can either cherry-pick/reapply that one-line prose-wrap diff as their own signed commit (then drop 7c3568ca), or force-push to remove it and re-run npx prettier --write packages/openapi-generator/README.md themselves. Diff is trivial — just re-wraps 3 paragraphs to ~80 cols, no content changes.

@vmccarty
vmccarty marked this pull request as ready for review July 17, 2026 14:34
@vmccarty
vmccarty requested a review from a team as a code owner July 17, 2026 14:34
…ody schema

Two gaps in route.ts prevented JSDoc descriptions from reaching the body
schema node for union-of-httpRequest request bodies:

1. derefRequestSchema resolved a named-const ref but dropped the third
   element of the findSymbolInitializer tuple (Block | undefined), so
   any JSDoc on the named const was silently discarded.

2. parseRequestUnion built the merged body as a brand-new
   { type: 'union', schemas: [] } node and never copied schema.comment
   to it, so even when the union schema already carried a comment
   (e.g. after fix 1), the body node the lint inspects had none.

Together these mean that a route like:
  /** Either a userId or an email */
  const AddMemberRequest = t.union([httpRequest({body:{userId}}), ...]);
  httpRoute({ request: AddMemberRequest, ... })

now emits a description on requestBody.content['application/json'].schema,
satisfying the missing-request-body-description quality check.

Fixes affect patterns such as POST /enterprise/{id}/safes/{id}/members
where the body enforces userId | email.

Ticket: TW-295
Session-Id: fc63a9af-dca6-4ec9-9637-7afb61367955
Task-Id: a1d22b41-99d1-4474-8e8a-82ff2fe84a38
@vmccarty
vmccarty force-pushed the fix/union-requestbody-description branch from 7c3568c to 78c823c Compare July 17, 2026 14:42
},
components: {
schemas: {
AddMemberRequest: {

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.

This looks a little funky -- there's a schema that isn't used? Is that expected?

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Good catch, and you're reading it right — AddMemberRequest in components.schemas is genuinely never $ref'd anywhere in paths here. It's expected though, not a leftover: the test harness (testCase in testHarness.ts) dumps every top-level declaration that isn't itself a valid route into schemas, regardless of whether it's referenced. AddMemberRequest has to be a named top-level const in this test — that's the point, verifying the comment propagates from a named union ref — but parseRequestUnion/derefRequestSchema fully flattens that union into an inline body schema, so no ref node survives pointing back at it. It's an artifact of the test harness's schema-collection convention, unrelated to the description-propagation fix itself — the real CLI's component-collection pass in cli.ts only includes schemas still reachable via ref, so this dangling entry likely wouldn't show up in a real generated spec. Happy to add a one-line comment above that block if it'd help future readers.

@vmccarty
vmccarty enabled auto-merge July 17, 2026 20:44
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