refactor(core): derive Responses/chat known-field lists from struct tags#485
Conversation
The Responses API field set was hand-maintained in ~8 places (three 25-field struct twins, known-field string lists, raw decode structs with per-field copy, a 25-field marshal struct, and a 24-field provider-side reduction); adding one field was shotgun surgery, and a missed site meant the field silently dropped or double-emitted. - jsonFieldNames derives known-field lists from json tags at package init, so extractUnknownJSONFields can never drift from the type definitions. - Decode goes through an embedded type alias (Input shadowed as RawMessage for the union), so new typed fields are populated automatically — the triple-entry raw structs and field-by-field copies are gone. - ResponseCompactRequest is now defined as ResponseInputTokensRequest (one written field set); the responseUtilityRequestJSON bridge and its two 25-field copies are deleted. - The full->utility reduction moves next to the types as ResponsesRequest.InputTokensRequest()/CompactRequest(); the openai provider strips the gateway-only provider hint at its boundary as before. - Guard tests pin the contract: utility field set == full set minus stream controls, and a reflection-filled request asserts the reduction copies every field, so the next added field fails a test instead of vanishing. Net -220 lines in the codecs. Wire behavior unchanged (existing round-trip and unknown-field tests pass untouched). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughAdds a reflection-based ChangesJSON field derivation and request refactor
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Provider as compatible_provider.go
participant Req as ResponsesRequest
participant ITR as InputTokensRequest
participant CR as CompactRequest
Provider->>Req: req.InputTokensRequest()
Req->>ITR: copy fields, clone ExtraFields
ITR-->>Provider: return *ResponseInputTokensRequest
Provider->>Req: req.CompactRequest()
Req->>ITR: InputTokensRequest()
ITR-->>CR: convert to ResponseCompactRequest
CR-->>Provider: return *ResponseCompactRequest
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Summary
The biggest remaining shotgun-surgery site from the architecture review (doc in #472, §3): the Responses API field set was maintained by hand in ~8 places. Adding one field meant editing three 25-field struct twins, two known-field string lists, two raw decode structs with field-by-field copies, a 25-field marshal struct, and a 24-field provider-side reduction — and a missed site silently dropped the field or double-emitted it via ExtraFields.
After this PR the field set is written once per type and everything else derives:
jsonFieldNamesderives the known-field lists fromjsontags at package init, soextractUnknownJSONFieldscan never drift from the type definitions.Inputshadowed asjson.RawMessagefor the string-or-array union), so new typed fields are populated by the JSON package automatically — the triple-entry raw structs and per-field copies are deleted.ResponseCompactRequestis nowtype ResponseCompactRequest ResponseInputTokensRequest— the byte-identical 25-field twin is gone.ResponsesRequest.InputTokensRequest()/.CompactRequest()); the openai provider keeps stripping the gateway-onlyproviderhint at its boundary, unchanged on the wire.ChatRequestdecode (its field list was triple-entry too).The safety net
Two new guard tests pin the contract mechanically: the utility field set must equal the full set minus the streaming controls, and a reflection-filled
ResponsesRequestasserts the reduction copies every field. The next person who adds a Responses field gets a failing test naming the missed spot, not a silent drop.User-visible impact
None — wire behavior is unchanged. The existing round-trip, unknown-field-preservation, and input-union tests pass untouched, which is the strongest evidence the alias decode is byte-equivalent.
Testing
Full
go build ./...andgo test ./...;go test -raceon core + openai + server;golangci-lint0 issues; pre-commitmake test-raceon the commit.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Tests