Skip to content

Commit 6d056d6

Browse files
haasonsaasclaude
andcommitted
TDD: fix path prefix matching boundary and feedback stats double-counting
- config.rs, rules.rs: path_matches now checks path component boundaries so pattern "src" won't incorrectly match "srcfoo/file.rs" - misc.rs: extract feedback accept/reject into testable functions and only increment stats when comment ID is newly inserted (fixes double-counting when same comment is accepted/rejected multiple times) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
1 parent b7daef5 commit 6d056d6

3 files changed

Lines changed: 123 additions & 24 deletions

File tree

src/commands/misc.rs

Lines changed: 74 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -193,29 +193,12 @@ pub async fn feedback_command(
193193
}
194194

195195
let mut store = review::load_feedback_store_from_path(&feedback_path);
196-
let mut updated = 0usize;
197196

198-
if action == "accept" {
199-
for comment in &comments {
200-
if store.accept.insert(comment.id.clone()) {
201-
updated += 1;
202-
}
203-
store.suppress.remove(&comment.id);
204-
let key = review::classify_comment_type(comment).as_str().to_string();
205-
let stats = store.by_comment_type.entry(key).or_default();
206-
stats.accepted = stats.accepted.saturating_add(1);
207-
}
197+
let updated = if action == "accept" {
198+
apply_feedback_accept(&mut store, &comments)
208199
} else {
209-
for comment in &comments {
210-
if store.suppress.insert(comment.id.clone()) {
211-
updated += 1;
212-
}
213-
store.accept.remove(&comment.id);
214-
let key = review::classify_comment_type(comment).as_str().to_string();
215-
let stats = store.by_comment_type.entry(key).or_default();
216-
stats.rejected = stats.rejected.saturating_add(1);
217-
}
218-
}
200+
apply_feedback_reject(&mut store, &comments)
201+
};
219202

