From d1c29a576c6c0cdf6c9486b5c2e8079a6bdc0c2f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E9=9B=B7=E7=94=B5=E8=8A=BD=E8=A1=A3?= Date: Sun, 13 Sep 2026 12:12:00 -0400 Subject: [PATCH] fix(compaction): failures carry the real error, with endpoint credentials redacted The pager rendered "Compaction failed." with the error dropped, and the shell's unsuppressed auto-compaction failure sent an empty error. Both now carry the decoded first line of the failure (retry guidance plus the detail), decoded from the ACP data rather than its JSON-escaped Display, propagated unchanged through the compact extension, capped for a banner. Endpoint credentials never reach the banner: the sampler redacts the parsed endpoint (userinfo, sensitive query values, fragment via the repository sanitizer) in every 401 message and the appended request URL, and the shell redacts every URL in the error text before line selection, including URLs whose password holds whitespace or that follow a multi-byte delimiter. Ported from upstream 1.0.13 (grok-build dispatch/prompt.rs, session/compaction.rs), extended with the redaction. Claude-Session: https://claude.ai/code/session_01RUqV8HVGXcAETyYbZ2tPQ4 --- Cargo.lock | 2 + crates/codegen/kigi-sampler/Cargo.toml | 1 + crates/codegen/kigi-sampler/src/client.rs | 55 ++++++- crates/codegen/kigi-shell/Cargo.toml | 1 + .../kigi-shell/src/extensions/memory.rs | 4 +- .../codegen/kigi-shell/src/sampling/error.rs | 135 ++++++++++++++++++ .../kigi-shell/src/session/compaction.rs | 40 +++++- .../kigi-tui/src/app/dispatch/prompt.rs | 5 +- .../codegen/kigi-tui/src/app/effects/mod.rs | 11 +- 9 files changed, 239 insertions(+), 15 deletions(-) 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), + ), + ) + }), } }); }