refactor(protocols): strip audit-process residue from comments - #1310
Conversation
Pure comment hygiene on crates/protocols/src/responses.rs (non-test
code only). Removes audit-process residue that leaked into doc
comments:
- Drop audit task-id markers ("P1", "P5 fail-fast contract") from
per-variant and per-field docs.
- Drop "Postel's law: liberal on input, conservative on output"
narrative from the `ResponsesFunctionToolChoice::Function` doc;
keep the wire-shape kernel and the tag-pinning invariant.
- Drop the "This type deliberately does NOT live in common.rs…"
split-rationale paragraph from `ResponsesToolChoice`.
- Drop the "OpenAI Python SDK 2.8.x TypedDict" narrative from
`ResponseInputOutputItem::ImageGenerationCall`; keep the HTTP
spec shape and the server-side strictness note.
- Drop the "Replaces the prior Vec<String> wire-type that broke
bidirectional interoperability" cycle-narrative from
`SummaryTextContent`.
Scope: lines 1-2397 of responses.rs (non-test code). Tests
(mod tests at L2398+) are skipped to avoid conflicts with a
parallel test-extraction task. common.rs, chat.rs, and the rest
of crates/protocols/src were already clean.
No semantic changes: no `pub` items, struct fields, enum variants,
`#[serde(...)]` attributes, function bodies, or tests were touched.
No new comments added; only removal / pruning of existing ones.
Acceptance:
- grep residue pattern on crates/protocols/src/ non-test code: 0 hits
- cargo check -p openai-protocol --lib --tests: green
- cargo clippy -p openai-protocol --lib --tests -- -D warnings: green
- cargo test -p openai-protocol --lib: 94 passed
- cargo doc --no-deps -p openai-protocol: no new warnings
(9 pre-existing warnings in interactions.rs / messages.rs, unchanged)
Refs: .claude/_audit/responses-api-gap-audit.md §C2
Signed-off-by: Simo Lin <linsimo.mark@gmail.com>
|
Caution Review failedPull request was closed or merged during review 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)
📝 WalkthroughWalkthroughDocumentation and comments in the responses module were updated and simplified. Changes include narrowing descriptions of Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~5 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 |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request simplifies documentation in crates/protocols/src/responses.rs by removing historical context, internal project references, and architectural explanations. A review comment suggests that the removal of the explanation for ResponsesToolChoice eliminates critical context regarding specification alignment and recommends its restoration to prevent future misuse of the API.
| @@ -174,13 +174,6 @@ impl<'de> Deserialize<'de> for ResponsesFunctionToolChoice { | |||
| /// whose `type` does not belong to that variant. Without the tag pinning, | |||
| /// the `#[serde(untagged)]` enum would accept any object shape that | |||
| /// happened to fit the field set of an earlier variant. | |||
There was a problem hiding this comment.
The removed paragraph explaining why ResponsesToolChoice is kept separate from common.rs contains critical architectural context. It warns that merging these types would cause the Chat API to incorrectly accept Responses-specific variants (like Mcp or Shell), leading to spec-invalid payloads. This aligns with the repository's priority to prioritize alignment with external specifications over internal consistency and to surface spec divergences at the edge.
/// happened to fit the field set of an earlier variant.
///
/// This type is kept separate from common.rs to prevent the Chat API
/// from incorrectly accepting Responses-specific variants (like Mcp or
/// Shell) which are not part of the Chat spec.
References
- For protocol data structures that mirror an external API, prioritize alignment with the external specification over internal consistency.
- Prioritize fail-fast behavior for unknown or new types to ensure spec divergences are surfaced at the edge as errors rather than silently confusing downstream consumers.
|
Thanks @gemini-code-assist — good catch. The original paragraph did carry a real architectural invariant (Chat vs Responses tool_choice must stay unshared to prevent spec-invalid payloads on |
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
1 participant
Summary
Pure comment hygiene: removes audit-process markers (`// P7:`, "Cycle-1 REJECT", "per audit", Postel narrative, etc.) from `crates/protocols/src/`. No semantic changes.
Scope: non-test code in `crates/protocols/src/responses.rs` (lines 1-2397). Tests at L2398+ are intentionally skipped to avoid conflicts with a parallel task that extracts the test module to a sibling file. `common.rs`, `chat.rs`, and the rest of `crates/protocols/src` were already clean.
Rules applied
Changes
`crates/protocols/src/responses.rs` only (1 file, +10 / -32):
Net: +10 / -32 lines.
Acceptance
Hard rules honored
Refs: `.claude/_audit/responses-api-gap-audit.md §C2`
Summary by CodeRabbit