feat(acp-agent-llm): add model-agnostic LLM adapter crate#109
Draft
tsitsiridakisiosif wants to merge 1 commit intoTrogonStack:mainfrom
Draft
feat(acp-agent-llm): add model-agnostic LLM adapter crate#109tsitsiridakisiosif wants to merge 1 commit intoTrogonStack:mainfrom
tsitsiridakisiosif wants to merge 1 commit intoTrogonStack:mainfrom
Conversation
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: itsitsiridakis <iosif.tsitsiridakis@fanatics.live>
PR SummaryMedium Risk Overview Adds CLI/env-based configuration ( Reviewed by Cursor Bugbot for commit ed7a11a. Bugbot is set up for automated code reviews on this repo. Configure here. |
WalkthroughThis pull request introduces a new LLM agent crate ( Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI Args & Env
participant Config as Config Builder
participant Telemetry as Telemetry
participant NATS as NATS Connection
participant Provider as Provider Instance
participant Model as Language Model
participant LLM_API as External LLM API
CLI->>Config: Parse args + environment variables
Config->>Config: Resolve provider, model, API keys
Config-->>Telemetry: LlmConfig (with ACP prefix)
Telemetry->>Telemetry: Initialize logger & metrics
Telemetry->>NATS: Connect with timeout
NATS-->>Provider: Ready to manage providers
Provider->>Provider: Create Anthropic/OpenAI instance
Provider->>LLM_API: fetch_models() - GET /v1/models
LLM_API-->>Provider: Model list + capabilities
Provider->>Provider: Store available models
Provider-->>Model: Model initialized with capabilities
Model-->>Telemetry: Metrics ready for streaming
Note over Model,LLM_API: Ready for completion requests
Model->>LLM_API: stream_completion (messages + stream: true)
LLM_API-->>Model: SSE text deltas
Model->>Model: Parse streaming events
Model-->>Telemetry: Emit text/stop/usage events
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
Author
|
![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
🧹 Nitpick comments (10)
devops/docker/compose/.env.example (1)
60-60: Consider documentingLLM_BASE_URLformat explicitly.A concrete example (e.g.,
https://api.anthropic.com) helps prevent misconfiguration for values without scheme.✏️ Suggested doc tweak
-# LLM_BASE_URL= +# LLM_BASE_URL=https://api.anthropic.com🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@devops/docker/compose/.env.example` at line 60, Update the .env.example entry for LLM_BASE_URL to explicitly document the expected URL format and include a concrete example; e.g., change the comment for LLM_BASE_URL to specify it must include the scheme (https://) and host (for example: https://api.anthropic.com) so users don’t omit the scheme or provide an invalid value.rsworkspace/crates/acp-agent-llm/src/session_store.rs (3)
12-17: Consider implementingDefaultforSessionStore.Since
new()creates an empty store with no parameters, implementingDefaultis idiomatic and enablesSessionStore::default().♻️ Suggested addition
+impl Default for SessionStore { + fn default() -> Self { + Self::new() + } +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rsworkspace/crates/acp-agent-llm/src/session_store.rs` around lines 12 - 17, Add a Default implementation for SessionStore so callers can use SessionStore::default(); implement std::default::Default for SessionStore and have fn default() -> Self construct the same state as SessionStore::new() (i.e. sessions: RefCell::new(HashMap::new())), or simply call Self::new() from default; ensure the impl is in the same module as the SessionStore struct so it finds the RefCell<HashMap<...>> field.
9-9: Consider aSessionIdvalue object for type safety.The store uses raw
Stringfor session IDs. For consistency with other value objects in this crate (ModelId,ProviderName,ApiKey), consider introducing aSessionIdtype with validation.As per coding guidelines: "Prefer domain-specific value objects over primitives."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rsworkspace/crates/acp-agent-llm/src/session_store.rs` at line 9, The sessions field currently uses raw String keys; introduce a domain-specific SessionId value object and swap the HashMap key type from String to SessionId to improve type safety. Create a SessionId struct (newtype around String) with parsing/validation logic (implement FromStr with proper validation), traits needed by the map (Eq, PartialEq, Hash, Clone), Display/AsRef<str> and serde Serialize/Deserialize as required, then update the sessions field signature (sessions: RefCell<HashMap<SessionId, Session>>) and all functions/methods that construct, accept, or return session IDs (look for usages referencing the sessions HashMap and the Session type) to use SessionId instead of String, adapting call sites to parse/format via SessionId::from_str or Display where necessary.
27-33: Document re-entrancy constraint onwith().The closure
fexecutes whileborrow_mut()is held. Iffcalls back intoSessionStoremethods on the same instance, it will panic at runtime. Consider adding a doc comment noting this constraint.📝 Suggested doc enhancement
/// Access a session mutably by ID. Returns `None` if not found. + /// + /// # Panics + /// The closure must not call other `SessionStore` methods on `self`, + /// as `RefCell` does not permit re-entrant borrows. pub fn with<F, R>(&self, id: &str, f: F) -> Option<R>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rsworkspace/crates/acp-agent-llm/src/session_store.rs` around lines 27 - 33, The with() method holds a RefMut on self.sessions while executing the closure, which means the closure must not call back into SessionStore methods that try to borrow self.sessions (it will panic); update the doc comment on pub fn with<F, R>(&self, id: &str, f: F) -> Option<R> to state this non-reentrancy constraint explicitly and give guidance (e.g., avoid calling other SessionStore methods from f, clone out needed data before calling back, or use patterns that release the borrow before re-entering) so callers know to not call back into the same SessionStore while the closure runs.rsworkspace/crates/acp-agent-llm/src/error.rs (1)
40-40:StreamParse(String)discards the original error context.Per coding guidelines, error context should not be discarded by converting typed errors into strings. If the stream parse failure originates from a deserializer (e.g.,
serde_json::Error), wrapping the source error preserves the chain for debugging.♻️ Suggested refactor to preserve error source
pub enum CompletionError { Http(reqwest::Error), Api { status: u16, message: String }, - StreamParse(String), + StreamParse(serde_json::Error), Cancelled, }Then update
Displayandsource():- Self::StreamParse(msg) => write!(f, "stream parse error: {msg}"), + Self::StreamParse(e) => write!(f, "stream parse error: {e}"),fn source(&self) -> Option<&(dyn std::error::Error + 'static)> { match self { Self::Http(e) => Some(e), + Self::StreamParse(e) => Some(e), _ => None, } }As per coding guidelines: "Never discard error context by converting a typed error into a string; wrap the source error as a field or variant instead."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rsworkspace/crates/acp-agent-llm/src/error.rs` at line 40, The StreamParse variant currently stores a String and loses the original error context—change the enum variant StreamParse to wrap the original error type (e.g., StreamParse(serde_json::Error) or a generic Box<dyn std::error::Error + Send + Sync>) instead of String, then update the Error impls (Display, source()) to forward the underlying error: make Display include a helpful message plus the wrapped error and have source() return Some(&wrapped_error) so the error chain is preserved; update any places constructing StreamParse to pass the original error (or use ?/From) rather than formatting it to a string.rsworkspace/crates/acp-agent-llm/src/provider/openai.rs (1)
80-82: Use a static empty slice instead of temporaryvec![].Same pattern as in the Anthropic provider - use a static empty slice for efficiency.
♻️ Suggested fix
+ const EMPTY: &[serde_json::Value] = &[]; self.models = body["data"] .as_array() - .unwrap_or(&vec![]) + .unwrap_or(EMPTY)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rsworkspace/crates/acp-agent-llm/src/provider/openai.rs` around lines 80 - 82, The code assigns self.models from body["data"].as_array().unwrap_or(&vec![]) which creates a temporary Vec reference; change this to avoid the temporary by either using cloning and unwrap_or_default (e.g. body["data"].as_array().cloned().unwrap_or_default()) or mapping-and-unwrapping to a new Vec (e.g. body["data"].as_array().map(|v| v.clone()).unwrap_or_default()), and ensure the target self.models is a Vec<Value> (or adjust types accordingly) so you use a permanent empty Vec instead of &vec![]; update the assignment in openai.rs where self.models is set.rsworkspace/crates/acp-agent-llm/src/provider/anthropic.rs (1)
79-82: Use a static empty slice instead of temporaryvec![].
unwrap_or(&vec![])creates a temporaryVecon each call whendatais missing. Use a static empty slice for efficiency.♻️ Suggested fix
- self.models = body["data"] - .as_array() - .unwrap_or(&vec![]) + const EMPTY: &[serde_json::Value] = &[]; + self.models = body["data"] + .as_array() + .unwrap_or(EMPTY)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rsworkspace/crates/acp-agent-llm/src/provider/anthropic.rs` around lines 79 - 82, The code calls body["data"].as_array().unwrap_or(&vec![]) which allocates a temporary Vec each time; replace that temporary with a static empty slice by using unwrap_or(&[]) so the expression becomes body["data"].as_array().unwrap_or(&[]).iter(); update the assignment to self.models accordingly in anthropic.rs (the line that sets self.models from body["data"] and uses unwrap_or).rsworkspace/crates/acp-agent-llm/src/config.rs (2)
102-109: Silent error discarding when API key validation fails.If
ApiKey::new()fails (e.g., empty or malformed key), the error is silently converted toNone. This could mask configuration issues where a user sets an invalid API key and expects it to be used.Consider logging a warning when an API key is present but invalid, so users understand why their provider wasn't configured.
Proposed approach
- let anthropic_key = env - .var("ANTHROPIC_API_KEY") - .ok() - .and_then(|k| ApiKey::new(k).ok()); + let anthropic_key = match env.var("ANTHROPIC_API_KEY") { + Ok(k) => match ApiKey::new(k) { + Ok(key) => Some(key), + Err(e) => { + tracing::warn!("ANTHROPIC_API_KEY is set but invalid: {e}"); + None + } + }, + Err(_) => None, + };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rsworkspace/crates/acp-agent-llm/src/config.rs` around lines 102 - 109, The current config silently drops invalid API keys because env.var(...).ok().and_then(|k| ApiKey::new(k).ok()) converts ApiKey::new failures to None; update the logic that builds anthropic_key and openai_key to detect when env.var(...) returns Some(key) but ApiKey::new(key) returns Err, and emit a warning (via whatever logger is used in this module) that the provided key was invalid and the provider will be skipped; keep the final Option<ApiKey> semantics (Some on valid, None otherwise) but ensure invalid-but-present values produce a warning instead of being silently ignored, referencing the ApiKey::new function and the anthropic_key/openai_key bindings to locate the change.
95-100:unreachable!()creates fragile coupling withProviderNamevalidation.If
ProviderNameis extended to allow additional providers in the future, this code will panic at runtime. Consider making this explicit by returning an error for unsupported providers in the default model selection, or add a TODO comment documenting the coupling.Safer alternative
None => match provider.as_str() { "anthropic" => ModelId::new("claude-sonnet-4-6").expect("known model"), "openai" => ModelId::new("gpt-4o").expect("known model"), - _ => unreachable!(), + other => { + // No default model known for this provider; require explicit --model + return Err(ConfigError::Model(crate::model_id::EmptyModelId)); + } },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rsworkspace/crates/acp-agent-llm/src/config.rs` around lines 95 - 100, The match fallback uses unreachable!() which will panic if ProviderName is later extended; update the default model selection to return an explicit error instead of panicking: replace the `_ => unreachable!()` arm with an Err variant (or propagate a custom error) that includes the unknown provider string, or add a clear TODO comment documenting the tight coupling; reference the match where provider.as_str() is used and the ModelId::new(...) arms so you change that match to return Result<ModelId, SomeError> (or propagate the existing function's error type) rather than calling unreachable!.rsworkspace/crates/acp-agent-llm/src/model/openai.rs (1)
122-175: Consider extracting shared SSE buffer parsing logic.The SSE line buffering pattern (lines 128-141) is duplicated between
AnthropicModelandOpenAiModel. While the event parsing differs, the buffer management could be shared.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rsworkspace/crates/acp-agent-llm/src/model/openai.rs` around lines 122 - 175, The SSE line-buffering loop used in OpenAiModel::parse_sse_buffer (and duplicated in AnthropicModel) should be extracted into a shared helper (e.g., drain_next_sse_line or iter_sse_lines) that takes &mut String and returns the next trimmed line or None when no full line is present; replace the while/find/strip_prefix logic in both parse_sse_buffer implementations with calls to that helper and keep the provider-specific JSON/event parsing in each model. Ensure the helper handles trimming, consuming the buffer up to the newline, and returns raw line strings (so parse_sse_buffer in OpenAiModel and AnthropicModel can still check "data: " and parse JSON).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@rsworkspace/crates/acp-agent-llm/src/base_url.rs`:
- Around line 22-30: BaseUrl::new currently only checks prefixes and allows
malformed values like "https://" — tighten validation by parsing and validating
the whole URL: in BaseUrl::new use a proper URL parser (e.g., url::Url::parse)
and ensure the scheme is "http" or "https", that a non-empty host is present,
and that there is at least one character beyond the scheme (reject bare
"http://"/"https://"); update/introduce a BaseUrlError variant (e.g., Invalid or
BadUrl) used when parsing/validation fails, and apply the same stricter checks
to the other constructor(s)/factory methods referenced around lines 63-67 so
invalid instances cannot be constructed.
In `@rsworkspace/crates/acp-agent-llm/src/main.rs`:
- Around line 73-76: Add a typed error variant for the "no providers configured"
case and return that instead of a raw string: add a variant like
NoProvidersConfigured to the LlmAgentError enum in error.rs, implement
Display/From as needed, then in main.rs replace the string error return when
providers.is_empty() with returning the typed error (e.g.
Err(LlmAgentError::NoProvidersConfigured.into())) and keep the existing error!
log (or log the enum value) so the code uses the new LlmAgentError variant
rather than a String.
In `@rsworkspace/crates/acp-agent-llm/src/model/anthropic.rs`:
- Around line 91-94: The code only captures the first system message by using
messages.iter().find() (the variable system) which silently drops additional
system messages; change this to collect and concatenate all messages with role
ChatRole::System (e.g., join their content with newlines or spaces) and assign
that combined string to system so all system instructions are preserved when
building the Anthropc request; alternatively, if you prefer strictness, replace
the find() logic with a check that returns an error when more than one
ChatRole::System message exists (use the same symbols: messages,
ChatRole::System, system) and update the caller behavior accordingly.
In `@rsworkspace/crates/acp-agent-llm/src/model/openai.rs`:
- Around line 70-74: The JSON request body built in openai.rs (the variable
`body` that currently contains "model": self.id.as_str(), "messages":
api_messages, "stream": true) must include stream options so the API returns
usage in the final stream chunk; add a "stream_options": { "include_usage": true
} field to the `body` object when constructing the request so token usage is
included for streaming responses.
In `@rsworkspace/crates/acp-agent-llm/src/provider/openai.rs`:
- Around line 86-87: The code currently hardcodes max_tokens = 128_000u64 in
openai.rs which will cause token-limit errors for smaller models; replace the
hardcoded default by deriving the model context limit: check the model metadata
returned by the OpenAI models/list call (or a local lookup table) and set
max_tokens from the model's context/window if present, otherwise fall back to a
conservative default (e.g., 16k) and document that per-model limits are
respected; update the logic around the max_tokens variable and any callers that
rely on it (referencing max_tokens and the model identifier string used when
creating requests) so the agent trims history based on the actual model limit.
---
Nitpick comments:
In `@devops/docker/compose/.env.example`:
- Line 60: Update the .env.example entry for LLM_BASE_URL to explicitly document
the expected URL format and include a concrete example; e.g., change the comment
for LLM_BASE_URL to specify it must include the scheme (https://) and host (for
example: https://api.anthropic.com) so users don’t omit the scheme or provide an
invalid value.
In `@rsworkspace/crates/acp-agent-llm/src/config.rs`:
- Around line 102-109: The current config silently drops invalid API keys
because env.var(...).ok().and_then(|k| ApiKey::new(k).ok()) converts ApiKey::new
failures to None; update the logic that builds anthropic_key and openai_key to
detect when env.var(...) returns Some(key) but ApiKey::new(key) returns Err, and
emit a warning (via whatever logger is used in this module) that the provided
key was invalid and the provider will be skipped; keep the final Option<ApiKey>
semantics (Some on valid, None otherwise) but ensure invalid-but-present values
produce a warning instead of being silently ignored, referencing the ApiKey::new
function and the anthropic_key/openai_key bindings to locate the change.
- Around line 95-100: The match fallback uses unreachable!() which will panic if
ProviderName is later extended; update the default model selection to return an
explicit error instead of panicking: replace the `_ => unreachable!()` arm with
an Err variant (or propagate a custom error) that includes the unknown provider
string, or add a clear TODO comment documenting the tight coupling; reference
the match where provider.as_str() is used and the ModelId::new(...) arms so you
change that match to return Result<ModelId, SomeError> (or propagate the
existing function's error type) rather than calling unreachable!.
In `@rsworkspace/crates/acp-agent-llm/src/error.rs`:
- Line 40: The StreamParse variant currently stores a String and loses the
original error context—change the enum variant StreamParse to wrap the original
error type (e.g., StreamParse(serde_json::Error) or a generic Box<dyn
std::error::Error + Send + Sync>) instead of String, then update the Error impls
(Display, source()) to forward the underlying error: make Display include a
helpful message plus the wrapped error and have source() return
Some(&wrapped_error) so the error chain is preserved; update any places
constructing StreamParse to pass the original error (or use ?/From) rather than
formatting it to a string.
In `@rsworkspace/crates/acp-agent-llm/src/model/openai.rs`:
- Around line 122-175: The SSE line-buffering loop used in
OpenAiModel::parse_sse_buffer (and duplicated in AnthropicModel) should be
extracted into a shared helper (e.g., drain_next_sse_line or iter_sse_lines)
that takes &mut String and returns the next trimmed line or None when no full
line is present; replace the while/find/strip_prefix logic in both
parse_sse_buffer implementations with calls to that helper and keep the
provider-specific JSON/event parsing in each model. Ensure the helper handles
trimming, consuming the buffer up to the newline, and returns raw line strings
(so parse_sse_buffer in OpenAiModel and AnthropicModel can still check "data: "
and parse JSON).
In `@rsworkspace/crates/acp-agent-llm/src/provider/anthropic.rs`:
- Around line 79-82: The code calls body["data"].as_array().unwrap_or(&vec![])
which allocates a temporary Vec each time; replace that temporary with a static
empty slice by using unwrap_or(&[]) so the expression becomes
body["data"].as_array().unwrap_or(&[]).iter(); update the assignment to
self.models accordingly in anthropic.rs (the line that sets self.models from
body["data"] and uses unwrap_or).
In `@rsworkspace/crates/acp-agent-llm/src/provider/openai.rs`:
- Around line 80-82: The code assigns self.models from
body["data"].as_array().unwrap_or(&vec![]) which creates a temporary Vec
reference; change this to avoid the temporary by either using cloning and
unwrap_or_default (e.g. body["data"].as_array().cloned().unwrap_or_default()) or
mapping-and-unwrapping to a new Vec (e.g. body["data"].as_array().map(|v|
v.clone()).unwrap_or_default()), and ensure the target self.models is a
Vec<Value> (or adjust types accordingly) so you use a permanent empty Vec
instead of &vec![]; update the assignment in openai.rs where self.models is set.
In `@rsworkspace/crates/acp-agent-llm/src/session_store.rs`:
- Around line 12-17: Add a Default implementation for SessionStore so callers
can use SessionStore::default(); implement std::default::Default for
SessionStore and have fn default() -> Self construct the same state as
SessionStore::new() (i.e. sessions: RefCell::new(HashMap::new())), or simply
call Self::new() from default; ensure the impl is in the same module as the
SessionStore struct so it finds the RefCell<HashMap<...>> field.
- Line 9: The sessions field currently uses raw String keys; introduce a
domain-specific SessionId value object and swap the HashMap key type from String
to SessionId to improve type safety. Create a SessionId struct (newtype around
String) with parsing/validation logic (implement FromStr with proper
validation), traits needed by the map (Eq, PartialEq, Hash, Clone),
Display/AsRef<str> and serde Serialize/Deserialize as required, then update the
sessions field signature (sessions: RefCell<HashMap<SessionId, Session>>) and
all functions/methods that construct, accept, or return session IDs (look for
usages referencing the sessions HashMap and the Session type) to use SessionId
instead of String, adapting call sites to parse/format via SessionId::from_str
or Display where necessary.
- Around line 27-33: The with() method holds a RefMut on self.sessions while
executing the closure, which means the closure must not call back into
SessionStore methods that try to borrow self.sessions (it will panic); update
the doc comment on pub fn with<F, R>(&self, id: &str, f: F) -> Option<R> to
state this non-reentrancy constraint explicitly and give guidance (e.g., avoid
calling other SessionStore methods from f, clone out needed data before calling
back, or use patterns that release the borrow before re-entering) so callers
know to not call back into the same SessionStore while the closure runs.
🪄 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: CHILL
Plan: Pro
Run ID: ff1e61c0-2aba-4b3f-a08e-b7e05e023c4e
⛔ Files ignored due to path filters (1)
rsworkspace/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (22)
.mise.tomldevops/docker/compose/.env.examplersworkspace/Cargo.tomlrsworkspace/crates/acp-agent-llm/Cargo.tomlrsworkspace/crates/acp-agent-llm/src/api_key.rsrsworkspace/crates/acp-agent-llm/src/base_url.rsrsworkspace/crates/acp-agent-llm/src/config.rsrsworkspace/crates/acp-agent-llm/src/error.rsrsworkspace/crates/acp-agent-llm/src/main.rsrsworkspace/crates/acp-agent-llm/src/model/anthropic.rsrsworkspace/crates/acp-agent-llm/src/model/mod.rsrsworkspace/crates/acp-agent-llm/src/model/openai.rsrsworkspace/crates/acp-agent-llm/src/model_id.rsrsworkspace/crates/acp-agent-llm/src/provider/anthropic.rsrsworkspace/crates/acp-agent-llm/src/provider/mod.rsrsworkspace/crates/acp-agent-llm/src/provider/openai.rsrsworkspace/crates/acp-agent-llm/src/provider_name.rsrsworkspace/crates/acp-agent-llm/src/session.rsrsworkspace/crates/acp-agent-llm/src/session_store.rsrsworkspace/crates/acp-agent-llm/src/telemetry/metrics.rsrsworkspace/crates/acp-agent-llm/src/telemetry/mod.rsrsworkspace/crates/acp-telemetry/src/service_name.rs
Comment on lines
+22
to
+30
| pub fn new(s: impl Into<String>) -> Result<Self, BaseUrlError> { | ||
| let s = s.into(); | ||
| if s.is_empty() { | ||
| return Err(BaseUrlError::Empty); | ||
| } | ||
| if !s.starts_with("http://") && !s.starts_with("https://") { | ||
| return Err(BaseUrlError::MissingScheme); | ||
| } | ||
| Ok(Self(s)) |
There was a problem hiding this comment.
Strengthen BaseUrl::new invariants to reject malformed URLs.
Line 27 only checks prefix, so malformed values (e.g., https://) can still be constructed as BaseUrl. That weakens the value-object guarantee and defers failure to runtime HTTP calls.
🔧 Suggested tightening
#[derive(Debug, Clone, PartialEq)]
pub enum BaseUrlError {
Empty,
MissingScheme,
+ MissingHost,
}
@@
impl BaseUrl {
pub fn new(s: impl Into<String>) -> Result<Self, BaseUrlError> {
- let s = s.into();
- if s.is_empty() {
+ let s = s.into();
+ let s = s.trim();
+ if s.is_empty() {
return Err(BaseUrlError::Empty);
}
- if !s.starts_with("http://") && !s.starts_with("https://") {
+ let rest = if let Some(rest) = s.strip_prefix("http://") {
+ rest
+ } else if let Some(rest) = s.strip_prefix("https://") {
+ rest
+ } else {
return Err(BaseUrlError::MissingScheme);
- }
- Ok(Self(s))
+ };
+ if rest.is_empty() || rest.starts_with('/') {
+ return Err(BaseUrlError::MissingHost);
+ }
+ Ok(Self(s.to_owned()))
}
@@
fn rejects_missing_scheme() {
assert_eq!(
BaseUrl::new("api.anthropic.com").unwrap_err(),
BaseUrlError::MissingScheme
);
}
+
+ #[test]
+ fn rejects_missing_host() {
+ assert_eq!(BaseUrl::new("https://").unwrap_err(), BaseUrlError::MissingHost);
+ }
}Also applies to: 63-67
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@rsworkspace/crates/acp-agent-llm/src/base_url.rs` around lines 22 - 30,
BaseUrl::new currently only checks prefixes and allows malformed values like
"https://" — tighten validation by parsing and validating the whole URL: in
BaseUrl::new use a proper URL parser (e.g., url::Url::parse) and ensure the
scheme is "http" or "https", that a non-empty host is present, and that there is
at least one character beyond the scheme (reject bare "http://"/"https://");
update/introduce a BaseUrlError variant (e.g., Invalid or BadUrl) used when
parsing/validation fails, and apply the same stricter checks to the other
constructor(s)/factory methods referenced around lines 63-67 so invalid
instances cannot be constructed.
Comment on lines
+73
to
+76
| if providers.is_empty() { | ||
| error!("No API keys configured. Set ANTHROPIC_API_KEY or OPENAI_API_KEY."); | ||
| return Err("no providers configured".into()); | ||
| } |
There was a problem hiding this comment.
Use a typed error instead of a string literal.
The coding guidelines require typed errors—use structs or enums, never String or format!(). The string "no providers configured" should be a variant of LlmAgentError or a dedicated error type.
Proposed fix
In error.rs, add a variant for this case, then use it here:
- if providers.is_empty() {
- error!("No API keys configured. Set ANTHROPIC_API_KEY or OPENAI_API_KEY.");
- return Err("no providers configured".into());
- }
+ if providers.is_empty() {
+ error!("No API keys configured. Set ANTHROPIC_API_KEY or OPENAI_API_KEY.");
+ return Err(Box::new(crate::error::LlmAgentError::NoProvidersConfigured));
+ }As per coding guidelines: "Errors must be typed—use structs or enums, never String or format!()."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@rsworkspace/crates/acp-agent-llm/src/main.rs` around lines 73 - 76, Add a
typed error variant for the "no providers configured" case and return that
instead of a raw string: add a variant like NoProvidersConfigured to the
LlmAgentError enum in error.rs, implement Display/From as needed, then in
main.rs replace the string error return when providers.is_empty() with returning
the typed error (e.g. Err(LlmAgentError::NoProvidersConfigured.into())) and keep
the existing error! log (or log the enum value) so the code uses the new
LlmAgentError variant rather than a String.
Comment on lines
+91
to
+94
| let system = messages | ||
| .iter() | ||
| .find(|m| m.role == ChatRole::System) | ||
| .map(|m| m.content.as_str()); |
There was a problem hiding this comment.
Only the first system message is used; subsequent ones are silently ignored.
The code uses .find() which returns only the first system message. If multiple system messages exist in the conversation, the rest are filtered out (line 78) and never sent. This silent data loss could cause unexpected behavior.
Consider either:
- Concatenating all system messages
- Returning an error if multiple system messages are provided
- Documenting this behavior explicitly
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@rsworkspace/crates/acp-agent-llm/src/model/anthropic.rs` around lines 91 -
94, The code only captures the first system message by using
messages.iter().find() (the variable system) which silently drops additional
system messages; change this to collect and concatenate all messages with role
ChatRole::System (e.g., join their content with newlines or spaces) and assign
that combined string to system so all system instructions are preserved when
building the Anthropc request; alternatively, if you prefer strictness, replace
the find() logic with a check that returns an error when more than one
ChatRole::System message exists (use the same symbols: messages,
ChatRole::System, system) and update the caller behavior accordingly.
Comment on lines
+70
to
+74
| let body = serde_json::json!({ | ||
| "model": self.id.as_str(), | ||
| "messages": api_messages, | ||
| "stream": true, | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
OpenAI chat completions API stream_options include_usage parameter
💡 Result:
stream_options: {"include_usage": true} is an option you can add to a streaming Chat Completions request to get token usage back when streaming.
- With streaming, usage data isn’t otherwise included; setting
include_usage: truecauses the API to send an extra final streamed chunk that contains the overallusagefor the request. In that final chunk,choicesmay be empty. [1] - OpenAI documents this as the way to “access usage data” while streaming. [2]
- This parameter was added to the Chat Completions (and Completions) APIs in May 2024. [3]
Example (request body):
{
"model": "gpt-4.1-mini",
"messages": [{"role": "user", "content": "Hello"}],
"stream": true,
"stream_options": { "include_usage": true }
}Sources: [1] [2] [3]
🏁 Script executed:
cd rsworkspace/crates/acp-agent-llm && sed -n '140,180p' src/model/openai.rsRepository: TrogonStack/trogonai
Length of output: 1891
Add stream_options to enable token usage reporting in streaming responses.
OpenAI's streaming Chat Completions API only includes the usage object in the final chunk when stream_options: { include_usage: true } is set in the request. Without this parameter, usage data is not sent and token counts cannot be reported.
Proposed fix
let body = serde_json::json!({
"model": self.id.as_str(),
"messages": api_messages,
"stream": true,
+ "stream_options": { "include_usage": true },
});📝 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.
Suggested change
| let body = serde_json::json!({ | |
| "model": self.id.as_str(), | |
| "messages": api_messages, | |
| "stream": true, | |
| }); | |
| let body = serde_json::json!({ | |
| "model": self.id.as_str(), | |
| "messages": api_messages, | |
| "stream": true, | |
| "stream_options": { "include_usage": true }, | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@rsworkspace/crates/acp-agent-llm/src/model/openai.rs` around lines 70 - 74,
The JSON request body built in openai.rs (the variable `body` that currently
contains "model": self.id.as_str(), "messages": api_messages, "stream": true)
must include stream options so the API returns usage in the final stream chunk;
add a "stream_options": { "include_usage": true } field to the `body` object
when constructing the request so token usage is included for streaming
responses.
Comment on lines
+86
to
+87
| // Default to 128k context — OpenAI doesn't expose this in the list endpoint | ||
| let max_tokens = 128_000u64; |
There was a problem hiding this comment.
Hardcoded 128k context may cause issues with smaller models.
OpenAI's model roster includes models with varying context limits (e.g., gpt-3.5-turbo has 16k). Using 128k uniformly could lead to token-limit errors when the agent sends full history to a smaller-context model.
Since the PR summary notes "context optimization (token counting and history trimming)" as a next step, this may be acceptable for now, but consider tracking this limitation.
Would you like me to open an issue to track per-model context limits, or suggest a lookup table for known models?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@rsworkspace/crates/acp-agent-llm/src/provider/openai.rs` around lines 86 -
87, The code currently hardcodes max_tokens = 128_000u64 in openai.rs which will
cause token-limit errors for smaller models; replace the hardcoded default by
deriving the model context limit: check the model metadata returned by the
OpenAI models/list call (or a local lookup table) and set max_tokens from the
model's context/window if present, otherwise fall back to a conservative default
(e.g., 16k) and document that per-model limits are respected; update the logic
around the max_tokens variable and any callers that rely on it (referencing
max_tokens and the model identifier string used when creating requests) so the
agent trims history based on the actual model limit.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 3 potential issues.
Reviewed by Cursor Bugbot for commit ed7a11a. Configure here.
| output_tokens: output, | ||
| }) | ||
| .await; | ||
| } |
There was a problem hiding this comment.
Anthropic usage event never emitted during streaming
Medium Severity
The Anthropic message_delta SSE event only contains output_tokens in its usage field — input_tokens is reported exclusively in the message_start event. Because the tuple match requires both input_tokens and output_tokens to be Some, the CompletionEvent::Usage is never emitted. The input_tokens lookup will always return None for message_delta events, silently dropping all token accounting data.
Reviewed by Cursor Bugbot for commit ed7a11a. Configure here.
| "model": self.id.as_str(), | ||
| "messages": api_messages, | ||
| "stream": true, | ||
| }); |
There was a problem hiding this comment.
OpenAI streaming never returns usage without stream_options
Medium Severity
The OpenAI request body sets "stream": true but does not include "stream_options": {"include_usage": true}. OpenAI's streaming API only sends a usage chunk in the final SSE event when this option is explicitly enabled. Without it, the usage-parsing code at lines 162–173 is dead code — CompletionEvent::Usage is never emitted for OpenAI completions, and token accounting silently fails.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit ed7a11a. Configure here.
| ); | ||
| } | ||
| if let Some(key) = config.openai_api_key { | ||
| let mut p = provider::openai::OpenAiProvider::new(key, config.base_url.clone()); |
There was a problem hiding this comment.
Single base URL shared breaks multi-provider setups
Medium Severity
Both AnthropicProvider and OpenAiProvider receive the same config.base_url.clone() value. When LLM_BASE_URL is set, it overrides the base URL for both providers simultaneously. Since Anthropic and OpenAI use completely different API endpoints, setting this env var causes one (or both) providers to make requests against the wrong API, resulting in HTTP errors at startup during fetch_models.
Reviewed by Cursor Bugbot for commit ed7a11a. Configure here.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds
acp-agent-llm, a model-agnostic LLM adapter crate that integrates AI models (Anthropic, OpenAI) into the ACP-over-NATS infrastructure.What's included
LanguageModelProvidermanages auth and model registry,LanguageModelhandles streaming completions with capability flags (supports_tools,supports_images,supports_thinking)GET /v1/models) — no hardcoded model liststokio::sync::watchText,Stop,Usage(token accounting) streamed throughmpscchannelsRefCell<HashMap>session store with conversation history, model switching (set_session_model), and session forking.envfile (ANTHROPIC_API_KEY,OPENAI_API_KEY,LLM_PROVIDER,LLM_MODEL), CLI args,mise run:agenttaskProviderName,ModelId,ApiKey(redacted in logs),BaseUrl— all validated at constructionCompletionError,LlmAgentErrorwith properDisplay/ErrorimplsWhat's NOT included (next steps)
LlmAgent— ACPAgenttrait implementation: the struct that wires prompt handling → model completion → NATS notification streaming. All building blocks are in place; this is the final integration piece.model.max_token_count()and history trimming/summarization before API calls. Currently sends full conversation history — API errors if token limit exceeded. Code comment marks where this belongs.Files changed
rsworkspace/Cargo.toml— addedreqwestworkspace dep,acp-nats-agentinternal deprsworkspace/crates/acp-telemetry/src/service_name.rs— addedAcpAgentLlmvariantdevops/docker/compose/.env.example— added LLM agent env vars.mise.toml— addedrun:agenttaskrsworkspace/crates/acp-agent-llm/— new crate (18 source files)