Skip to content

Commit 036049e

Browse files
haasonsaashaasonsaas
committed
refactor: split review triage helpers
Made-with: Cursor
1 parent 95e764c commit 036049e

File tree

7 files changed

+193
-208
lines changed

7 files changed

+193
-208
lines changed

‎TODO.md‎

Lines changed: 3 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -10,19 +10,14 @@
1010

1111
## Immediate Queue
1212

13-
- [ ] `src/review/triage.rs`
14-
- Split file-kind heuristics (`lock`, `generated`) from change-shape heuristics.
15-
- Split whitespace-only detection from comment-only detection.
16-
- Isolate `TriageResult` reporting helpers from diff classification.
17-
- Pull shared diff-line collection helpers out of `triage_diff()`.
18-
19-
## Review Backlog
20-
2113
- [ ] `src/review/compression.rs`
2214
- Split token estimation and skipped-summary helpers from compression stages.
2315
- Split deletion-only compression and clipping primitives from stage selection.
2416
- Split stage 2 compressed selection from stage 4 multi-call batching.
2517
- Isolate per-stage budget accounting and file-placement helpers.
18+
19+
## Review Backlog
20+
2621
- [ ] `src/review/verification/parser.rs`
2722
- Split response schema construction from response parsing.
2823
- Split fenced-JSON extraction / JSON decoding from regex fallback parsing.

‎src/review/triage.rs‎

