feat(switchyard-llm-client): add HTTP LLM client crate#79
Conversation
fcf75d3 to
de9ddbe
Compare
f524f3e to
30d8979
Compare
de9ddbe to
bb40d0f
Compare
|
@sir-merge-a-lot groom |
Previous updates (collapsed)🤖 sir-merge-a-lot Resolving conflicts🔄 sir-merge-a-lot is resolving conflicts on this MR. Detected 3 conflicting file(s) (REGENERATE). I'll update this comment with the result. This can take a couple of minutes. 🤖 sir-merge-a-lot Coding agent working🤖 sir-merge-a-lot handed a large conflict to its sandboxed coding agent. The LLM tier could not resolve 🤖 sir-merge-a-lot Coding agent could not resolve the conflict
Resolution ladder trace
|
…eutral IR Signed-off-by: Greg Clark <grclark@nvidia.com>
30d8979 to
a3c4498
Compare
WalkthroughAdds the ChangesLLM Client Crate
Estimated code review effort: 4 (Complex) | ~60 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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: 5
🤖 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/README.md`:
- Around line 8-15: Rewrite all README examples to use the crate’s actual API:
import ModelConfig and TranslatingLlmClient from switchyard_llm_client,
construct the client with TranslatingLlmClient::new, and invoke
call_rewrite_model instead of the unsupported LlmModelClient, libsy_llm_client,
nested backend maps, call, formats_for, or aggregate interfaces. Remove the
unused LlmModelClient link definition while preserving the examples’ intended
behavior.
In `@crates/libsy-llm-client/src/backend.rs`:
- Around line 33-41: Replace the derived Debug implementation on
HttpBackendConfig with a manual implementation that preserves base_url and
extra_headers while rendering any present api_key as “[REDACTED]” rather than
its value. Keep Debug available for containing types such as Backend and
ModelConfig without exposing credentials.
In `@crates/libsy-llm-client/src/client.rs`:
- Around line 491-495: Replace the `.expect("buffered response")` calls in the
aggregate-response handling at crates/libsy-llm-client/src/client.rs lines
491-495 and 683-687 with explicit matching, asserting or handling the success
and failure cases directly in each test while preserving the existing successful
aggregate behavior.
- Around line 25-38: Update the header-filtering logic using RESERVED_HEADERS so
credential-bearing headers such as cookie and proxy-authorization are never
forwarded from caller metadata. Prefer an explicit allowlist for headers
eligible for forwarding; otherwise expand the denylist to cover all browser,
proxy, and authentication credential headers, and add tests covering the
forwarding policy.
In `@crates/libsy-llm-client/src/lib.rs`:
- Around line 14-19: Update the module-level documentation near the
dependency-boundary explanation to remove the speculative
future-refactor/project-management statement, while retaining the explanation of
the vendored context-overflow detection and crate dependencies.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 5051dedb-431a-4206-b555-88a8b1b832ea
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock,!Cargo.lock
📒 Files selected for processing (8)
Cargo.tomlcrates/libsy-llm-client/Cargo.tomlcrates/libsy-llm-client/README.mdcrates/libsy-llm-client/src/backend.rscrates/libsy-llm-client/src/client.rscrates/libsy-llm-client/src/error.rscrates/libsy-llm-client/src/lib.rscrates/libsy-llm-client/src/raw.rs
| An HTTP client that speaks Switchyard's neutral IR directly. You hand it a | ||
| [`libsy_protocol::Request`] and a model name; it looks up the configured backend, | ||
| encodes the request to that backend's wire format, adds auth and forwards your | ||
| headers, makes the call with a shared `reqwest::Client`, and decodes the reply | ||
| back into a [`libsy_protocol::Response`] — buffered or streamed. | ||
|
|
||
| It depends only on `libsy-protocol` and `switchyard-translation`; no server, no | ||
| provider SDK. For an HTTP front door built on top of it, see [`libsy-proxy`]. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Update the README to use the API shipped by this crate.
The examples target a different interface: LlmModelClient, libsy_llm_client, nested backend maps, call(request, model, format), formats_for, and aggregate() are not provided here. Rewrite them using switchyard_llm_client::{ModelConfig, TranslatingLlmClient}, TranslatingLlmClient::new, and call_rewrite_model; also remove the now-unused LlmModelClient link definition.
Also applies to: 47-65, 71-90, 97-121, 124-141, 153-174
🤖 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/README.md` around lines 8 - 15, Rewrite all README
examples to use the crate’s actual API: import ModelConfig and
TranslatingLlmClient from switchyard_llm_client, construct the client with
TranslatingLlmClient::new, and invoke call_rewrite_model instead of the
unsupported LlmModelClient, libsy_llm_client, nested backend maps, call,
formats_for, or aggregate interfaces. Remove the unused LlmModelClient link
definition while preserving the examples’ intended behavior.
Source: Linters/SAST tools
| /// Shared HTTP configuration for one upstream backend. | ||
| #[derive(Clone, Debug)] | ||
| pub struct HttpBackendConfig { | ||
| /// Base URL of the provider API (e.g. `https://api.openai.com/v1`). | ||
| pub base_url: String, | ||
| /// API key for the provider, loaded by the caller. `None` sends no auth. | ||
| pub api_key: Option<String>, | ||
| /// Static headers added to every outbound call to this backend. | ||
| pub extra_headers: BTreeMap<String, String>, |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Redact API keys from Debug output.
The derived implementation prints api_key verbatim, so logging HttpBackendConfig, Backend, or ModelConfig can disclose provider credentials. Implement a redacted Debug representation or remove Debug from the containing public types.
🛡️ Redact the field
-#[derive(Clone, Debug)]
+#[derive(Clone)]
pub struct HttpBackendConfig {impl std::fmt::Debug for HttpBackendConfig {
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
f.debug_struct("HttpBackendConfig")
.field("base_url", &self.base_url)
.field("api_key", &self.api_key.as_ref().map(|_| "[REDACTED]"))
.field("extra_headers", &self.extra_headers)
.finish()
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /// Shared HTTP configuration for one upstream backend. | |
| #[derive(Clone, Debug)] | |
| pub struct HttpBackendConfig { | |
| /// Base URL of the provider API (e.g. `https://api.openai.com/v1`). | |
| pub base_url: String, | |
| /// API key for the provider, loaded by the caller. `None` sends no auth. | |
| pub api_key: Option<String>, | |
| /// Static headers added to every outbound call to this backend. | |
| pub extra_headers: BTreeMap<String, String>, | |
| /// Shared HTTP configuration for one upstream backend. | |
| #[derive(Clone)] | |
| pub struct HttpBackendConfig { | |
| /// Base URL of the provider API (e.g. `https://api.openai.com/v1`). | |
| pub base_url: String, | |
| /// API key for the provider, loaded by the caller. `None` sends no auth. | |
| pub api_key: Option<String>, | |
| /// Static headers added to every outbound call to this backend. | |
| pub extra_headers: BTreeMap<String, String>, | |
| } | |
| impl std::fmt::Debug for HttpBackendConfig { | |
| fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { | |
| f.debug_struct("HttpBackendConfig") | |
| .field("base_url", &self.base_url) | |
| .field("api_key", &self.api_key.as_ref().map(|_| "[REDACTED]")) | |
| .field("extra_headers", &self.extra_headers) | |
| .finish() | |
| } | |
| } |
🤖 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/backend.rs` around lines 33 - 41, Replace the
derived Debug implementation on HttpBackendConfig with a manual implementation
that preserves base_url and extra_headers while rendering any present api_key as
“[REDACTED]” rather than its value. Keep Debug available for containing types
such as Backend and ModelConfig without exposing credentials.
| // Headers this client owns or that are hop-by-hop; never forwarded from the | ||
| // caller's metadata. Auth/version/content-type are set by the backend or the | ||
| // JSON body, so a forwarded copy would either be ignored or conflict. Compared | ||
| // case-insensitively. Aligns with `_SENSITIVE_HEADERS` in the Python | ||
| // `switchyard/lib/request_metadata.py` forwarding logic. | ||
| const RESERVED_HEADERS: &[&str] = &[ | ||
| "host", | ||
| "content-length", | ||
| "connection", | ||
| "authorization", | ||
| "x-api-key", | ||
| "anthropic-version", | ||
| "content-type", | ||
| ]; |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Do not forward credential-bearing caller headers by default.
The denylist still permits headers such as cookie and proxy-authorization, potentially leaking session or proxy credentials to an external backend. Prefer an explicit forwarding allowlist, or at minimum reject all credential-bearing browser/proxy headers and test the policy.
🛡️ Minimal hardening
const RESERVED_HEADERS: &[&str] = &[
"host",
"content-length",
"connection",
"authorization",
+ "proxy-authorization",
+ "proxy-authenticate",
+ "cookie",
+ "set-cookie",
"x-api-key",
"anthropic-version",
"content-type",
];Also applies to: 294-307
🤖 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 25 - 38, Update the
header-filtering logic using RESERVED_HEADERS so credential-bearing headers such
as cookie and proxy-authorization are never forwarded from caller metadata.
Prefer an explicit allowlist for headers eligible for forwarding; otherwise
expand the denylist to cover all browser, proxy, and authentication credential
headers, and add tests covering the forwarding policy.
| let agg = response | ||
| .llm_response | ||
| .into_agg() | ||
| .await | ||
| .expect("buffered response"); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Replace .expect() in these tests with explicit matching.
crates/libsy-llm-client/src/client.rs#L491-L495: explicitly match the buffered aggregate result.crates/libsy-llm-client/src/client.rs#L683-L687: apply the same explicit matching pattern.
As per coding guidelines, “Do not use .expect() in Rust source or tests; propagate errors with ?, return typed errors, or match explicitly in tests.”
📍 Affects 1 file
crates/libsy-llm-client/src/client.rs#L491-L495(this comment)crates/libsy-llm-client/src/client.rs#L683-L687
🤖 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 491 - 495, Replace the
`.expect("buffered response")` calls in the aggregate-response handling at
crates/libsy-llm-client/src/client.rs lines 491-495 and 683-687 with explicit
matching, asserting or handling the success and failure cases directly in each
test while preserving the existing successful aggregate behavior.
Source: Coding guidelines
| //! The crate depends only on `libsy-protocol` and `switchyard-translation`. Neutral | ||
| //! IR encode/decode — including SSE stream decoding — lives in | ||
| //! `switchyard-translation`; this crate is the HTTP transport around it. The | ||
| //! context-overflow detection in [`mod@error`] is vendored from | ||
| //! `switchyard-components` (whose copy is crate-private and unavailable here); a | ||
| //! future refactor could promote it to a shared location. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Remove the project-management note from the module documentation.
Keep the dependency-boundary explanation, but omit the speculative “future refactor” statement.
As per coding guidelines, “Source comments must explain code behavior…; do not include project-management references such as tracker steps, issue IDs, or plan links.”
🤖 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/lib.rs` around lines 14 - 19, Update the
module-level documentation near the dependency-boundary explanation to remove
the speculative future-refactor/project-management statement, while retaining
the explanation of the vendored context-overflow detection and crate
dependencies.
Source: Coding guidelines
2 participants
Summary
Adds
switchyard-llm-client— an HTTP LLM client that speaks Switchyard's neutral IRend-to-end, built on the protocol traits and translation helpers from the base branches.
Base:
translation-helpersChanges
crates/libsy-llm-client/(packageswitchyard-llm-client):TranslatingLlmClient— maps a model + wire format to a per-modelBackend(built froma list of
ModelConfigs), encodes the request, makes the HTTP call (auth + caller-headerforwarding), and decodes the response — buffered JSON or SSE — back to the IR.
RoutedLlmClient, so it can back alibsyLlmTargetdirectly.call_rewrite_model_raw(ctx, raw_http_request, http_headers, model, wire_format)— thewhole decode → call → encode path in one call, returning a buffered JSON body or an
unframed stream of wire events (
RawResponse); SSE framing stays a transport concern, sothe crate has no HTTP-server dependency.
Cargo.toml— registers the crate as a workspace member.Notes
All dependencies were already in place on the base —
switchyard-protocolprovides theRoutedLlmClient/Decision/Contextvocabulary andswitchyard-translationthe neutral-IRencode/decode + SSE helpers — so no other changes were needed.
Validation
Full workspace builds clean, clippy at zero, the crate's 27 tests pass (buffered/streaming
round-trips, header forwarding, the
RoutedLlmClientimpl, andcall_rewrite_model_raw).Checklist
snake_caseof the primary class.switchyard/__init__.py.__all__if intended for downstream use.--helpupdated if customer-facing surface changed.Signed-off-by: Your Name <email>) per the DCO.Notes for reviewers
Anything reviewers should pay extra attention to — risky paths, follow-up tickets, intentional trade-offs.
Summary by CodeRabbit