diff --git a/Cargo.lock b/Cargo.lock index 2014aa6..0e252a1 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -6141,6 +6141,7 @@ dependencies = [ "futures-util", "indexmap", "kigi-sampling-types", + "kigi-secrets", "kigi-test-support", "kigi-version", "reqwest 0.12.28", @@ -6294,6 +6295,7 @@ dependencies = [ "kigi-sampler", "kigi-sampling-types", "kigi-sandbox", + "kigi-secrets", "kigi-shared", "kigi-shell-base", "kigi-sqlite-journal", diff --git a/crates/codegen/kigi-sampler/Cargo.toml b/crates/codegen/kigi-sampler/Cargo.toml index cd19866..86c1359 100644 --- a/crates/codegen/kigi-sampler/Cargo.toml +++ b/crates/codegen/kigi-sampler/Cargo.toml @@ -18,6 +18,7 @@ eventsource-stream = { workspace = true } futures-util = { workspace = true } indexmap = { workspace = true, features = ["serde"] } reqwest = { workspace = true, features = ["stream"] } +kigi-secrets = { path = "../kigi-secrets" } serde = { workspace = true, features = ["derive"] } serde_json = { workspace = true } tokio = { workspace = true, features = ["rt", "macros", "time", "sync"] } diff --git a/crates/codegen/kigi-sampler/src/client.rs b/crates/codegen/kigi-sampler/src/client.rs index 584f5c4..88d1db5 100644 --- a/crates/codegen/kigi-sampler/src/client.rs +++ b/crates/codegen/kigi-sampler/src/client.rs @@ -193,6 +193,16 @@ fn extract_context_total(value: &serde_json::Value) -> Option { Some(i.saturating_add(o)) } +/// The endpoint as the HTTP client parses it, minus userinfo, sensitive query values and fragment; an unparseable one is not echoed. +fn redacted_endpoint(endpoint: &str) -> String { + match reqwest::Url::parse(endpoint) { + Ok(mut url) => { + kigi_secrets::redact_url(&mut url); + url.to_string() + } + Err(_) => "".to_owned(), + } +} /// Record `success=false` + `error` on the active inference span when a stream /// request fails before any response (transport/connect/TLS errors). Without /// this the `#[instrument]` span closes with both fields Empty, so an outage @@ -806,7 +816,7 @@ impl SamplingClient { let server_message_lower = server_message.to_lowercase(); let mut context_parts = vec![server_message.to_string()]; - context_parts.push(format!("\nRequest URL: {}", endpoint)); + context_parts.push(format!("\nRequest URL: {}", redacted_endpoint(endpoint))); // Show headers if error mentions headers if server_message_lower.contains("header") { @@ -1006,7 +1016,8 @@ impl SamplingClient { let endpoint = self.endpoint("chat/completions"); let server_message = response.text().await.unwrap_or_default(); return Err(SamplingError::Auth(format!( - "Unauthorized (401) from {endpoint}: {server_message}" + "Unauthorized (401) from {}: {server_message}", + redacted_endpoint(&endpoint) ))); } @@ -1196,7 +1207,8 @@ impl SamplingClient { let endpoint = self.endpoint("responses"); let server_message = parse_error_bytes(bytes.as_ref()); return Err(SamplingError::Auth(format!( - "Unauthorized (401) from {endpoint}: {server_message}" + "Unauthorized (401) from {}: {server_message}", + redacted_endpoint(&endpoint) ))); } @@ -1363,7 +1375,8 @@ impl SamplingClient { let endpoint = self.endpoint("responses"); let server_message = response.text().await.unwrap_or_default(); return Err(SamplingError::Auth(format!( - "Unauthorized (401) from {endpoint}: {server_message}" + "Unauthorized (401) from {}: {server_message}", + redacted_endpoint(&endpoint) ))); } let model_metadata = extract_model_metadata(response.headers()); @@ -1542,7 +1555,8 @@ impl SamplingClient { let endpoint = self.endpoint("messages"); let server_message = parse_error_bytes(bytes.as_ref()); return Err(SamplingError::Auth(format!( - "Unauthorized (401) from {endpoint}: {server_message}" + "Unauthorized (401) from {}: {server_message}", + redacted_endpoint(&endpoint) ))); } @@ -1657,7 +1671,8 @@ impl SamplingClient { let endpoint = self.endpoint("messages"); let server_message = response.text().await.unwrap_or_default(); return Err(SamplingError::Auth(format!( - "Unauthorized (401) from {endpoint}: {server_message}" + "Unauthorized (401) from {}: {server_message}", + redacted_endpoint(&endpoint) ))); } let model_metadata = extract_model_metadata(response.headers()); @@ -3048,3 +3063,31 @@ mod tests { )); } } + +#[cfg(test)] +mod redacted_endpoint_tests { + /// Every spelling the client accepts (LF inside, one slash, no slash) loses its userinfo. + #[test] + fn redacted_endpoint_drops_userinfo_in_every_accepted_spelling() { + for spelling in [ + "http://u:TEST_PASSWORD@127.0.0.1:1/v1/chat/completions", + "http://u:TEST PASSWORD@127.0.0.1:1/v1/chat/completions", + "http://u:TEST_PASSWORD\n@127.0.0.1:1/v1/chat/completions", + "http://u:TEST_PASSWORD\r\n@127.0.0.1:1/v1/chat/completions", + "http:/u:TEST_PASSWORD@127.0.0.1:1/v1/chat/completions", + "http:u:TEST_PASSWORD@127.0.0.1:1/v1/chat/completions", + "http:\\u:TEST_PASSWORD@127.0.0.1:1/v1/chat/completions", + ] { + let redacted = super::redacted_endpoint(spelling); + assert_eq!( + redacted, "http://127.0.0.1:1/v1/chat/completions", + "{spelling:?}" + ); + } + let gateway = super::redacted_endpoint( + "http://127.0.0.1:1/?access_token=TEST_QUERY_TOKEN&route=x#frag", + ); + assert!(!gateway.contains("TEST_QUERY_TOKEN"), "{gateway}"); + assert!(gateway.contains("route=x"), "{gateway}"); + } +} diff --git a/crates/codegen/kigi-shell/Cargo.toml b/crates/codegen/kigi-shell/Cargo.toml index ff6581e..f36d265 100644 --- a/crates/codegen/kigi-shell/Cargo.toml +++ b/crates/codegen/kigi-shell/Cargo.toml @@ -150,6 +150,7 @@ fs2 = { workspace = true } zstd = { workspace = true } tar = { workspace = true } url = { workspace = true } +kigi-secrets = { path = "../kigi-secrets" } walkdir = { workspace = true } tdigests = "1.0" semver = { workspace = true } diff --git a/crates/codegen/kigi-shell/src/extensions/memory.rs b/crates/codegen/kigi-shell/src/extensions/memory.rs index b96f9be..7caa723 100644 --- a/crates/codegen/kigi-shell/src/extensions/memory.rs +++ b/crates/codegen/kigi-shell/src/extensions/memory.rs @@ -37,9 +37,9 @@ async fn handle_compact(agent: &MvpAgent, args: &acp::ExtRequest) -> ExtResult { respond_to: tx, }); } + // The session's own ACP error goes through unchanged: a Debug wrapper would escape its detail. rx.await - .map_err(|_| acp::Error::internal_error().data("session failed to respond"))? - .map_err(|e| acp::Error::internal_error().data(format!("Internal error: {:?}", e)))?; + .map_err(|_| acp::Error::internal_error().data("session failed to respond"))??; to_raw_response(&CompactConversationResponse {}) } diff --git a/crates/codegen/kigi-shell/src/sampling/error.rs b/crates/codegen/kigi-shell/src/sampling/error.rs index c27e2b3..6c26988 100644 --- a/crates/codegen/kigi-shell/src/sampling/error.rs +++ b/crates/codegen/kigi-shell/src/sampling/error.rs @@ -171,6 +171,70 @@ pub fn error_detail_from_data(data: &serde_json::Value) -> Option { .map(str::to_owned) } +/// The human-readable text of an ACP error: its `data` detail when present (Display would JSON-escape it), else the message. +pub fn acp_error_message(err: &acp::Error) -> String { + err.data + .as_ref() + .and_then(error_detail_from_data) + .unwrap_or_else(|| err.message.clone()) +} + +/// First line of a compaction error without internal `compact failed:` prefixes, capped for a banner; diagnostic lines (request URLs) are dropped. +pub fn user_facing_compact_error(raw: &str) -> String { + const INTERNAL_PREFIX: &str = "compact failed:"; + const MAX_CHARS: usize = 200; + // Redact before line selection: a line break inside a URL must not hide its `@`. + let raw = redact_urls_in_text(raw); + let mut rest = raw.trim(); + while rest + .get(..INTERNAL_PREFIX.len()) + .is_some_and(|head| head.eq_ignore_ascii_case(INTERNAL_PREFIX)) + { + rest = rest[INTERNAL_PREFIX.len()..].trim_start(); + } + rest.lines() + .next() + .unwrap_or_default() + .trim() + .chars() + .take(MAX_CHARS) + .collect() +} + +/// Every canonical `scheme://` URL in `text` loses userinfo, sensitive query values and fragment; one that fails to parse is not echoed. +fn redact_urls_in_text(text: &str) -> String { + let mut out = String::with_capacity(text.len()); + let mut rest = text; + while let Some(sep) = rest.find("://") { + // The delimiter before the scheme may be multi-byte (`“`, `：`): step past its UTF-8 length. + let scheme_start = rest[..sep] + .char_indices() + .rev() + .find(|(_, c)| !(c.is_ascii_alphanumeric() || matches!(c, '+' | '-' | '.'))) + .map_or(0, |(at, c)| at + c.len_utf8()); + out.push_str(&rest[..scheme_start]); + // The authority may hold whitespace (a configured password); the token ends at whitespace after it. + let authority = &rest[sep + 3..]; + let authority_end = authority.find(['/', '?', '#']).unwrap_or(authority.len()); + let token_end = authority[authority_end..] + .find(char::is_whitespace) + .map_or(rest.len(), |at| sep + 3 + authority_end + at); + let token = &rest[scheme_start..token_end]; + let url_text = token.trim_end_matches([')', ']', '}', ',', '.', ';', '\'', '"', '>']); + match url::Url::parse(url_text) { + Ok(mut url) => { + kigi_secrets::redact_url(&mut url); + out.push_str(url.as_str()); + } + Err(_) => out.push_str(""), + } + out.push_str(&token[url_text.len()..]); + rest = &rest[token_end..]; + } + out.push_str(rest); + out +} + pub fn http_status_from_error(err: &acp::Error) -> Option { err.data .as_ref()? @@ -254,6 +318,77 @@ pub fn prompt_complete_fields( #[cfg(test)] mod tests { + #[test] + fn compact_error_text_keeps_the_first_decoded_line_only() { + let err = super::acp::Error::internal_error().data( + "compact failed: API error (status 500): overloaded\n\nRequest URL: http://u:TEST_PASSWORD@127.0.0.1/v1", + ); + let detail = super::user_facing_compact_error(&super::acp_error_message(&err)); + assert_eq!(detail, "API error (status 500): overloaded"); + assert!(!detail.contains("TEST_PASSWORD")); + assert_eq!( + super::user_facing_compact_error(&"x".repeat(500)) + .chars() + .count(), + 200 + ); + assert_eq!( + super::acp_error_message(&super::acp::Error::internal_error()), + "Internal error" + ); + } + + /// A sampler 401 names the endpoint on the retained line; its userinfo must not survive. + #[test] + fn compact_error_text_redacts_url_credentials_on_the_first_line() { + for password in [ + "TEST_PASSWORD", + "TEST PASSWORD", + "TEST%20PASSWORD", + "TEST_PASSWORD\n", + "TEST_PASSWORD\r\n", + ] { + let raw = format!( + "compact failed: Unauthorized (401) from http://u:{password}@127.0.0.1:33263/401/v1/chat/completions: bad auth" + ); + assert_eq!( + super::user_facing_compact_error(&raw), + "Unauthorized (401) from http://127.0.0.1:33263/401/v1/chat/completions: bad auth", + "{password:?}" + ); + } + assert_eq!( + super::redact_urls_in_text("see https://a:b@h.example/x and http://h2/"), + "see https://h.example/x and http://h2/" + ); + } + + /// A credential carried as a query value survives no failure shape: 401 message or transport error. + #[test] + fn compact_error_text_redacts_query_credentials_in_every_failure_shape() { + let unauthorized = "compact failed: Unauthorized (401) from http://127.0.0.1:1/?access_token=TEST_QUERY_TOKEN&route=/v1/chat/completions: bad auth"; + let transport = "compact failed: request error: error sending request for url (http://127.0.0.1:1/?access_token=TEST_QUERY_TOKEN&route=/500/v1/chat/completions)"; + for raw in [unauthorized, transport] { + let detail = super::user_facing_compact_error(raw); + assert!(!detail.contains("TEST_QUERY_TOKEN"), "{detail}"); + assert!(detail.contains("route="), "{detail}"); + } + assert!(super::user_facing_compact_error("see http://:://").contains("")); + } + + /// A multi-byte delimiter right before the URL must not break the scan. + #[test] + fn compact_error_text_handles_unicode_before_a_url() { + for raw in [ + "compact failed: Service unavailable. See “https://u:TEST_PASSWORD@status.example/”.", + "compact failed: 请参阅：https://u:TEST_PASSWORD@status.example/", + ] { + let detail = super::user_facing_compact_error(raw); + assert!(!detail.contains("TEST_PASSWORD"), "{detail}"); + assert!(detail.contains("https://status.example/"), "{detail}"); + } + } + use super::*; use reqwest::StatusCode; diff --git a/crates/codegen/kigi-shell/src/session/compaction.rs b/crates/codegen/kigi-shell/src/session/compaction.rs index 6226160..664aa2a 100644 --- a/crates/codegen/kigi-shell/src/session/compaction.rs +++ b/crates/codegen/kigi-shell/src/session/compaction.rs @@ -689,6 +689,19 @@ impl SessionActor { .await; } } + /// Retry guidance headline, then the user-facing detail on its own line. + fn failure_with_retry_guidance(err: &acp::Error) -> String { + const RETRY_GUIDANCE: &str = + "it'll retry on the next turn, or start a new session using /new."; + let detail = crate::sampling::error::user_facing_compact_error( + &crate::sampling::error::acp_error_message(err), + ); + if detail.is_empty() { + RETRY_GUIDANCE.to_string() + } else { + format!("{RETRY_GUIDANCE}\n{detail}") + } + } /// Map a deterministic failure's error text to a fixed, content-free /// [`SuppressReason`] (drives telemetry + sticky-vs-per-turn scope). fn classify_suppress_reason(error_msg: &str) -> SuppressReason { @@ -1945,7 +1958,7 @@ impl SessionActor { == SUPPRESS_NONE { self.send_xai_notification(XaiSessionUpdate::AutoCompactFailed { - error: String::new(), + error: Self::failure_with_retry_guidance(&e), }) .await; } @@ -3286,3 +3299,28 @@ mod inline_auto_compact_flow_tests { .await; } } + +#[cfg(test)] +mod failure_message_tests { + use super::SessionActor; + use agent_client_protocol as acp; + + #[test] + fn transient_failure_message_carries_guidance_and_the_decoded_detail() { + let err = acp::Error::internal_error().data( + "Compact failed: compact failed: HTTP 500 from upstream\n\nRequest URL: http://u:TEST_PASSWORD@host/v1", + ); + let text = SessionActor::failure_with_retry_guidance(&err); + let (headline, detail) = text.split_once('\n').expect("guidance + detail"); + assert_eq!( + headline, + "it'll retry on the next turn, or start a new session using /new." + ); + assert_eq!(detail, "HTTP 500 from upstream"); + assert!(!text.contains("TEST_PASSWORD")); + assert_eq!( + SessionActor::failure_with_retry_guidance(&acp::Error::internal_error().data(" ")), + "it'll retry on the next turn, or start a new session using /new." + ); + } +} diff --git a/crates/codegen/kigi-tui/src/app/dispatch/prompt.rs b/crates/codegen/kigi-tui/src/app/dispatch/prompt.rs index 7a5c235..61c686a 100644 --- a/crates/codegen/kigi-tui/src/app/dispatch/prompt.rs +++ b/crates/codegen/kigi-tui/src/app/dispatch/prompt.rs @@ -1290,10 +1290,9 @@ pub(super) fn handle_compact_complete( } Err(err) => { tracing::error!(agent = ?agent_id, error = %err, "Compaction failed"); + // Already sanitized by the effect layer. agent.scrollback.push_block(RenderBlock::session_event( - SessionEvent::CompactionFailed { - error: String::new(), - }, + SessionEvent::CompactionFailed { error: err.clone() }, )); } } diff --git a/crates/codegen/kigi-tui/src/app/effects/mod.rs b/crates/codegen/kigi-tui/src/app/effects/mod.rs index be84870..f805649 100644 --- a/crates/codegen/kigi-tui/src/app/effects/mod.rs +++ b/crates/codegen/kigi-tui/src/app/effects/mod.rs @@ -1409,9 +1409,14 @@ pub(crate) fn execute( let result = acp_send(req, &tx).await; TaskResult::CompactComplete { agent_id, - result: result - .map(|_| ()) - .map_err(|e| sanitize_user_error(&e.to_string())), + // The decoded first line only: diagnostic lines (request URLs) never reach the banner. + result: result.map(|_| ()).map_err(|e| { + sanitize_user_error( + &kigi_shell::sampling::error::user_facing_compact_error( + &kigi_shell::sampling::error::acp_error_message(&e), + ), + ) + }), } }); }