Skip to content

Commit 273232f

Browse files
haasonsaasclaude
andcommitted
Fix self-review findings: overflow safety, config bounds, language matching
- Use saturating_mul for retry delay calculations to prevent integer overflow - Enforce minimum bounds for adapter timeout (5s) and retry delay (50ms) - Handle "en-US" style language codes in output language check Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
1 parent afff03e commit 273232f

File tree

3 files changed

+35
-5
lines changed

3 files changed

+35
-5
lines changed

src/adapters/common.rs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -47,7 +47,7 @@ where
4747
let status = response.status();
4848
let body = response.text().await.unwrap_or_default();
4949
if is_retryable_status(status) && attempt < max_retries {
50-
sleep(Duration::from_millis(base_delay_ms * (attempt as u64 + 1))).await;
50+
sleep(Duration::from_millis(base_delay_ms.saturating_mul(attempt as u64 + 1))).await;
5151
continue;
5252
}
5353

@@ -62,7 +62,7 @@ where
6262
}
6363
Err(err) => {
6464
if attempt < max_retries {
65-
sleep(Duration::from_millis(base_delay_ms * (attempt as u64 + 1))).await;
65+
sleep(Duration::from_millis(base_delay_ms.saturating_mul(attempt as u64 + 1))).await;
6666
continue;
6767
}
6868
return Err(err.into());

src/config.rs

Lines changed: 22 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -568,7 +568,7 @@ impl Config {
568568
if timeout == 0 {
569569
self.adapter_timeout_secs = None; // use default
570570
} else {
571-
self.adapter_timeout_secs = Some(timeout.min(600));
571+
self.adapter_timeout_secs = Some(timeout.clamp(5, 600));
572572
}
573573
}
574574
// Clamp adapter retries (0-10)
@@ -580,7 +580,7 @@ impl Config {
580580
if delay == 0 {
581581
self.adapter_retry_delay_ms = None;
582582
} else {
583-
self.adapter_retry_delay_ms = Some(delay.min(30_000));
583+
self.adapter_retry_delay_ms = Some(delay.clamp(50, 30_000));
584584
}
585585
}
586586
// Normalize output language
@@ -1113,6 +1113,26 @@ mod tests {
11131113
assert_eq!(config.output_language, None);
11141114
}
11151115

1116+
#[test]
1117+
fn normalize_adapter_timeout_clamps_minimum() {
1118+
let mut config = Config {
1119+
adapter_timeout_secs: Some(2),
1120+
..Config::default()
1121+
};
1122+
config.normalize();
1123+
assert_eq!(config.adapter_timeout_secs, Some(5));
1124+
}
1125+
1126+
#[test]
1127+
fn normalize_adapter_retry_delay_clamps_minimum() {
1128+
let mut config = Config {
1129+
adapter_retry_delay_ms: Some(10),
1130+
..Config::default()
1131+
};
1132+
config.normalize();
1133+
assert_eq!(config.adapter_retry_delay_ms, Some(50));
1134+
}
1135+
11161136
#[test]
11171137
fn normalize_feedback_suppression_zero_resets() {
11181138
let mut config = Config {

src/review/pipeline.rs

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -483,7 +483,7 @@ pub fn build_review_guidance(
483483

484484
// Output language directive
485485
if let Some(ref lang) = config.output_language {
486-
if lang != "en" {
486+
if lang != "en" && !lang.starts_with("en-") {
487487
sections.push(format!(
488488
"Write all review comments and suggestions in {}.",
489489
lang
@@ -938,6 +938,16 @@ mod tests {
938938
assert!(!guidance.contains("Write all review comments"));
939939
}
940940

941+
#[test]
942+
fn build_review_guidance_skips_en_us_language() {
943+
let config = config::Config {
944+
output_language: Some("en-us".to_string()),
945+
..config::Config::default()
946+
};
947+
let guidance = build_review_guidance(&config, None).unwrap();
948+
assert!(!guidance.contains("Write all review comments"));
949+
}
950+
941951
#[test]
942952
fn build_review_guidance_no_fix_suggestions() {
943953
let config = config::Config {

0 commit comments

Comments
 (0)