Skip to content

Commit bc998bc

Browse files
haasonsaashaasonsaas
committed
refactor: split review prepare runner helpers
Made-with: Cursor
1 parent d4775ba commit bc998bc

File tree

6 files changed

+201
-115
lines changed

6 files changed

+201
-115
lines changed

‎TODO.md‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -97,7 +97,7 @@
9797

9898
- [x] `src/review/pipeline/execution/responses/processing.rs`: split raw response normalization, comment extraction, and merge logic.
9999
- [x] `src/review/pipeline/execution/responses/validation.rs`: separate schema validation from per-comment sanitization.
100-
- [ ] `src/review/pipeline/prepare/runner.rs`: split per-diff orchestration, pre-analysis/triage decisions, and progress updates.
100+
- [x] `src/review/pipeline/prepare/runner.rs`: split per-diff orchestration, pre-analysis/triage decisions, and progress updates.
101101
- [ ] `src/review/pipeline/context/symbols.rs`: split symbol search, snippet selection, and fallback behavior.
102102
- [ ] `src/review/pipeline/context/related.rs`: separate related-file discovery from ranking/selection.
103103
- [ ] `src/review/pipeline/guidance.rs`: carve guidance assembly, repo-support guidance, and prompt-facing formatting.

‎src/review/pipeline/prepare/runner.rs‎

Lines changed: 10 additions & 114 deletions
Original file line numberDiff line numberDiff line change
@@ -1,114 +1,10 @@
1-
use anyhow::Result;
2-
3-
use crate::core;
4-
use crate::review::triage::triage_diff;
5-
6-
use super::super::comments::{filter_comments_for_diff, synthesize_analyzer_comments};
7-
use super::super::contracts::PreparedReviewJobs;
8-
use super::super::file_context::assemble_file_context;
9-
use super::super::services::PipelineServices;
10-
use super::super::session::ReviewSession;
11-
use super::jobs::build_file_review_jobs;
12-
use super::progress::PreparationProgress;
13-
14-
pub(in super::super) async fn prepare_file_review_jobs(
15-
services: &PipelineServices,
16-
session: &mut ReviewSession,
17-
) -> Result<PreparedReviewJobs> {
18-
let mut progress = PreparationProgress::new();
19-
let mut jobs = Vec::new();
20-
let mut next_job_order = 0usize;
21-
let repo_path_str = services.repo_path_str();
22-
23-
let mut batched_pre_analysis = services
24-
.plugin_manager
25-
.run_pre_analyzers_for_review(&session.diffs, &repo_path_str)
26-
.await?;
27-
28-
for (diff_index, diff) in session.diffs.iter().enumerate() {
29-
if skip_diff_if_needed(services, diff, &mut progress) {
30-
continue;
31-
}
32-
33-
let pre_analysis = batched_pre_analysis
34-
.remove(&diff.file_path)
35-
.unwrap_or_default();
36-
let deterministic_comments = filter_comments_for_diff(
37-
diff,
38-
synthesize_analyzer_comments(pre_analysis.findings.clone())?,
39-
);
40-
41-
let triage_result = triage_diff(diff);
42-
if triage_result.should_skip() {
43-
if deterministic_comments.is_empty() {
44-
tracing::info!(
45-
"Skipping {} (triage: {})",
46-
diff.file_path.display(),
47-
triage_result.reason()
48-
);
49-
progress.skip_file();
50-
} else {
51-
tracing::info!(
52-
"Skipping expensive LLM review for {} (triage: {}), keeping {} analyzer finding(s)",
53-
diff.file_path.display(),
54-
triage_result.reason(),
55-
deterministic_comments.len()
56-
);
57-
progress.complete_with_comments(session, diff, deterministic_comments);
58-
}
59-
continue;
60-
}
61-
62-
progress.report_current_file(session, diff);
63-
64-
let prepared_file = assemble_file_context(
65-
services,
66-
session,
67-
diff,
68-
pre_analysis.context_chunks,
69-
deterministic_comments,
70-
)
71-
.await?;
72-
73-
session
74-
.verification_context
75-
.insert(diff.file_path.clone(), prepared_file.context_chunks.clone());
76-
77-
let file_jobs = build_file_review_jobs(
78-
services,
79-
session,
80-
diff_index,
81-
diff,
82-
&prepared_file,
83-
next_job_order,
84-
)?;
85-
next_job_order += file_jobs.len();
86-
jobs.extend(file_jobs);
87-
}
88-
89-
Ok(progress.into_prepared_review_jobs(jobs))
90-
}
91-
92-
fn skip_diff_if_needed(
93-
services: &PipelineServices,
94-
diff: &core::UnifiedDiff,
95-
progress: &mut PreparationProgress,
96-
) -> bool {
97-
let skip_message = if services.config.should_exclude(&diff.file_path) {
98-
Some("Skipping excluded file")
99-
} else if diff.is_deleted {
100-
Some("Skipping deleted file")
101-
} else if diff.is_binary || diff.hunks.is_empty() {
102-
Some("Skipping non-text diff")
103-
} else {
104-
None
105-
};
106-
107-
let Some(skip_message) = skip_message else {
108-
return false;
109-
};
110-
111-
tracing::info!("{}: {}", skip_message, diff.file_path.display());
112-
progress.skip_file();
113-
true
114-
}
1+
#[path = "runner/analysis.rs"]
2+
mod analysis;
3+
#[path = "runner/diff.rs"]
4+
mod diff;
5+
#[path = "runner/run.rs"]
6+
mod run;
7+
#[path = "runner/skip.rs"]
8+
mod skip;
9+
10+
pub(in super::super) use run::prepare_file_review_jobs;

