Skip to content

Commit 69bafce

Browse files
committed
refactor: split pipeline session support modules
1 parent 72e0019 commit 69bafce

File tree

9 files changed

+354
-329
lines changed

9 files changed

+354
-329
lines changed

TODO.md

Lines changed: 11 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -2,11 +2,11 @@
22

33
## Goals
44

5-
- [ ] Extract shared phase contracts so `prepare.rs`, `execution.rs`, and `postprocess.rs` stop depending on `execution.rs` internals.
6-
- [ ] Decompose `prepare_file_review_jobs()` into smaller context-assembly and request-building steps.
7-
- [ ] Split `session.rs` into service/bootstrap concerns and repo-support concerns.
8-
- [ ] Keep the refactor behavior-preserving and validate with `cargo fmt`, `cargo clippy --all-targets --all-features -- -D warnings`, and `cargo test` after each slice.
9-
- [ ] Commit and push regularly after each completed slice.
5+
- [x] Extract shared phase contracts so `prepare.rs`, `execution.rs`, and `postprocess.rs` stop depending on `execution.rs` internals.
6+
- [x] Decompose `prepare_file_review_jobs()` into smaller context-assembly and request-building steps.
7+
- [x] Split `session.rs` into service/bootstrap concerns and repo-support concerns.
8+
- [x] Keep the refactor behavior-preserving and validate with `cargo fmt`, `cargo clippy --all-targets --all-features -- -D warnings`, and `cargo test` after each slice.
9+
- [x] Commit and push regularly after each completed slice.
1010

1111
## Slice 1 — shared phase contracts
1212

@@ -29,16 +29,16 @@
2929
- file eligibility / triage handling
3030
- context assembly
3131
- request/job construction
32-
- [ ] Validate, commit, and push.
32+
- [x] Validate, commit, and push.
3333

3434
## Slice 3 — session split
3535

36-
- [ ] Create `src/review/pipeline/services.rs` for `PipelineServices` and service bootstrapping.
37-
- [ ] Create `src/review/pipeline/repo_support.rs` for repo/runtime helpers:
36+
- [x] Create `src/review/pipeline/services.rs` for `PipelineServices` and service bootstrapping.
37+
- [x] Create `src/review/pipeline/repo_support.rs` for repo/runtime helpers:
3838
- diff chunking
3939
- instruction file detection
4040
- git log gathering
4141
- convention store persistence
42-
- [ ] Keep `ReviewSession` focused on per-review state in `session.rs`.
43-
- [ ] Update imports in `pipeline.rs`, `prepare.rs`, and `postprocess.rs`.
44-
- [ ] Validate, commit, and push.
42+
- [x] Keep `ReviewSession` focused on per-review state in `session.rs`.
43+
- [x] Update imports in `pipeline.rs`, `prepare.rs`, and `postprocess.rs`.
44+
- [x] Validate, commit, and push.

src/review/pipeline.rs

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -18,8 +18,12 @@ mod guidance;
1818
mod postprocess;
1919
#[path = "pipeline/prepare.rs"]
2020
mod prepare;
21+
#[path = "pipeline/repo_support.rs"]
22+
mod repo_support;
2123
#[path = "pipeline/request.rs"]
2224
mod request;
25+
#[path = "pipeline/services.rs"]
26+
mod services;
2327
#[path = "pipeline/session.rs"]
2428
mod session;
2529
#[path = "pipeline/types.rs"]
@@ -29,7 +33,9 @@ use contracts::ReviewExecutionContext;
2933
use execution::execute_review_jobs;
3034
use postprocess::run_postprocess;
3135
use prepare::prepare_file_review_jobs;
32-
use session::{chunk_diff_for_context, should_optimize_for_local, PipelineServices, ReviewSession};
36+
use repo_support::chunk_diff_for_context;
37+
use services::{should_optimize_for_local, PipelineServices};
38+
use session::ReviewSession;
3339

3440
pub use comments::{filter_comments_for_diff, is_line_in_diff};
3541
pub use context::{build_symbol_index, extract_symbols_from_diff};

src/review/pipeline/contracts.rs

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,8 @@ use crate::adapters;
55
use crate::config;
66
use crate::core;
77

8-
use super::session::{PipelineServices, ReviewSession};
8+
use super::services::PipelineServices;
9+
use super::session::ReviewSession;
910
use super::types::{AgentActivity, FileMetric};
1011

