Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
# will have compiled files and executables
debug/
target/
jobs/**

# Remove Cargo.lock from gitignore if creating an executable, leave it for libraries
# More information here https://doc.rust-lang.org/cargo/guide/cargo-toml-vs-cargo-lock.html
Expand Down
126 changes: 113 additions & 13 deletions crates/forge_services/src/tool_services/fs_patch.rs
Original file line number Diff line number Diff line change
Expand Up @@ -67,24 +67,22 @@ impl Range {
return Self::new(0, 0);
}

// Detect the line ending style used in the source
let line_ending = Self::detect_line_ending(source);
let line_ending_len = line_ending.len();

// SearchMatch uses 0-based inclusive line numbers
// Convert to 0-based array indices
let start_idx = (search_match.start_line as usize).min(lines.len());
// end_line is 0-based inclusive, convert to 0-based exclusive for slicing
// Add 1 to make it exclusive: line 0 to line 0 means [0..1], one line
let end_idx = ((search_match.end_line as usize) + 1).min(lines.len());

// Find the character position of the start line
// Sum the lengths of all lines before start_idx, adding the appropriate line
// ending length
let start_pos = lines[..start_idx]
.iter()
.map(|l| l.len() + line_ending_len)
.sum::<usize>();
// Find the byte position of the start line.
// Split on '\n' so each segment retains its '\r' (if any), giving the
// correct per-line byte length regardless of mixed line endings.
let start_pos = source
.split('\n')
.take(start_idx)
.map(|l| l.len() + 1)
.sum::<usize>()
.min(source.len());

// Calculate the length
let length = if start_idx == end_idx {
Expand All @@ -97,14 +95,20 @@ impl Range {
} else {
// Multi-line match: include newlines between lines but NOT after the last line
// Sum lengths of lines from start_idx to end_idx (exclusive)
// Add appropriate line ending length for each newline between lines
let content_len: usize = if start_idx >= lines.len() || end_idx > lines.len() {
0 // Out of bounds match
} else {
lines[start_idx..end_idx].iter().map(|l| l.len()).sum()
};
let newlines_between = end_idx - start_idx - 1;
content_len + (newlines_between * line_ending_len)
// Count actual newline bytes (\r\n = 2, \n = 1) to handle mixed endings
let newline_bytes: usize = source
.split('\n')
.skip(start_idx)
.take(newlines_between)
.map(|l| if l.ends_with('\r') { 2 } else { 1 })
.sum();
content_len + newline_bytes
};

Self::new(start_pos, length)
Expand Down Expand Up @@ -135,6 +139,10 @@ enum Error {
"Multiple matches found for search text: '{0}'. Either provide a more specific search pattern or use replace_all to replace all occurrences."
)]
MultipleMatches(String),
#[error(
"Match range [{0}..{1}) is out of bounds for content of length {2}. File may have changed externally, consider reading the file again."
)]
RangeOutOfBounds(usize, usize, usize),
}

/// Compute a range from search text, with operation-aware error handling
Expand Down Expand Up @@ -189,6 +197,15 @@ fn apply_replacement(
let normalized_content = Range::normalize_search_line_endings(&haystack, content);
// Handle case where range is provided (match found)
if let Some(patch) = range {
// Validate the range is within bounds before indexing
if patch.end() > haystack.len() {
return Err(Error::RangeOutOfBounds(
patch.start,
patch.end(),
haystack.len(),
));
}

// Extract the matched text from haystack
let needle = &haystack[patch.start..patch.end()];

Expand Down Expand Up @@ -1043,4 +1060,87 @@ mod tests {

assert_eq!(actual, expected);
}

// --- Out-of-bounds safety tests ---

#[test]
fn test_range_from_search_match_out_of_bounds_start_line() {
let source = "line1\nline2\nline3";
// start_line way past end of file
let search_match = SearchMatch { start_line: 100, end_line: 200 };

let range = super::Range::from_search_match(source, &search_match);
// Should not panic; range should be clamped so it doesn't exceed source
assert!(range.end() <= source.len());
}

#[test]
fn test_range_from_search_match_end_line_past_eof() {
let source = "line1\nline2\nline3";
// start_line valid, end_line past end
let search_match = SearchMatch { start_line: 1, end_line: 100 };

let range = super::Range::from_search_match(source, &search_match);
assert!(range.end() <= source.len());
// Should include from line2 to end of source
let actual = &source[range.start..range.end()];
assert!(actual.contains("line2"));
assert!(actual.contains("line3"));
}

#[test]
fn test_range_from_search_match_trailing_newline() {
let source = "line1\nline2\nline3\n"; // trailing newline
let search_match = SearchMatch { start_line: 2, end_line: 2 };

let range = super::Range::from_search_match(source, &search_match);
assert!(range.end() <= source.len());
let actual = &source[range.start..range.end()];
assert_eq!(actual, "line3");
}

#[test]
fn test_range_from_search_match_unicode_content() {
let source = "héllo\nwørld\n🌍";
let search_match = SearchMatch { start_line: 1, end_line: 1 };

let range = super::Range::from_search_match(source, &search_match);
assert!(range.end() <= source.len());
let actual = &source[range.start..range.end()];
assert_eq!(actual, "wørld");
}

#[test]
fn test_range_from_search_match_unicode_multiline() {
let source = "héllo\nwørld\n🌍";
let search_match = SearchMatch { start_line: 0, end_line: 2 };

let range = super::Range::from_search_match(source, &search_match);
assert!(range.end() <= source.len());
let actual = &source[range.start..range.end()];
assert_eq!(actual, source);
}

#[test]
fn test_range_from_search_match_mixed_line_endings() {
let source = "line1\r\nline2\nline3";
let search_match = SearchMatch { start_line: 1, end_line: 1 };

let range = super::Range::from_search_match(source, &search_match);
assert!(range.end() <= source.len());
let actual = &source[range.start..range.end()];
assert_eq!(actual, "line2");
}

#[test]
fn test_apply_replacement_with_out_of_bounds_range_returns_error() {
let source = "short";
// Simulate a bad range that exceeds source length
let bad_range = Some(super::Range::new(0, 1000));
let operation = PatchOperation::Replace;
let content = "replacement";

let result = super::apply_replacement(source.to_string(), bad_range, &operation, content);
assert!(result.is_err());
}
}
Loading