‎src/review/pipeline/prepare/runner/analysis.rs‎

Lines changed: 60 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,60 @@
1+
use anyhow::Result;
2+
use std::collections::HashMap;
3+
use std::path::PathBuf;
4+
5+
use crate::core;
6+
use crate::plugins::PreAnalysis;
7+
use crate::review::triage::triage_diff;
8+
9+
use super::super::super::comments::{filter_comments_for_diff, synthesize_analyzer_comments};
10+
11+
pub(super) enum DiffPreparationDecision {
12+
Skip,
13+
CompleteWithComments(Vec<core::Comment>),
14+
Review(PreparedDiffAnalysis),
15+
}
16+
17+
pub(super) struct PreparedDiffAnalysis {
18+
pub context_chunks: Vec<core::LLMContextChunk>,
19+
pub deterministic_comments: Vec<core::Comment>,
20+
}
21+
22+
pub(super) fn prepare_diff_analysis(
23+
diff: &core::UnifiedDiff,
24+
batched_pre_analysis: &mut HashMap<PathBuf, PreAnalysis>,
25+
) -> Result<DiffPreparationDecision> {
26+
let pre_analysis = batched_pre_analysis
27+
.remove(&diff.file_path)
28+
.unwrap_or_default();
29+
let deterministic_comments = filter_comments_for_diff(
30+
diff,
31+
synthesize_analyzer_comments(pre_analysis.findings.clone())?,
32+
);
33+
34+
let triage_result = triage_diff(diff);
35+
if triage_result.should_skip() {
36+
if deterministic_comments.is_empty() {
37+
tracing::info!(
38+
"Skipping {} (triage: {})",
39+
diff.file_path.display(),
40+
triage_result.reason()
41+
);
42+
return Ok(DiffPreparationDecision::Skip);
43+
}
44+
45+
tracing::info!(
46+
"Skipping expensive LLM review for {} (triage: {}), keeping {} analyzer finding(s)",
47+
diff.file_path.display(),
48+
triage_result.reason(),
49+
deterministic_comments.len()
50+
);
51+
return Ok(DiffPreparationDecision::CompleteWithComments(
52+
deterministic_comments,
53+
));
54+
}
55+
56+
Ok(DiffPreparationDecision::Review(PreparedDiffAnalysis {
57+
context_chunks: pre_analysis.context_chunks,
58+
deterministic_comments,
59+
}))
60+
}

‎src/review/pipeline/prepare/runner/diff.rs‎