Lines changed: 14 additions & 200 deletions
Original file line numberDiff line numberDiff line change
@@ -1,203 +1,17 @@
1-
use crate::core::diff_parser::UnifiedDiff;
2-
3-
#[derive(Debug, Clone, Copy, PartialEq, Eq)]
4-
pub enum TriageResult {
5-
NeedsReview,
6-
SkipLockFile,
7-
SkipWhitespaceOnly,
8-
SkipDeletionOnly,
9-
SkipGenerated,
10-
SkipCommentOnly,
11-
}
12-
13-
impl TriageResult {
14-
pub fn should_skip(&self) -> bool {
15-
!matches!(self, TriageResult::NeedsReview)
16-
}
17-
18-
pub fn reason(&self) -> &'static str {
19-
match self {
20-
TriageResult::NeedsReview => "needs review",
21-
TriageResult::SkipLockFile => "lock file",
22-
TriageResult::SkipWhitespaceOnly => "whitespace-only changes",
23-
TriageResult::SkipDeletionOnly => "deletion-only changes",
24-
TriageResult::SkipGenerated => "generated file",
25-
TriageResult::SkipCommentOnly => "comment-only changes",
26-
}
27-
}
28-
}
29-
30-
/// Lock file names that should be auto-skipped.
31-
const LOCK_FILES: &[&str] = &[
32-
"Cargo.lock",
33-
"package-lock.json",
34-
"yarn.lock",
35-
"pnpm-lock.yaml",
36-
"Gemfile.lock",
37-
"poetry.lock",
38-
"composer.lock",
39-
"go.sum",
40-
"Pipfile.lock",
41-
];
42-
43-
/// Comment line prefixes (after trimming leading whitespace).
44-
const COMMENT_PREFIXES: &[&str] = &["//", "# ", "/*", "*/", "* ", "--", "<!--", "\"\"\"", "'''"];
45-
46-
/// Patterns that start with `#` but are NOT comments (Rust attributes, C preprocessor, etc.).
47-
const HASH_NON_COMMENT_PREFIXES: &[&str] = &[
48-
"#[", "#![", "#!/", "#include", "#define", "#ifdef", "#ifndef", "#endif", "#pragma", "#undef",
49-
"#elif", "#else", "#if ", "#error", "#warning", "#line",
50-
];
51-
52-
/// Classify a diff using fast heuristics (no LLM call).
53-
///
54-
/// Checks are applied in priority order:
55-
/// 1. Lock files
56-
/// 2. Generated files
57-
/// 3. Deletion-only changes
58-
/// 4. Whitespace-only changes
59-
/// 5. Comment-only changes
60-
/// 6. Default → NeedsReview
61-
pub fn triage_diff(diff: &UnifiedDiff) -> TriageResult {
62-
let path_str = diff.file_path.to_string_lossy();
63-
64-
// 1. Lock files — match by file name (final component)
65-
if let Some(file_name) = diff.file_path.file_name().and_then(|n| n.to_str()) {
66-
if LOCK_FILES.contains(&file_name) {
67-
return TriageResult::SkipLockFile;
68-
}
69-
}
70-
71-
// 2. Generated files — match by path patterns and extensions
72-
if is_generated_file(&path_str) {
73-
return TriageResult::SkipGenerated;
74-
}
75-
76-
// Collect all non-context changes across all hunks
77-
let all_changes: Vec<&DiffLine> = diff
78-
.hunks
79-
.iter()
80-
.flat_map(|h| h.changes.iter())
81-
.filter(|c| !matches!(c.change_type, ChangeType::Context))
82-
.collect();
83-
84-
// If there are no actual changes, default to NeedsReview
85-
if all_changes.is_empty() {
86-
return TriageResult::NeedsReview;
87-
}
88-
89-
// 3. Deletion-only — all non-context lines are Removed
90-
if all_changes
91-
.iter()
92-
.all(|c| matches!(c.change_type, ChangeType::Removed))
93-
{
94-
return TriageResult::SkipDeletionOnly;
95-
}
96-
97-
// 4. Whitespace-only — every added line has a corresponding removed line
98-
// that differs only in whitespace
99-
if is_whitespace_only_change(&all_changes) {
100-
return TriageResult::SkipWhitespaceOnly;
101-
}
102-
103-
// 5. Comment-only — all changed lines are comment lines
104-
if all_changes.iter().all(|c| is_comment_line(&c.content)) {
105-
return TriageResult::SkipCommentOnly;
106-
}
107-
108-
// 6. Default
109-
TriageResult::NeedsReview
110-
}
111-
112-
/// Check if a file path matches generated-file patterns.
113-
fn is_generated_file(path: &str) -> bool {
114-
// Path contains marker segments
115-
if path.contains(".generated.")
116-
|| path.contains(".g.")
117-
|| path.starts_with("_generated/")
118-
|| path.contains("/_generated/")
119-
|| path.starts_with("generated/")
120-
|| path.contains("/generated/")
121-
{
122-
return true;
123-
}
124-
125-
// File extension patterns
126-
if path.ends_with(".pb.go")
127-
|| path.ends_with(".pb.rs")
128-
|| path.ends_with(".swagger.json")
129-
|| path.ends_with(".min.js")
130-
|| path.ends_with(".min.css")
131-
{
132-
return true;
133-
}
134-
135-
// Vendor prefix
136-
if path.starts_with("vendor/") {
137-
return true;
138-
}
139-
140-
false
141-
}
142-
143-
/// Check if all changes are whitespace-only by comparing stripped content
144-
/// of removed vs added lines.
145-
fn is_whitespace_only_change(changes: &[&DiffLine]) -> bool {
146-
let removed: Vec<&str> = changes
147-
.iter()
148-
.filter(|c| matches!(c.change_type, ChangeType::Removed))
149-
.map(|c| c.content.as_str())
150-
.collect();
151-
152-
let added: Vec<&str> = changes
153-
.iter()
154-
.filter(|c| matches!(c.change_type, ChangeType::Added))
155-
.map(|c| c.content.as_str())
156-
.collect();
157-
158-
// Must have the same number of added and removed lines
159-
if removed.len() != added.len() {
160-
return false;
161-
}
162-
163-
// Each pair must differ only in whitespace
164-
removed
165-
.iter()
166-
.zip(added.iter())
167-
.all(|(r, a)| strip_whitespace(r) == strip_whitespace(a))
168-
}
169-
170-
/// Remove all whitespace characters from a string for comparison.
171-
fn strip_whitespace(s: &str) -> String {
172-
s.chars().filter(|c| !c.is_whitespace()).collect()
173-
}
174-
175-
/// Check if a line (after trimming leading whitespace) starts with a comment prefix.
176-
fn is_comment_line(content: &str) -> bool {
177-
let trimmed = content.trim();
178-
if trimmed.is_empty() {
179-
// Blank lines in a comment-only change are fine
180-
return true;
181-
}
182-
// Handle `#` lines: treat as comment UNLESS it matches a known non-comment prefix
183-
// (Rust attributes, C preprocessor, etc.)
184-
if trimmed.starts_with('#') {
185-
// If it matches a non-comment prefix like #[, #include, etc., it's code
186-
if HASH_NON_COMMENT_PREFIXES
187-
.iter()
188-
.any(|p| trimmed.starts_with(p))
189-
{
190-
return false;
191-
}
192-
// Otherwise bare `#`, `#comment`, `# comment` are all comments
193-
return true;
194-
}
195-
COMMENT_PREFIXES
196-
.iter()
197-
.any(|prefix| trimmed.starts_with(prefix))
198-
}
199-
200-
use crate::core::diff_parser::{ChangeType, DiffLine};
1+
#[path = "triage/changes.rs"]
2+
mod changes;
3+
#[path = "triage/comments.rs"]
4+
mod comments;
5+
#[path = "triage/files.rs"]
6+
mod files;
7+
#[path = "triage/result.rs"]
8+
mod result;
9+
#[path = "triage/run.rs"]
10+
mod run;
11+
12+
#[allow(unused_imports)]
13+
pub use result::TriageResult;
14+
pub use run::triage_diff;
20115

