feat(protocols): implement P4 IncludeField web_search_call variants - #1274
Conversation
|
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 (1)
📝 WalkthroughWalkthroughAdded two new public Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Code Review
This pull request adds the WebSearchCallActionSources and WebSearchCallResults variants to the IncludeField enum, including their corresponding serialization mappings. A unit test was also added to verify the round-trip serialization and deserialization of these new fields. I have no feedback to provide.
What: - Add two variants to `IncludeField` in crates/protocols/src/responses.rs: - WebSearchCallActionSources -> "web_search_call.action.sources" - WebSearchCallResults -> "web_search_call.results" - Add one serde round-trip test in the existing `mod tests` that deserializes both rename strings into the new variants and serializes them back byte-identically to the spec allowed-values list. Why: - OpenAI Responses API spec lists 8 allowed values for the top-level `include` array (openai-responses-api-spec.md:71-79). Prior to this change smg only modeled 6 variants; spec-valid payloads with "web_search_call.action.sources" or "web_search_call.results" failed to deserialize into `Option<Vec<IncludeField>>`, causing 400-style errors on otherwise-compliant clients and blocking audit task T3 (non-preview web_search tool) that depends on P4. How: - Purely additive: `IncludeField` uses explicit `#[serde(rename)]` per variant with no `#[serde(other)]` or `#[serde(untagged)]` fallback, so adding variants cannot alter deserialization of existing inputs. - No downstream gating logic changes in this PR. Spec wiring for web search result/source inclusion is owned by T3 (currently blocked on P4). Only one existing variant (MessageOutputTextLogprobs) is inspected in code (cross-field validation at responses.rs:1097); the new variants keep the same "pure schema" posture. - Round-trip test mirrors the pattern of the adjacent test_require_approval_*_round_trip tests: one assert on parsed value, one assert on re-serialized JSON. Refs: P4 Signed-off-by: Simo Lin <linsimo.mark@gmail.com>
d1dc9f9 to
16cf2e0
Compare
Summary
Implements audit task P4: add the two missing
IncludeFieldvariants (web_search_call.action.sources,web_search_call.results) so spec-valid payloads for the top-levelinclude[]array deserialize cleanly. Unblocks audit task T3 (web_searchnon-preview tool).What changed
crates/protocols/src/responses.rs:+2IncludeFieldvariants with#[serde(rename)]strings byte-identical to spec:WebSearchCallActionSources→"web_search_call.action.sources"WebSearchCallResults→"web_search_call.results"+1serde round-trip unit test covering both new renames and spec-listed existing variants.+27 / -0.Why
OpenAI Responses API spec lists 8 allowed values for the top-level
include[]array (see.claude/_audit/openai-responses-api-spec.md:71-79, specifically lines 73-74). smg previously modeled only 6; spec-valid clients sending"web_search_call.results"or"web_search_call.action.sources"hit deserialization failure. Pure additive schema change.Verification
cargo check -p openai-protocolpasses (isolatedCARGO_TARGET_DIR=/tmp/p4-targetto avoid worktree cache collision)cargo test -p openai-protocol→ 83/83 pass including the newtest_include_field_web_search_call_variants_round_tripcargo +nightly fmt --all -- --checksilentcargo clippy -p openai-protocol --all-targets --all-features -- -D warningscleanunavailable(harness skill permission; Lead proceeded solo per playbook §8 fallback after own roundtrip passed)grep -rn 'match.*IncludeField'acrosscrates/,model_gateway/,bindings/,e2e_test/→ zero hits (no exhaustive match needs updating)grep -rn 'IncludeField::'→ 4 references, all construction or.contains(…), additive-safegrep -rn 'IncludeField' bindings/→ zero hits (no Python/Go mirror)Blast radius
Exactly 1 file:
crates/protocols/src/responses.rs. Matches audit's declared Blast Radius.Out of scope
WebSearchCalloutput struct missing typedresultsfield — belongs to T3 (P4-dependent), not P4's acceptance criteria.Refs: audit task P4 ·
.claude/_audit/responses-api-gap-audit.mdSummary by CodeRabbit
New Features
Tests