Lines changed: 45 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,45 @@
1+
use anyhow::Result;
2+
3+
use crate::core;
4+
5+
use super::super::super::contracts::FileReviewJob;
6+
use super::super::super::file_context::assemble_file_context;
7+
use super::super::super::services::PipelineServices;
8+
use super::super::super::session::ReviewSession;
9+
use super::super::jobs::build_file_review_jobs;
10+
use super::super::progress::PreparationProgress;
11+
use super::analysis::PreparedDiffAnalysis;
12+
13+
pub(super) async fn prepare_diff_review_jobs(
14+
services: &PipelineServices,
15+
session: &mut ReviewSession,
16+
progress: &PreparationProgress,
17+
diff_index: usize,
18+
diff: &core::UnifiedDiff,
19+
analysis: PreparedDiffAnalysis,
20+
next_job_order: usize,
21+
) -> Result<Vec<FileReviewJob>> {
22+
progress.report_current_file(session, diff);
23+
24+
let prepared_file = assemble_file_context(
25+
services,
26+
session,
27+
diff,
28+
analysis.context_chunks,
29+
analysis.deterministic_comments,
30+
)
31+
.await?;
32+
33+
session
34+
.verification_context
35+
.insert(diff.file_path.clone(), prepared_file.context_chunks.clone());
36+
37+
build_file_review_jobs(
38+
services,
39+
session,
40+
diff_index,
41+
diff,
42+
&prepared_file,
43+
next_job_order,
44+
)
45+
}

‎src/review/pipeline/prepare/runner/run.rs‎

Lines changed: 57 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,57 @@
1+
use anyhow::Result;
2+
3+
use super::super::super::contracts::PreparedReviewJobs;
4+
use super::super::super::services::PipelineServices;
5+
use super::super::super::session::ReviewSession;
6+
use super::super::progress::PreparationProgress;
7+
use super::analysis::{prepare_diff_analysis, DiffPreparationDecision};
8+
use super::diff::prepare_diff_review_jobs;
9+
use super::skip::skip_diff_if_needed;
10+
11+
pub(in super::super::super) async fn prepare_file_review_jobs(
12+
services: &PipelineServices,
13+
session: &mut ReviewSession,
14+
) -> Result<PreparedReviewJobs> {
15+
let mut progress = PreparationProgress::new();
16+
let mut jobs = Vec::new();
17+
let mut next_job_order = 0usize;
18+
let repo_path_str = services.repo_path_str();
19+
20+
let mut batched_pre_analysis = services
21+
.plugin_manager
22+
.run_pre_analyzers_for_review(&session.diffs, &repo_path_str)
23+
.await?;
24+
25+
for diff_index in 0..session.diffs.len() {
26+
let diff = session.diffs[diff_index].clone();
27+
28+
if skip_diff_if_needed(services, &diff, &mut progress) {
29+
continue;
30+
}
31+
32+
match prepare_diff_analysis(&diff, &mut batched_pre_analysis)? {
33+
DiffPreparationDecision::Skip => {
34+
progress.skip_file();
35+
}
36+
DiffPreparationDecision::CompleteWithComments(comments) => {
37+
progress.complete_with_comments(session, &diff, comments);
38+
}
39+
DiffPreparationDecision::Review(analysis) => {
40+
let file_jobs = prepare_diff_review_jobs(
41+
services,
42+
session,
43+
&progress,
44+
diff_index,
45+
&diff,
46+
analysis,
47+
next_job_order,
48+
)
49+
.await?;
50+
next_job_order += file_jobs.len();
51+
jobs.extend(file_jobs);
52+
}
53+
}
54+
}
55+
56+
Ok(progress.into_prepared_review_jobs(jobs))
57+
}

‎src/review/pipeline/prepare/runner/skip.rs‎

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,28 @@
1+
use crate::core;
2+
3+
use super::super::super::services::PipelineServices;
4+
use super::super::progress::PreparationProgress;
5+
6+
pub(super) fn skip_diff_if_needed(
7+
services: &PipelineServices,
8+
diff: &core::UnifiedDiff,
9+
progress: &mut PreparationProgress,
10+
) -> bool {
11+
let skip_message = if services.config.should_exclude(&diff.file_path) {
12+
Some("Skipping excluded file")
13+
} else if diff.is_deleted {
14+
Some("Skipping deleted file")
15+
} else if diff.is_binary || diff.hunks.is_empty() {
16+
Some("Skipping non-text diff")
17+
} else {
18+
None
19+
};
20+
21+
let Some(skip_message) = skip_message else {
22+
return false;
23+
};
24+
25+
tracing::info!("{}: {}", skip_message, diff.file_path.display());
26+
progress.skip_file();
27+
true
28+
}

0 commit comments

Comments
 (0)