fix: filter target input modalities - #367
Conversation
Signed-off-by: Coding Agent <svc-glamr@nvidia.com>
WalkthroughThe change adds ChangesInput modality support
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
crates/libsy-llm-client/src/client.rs (1)
1296-1396: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider extending coverage to the other two wire formats.
The test covers
filter_openai_responses_bodyonly.filter_anthropic_bodyandfilter_openai_chat_bodyuse different block type strings (imageanddocumentfor Anthropic,image_urlandfilefor Chat). A typo in either mapper silently disables filtering for that provider, and no test would fail.Add one case per format. The Anthropic case also exercises the
systemarray branch and thetool_resultrecursion, which the Responses case does not reach.🤖 Prompt for 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. In `@crates/libsy-llm-client/src/client.rs` around lines 1296 - 1396, The existing input-modality preservation test only covers OpenAI Responses; add separate cases for filter_anthropic_body and filter_openai_chat_body. Exercise each format’s image/document or image_url/file block mappings, assert unsupported modalities are removed while text and vendor extensions remain, and ensure the Anthropic case includes system-array handling and recursive tool_result content filtering.crates/switchyard-py/src/libsy_bindings.rs (1)
146-157: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe modality name table is duplicated across crates.
parse_input_modalityhardcodes the five names.crates/protocol/src/llm.rsderives the same names from#[serde(rename_all = "snake_case")]. The two must stay in sync by hand.If a variant is added to
InputModality, this function still compiles and rejects the new name at the Python boundary with a confusing error. Derive the mapping from one source instead. AFromStrimplementation onInputModalityin the protocol crate, reused here, keeps the names in one place.🤖 Prompt for 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. In `@crates/switchyard-py/src/libsy_bindings.rs` around lines 146 - 157, Replace the hardcoded string match in parse_input_modality with the shared InputModality FromStr implementation from the protocol crate, converting its parse failure into PyValueError while preserving the existing PyResult contract. Add the FromStr mapping beside InputModality in crates/protocol/src/llm.rs, using the serde snake_case names and an appropriate invalid-input error so future variants remain synchronized automatically.
🤖 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/libsy-llm-client/src/client.rs`:
- Around line 702-712: Prevent modality filtering from leaving Anthropic
messages with empty content: update filter_content_blocks in
crates/libsy-llm-client/src/client.rs#L702-L712 to report emptied content and
remove the affected Message or InstructionBlock from request.messages or
request.instructions; apply the same policy in filter_json_content_array at
crates/libsy-llm-client/src/client.rs#L801-L815 for nested tool_result and
top-level content arrays. In
crates/switchyard-py/src/libsy_bindings.rs#L105-L134, reject an empty
input_modalities list with PyValueError so it cannot strip every recognized
block.
---
Nitpick comments:
In `@crates/libsy-llm-client/src/client.rs`:
- Around line 1296-1396: The existing input-modality preservation test only
covers OpenAI Responses; add separate cases for filter_anthropic_body and
filter_openai_chat_body. Exercise each format’s image/document or image_url/file
block mappings, assert unsupported modalities are removed while text and vendor
extensions remain, and ensure the Anthropic case includes system-array handling
and recursive tool_result content filtering.
In `@crates/switchyard-py/src/libsy_bindings.rs`:
- Around line 146-157: Replace the hardcoded string match in
parse_input_modality with the shared InputModality FromStr implementation from
the protocol crate, converting its parse failure into PyValueError while
preserving the existing PyResult contract. Add the FromStr mapping beside
InputModality in crates/protocol/src/llm.rs, using the serde snake_case names
and an appropriate invalid-input error so future variants remain synchronized
automatically.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: d99fd085-1a34-4931-818f-3ba0091cc588
📒 Files selected for processing (14)
crates/libsy-llm-client/src/client.rscrates/libsy/src/algorithms/fall_through.rscrates/libsy/src/algorithms/llm_class.rscrates/libsy/src/algorithms/passthrough.rscrates/libsy/src/algorithms/rand.rscrates/libsy/src/algorithms/stage.rscrates/libsy/src/algorithms/subagent_affinity_tests.rscrates/libsy/src/algorithms/util/llm_judge.rscrates/libsy/src/core/algorithm.rscrates/protocol/src/llm.rscrates/switchyard-py/src/libsy_bindings.rscrates/switchyard-server/src/config.rscrates/switchyard-server/tests/server.rsswitchyard_rust/libsy.py
| fn filter_content_blocks(content: &mut Vec<ContentBlock>, allowed: &BTreeSet<InputModality>) { | ||
| for block in content.iter_mut() { | ||
| if let ContentBlock::ToolResult(result) = block { | ||
| filter_content_blocks(&mut result.content, allowed); | ||
| } | ||
| } | ||
| content.retain(|block| match block_input_modality(block) { | ||
| Some(modality) => allowed.contains(&modality), | ||
| None => true, | ||
| }); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Modality filtering can empty a message content array, and no site guards against it. Every filter helper calls retain on a content container without checking whether the container becomes empty. Anthropic rejects a message whose content is []. This file already treats that state as a defect: anthropic_requests_drop_unsigned_thinking_blocks asserts message.get("content") != Some(&json!([])). Choose one policy — drop the emptied message, or insert a placeholder text block when Text is allowed — and apply it at all three sites.
crates/libsy-llm-client/src/client.rs#L702-L712: aftercontent.retain(...)infilter_content_blocks, handle the case wherecontentis now empty, and propagate that decision to the caller so an emptiedMessageorInstructionBlockis removed fromrequest.messagesorrequest.instructions.crates/libsy-llm-client/src/client.rs#L801-L815: apply the same policy aftercontent.retain(...)infilter_json_content_array, covering both the nestedtool_resultcontent and the top-level message content.crates/switchyard-py/src/libsy_bindings.rs#L105-L134: an emptyinput_modalitieslist currently producesSome(vec![]), which strips every recognized block and makes the empty-content case certain. Either reject an empty list with aPyValueError, or document that an empty list means "allow no recognized modality" once the emptied-content policy above is in place.
📍 Affects 2 files
crates/libsy-llm-client/src/client.rs#L702-L712(this comment)crates/libsy-llm-client/src/client.rs#L801-L815crates/switchyard-py/src/libsy_bindings.rs#L105-L134
🤖 Prompt for 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.
In `@crates/libsy-llm-client/src/client.rs` around lines 702 - 712, Prevent
modality filtering from leaving Anthropic messages with empty content: update
filter_content_blocks in crates/libsy-llm-client/src/client.rs#L702-L712 to
report emptied content and remove the affected Message or InstructionBlock from
request.messages or request.instructions; apply the same policy in
filter_json_content_array at crates/libsy-llm-client/src/client.rs#L801-L815 for
nested tool_result and top-level content arrays. In
crates/switchyard-py/src/libsy_bindings.rs#L105-L134, reject an empty
input_modalities list with PyValueError so it cannot strip every recognized
block.
Signed-off-by: Coding Agent <svc-glamr@nvidia.com>
1 participant
Implements target-local input modality allowlists for issue #152 and refactors JSON modality parsing to avoid duplicate helpers.
Assigned issue: #152
Summary
input_modalitiesvalues fortext,image,audio,video, andfile.LlmTarget,ModelConfig, and Python target bindings/stubs.Validation
cargo fmt --checkcargo test -p switchyard-llm-client input_modalitiesfailed because the runner lacks a system C linker:linker cc not found.cargo testfailed for the same environment reason:linker cc not found.Notes
NVIDIA-NeMo/Switchyardwas denied forglamr-agent, so the follow-up commit was pushed to the existing PR branch inglamr-agent/Switchyard.