220203
review::save_feedback_store(&feedback_path, &store)?;
221204
println!(
@@ -228,6 +211,42 @@ pub async fn feedback_command(
228211
Ok(())
229212
}
230213

214+
fn apply_feedback_accept(
215+
store: &mut review::FeedbackStore,
216+
comments: &[core::Comment],
217+
) -> usize {
218+
let mut updated = 0;
219+
for comment in comments {
220+
let is_new = store.accept.insert(comment.id.clone());
221+
if is_new {
222+
updated += 1;
223+
let key = review::classify_comment_type(comment).as_str().to_string();
224+
let stats = store.by_comment_type.entry(key).or_default();
225+
stats.accepted = stats.accepted.saturating_add(1);
226+
}
227+
store.suppress.remove(&comment.id);
228+
}
229+
updated
230+
}
231+
232+
fn apply_feedback_reject(
233+
store: &mut review::FeedbackStore,
234+
comments: &[core::Comment],
235+
) -> usize {
236+
let mut updated = 0;
237+
for comment in comments {
238+
let is_new = store.suppress.insert(comment.id.clone());
239+
if is_new {
240+
updated += 1;
241+
let key = review::classify_comment_type(comment).as_str().to_string();
242+
let stats = store.by_comment_type.entry(key).or_default();
243+
stats.rejected = stats.rejected.saturating_add(1);
244+
}
245+
store.accept.remove(&comment.id);
246+
}
247+
updated
248+
}
249+
231250
#[derive(Debug, Clone, Serialize, Deserialize, Default)]
232251
struct DiscussionTurn {
233252
role: String,
@@ -387,6 +406,40 @@ mod tests {
387406
let result = select_discussion_comment(&[comment.clone()], None, None).unwrap();
388407
assert_eq!(result.id, "cmt_1");
389408
}
409+
410+
#[test]
411+
fn test_feedback_stats_not_double_counted() {
412+
// Simulate accepting the same comment twice — stats should only increment once
413+
let mut store = review::FeedbackStore::default();
414+
let comment = core::Comment {
415+
id: "cmt_dup".to_string(),
416+
file_path: PathBuf::from("test.rs"),
417+
line_number: 1,
418+
content: "test".to_string(),
419+
rule_id: None,
420+
severity: core::comment::Severity::Warning,
421+
category: core::comment::Category::Bug,
422+
suggestion: None,
423+
confidence: 0.8,
424+
code_suggestion: None,
425+
tags: vec![],
426+
fix_effort: core::comment::FixEffort::Low,
427+
};
428+
429+
let comments = vec![comment];
430+
431+
// Accept the same batch of comments twice
432+
for _ in 0..2 {
433+
apply_feedback_accept(&mut store, &comments);
434+
}
435+
436+
let key = review::classify_comment_type(&comments[0]).as_str().to_string();
437+
let stats = &store.by_comment_type[&key];
438+
assert_eq!(
439+
stats.accepted, 1,
440+
"Stats should only count 1 acceptance, not 2 (double-counting bug)"
441+
);
442+
}
390443
}
391444

392445
fn load_discussion_thread(path: Option<&std::path::Path>, comment_id: &str) -> DiscussionThread {

src/config.rs

Lines changed: 29 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -517,8 +517,9 @@ impl Config {
517517
false
518518
}
519519
} else {
520-
// Direct path prefix matching
521-
path.starts_with(pattern)
520+
// Path prefix matching with component boundary check
521+
path == pattern
522+
|| path.starts_with(&format!("{}/", pattern.trim_end_matches('/')))
522523
}
523524
}
524525
}
@@ -690,4 +691,30 @@ mod tests {
690691

691692
assert_eq!(config.comment_types, vec!["logic", "style"]);
692693
}
694+
695+
#[test]
696+
fn path_matches_respects_component_boundary() {
697+
let config = Config::default();
698+
699+
// Exact prefix with separator should match
700+
assert!(config.path_matches("src/file.rs", "src/"));
701+
assert!(config.path_matches("src/sub/file.rs", "src"));
702+
703+
// Glob patterns should still work
704+
assert!(config.path_matches("src/file.rs", "src/*.rs"));
705+
706+
// Non-glob pattern must NOT match a different path component
707+
// "src" should not match "srcfoo/file.rs" or "src-backup/file.rs"
708+
assert!(
709+
!config.path_matches("srcfoo/file.rs", "src"),
710+
"pattern 'src' should not match 'srcfoo/file.rs' (different path component)"
711+
);
712+
assert!(
713+
!config.path_matches("src-backup/file.rs", "src"),
714+
"pattern 'src' should not match 'src-backup/file.rs'"
715+
);
716+
717+
// Exact match should work
718+
assert!(config.path_matches("src/file.rs", "src/file.rs"));
719+
}
693720
}

src/core/rules.rs

Lines changed: 20 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -186,6 +186,25 @@ fn path_matches(path: &str, pattern: &str) -> bool {
186186
.map(|pattern| pattern.matches(path))
187187
.unwrap_or(false)
188188
} else {
189-
path.starts_with(pattern)
189+
path == pattern
190+
|| path.starts_with(&format!("{}/", pattern.trim_end_matches('/')))
191+
}
192+
}
193+
194+
#[cfg(test)]
195+
mod tests {
196+
use super::*;
197+
198+
#[test]
199+
fn rules_path_matches_respects_component_boundary() {
200+
// "src" should match "src/file.rs" (path starts with component "src")
201+
assert!(path_matches("src/file.rs", "src"));
202+
assert!(path_matches("src/file.rs", "src/"));
203+
204+
// "src" should NOT match "srcfoo/file.rs" (different component)
205+
assert!(
206+
!path_matches("srcfoo/file.rs", "src"),
207+
"pattern 'src' should not match 'srcfoo/file.rs'"
208+
);
190209
}
191210
}

0 commit comments

Comments
 (0)