Repository navigation
feat(cli): pass OpenRPC paramStructure through IR to FDR - #18469
willkendall01 wants to merge 3 commits into
Conversation
Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
|
I'll fix CI failures and address comments from users with write access. I'll skip comments containing "(aside)".
|
![nitpickybot[bot]](https://avatars.githubusercontent.com/in/2901896?s=60&v=4){kind=small}
There was a problem hiding this comment.
AI Review Summary
Adds OpenRPC paramStructure to the IR (v67.36.0) and threads it through to the FDR register payload, with a new importer fixture/test. The IR definition and hand-written generated SDK changes look consistent; main concerns are the missing IR migration for older IR versions and a changelog date that's in the future.
- 🟡 1 warning(s)
- 🔵 2 suggestion(s)
Review Scope
- Mode: Full pull request
- Range:
Full pull request through e61cd9a89a8327566a13868e3f32d34c673eb14f - Current range coverage: Complete
- Cumulative pull request coverage: Complete
This review is complete for the current scope. Addressing all critical and warning findings above satisfies the review; suggestions are optional. Request another review only if subsequent changes introduce new behavior or materially expand the pull request's scope.
| proto: ProtoSourceInfo | ||
| openapi: {} | ||
| openrpc: {} | ||
| openrpc: OpenRpcSourceInfo |
There was a problem hiding this comment.
🟡 warning
Changing openrpc: {} to openrpc: OpenRpcSourceInfo makes the union variant carry a payload. Older IR versions' schemas still expect {}, so unless packages/ir-migrations has a migration (or the existing identity migrations drop unknown fields safely), generators pinned to IR ≤67.35 could receive the extra paramStructure key. Worth confirming an ir-migrations entry was added for 67.36.0 — the PR description mentions the migrations package only as "tests pass".
There was a problem hiding this comment.
No migration is needed. This is a minor, additive IR bump (67.35 → 67.36), and per CLAUDE.md / packages/ir-sdk/CLAUDE.md migrations are only required for major bumps. Older IR 67.x consumers get an extra optional key on source.openrpc, and generators parse IR with unrecognizedObjectKeys: "passthrough" (generators/base/src/AbstractGeneratorCli.ts, utils/parseIR.ts), so it's ignored. v67-to-v66 also serializes with passthrough. The ir-migrations tests pass on this branch.
| The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), | ||
| and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). | ||
|
|
||
| ## [v67.36.0] - 2026-10-22 |
There was a problem hiding this comment.
🔵 suggestion
2026-10-22 is a year ahead of the preceding entry's convention (and presumably today). Looks like a typo — the previous entry is v67.35.0 - 2026-10-21, so it may be consistent, but double-check the year before merging.
There was a problem hiding this comment.
The date is correct. Today is 2026-10-22, and the entry follows v67.35.0 - 2026-10-21.
| const endpoints = endpointsByName(ir); | ||
| expect(endpoints["plants.water"]?.source).toMatchObject({ type: "openrpc", paramStructure: "BY_NAME" }); | ||
| expect(endpoints["plants.prune"]?.source).toMatchObject({ type: "openrpc", paramStructure: "BY_POSITION" }); | ||
| expect(endpoints["plants.list"]?.source).toMatchObject({ type: "openrpc", paramStructure: undefined }); |
There was a problem hiding this comment.
🔵 suggestion
toMatchObject({ ..., paramStructure: undefined }) passes even if paramStructure is a non-undefined value in some Vitest versions? No — it does assert undefined, but it also passes when the key is absent. That's fine here; however consider toEqual for the whole source object so a regression that adds a stray field is caught. Optional.
There was a problem hiding this comment.
I'm leaving this as is. The FDR protocol assertions further down already use toEqual on the full object, so a stray field would show up there. The IR check only cares about paramStructure, and the v3-sdks/baseline-sdks snapshots for this fixture cover the full source shape.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ Devin resolved the finding on e61cd9a
Fixed by Devin (1)
- Invalid parameter structure aborts OpenRPC import
View all findings in Devin Review
…ling import Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Docs Generation Benchmark ResultsComparing PR branch against median of 5 nightly run(s) on
Docs generation runs |
SDK Generation Benchmark ResultsComparing PR branch against median of 5 nightly run(s) on Full benchmark table (click to expand)
main (generator): generator-only time via --skip-scripts (includes Docker image build, container startup, IR parsing, and code generation — this is the same Docker-based flow customers use via |
Description
Linear ticket: Refs none
OpenRPC methods with
paramStructure: by-nameshow up in docs with a positionalparamsarray, both in Try it and in the snippets. Servers that only accept named params reject that request. The importer ignoredparamStructure, and the IR and FDR OpenRPC protocol had nowhere to store it. This PR carries the field through to the FDR register payload. The renderer change is in fern-api/fern-platform#15897.Changes Made
HttpEndpointSource.openrpcchanges from{}toOpenRpcSourceInfo { paramStructure: optional<OpenRpcParamStructure> }(BY_NAME/BY_POSITION/EITHER). Wire format is unchanged when the field is absent. The IR changelog is updated.packages/ir-sdk/src/sdk: generated types updated by hand to match the TS generator's output.pnpm ir:generatecouldn't pullfernapi/fern-typescript-sdk:3.69.0here because Docker Hub returned 429.MethodConverter: mapsmethod.paramStructureinto the source and ignores unknown values instead of failing the import. Forby-namemethods, example request bodies are now objects keyed by param name. Before this they were arrays, which rendered as{"0": ..., "1": ...}once the renderer sentby-nameparams as an object. Other methods keep array examples.convertPackage: emits{ type: "openrpc", methodName, paramStructure: "by-name" | "by-position" | "either" | undefined }.feat).Until the fdr-sdk catalog pin includes fern-platform#15897, FDR's zod schema strips the new field. So it has no effect until the platform change is deployed, and existing OpenRPC sites stay positional either way.
Testing
openrpc-param-structurefixture and test in v3-importer-tests. They check the IR source (BY_NAME/BY_POSITION/ unset), the example bodies (named object vs array) and the FDRprotocoloutput.@fern-api/register,@fern-api/ir-migrations,@fern-api/v3-importer-testsand openrpc-to-ir tests and compile pass.paramsobject to Postman Echo, and a method withoutparamStructurestayed positional. Screenshots are in a comment on fern-platform#15897.Link to Devin session: https://app.devin.ai/sessions/93b3ae4a969442b29452f9f42d9fe342
Open in Devin Desktop: https://app.devin.ai/desktop/session/93b3ae4a969442b29452f9f42d9fe342?variant=devin
Requested by: @willkendall01