Skip to content

Commit 9b1d585

Browse files
haasonsaasclaude
andcommitted
TDD: fix UTF-8 truncation panic in eval summarizer and invalid slice in git title extraction
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
1 parent 5600d0f commit 9b1d585

2 files changed

Lines changed: 90 additions & 18 deletions

File tree

src/commands/eval.rs

Lines changed: 45 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -892,8 +892,52 @@ impl EvalPattern {
892892
fn summarize_for_eval(content: &str) -> String {
893893
let mut summary = content.trim().replace('\n', " ");
894894
if summary.len() > 120 {
895-
summary.truncate(117);
895+
let mut end = 117;
896+
while end > 0 && !summary.is_char_boundary(end) {
897+
end -= 1;
898+
}
899+
summary.truncate(end);
896900
summary.push_str("...");
897901
}
898902
summary
899903
}
904+
905+
#[cfg(test)]
906+
mod tests {
907+
use super::*;
908+
909+
#[test]
910+
fn test_summarize_for_eval_short() {
911+
let result = summarize_for_eval("hello world");
912+
assert_eq!(result, "hello world");
913+
}
914+
915+
#[test]
916+
fn test_summarize_for_eval_utf8_safety() {
917+
// Create a string with multi-byte UTF-8 chars that will land truncation mid-char
918+
// '€' is 3 bytes. 39 euros = 117 bytes exactly, but 38 euros = 114 bytes.
919+
// We need byte 117 to land mid-character.
920+
// 38 euros (114 bytes) + "abcd" (4 bytes) = 118 bytes > 120? No.
921+
// Let's use: 37 euros (111 bytes) + "abcdefghij" (10 bytes) = 121 bytes > 120
922+
// truncate(117): byte 117 = 111 + 6 = within "abcdefghij", which is ASCII. Safe.
923+
// Better: 39 euros (117 bytes) + "abcd" (4 bytes) = 121 bytes > 120
924+
// truncate(117): byte 117 is the end of the 39th euro. Safe boundary.
925+
// Better still: 38 euros (114 bytes) + "abc" (3 bytes) = 117. Not > 120.
926+
// We need: content where byte 117 is mid-char.
927+
// 39 euros = 117 bytes. Add "a" = 118 bytes. Not > 120.
928+
// 40 euros = 120 bytes. Add "a" = 121 bytes > 120.
929+
// truncate(117): byte 117 = end of 39th euro. 40th euro starts at 117.
930+
// Euro at bytes 117, 118, 119. truncate(117) is AT the start of the 40th euro.
931+
// is_char_boundary(117) — 117 is the start of a 3-byte char, so it IS a boundary!
932+
// Need byte 118: 40 euros (120 bytes) + "ab" = 122 bytes > 120.
933+
// Still truncate(117), which is start of 40th euro = valid boundary.
934+
// Use a mix: "a" + 39 euros = 1 + 117 = 118 bytes. Add "abc" = 121 > 120.
935+
// truncate(117): byte 117 = 1 + 38*3 = 115 is start of 39th euro.
936+
// byte 117 = 115 + 2 = mid-euro! This will panic!
937+
let content = format!("a{}{}", "€".repeat(39), "abc");
938+
// length = 1 + 117 + 3 = 121 bytes
939+
// truncate(117) = byte 117 = 1 + 38*3 + 2 = inside 39th euro
940+
let result = summarize_for_eval(&content);
941+
assert!(result.len() <= 120);
942+
}
943+
}

src/commands/git.rs

Lines changed: 45 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -143,23 +143,7 @@ async fn suggest_pr_title(config: config::Config) -> Result<()> {
143143

144144
let response = adapter.complete(request).await?;
145145

146-
// Extract title from response
147-
let title = if let Some(start) = response.content.find("<title>") {
148-
if let Some(end) = response.content.find("</title>") {
149-
response.content[start + 7..end].trim().to_string()
150-
} else {
151-
response.content.trim().to_string()
152-
}
153-
} else {
154-
// Fallback: take the first non-empty line
155-
response
156-
.content
157-
.lines()
158-
.find(|line| !line.trim().is_empty())
159-
.unwrap_or("")
160-
.trim()
161-
.to_string()
162-
};
146+
let title = extract_title_from_response(&response.content);
163147

164148
println!("\nSuggested PR title:");
165149
println!("{}", title);
@@ -173,3 +157,47 @@ async fn suggest_pr_title(config: config::Config) -> Result<()> {
173157

174158
Ok(())
175159
}
160+
161+
fn extract_title_from_response(content: &str) -> String {
162+
if let Some(start) = content.find("<title>") {
163+
let after_tag = start + 7;
164+
if let Some(end) = content[after_tag..].find("</title>") {
165+
content[after_tag..after_tag + end].trim().to_string()
166+
} else {
167+
content.trim().to_string()
168+
}
169+
} else {
170+
content
171+
.lines()
172+
.find(|line| !line.trim().is_empty())
173+
.unwrap_or("")
174+
.trim()
175+
.to_string()
176+
}
177+
}
178+
179+
#[cfg(test)]
180+
mod tests {
181+
use super::*;
182+
183+
#[test]
184+
fn test_extract_title_normal() {
185+
let content = "<title>Fix login bug</title>";
186+
assert_eq!(extract_title_from_response(content), "Fix login bug");
187+
}
188+
189+
#[test]
190+
fn test_extract_title_malformed_closing_before_opening() {
191+
// Malformed: closing tag appears before opening tag
192+
// This should NOT panic
193+
let content = "Some text</title> more <title>Real Title</title>";
194+
let title = extract_title_from_response(content);
195+
assert!(!title.is_empty());
196+
}
197+
198+
#[test]
199+
fn test_extract_title_no_tags() {
200+
let content = "Just a plain title\nSecond line";
201+
assert_eq!(extract_title_from_response(content), "Just a plain title");
202+
}
203+
}

0 commit comments

Comments
 (0)