20216
#[cfg(test)]
20317
mod tests {

‎src/review/triage/changes.rs‎

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,42 @@
1+
use crate::core::diff_parser::{ChangeType, DiffLine, UnifiedDiff};
2+
3+
pub(super) fn collect_non_context_changes(diff: &UnifiedDiff) -> Vec<&DiffLine> {
4+
diff.hunks
5+
.iter()
6+
.flat_map(|hunk| hunk.changes.iter())
7+
.filter(|change| !matches!(change.change_type, ChangeType::Context))
8+
.collect()
9+
}
10+
11+
pub(super) fn is_deletion_only_change(changes: &[&DiffLine]) -> bool {
12+
changes
13+
.iter()
14+
.all(|change| matches!(change.change_type, ChangeType::Removed))
15+
}
16+
17+
pub(super) fn is_whitespace_only_change(changes: &[&DiffLine]) -> bool {
18+
let removed: Vec<&str> = changes
19+
.iter()
20+
.filter(|change| matches!(change.change_type, ChangeType::Removed))
21+
.map(|change| change.content.as_str())
22+
.collect();
23+
24+
let added: Vec<&str> = changes
25+
.iter()
26+
.filter(|change| matches!(change.change_type, ChangeType::Added))
27+
.map(|change| change.content.as_str())
28+
.collect();
29+
30+
if removed.len() != added.len() {
31+
return false;
32+
}
33+
34+
removed
35+
.iter()
36+
.zip(added.iter())
37+
.all(|(removed, added)| strip_whitespace(removed) == strip_whitespace(added))
38+
}
39+
40+
fn strip_whitespace(value: &str) -> String {
41+
value.chars().filter(|ch| !ch.is_whitespace()).collect()
42+
}

‎src/review/triage/comments.rs‎

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,27 @@
1+
const COMMENT_PREFIXES: &[&str] = &["//", "# ", "/*", "*/", "* ", "--", "<!--", "\"\"\"", "'''"];
2+
3+
const HASH_NON_COMMENT_PREFIXES: &[&str] = &[
4+
"#[", "#![", "#!/", "#include", "#define", "#ifdef", "#ifndef", "#endif", "#pragma", "#undef",
5+
"#elif", "#else", "#if ", "#error", "#warning", "#line",
6+
];
7+
8+
pub(super) fn is_comment_line(content: &str) -> bool {
9+
let trimmed = content.trim();
10+
if trimmed.is_empty() {
11+
return true;
12+
}
13+
14+
if trimmed.starts_with('#') {
15+
if HASH_NON_COMMENT_PREFIXES
16+
.iter()
17+
.any(|prefix| trimmed.starts_with(prefix))
18+
{
19+
return false;
20+
}
21+
return true;
22+
}
23+
24+
COMMENT_PREFIXES
25+
.iter()
26+
.any(|prefix| trimmed.starts_with(prefix))
27+
}

‎src/review/triage/files.rs‎

Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,40 @@
1+
const LOCK_FILES: &[&str] = &[
2+
"Cargo.lock",
3+
"package-lock.json",
4+
"yarn.lock",
5+
"pnpm-lock.yaml",
6+
"Gemfile.lock",
7+
"poetry.lock",
8+
"composer.lock",
9+
"go.sum",
10+
"Pipfile.lock",
11+
];
12+
13+
pub(super) fn is_lock_file(path: &std::path::Path) -> bool {
14+
path.file_name()
15+
.and_then(|name| name.to_str())
16+
.is_some_and(|file_name| LOCK_FILES.contains(&file_name))
17+
}
18+
19+
pub(super) fn is_generated_file(path: &str) -> bool {
20+
if path.contains(".generated.")
21+
|| path.contains(".g.")
22+
|| path.starts_with("_generated/")
23+
|| path.contains("/_generated/")
24+
|| path.starts_with("generated/")
25+
|| path.contains("/generated/")
26+
{
27+
return true;
28+
}
29+
30+
if path.ends_with(".pb.go")
31+
|| path.ends_with(".pb.rs")
32+
|| path.ends_with(".swagger.json")
33+
|| path.ends_with(".min.js")
34+
|| path.ends_with(".min.css")
35+
{
36+
return true;
37+
}
38+
39+
path.starts_with("vendor/")
40+
}

‎src/review/triage/result.rs‎

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,26 @@
1+
#[derive(Debug, Clone, Copy, PartialEq, Eq)]
2+
pub enum TriageResult {
3+
NeedsReview,
4+
SkipLockFile,
5+
SkipWhitespaceOnly,
6+
SkipDeletionOnly,
7+
SkipGenerated,
8+
SkipCommentOnly,
9+
}
10+
11+
impl TriageResult {
12+
pub fn should_skip(&self) -> bool {
13+
!matches!(self, TriageResult::NeedsReview)
14+
}
15+
16+
pub fn reason(&self) -> &'static str {
17+
match self {
18+
TriageResult::NeedsReview => "needs review",
19+
TriageResult::SkipLockFile => "lock file",
20+
TriageResult::SkipWhitespaceOnly => "whitespace-only changes",
21+
TriageResult::SkipDeletionOnly => "deletion-only changes",
22+
TriageResult::SkipGenerated => "generated file",
23+
TriageResult::SkipCommentOnly => "comment-only changes",
24+
}
25+
}
26+
}

0 commit comments

Comments
 (0)