Skip to content

Commit 8dbd7de

Browse files
fix: apply CodeRabbit inline findings (5 issues)
- prompt_event: add #[serde(default)] to PromptPayload.content so legacy Bridge messages without the field deserialize as empty Vec; add regression test for the legacy payload format - prompt_converter: ToolCallFinished status now treats exit_code=None + signal=None as Completed (covers built-in/MCP tools that don't set exit_code), not just exit_code=Some(0) - agent_loop: run() and run_chat() now apply thinking_budget the same way run_chat_streaming() does — serialize to Value then inject the thinking block before sending, preventing divergence when thinking is enabled - trogon-mcp: sanitize MCP server URLs before debug-logging them to strip userinfo, path, query and fragment (log only scheme+host) - trogon-nats: add structured match arm for Event::ServerError(ServerError::AuthorizationViolation) before the ClientError substring-match fallback; add corresponding unit test Signed-off-by: Jorge Gonzalez <jgonzalez@straw-hat.co> Signed-off-by: Jorge <jramirezhdez02@gmail.com>
1 parent 8efee2b commit 8dbd7de

5 files changed

Lines changed: 95 additions & 7 deletions

File tree

rsworkspace/crates/acp-nats/src/prompt_event.rs

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,7 @@ pub struct PromptPayload {
2929
pub session_id: String,
3030
/// Rich content blocks from the ACP prompt (text, images, resources).
3131
/// Always populated by current Bridge versions.
32+
#[serde(default)]
3233
pub content: Vec<UserContentBlock>,
3334
/// Plain-text fallback for backward compatibility.
3435
/// Used only when `content` is empty (old Bridge versions).
@@ -92,6 +93,16 @@ pub enum PromptEvent {
9293
mod tests {
9394
use super::*;
9495

96+
/// Legacy Bridge messages omit the `content` field; `#[serde(default)]` must
97+
/// deserialize them as an empty Vec instead of returning an error.
98+
#[test]
99+
fn prompt_payload_legacy_without_content_deserializes() {
100+
let legacy = r#"{"req_id":"r1","session_id":"s1","user_message":"hello"}"#;
101+
let p: PromptPayload = serde_json::from_str(legacy).unwrap();
102+
assert!(p.content.is_empty());
103+
assert_eq!(p.user_message, "hello");
104+
}
105+
95106
#[test]
96107
fn prompt_payload_roundtrip() {
97108
let p = PromptPayload {

rsworkspace/crates/trogon-acp-runner/src/prompt_converter.rs

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -173,7 +173,9 @@ impl PromptEventConverter {
173173
return (vec![], None);
174174
}
175175

176-
let status = if exit_code == Some(0) && signal.is_none() {
176+
let status = if exit_code == Some(0)
177+
|| (exit_code.is_none() && signal.is_none())
178+
{
177179
ToolCallStatus::Completed
178180
} else {
179181
ToolCallStatus::Failed

rsworkspace/crates/trogon-agent-core/src/agent_loop.rs

Lines changed: 24 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -335,6 +335,17 @@ impl AgentLoop {
335335
messages: &messages,
336336
};
337337

338+
let mut body =
339+
serde_json::to_value(&request).expect("request serialization is infallible");
340+
if let Some(budget) = self.thinking_budget
341+
&& budget > 0
342+
{
343+
body["thinking"] = serde_json::json!({
344+
"type": "enabled",
345+
"budget_tokens": budget
346+
});
347+
}
348+
338349
let mut req_builder = self
339350
.http_client
340351
.post(self.messages_url())
@@ -344,7 +355,7 @@ impl AgentLoop {
344355
req_builder = req_builder.header(k.as_str(), v.as_str());
345356
}
346357
let response = req_builder
347-
.json(&request)
358+
.json(&body)
348359
.send()
349360
.await
350361
.map_err(AgentError::Http)?
@@ -434,6 +445,17 @@ impl AgentLoop {
434445
messages: &messages,
435446
};
436447

448+
let mut body =
449+
serde_json::to_value(&request).expect("request serialization is infallible");
450+
if let Some(budget) = self.thinking_budget
451+
&& budget > 0
452+
{
453+
body["thinking"] = serde_json::json!({
454+
"type": "enabled",
455+
"budget_tokens": budget
456+
});
457+
}
458+
437459
let mut req_builder = self
438460
.http_client
439461
.post(self.messages_url())
@@ -443,7 +465,7 @@ impl AgentLoop {
443465
req_builder = req_builder.header(k.as_str(), v.as_str());
444466
}
445467
let response = req_builder
446-
.json(&request)
468+
.json(&body)
447469
.send()
448470
.await
449471
.map_err(AgentError::Http)?

rsworkspace/crates/trogon-mcp/src/client.rs

Lines changed: 23 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,27 @@ fn next_id() -> u64 {
1414
REQUEST_ID.fetch_add(1, Ordering::Relaxed)
1515
}
1616

17+
/// Return `scheme://host[:port]` from `url`, stripping userinfo, path, query, and fragment.
18+
/// Falls back to the original string if parsing fails.
19+
fn safe_url(url: &str) -> String {
20+
// Locate "://" to split scheme from the rest.
21+
let Some(scheme_end) = url.find("://") else {
22+
return url.to_string();
23+
};
24+
let scheme = &url[..scheme_end];
25+
let after_scheme = &url[scheme_end + 3..];
26+
// Strip userinfo (user:pass@).
27+
let authority = match after_scheme.rfind('@') {
28+
Some(at) => &after_scheme[at + 1..],
29+
None => after_scheme,
30+
};
31+
// Keep only host[:port] — stop at first '/', '?', or '#'.
32+
let host_end = authority
33+
.find(['/', '?', '#'])
34+
.unwrap_or(authority.len());
35+
format!("{}://{}", scheme, &authority[..host_end])
36+
}
37+
1738
// ── Public types ──────────────────────────────────────────────────────────────
1839

1940
/// A tool advertised by an MCP server.
@@ -86,7 +107,7 @@ impl McpClient {
86107
if let Some(err) = resp.get("error") {
87108
return Err(format!("MCP initialize error: {err}"));
88109
}
89-
debug!(url = %self.url, "MCP server initialized");
110+
debug!(url = %safe_url(&self.url), "MCP server initialized");
90111
Ok(())
91112
}
92113

@@ -105,7 +126,7 @@ impl McpClient {
105126
}
106127
let result: ListToolsResult = serde_json::from_value(resp["result"].take())
107128
.map_err(|e| format!("MCP tools/list deserialize error: {e}"))?;
108-
debug!(url = %self.url, count = result.tools.len(), "MCP tools listed");
129+
debug!(url = %safe_url(&self.url), count = result.tools.len(), "MCP tools listed");
109130
Ok(result.tools)
110131
}
111132

rsworkspace/crates/trogon-nats/src/connect.rs

Lines changed: 34 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
use crate::auth::{NatsAuth, NatsConfig};
2-
use async_nats::{Client, ClientError, ConnectOptions, Event};
2+
use async_nats::{Client, ClientError, ConnectOptions, Event, ServerError};
33
use std::sync::{Arc, Mutex};
44
use std::time::Duration;
55
use tokio::sync::oneshot;
@@ -103,6 +103,7 @@ fn apply_reconnect_options(
103103
async move {
104104
let signal: Option<bool> = match &event {
105105
Event::Connected => Some(true),
106+
Event::ServerError(ServerError::AuthorizationViolation) => Some(false),
106107
Event::ClientError(ClientError::Other(msg))
107108
if msg.contains("authorization violation") =>
108109
{
@@ -384,7 +385,7 @@ mod tests {
384385
}
385386

386387
/// When `Event::ClientError(ClientError::Other("authorization violation"))` fires,
387-
/// the outcome sender receives `false`.
388+
/// the outcome sender receives `false` (unstructured fallback path).
388389
#[cfg_attr(coverage, coverage(off))]
389390
#[tokio::test]
390391
async fn apply_reconnect_options_signals_auth_violation() {
@@ -395,6 +396,7 @@ mod tests {
395396
let event = Event::ClientError(ClientError::Other("authorization violation".to_string()));
396397
let signal: Option<bool> = match &event {
397398
Event::Connected => Some(true),
399+
Event::ServerError(ServerError::AuthorizationViolation) => Some(false),
398400
Event::ClientError(ClientError::Other(msg))
399401
if msg.contains("authorization violation") =>
400402
{
@@ -413,6 +415,36 @@ mod tests {
413415
assert!(!result, "authorization violation should send false");
414416
}
415417

418+
/// When `Event::ServerError(ServerError::AuthorizationViolation)` fires (structured variant),
419+
/// the outcome sender receives `false`.
420+
#[cfg_attr(coverage, coverage(off))]
421+
#[tokio::test]
422+
async fn apply_reconnect_options_signals_server_auth_violation() {
423+
let (tx, rx) = oneshot::channel::<bool>();
424+
let tx_arc = Arc::new(Mutex::new(Some(tx)));
425+
426+
let event = Event::ServerError(ServerError::AuthorizationViolation);
427+
let signal: Option<bool> = match &event {
428+
Event::Connected => Some(true),
429+
Event::ServerError(ServerError::AuthorizationViolation) => Some(false),
430+
Event::ClientError(ClientError::Other(msg))
431+
if msg.contains("authorization violation") =>
432+
{
433+
Some(false)
434+
}
435+
_ => None,
436+
};
437+
if let Some(ok) = signal
438+
&& let Ok(mut guard) = tx_arc.lock()
439+
&& let Some(sender) = guard.take()
440+
{
441+
let _ = sender.send(ok);
442+
}
443+
444+
let result = rx.await.expect("sender must have fired");
445+
assert!(!result, "ServerError::AuthorizationViolation should send false");
446+
}
447+
416448
/// Covers the `Err(_)` arm in the `select!` inside `connect()`:
417449
/// when the outcome sender is dropped before sending, the receiver
418450
/// returns `Err(RecvError)` and the connect() function continues normally.

0 commit comments

Comments
 (0)