feat(openai-protocol): add ResponseTool::as_str()#1457
Conversation
`harmony/builder.rs` carried a 14-arm match converting `ResponseTool` variants to their wire `type` tags. The strings are exactly the variants' `#[serde(rename = ...)]` attributes — generic enum-to-tag data masquerading as harmony-specific routing logic. Adding a new `ResponseTool` variant required remembering to update this match; nothing flagged it. Add `ResponseTool::as_str(&self) -> &'static str` next to the enum, matching the `as_str` convention used by `ItemType` and friends. Migrate the harmony match to a one-liner. The match in `as_str` is exhaustive — adding a variant fails compilation in `openai-protocol` itself, where the vocabulary lives. Test pins the unit variants (Computer, ApplyPatch, LocalShell) against a serde roundtrip so `as_str` can't drift from the canonical wire tag. Signed-off-by: Simo Lin <25425177+slin1237@users.noreply.github.com>
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
📝 WalkthroughWalkthroughThis PR centralizes tool type-to-string mapping by adding a ChangesTool Type String Mapping & Helper Methods
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
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.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/protocols/src/responses.rs`:
- Around line 3915-3929: The test
response_tool_as_str_matches_serde_tag_for_unit_variants only covers unit
variants; extend it to also instantiate a few payload-carrying variants of
ResponseTool (e.g., the Browser and any Tool/External variants that carry
fields) and include them in the loop where you call serde_json::to_value(&tool)
and compare tool.as_str() with the serialized "type" tag; ensure you create
minimal valid instances for those payload variants so the serialization succeeds
and the assertion checks parity for both unit and payload variants using the
same serde_json::to_value / get("type") / as_str() pattern.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5dcecd51-ca5b-4bbc-b875-334b13468038
📒 Files selected for processing (2)
crates/protocols/src/responses.rsmodel_gateway/src/routers/grpc/harmony/builder.rs
Summary
Replace the 14-arm
ResponseTool::* => &strmatch inharmony/builder.rswith a one-liner that callstool.as_str(). The new method lives inopenai-protocolnext to the enum, matches theas_strconvention used byItemTypeetc., and returns each variant's#[serde(rename = ...)]tag verbatim.Motivation
Adding a new
ResponseToolvariant (e.g. a future hosted family) used to require remembering to update the harmony match. Forgetting it would mean the new variant's wire tag silently became whatever the catch-all of the day was — and there was no compile error to flag the omission. Moving the table to the enum's owning crate makes the match exhaustive inopenai-protocol, so adding a variant there fails compilation untilas_stris updated.This is the follow-up the PR #1455 audit flagged as out-of-scope — the harmony match wasn't a bridge concern, it was generic enum hygiene.
Test plan
response_tool_as_str_matches_serde_tag_for_unit_variantspins the three unit variants (Computer, ApplyPatch, LocalShell) against a serde roundtrip soas_strcan't drift from the canonical wire tagcargo test -p openai-protocol --lib— passescargo test -p smg --lib— 751 passedcargo clippy -p smg -p openai-protocol --lib --tests -- -D warnings— cleancargo fmt— cleanSummary by CodeRabbit
New Features
Tests
Refactor