1112
pub(super) struct PreparedReviewJobs {

src/review/pipeline/postprocess.rs

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,9 @@ use super::super::feedback::derive_file_patterns;
99
use super::super::filters::{apply_feedback_confidence_adjustment, apply_review_filters};
1010
use super::comments::is_analyzer_comment;
1111
use super::contracts::ExecutionSummary;
12-
use super::session::{save_convention_store, PipelineServices, ReviewSession};
12+
use super::repo_support::save_convention_store;
13+
use super::services::PipelineServices;
14+
use super::session::ReviewSession;
1315
use super::types::ReviewResult;
1416

1517
pub(super) async fn run_postprocess(

src/review/pipeline/prepare.rs

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,8 @@ use super::comments::{filter_comments_for_diff, synthesize_analyzer_comments};
77
use super::context::{extract_symbols_from_diff, gather_related_file_context};
88
use super::contracts::{FileReviewJob, PreparedReviewJobs};
99
use super::request::{build_review_request, specialized_passes};
10-
use super::session::{PipelineServices, ReviewSession};
10+
use super::services::PipelineServices;
11+
use super::session::ReviewSession;
1112
use super::types::ProgressUpdate;
1213

1314
struct PreparedFileContext {
Lines changed: 193 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,193 @@
1+
use std::path::{Path, PathBuf};
2+
use tracing::{info, warn};
3+
4+
use crate::config;
5+
use crate::core;
6+
7+
pub(super) fn chunk_diff_for_context(diff_content: &str, max_chars: usize) -> Vec<String> {
8+
if diff_content.len() <= max_chars {
9+
return vec![diff_content.to_string()];
10+
}
11+
12+
let mut chunks = Vec::new();
13+
let mut current_chunk = String::new();
14+
15+
for section in diff_content.split("\ndiff --git ") {
16+
let section = if chunks.is_empty() && current_chunk.is_empty() {
17+
section.to_string()
18+
} else {
19+
format!("diff --git {}", section)
20+
};
21+
22+
if current_chunk.len() + section.len() > max_chars && !current_chunk.is_empty() {
23+
chunks.push(current_chunk);
24+
current_chunk = section;
25+
} else {
26+
current_chunk.push_str(&section);
27+
}
28+
}
29+
30+
if !current_chunk.is_empty() {
31+
chunks.push(current_chunk);
32+
}
33+
34+
chunks
35+
}
36+
37+
pub(super) fn detect_instruction_files(repo_path: &Path) -> Vec<(String, String)> {
38+
const INSTRUCTION_FILES: &[&str] = &[
39+
".cursorrules",
40+
"CLAUDE.md",
41+
".claude/CLAUDE.md",
42+
"agents.md",
43+
".github/copilot-instructions.md",
44+
"GEMINI.md",
45+
".diffscope-instructions.md",
46+
];
47+
const MAX_INSTRUCTION_SIZE: u64 = 10_000;
48+
49+
let mut results = Vec::new();
50+
for filename in INSTRUCTION_FILES {
51+
let path = repo_path.join(filename);
52+
if path.is_file() {
53+
if let Ok(meta) = std::fs::metadata(&path) {
54+
if meta.len() > MAX_INSTRUCTION_SIZE {
55+
warn!(
56+
"Skipping instruction file {} ({} bytes exceeds {})",
57+
filename,
58+
meta.len(),
59+
MAX_INSTRUCTION_SIZE
60+
);
61+
continue;
62+
}
63+
}
64+
if let Ok(content) = std::fs::read_to_string(&path) {
65+
let trimmed = content.trim().to_string();
66+
if !trimmed.is_empty() {
67+
info!("Auto-detected instruction file: {}", filename);
68+
results.push((filename.to_string(), trimmed));
69+
}
70+
}
71+
}
72+
}
73+
results
74+
}
75+
76+
pub(super) fn gather_git_log(repo_path: &Path) -> Option<String> {
77+
let output = std::process::Command::new("git")
78+
.args([
79+
"log",
80+
"--numstat",
81+
"--format=commit %H%nAuthor: %an <%ae>%nDate: %ai%n%n %s%n",
82+
"-100",
83+
])
84+
.current_dir(repo_path)
85+
.output();
86+
match output {
87+
Ok(out) if out.status.success() => {
88+
let log_text = String::from_utf8_lossy(&out.stdout).to_string();
89+
if log_text.trim().is_empty() {
90+
None
91+
} else {
92+
info!("Gathered git log ({} bytes)", log_text.len());
93+
Some(log_text)
94+
}
95+
}
96+
_ => {
97+
info!("Git log unavailable (not a git repo or git not found)");
98+
None
99+
}
100+
}
101+
}
102+
103+
pub(super) fn resolve_convention_store_path(config: &config::Config) -> Option<PathBuf> {
104+
if let Some(ref path) = config.convention_store_path {
105+
return Some(PathBuf::from(path));
106+
}
107+
dirs::data_local_dir().map(|dir| dir.join("diffscope").join("conventions.json"))
108+
}
109+
110+
pub(super) fn save_convention_store(
111+
store: &core::convention_learner::ConventionStore,
112+
path: &PathBuf,
113+
) {
114+
if let Ok(json) = store.to_json() {
115+
if let Some(parent) = path.parent() {
116+
let _ = std::fs::create_dir_all(parent);
117+
}
118+
if let Err(error) = std::fs::write(path, json) {
119+
warn!(
120+
"Failed to save convention store to {}: {}",
121+
path.display(),
122+
error
123+
);
124+
}
125+
}
126+
}
127+
128+
#[cfg(test)]
129+
mod tests {
130+
use super::*;
131+
132+
#[test]
133+
fn detect_instruction_files_empty_dir() {
134+
let dir = tempfile::tempdir().unwrap();
135+
let results = detect_instruction_files(dir.path());
136+
assert!(results.is_empty());
137+
}
138+
139+
#[test]
140+
fn detect_instruction_files_finds_cursorrules() {
141+
let dir = tempfile::tempdir().unwrap();
142+
std::fs::write(dir.path().join(".cursorrules"), "Use tabs not spaces").unwrap();
143+
let results = detect_instruction_files(dir.path());
144+
assert_eq!(results.len(), 1);
145+
assert_eq!(results[0].0, ".cursorrules");
146+
assert!(results[0].1.contains("Use tabs"));
147+
}
148+
149+
#[test]
150+
fn chunk_diff_small_diff_returns_single_chunk() {
151+
let diff = "diff --git a/foo.rs b/foo.rs\n+hello\n";
152+
let chunks = chunk_diff_for_context(diff, 1000);
153+
assert_eq!(chunks.len(), 1);
154+
assert_eq!(chunks[0], diff);
155+
}
156+
157+
#[test]
158+
fn chunk_diff_splits_at_file_boundaries() {
159+
let diff = "diff --git a/a.rs b/a.rs\n+line1\n\ndiff --git a/b.rs b/b.rs\n+line2\n\ndiff --git a/c.rs b/c.rs\n+line3\n";
160+
let chunks = chunk_diff_for_context(diff, 40);
161+
assert!(chunks.len() >= 2);
162+
for chunk in &chunks {
163+
assert!(chunk.contains("diff --git"));
164+
}
165+
}
166+
167+
#[test]
168+
fn chunk_diff_empty_input() {
169+
let chunks = chunk_diff_for_context("", 100);
170+
assert_eq!(chunks.len(), 1);
171+
assert_eq!(chunks[0], "");
172+
}
173+
174+
#[test]
175+
fn chunk_diff_single_large_file_not_split_midfile() {
176+
let diff = format!("diff --git a/big.rs b/big.rs\n{}", "+line\n".repeat(100));
177+
let chunks = chunk_diff_for_context(&diff, 50);
178+
assert_eq!(chunks.len(), 1);
179+
}
180+
181+
#[test]
182+
fn chunk_diff_preserves_all_content() {
183+
let file_a = "diff --git a/a.rs b/a.rs\n+alpha\n";
184+
let file_b = "\ndiff --git a/b.rs b/b.rs\n+beta\n";
185+
let file_c = "\ndiff --git a/c.rs b/c.rs\n+gamma\n";
186+
let diff = format!("{}{}{}", file_a, file_b, file_c);
187+
let chunks = chunk_diff_for_context(&diff, 50);
188+
let rejoined = chunks.join("");
189+
assert!(rejoined.contains("+alpha"));
190+
assert!(rejoined.contains("+beta"));
191+
assert!(rejoined.contains("+gamma"));
192+
}
193+
}

src/review/pipeline/request.rs

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,8 @@ use crate::core;
66
use crate::core::offline::optimize_prompt_for_local;
77

88
use super::guidance::build_review_guidance;
9-
use super::session::{PipelineServices, ReviewSession};
9+
use super::services::PipelineServices;
10+
use super::session::ReviewSession;
1011

1112
pub(super) fn specialized_passes(config: &config::Config) -> Vec<core::SpecializedPassKind> {
1213
if !config.multi_pass_specialized {

0 commit comments

Comments
 (0)