Fold input system/developer messages into native provider system param#48
Fold input system/developer messages into native provider system param#48ScriptSmith wants to merge 1 commit into
Conversation
Greptile SummaryThis PR fixes silent message loss across three LLM provider adapters (Anthropic, Bedrock, Vertex) and closes a field-leakage gap in the Azure OpenAI adapter. Previously,
Confidence Score: 4/5The change is safe to merge; the core folding logic is correct and well-tested across all three providers, and no existing behaviour is regressed. The system-message folding is implemented consistently and symmetrically across Anthropic, Bedrock, and Vertex. Each provider's The duplicated helper functions in Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["convert_responses_input_to_messages()"] --> B{{"instructions present?"}}
B -- yes --> C["system_parts.push(instructions)"]
B -- no --> D["system_parts = []"]
C --> E["Iterate input messages"]
D --> E
E --> F{{"message role?"}}
F -- "System / Developer" --> G["extract text"]
G --> H{"text non-empty?"}
H -- yes --> I["system_parts.push(text)"]
H -- no --> E
I --> E
F -- "User / Assistant" --> J["append to messages[]"]
J --> E
E -- done --> K["join_system_parts(system_parts)"]
K --> L{{"parts empty?"}}
L -- yes --> M["return None"]
L -- no --> N["return Some(parts.join('\n\n'))"]
N --> O["Provider-specific wrapping"]
O --> P["Anthropic: Option<String>"]
O --> Q["Bedrock: Option<Vec<BedrockSystemContent>>"]
O --> R["Vertex: Option<VertexContent>"]
Prompt To Fix All With AIFix the following 3 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 3
src/providers/anthropic/convert.rs:650-668
**Duplicated text-extraction helpers across three provider files**
`easy_content_text` / `input_content_text` in `anthropic/convert.rs`, `easy_content_text_bedrock` / `input_content_text_bedrock` in `bedrock/convert.rs`, and `easy_content_text_vertex` / `input_content_text_vertex` in `vertex/convert.rs` are byte-for-byte identical in logic. The only difference is the suffix in the name. If the extraction logic ever needs to change (e.g., to handle a new `ResponseInputContentItem` variant), it must be updated in three places. Extracting them into a shared module (e.g., `src/providers/convert_utils.rs`) would eliminate the duplication.
### Issue 2 of 3
src/providers/bedrock/convert.rs:1774-1813
**Bedrock test omits the `Developer` role**
The new test exercises the `System` role being folded into the system blocks but doesn't also cover `EasyInputMessageRole::Developer`, even though both are handled by the same branch and the anthropic test does cover both. Adding a `Developer` message to the test item list would increase confidence that the symmetry holds under future refactors.
### Issue 3 of 3
src/providers/azure_openai/mod.rs:231-236
**Unrelated fix bundled into the system-message folding PR**
The `strip_gateway_fields()` call here mirrors the OpenAI adapter and is a distinct correctness fix — gateway-managed fields were previously leaking to the upstream Azure endpoint. Mixing it into a PR titled "fold input system/developer messages into native provider system param" makes the change harder to bisect. Consider whether it warrants its own PR or at least a clear note in the PR description.
Reviews (1): Last reviewed commit: "Fold input system/developer messages int..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| /// Extract the concatenated text from an easy-input message content value. | ||
| fn easy_content_text(content: &EasyInputMessageContent) -> String { | ||
| match content { | ||
| EasyInputMessageContent::Text(text) => text.clone(), | ||
| EasyInputMessageContent::Parts(parts) => input_content_text(parts), | ||
| } | ||
| } | ||
|
|
||
| /// Extract the concatenated `input_text` from a list of input content items. | ||
| fn input_content_text(parts: &[ResponseInputContentItem]) -> String { | ||
| parts | ||
| .iter() | ||
| .filter_map(|part| match part { | ||
| ResponseInputContentItem::InputText { text, .. } => Some(text.as_str()), | ||
| _ => None, | ||
| }) | ||
| .collect::<Vec<_>>() | ||
| .join("\n\n") | ||
| } |
There was a problem hiding this comment.
easy_content_text / input_content_text in anthropic/convert.rs, easy_content_text_bedrock / input_content_text_bedrock in bedrock/convert.rs, and easy_content_text_vertex / input_content_text_vertex in vertex/convert.rs are byte-for-byte identical in logic. The only difference is the suffix in the name. If the extraction logic ever needs to change (e.g., to handle a new ResponseInputContentItem variant), it must be updated in three places. Extracting them into a shared module (e.g., src/providers/convert_utils.rs) would eliminate the duplication.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/providers/anthropic/convert.rs
Line: 650-668
Comment:
**Duplicated text-extraction helpers across three provider files**
`easy_content_text` / `input_content_text` in `anthropic/convert.rs`, `easy_content_text_bedrock` / `input_content_text_bedrock` in `bedrock/convert.rs`, and `easy_content_text_vertex` / `input_content_text_vertex` in `vertex/convert.rs` are byte-for-byte identical in logic. The only difference is the suffix in the name. If the extraction logic ever needs to change (e.g., to handle a new `ResponseInputContentItem` variant), it must be updated in three places. Extracting them into a shared module (e.g., `src/providers/convert_utils.rs`) would eliminate the duplication.
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| let tool_result = json.get("toolResult").unwrap(); | ||
| assert_eq!(tool_result.get("status").unwrap(), "success"); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_responses_input_folds_system_messages() { | ||
| use crate::api_types::responses::EasyInputMessage; | ||
|
|
||
| // System/developer messages in the input must be folded into the system | ||
| // blocks (Bedrock Converse has no system role inside the message list). | ||
| let items = vec![ | ||
| ResponsesInputItem::EasyMessage(EasyInputMessage { | ||
| type_: None, | ||
| role: EasyInputMessageRole::System, | ||
| content: EasyInputMessageContent::Text("Be concise.".to_string()), | ||
| }), | ||
| ResponsesInputItem::EasyMessage(EasyInputMessage { | ||
| type_: None, | ||
| role: EasyInputMessageRole::User, | ||
| content: EasyInputMessageContent::Text("Hi".to_string()), | ||
| }), | ||
| ]; | ||
|
|
||
| let (system, messages) = convert_responses_input_to_bedrock_messages( | ||
| Some(ResponsesInput::Items(items)), | ||
| Some("You are a helpful assistant.".to_string()), | ||
| ); | ||
|
|
||
| let system = system.expect("system blocks present"); | ||
| assert_eq!(system.len(), 1); | ||
| assert_eq!( | ||
| system[0].text.as_deref(), | ||
| Some("You are a helpful assistant.\n\nBe concise.") | ||
| ); | ||
| assert_eq!(messages.len(), 1); | ||
| assert_eq!(messages[0].role, "user"); | ||
| } | ||
| } | ||
|
|
||
| #[cfg(test)] |
There was a problem hiding this comment.
Developer role
The new test exercises the System role being folded into the system blocks but doesn't also cover EasyInputMessageRole::Developer, even though both are handled by the same branch and the anthropic test does cover both. Adding a Developer message to the test item list would increase confidence that the symmetry holds under future refactors.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/providers/bedrock/convert.rs
Line: 1774-1813
Comment:
**Bedrock test omits the `Developer` role**
The new test exercises the `System` role being folded into the system blocks but doesn't also cover `EasyInputMessageRole::Developer`, even though both are handled by the same branch and the anthropic test does cover both. Adding a `Developer` message to the test item list would increase confidence that the symmetry holds under future refactors.
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| // Drop gateway-managed fields (store, background, models, provider, | ||
| // plugins, sovereignty_requirements, skills) that Hadrian consumes | ||
| // itself and the upstream must never see, mirroring the OpenAI adapter. | ||
| let mut payload = payload; | ||
| payload.strip_gateway_fields(); | ||
|
|
There was a problem hiding this comment.
The strip_gateway_fields() call here mirrors the OpenAI adapter and is a distinct correctness fix — gateway-managed fields were previously leaking to the upstream Azure endpoint. Mixing it into a PR titled "fold input system/developer messages into native provider system param" makes the change harder to bisect. Consider whether it warrants its own PR or at least a clear note in the PR description.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/providers/azure_openai/mod.rs
Line: 231-236
Comment:
**Unrelated fix bundled into the system-message folding PR**
The `strip_gateway_fields()` call here mirrors the OpenAI adapter and is a distinct correctness fix — gateway-managed fields were previously leaking to the upstream Azure endpoint. Mixing it into a PR titled "fold input system/developer messages into native provider system param" makes the change harder to bisect. Consider whether it warrants its own PR or at least a clear note in the PR description.
How can I resolve this? If you propose a fix, please make it concise.1 participant
No description provided.