Merge pull request #3332 from ultraworkers/lane/gjc-s21

feat(tools): GJC-compatible read selectors (#3323)
This commit is contained in:
Bellman authored and GitHub committed 2026-10-13 08:52:18 +09:00
commit a2eb74c00e
5 files changed
+958 -11

No files matched your search

+224 -7
View File
@@ -10,6 +10,8 @@ use regex::RegexBuilder;
use serde::{Deserialize, Serialize};
use walkdir::{DirEntry, WalkDir};
use crate::line_range_selector::{self, LineRangeSelector};
/// Maximum file size that can be read (10 MB).
const MAX_READ_SIZE: u64 = 10 * 1024 * 1024;
@@ -88,6 +90,9 @@ pub struct ReadFileOutput {
#[serde(rename = "type")]
pub kind: String,
pub file: TextFilePayload,
#[serde(skip_serializing_if = "Option::is_none")]
#[serde(rename = "truncationFooter")]
pub truncation_footer: Option<String>,
}
/// Structured patch hunk emitted by write and edit operations.
@@ -201,6 +206,19 @@ pub fn read_file(
path: &str,
offset: Option<usize>,
limit: Option<usize>,
) -> io::Result<ReadFileOutput> {
read_file_with_selector(path, offset, limit, None)
}
/// Reads a text file with optional GJC-style line range selector.
///
/// The selector takes precedence over offset/limit if provided.
/// Selector syntax: `:50`, `:50-200`, `:50+150`, `:raw`, `:conflicts`, `:5-16,960-973`
pub fn read_file_with_selector(
path: &str,
offset: Option<usize>,
limit: Option<usize>,
selector: Option<&str>,
) -> io::Result<ReadFileOutput> {
let absolute_path = normalize_path(path)?;
@@ -227,11 +245,28 @@ pub fn read_file(
let content = fs::read_to_string(&absolute_path)?;
let lines: Vec<&str> = content.lines().collect();
let start_index = offset.unwrap_or(0).min(lines.len());
let end_index = limit.map_or(lines.len(), |limit| {
start_index.saturating_add(limit).min(lines.len())
});
let selected = lines[start_index..end_index].join("\n");
let total_lines = lines.len();
// Determine which lines to select
let (start_index, end_index, truncation_footer) = if let Some(sel) = selector {
// Parse and apply selector
let parsed = line_range_selector::parse_selector(sel)
.map_err(|e| io::Error::new(io::ErrorKind::InvalidInput, e.to_string()))?;
apply_line_selector(&lines, parsed, path)
} else {
// Use offset/limit for backward compatibility
let start = offset.unwrap_or(0).min(lines.len());
let end = limit.map_or(lines.len(), |lim| {
start.saturating_add(lim).min(lines.len())
});
(start, end, None)
};
let selected = if start_index < end_index {
lines[start_index..end_index].join("\n")
} else {
String::new()
};
Ok(ReadFileOutput {
kind: String::from("text"),
@@ -240,11 +275,66 @@ pub fn read_file(
content: selected,
num_lines: end_index.saturating_sub(start_index),
start_line: start_index.saturating_add(1),
total_lines: lines.len(),
total_lines,
},
truncation_footer,
})
}
/// Apply a parsed line selector to file lines and return the selected range.
///
/// Returns (start_index, end_index, truncation_footer).
fn apply_line_selector(
lines: &[&str],
selector: LineRangeSelector,
_file_path: &str,
) -> (usize, usize, Option<String>) {
match selector {
LineRangeSelector::Raw => {
// Return entire file without truncation footer for raw mode
(0, lines.len(), None)
}
LineRangeSelector::Conflicts => {
// For now, return entire file (git merge conflict extraction would go here)
(0, lines.len(), None)
}
LineRangeSelector::Lines(ranges) => {
// Collect all lines specified by ranges
let mut selected_indices = std::collections::BTreeSet::new();
for range in ranges {
let (start_zero, end_zero) = range.zero_indexed();
for i in start_zero..=end_zero.min(lines.len().saturating_sub(1)) {
selected_indices.insert(i);
}
}
if selected_indices.is_empty() {
return (0, 0, None);
}
// Find contiguous blocks and return the first one (or could merge all)
let indices: Vec<usize> = selected_indices.iter().copied().collect();
let first = indices[0];
let last = indices[indices.len() - 1];
// Check if it's a contiguous range
let is_contiguous = (last - first + 1) == indices.len();
if is_contiguous {
(first, last + 1, None)
} else {
// Return all selected indices as one continuous block from first to last
// In the future, this could be improved to show multiple blocks
(first, last + 1, None)
}
}
LineRangeSelector::None => {
// No selector, return entire file
(0, lines.len(), None)
}
}
}
/// Replaces a file's contents and returns patch metadata.
pub fn write_file(path: &str, content: &str) -> io::Result<WriteFileOutput> {
if content.len() > MAX_WRITE_SIZE {
@@ -715,11 +805,23 @@ pub fn read_file_in_workspace(
offset: Option<usize>,
limit: Option<usize>,
workspace_root: &Path,
) -> io::Result<ReadFileOutput> {
read_file_in_workspace_with_selector(path, offset, limit, None, workspace_root)
}
/// Read a file with workspace boundary enforcement and optional line range selector.
#[allow(dead_code)]
pub fn read_file_in_workspace_with_selector(
path: &str,
offset: Option<usize>,
limit: Option<usize>,
selector: Option<&str>,
workspace_root: &Path,
) -> io::Result<ReadFileOutput> {
let absolute_path = normalize_path(path)?;
let canonical_root = canonicalize_workspace_root(workspace_root);
validate_workspace_boundary(&absolute_path, &canonical_root)?;
read_file(path, offset, limit)
read_file_with_selector(path, offset, limit, selector)
}
/// Write a file with workspace boundary enforcement.
@@ -1183,4 +1285,119 @@ mod tests {
assert!(component_contains_glob("*.rs"));
assert!(!component_contains_glob("src"));
}
#[test]
fn read_with_selector_single_line() {
let path = temp_path("selector-single.txt");
write_file(
path.to_string_lossy().as_ref(),
"line1\nline2\nline3\nline4\nline5",
)
.expect("write should succeed");
let output =
super::read_file_with_selector(path.to_string_lossy().as_ref(), None, None, Some("2"))
.expect("read with selector should succeed");
assert_eq!(output.file.content, "line2");
assert_eq!(output.file.start_line, 2);
assert_eq!(output.file.num_lines, 1);
}
#[test]
fn read_with_selector_range() {
let path = temp_path("selector-range.txt");
write_file(
path.to_string_lossy().as_ref(),
"line1\nline2\nline3\nline4\nline5",
)
.expect("write should succeed");
let output = super::read_file_with_selector(
path.to_string_lossy().as_ref(),
None,
None,
Some("2-4"),
)
.expect("read with selector should succeed");
assert_eq!(output.file.content, "line2\nline3\nline4");
assert_eq!(output.file.start_line, 2);
assert_eq!(output.file.num_lines, 3);
}
#[test]
fn read_with_selector_plus_count() {
let path = temp_path("selector-plus.txt");
write_file(
path.to_string_lossy().as_ref(),
"line1\nline2\nline3\nline4\nline5",
)
.expect("write should succeed");
let output = super::read_file_with_selector(
path.to_string_lossy().as_ref(),
None,
None,
Some("2+2"),
)
.expect("read with selector should succeed");
assert_eq!(output.file.content, "line2\nline3");
assert_eq!(output.file.start_line, 2);
assert_eq!(output.file.num_lines, 2);
}
#[test]
fn read_with_selector_raw() {
let path = temp_path("selector-raw.txt");
write_file(
path.to_string_lossy().as_ref(),
"line1\nline2\nline3\nline4\nline5",
)
.expect("write should succeed");
let output = super::read_file_with_selector(
path.to_string_lossy().as_ref(),
None,
None,
Some("raw"),
)
.expect("read with selector should succeed");
assert_eq!(output.file.content, "line1\nline2\nline3\nline4\nline5");
assert_eq!(output.file.start_line, 1);
assert_eq!(output.file.num_lines, 5);
}
#[test]
fn read_with_selector_none() {
let path = temp_path("selector-none.txt");
write_file(
path.to_string_lossy().as_ref(),
"line1\nline2\nline3\nline4\nline5",
)
.expect("write should succeed");
let output =
super::read_file_with_selector(path.to_string_lossy().as_ref(), None, None, None)
.expect("read with selector should succeed");
assert_eq!(output.file.content, "line1\nline2\nline3\nline4\nline5");
assert_eq!(output.file.start_line, 1);
assert_eq!(output.file.num_lines, 5);
}
#[test]
fn read_with_invalid_selector() {
let path = temp_path("selector-invalid.txt");
write_file(
path.to_string_lossy().as_ref(),
"line1\nline2\nline3\nline4\nline5",
)
.expect("write should succeed");
let result = super::read_file_with_selector(
path.to_string_lossy().as_ref(),
None,
None,
Some("invalid-selector"),
);
assert!(result.is_err());
}
}
+3 -1
View File
@@ -20,6 +20,7 @@ pub mod green_contract;
mod hooks;
mod json;
mod lane_events;
pub mod line_range_selector;
pub mod lsp_client;
mod mcp;
mod mcp_client;
@@ -87,7 +88,8 @@ pub use conversation::{
};
pub use file_ops::{
edit_file, edit_file_in_workspace, glob_search, glob_search_in_workspace, grep_search,
grep_search_in_workspace, read_file, read_file_in_workspace, write_file,
grep_search_in_workspace, read_file, read_file_in_workspace,
read_file_in_workspace_with_selector, read_file_with_selector, write_file,
write_file_in_workspace, EditFileOutput, GlobSearchOutput, GrepSearchInput, GrepSearchOutput,
ReadFileOutput, StructuredPatchHunk, TextFilePayload, WriteFileOutput,
};
@@ -0,0 +1,347 @@
/// GJC-compatible line range selector parsing for the read tool.
///
/// Supports selector syntax:
/// - `:50` = start at line 50
/// - `:50-200` = lines 50-200 inclusive
/// - `:50+150` = 150 lines starting at 50
/// - `:50+1` = exactly one line
/// - `:5-16,960-973` = multiple ranges
/// - `:raw` = full verbatim
/// - `:conflicts` = git merge conflicts only
use std::num::ParseIntError;
/// Parsed line range specification.
#[derive(Debug, Clone, PartialEq, Eq)]
pub enum LineRangeSelector {
/// Full file verbatim (`:raw`)
Raw,
/// Git merge conflict sections only (`:conflicts`)
Conflicts,
/// Line range(s) (`:L-L+N` syntax)
Lines(Vec<LineRange>),
/// No selector (full file, may be truncated by default budget)
None,
}
/// A single line range, zero-indexed internally but displayed as 1-indexed.
#[derive(Debug, Clone, Copy, PartialEq, Eq)]
pub struct LineRange {
/// Start line (1-indexed, user-facing)
pub start_line: usize,
/// End line inclusive (1-indexed, user-facing)
pub end_line: usize,
}
impl LineRange {
/// Create a new line range (1-indexed).
pub fn new(start_line: usize, end_line: usize) -> Self {
Self {
start_line,
end_line,
}
}
/// Convert to zero-indexed range for internal use.
pub fn zero_indexed(&self) -> (usize, usize) {
(
self.start_line.saturating_sub(1),
self.end_line.saturating_sub(1),
)
}
/// Get the number of lines in this range.
pub fn len(&self) -> usize {
if self.start_line <= self.end_line {
self.end_line - self.start_line + 1
} else {
0
}
}
/// Check if this range is valid.
pub fn is_valid(&self) -> bool {
self.start_line > 0 && self.end_line > 0 && self.start_line <= self.end_line
}
}
/// Parse error types for line range selectors.
#[derive(Debug, Clone, PartialEq, Eq)]
pub enum ParseError {
InvalidFormat(String),
InvalidNumber(ParseIntError),
InvalidRange(String),
}
impl std::fmt::Display for ParseError {
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
match self {
ParseError::InvalidFormat(msg) => write!(f, "Invalid format: {}", msg),
ParseError::InvalidNumber(e) => write!(f, "Invalid number: {}", e),
ParseError::InvalidRange(msg) => write!(f, "Invalid range: {}", msg),
}
}
}
impl std::error::Error for ParseError {}
impl From<ParseIntError> for ParseError {
fn from(e: ParseIntError) -> Self {
ParseError::InvalidNumber(e)
}
}
/// Parse a line range selector string.
///
/// Examples:
/// - `"50"` → `LineRangeSelector::Lines(vec![LineRange::new(50, 50)])`
/// - `"50-200"` → `LineRangeSelector::Lines(vec![LineRange::new(50, 200)])`
/// - `"50+150"` → `LineRangeSelector::Lines(vec![LineRange::new(50, 199)])`
/// - `"raw"` → `LineRangeSelector::Raw`
/// - `"conflicts"` → `LineRangeSelector::Conflicts`
pub fn parse_selector(selector: &str) -> Result<LineRangeSelector, ParseError> {
if selector.is_empty() {
return Ok(LineRangeSelector::None);
}
match selector {
"raw" => Ok(LineRangeSelector::Raw),
"conflicts" => Ok(LineRangeSelector::Conflicts),
_ => parse_line_ranges(selector),
}
}
/// Parse comma-separated line ranges.
fn parse_line_ranges(selector: &str) -> Result<LineRangeSelector, ParseError> {
let mut ranges = Vec::new();
for range_str in selector.split(',') {
let range_str = range_str.trim();
if range_str.is_empty() {
continue;
}
let range = parse_single_range(range_str)?;
ranges.push(range);
}
if ranges.is_empty() {
Ok(LineRangeSelector::None)
} else {
// Merge overlapping/adjacent ranges
ranges.sort_by_key(|r| r.start_line);
let merged = merge_ranges(ranges);
Ok(LineRangeSelector::Lines(merged))
}
}
/// Parse a single line range in formats: "50", "50-200", "50+150".
fn parse_single_range(range_str: &str) -> Result<LineRange, ParseError> {
// Try "50+150" format first (start+count)
if let Some(plus_idx) = range_str.find('+') {
let start_str = &range_str[..plus_idx];
let count_str = &range_str[plus_idx + 1..];
if start_str.is_empty() || count_str.is_empty() {
return Err(ParseError::InvalidFormat(range_str.to_string()));
}
let start = start_str.parse::<usize>()?;
let count = count_str.parse::<usize>()?;
if start == 0 || count == 0 {
return Err(ParseError::InvalidRange(
"Line numbers and counts must be >= 1".to_string(),
));
}
return Ok(LineRange::new(start, start + count - 1));
}
// Try "50-200" format (start-end)
if let Some(dash_idx) = range_str.find('-') {
// Skip leading dash (negative numbers aren't valid here)
if dash_idx == 0 {
return Err(ParseError::InvalidFormat(range_str.to_string()));
}
let start_str = &range_str[..dash_idx];
let end_str = &range_str[dash_idx + 1..];
if start_str.is_empty() || end_str.is_empty() {
return Err(ParseError::InvalidFormat(range_str.to_string()));
}
let start = start_str.parse::<usize>()?;
let end = end_str.parse::<usize>()?;
if start == 0 || end == 0 {
return Err(ParseError::InvalidRange(
"Line numbers must be >= 1".to_string(),
));
}
if start > end {
return Err(ParseError::InvalidRange(format!(
"Start line {} cannot be greater than end line {}",
start, end
)));
}
return Ok(LineRange::new(start, end));
}
// Try plain "50" format (single line)
let line_num = range_str.parse::<usize>()?;
if line_num == 0 {
return Err(ParseError::InvalidRange(
"Line number must be >= 1".to_string(),
));
}
Ok(LineRange::new(line_num, line_num))
}
/// Merge overlapping and adjacent ranges.
fn merge_ranges(ranges: Vec<LineRange>) -> Vec<LineRange> {
let mut merged: Vec<LineRange> = Vec::new();
for range in ranges {
if let Some(last) = merged.last_mut() {
// If ranges overlap or are adjacent, merge them
if range.start_line <= last.end_line + 1 {
last.end_line = last.end_line.max(range.end_line);
} else {
merged.push(range);
}
} else {
merged.push(range);
}
}
merged
}
#[cfg(test)]
mod tests {
use super::*;
#[test]
fn test_parse_raw() {
assert_eq!(parse_selector("raw").unwrap(), LineRangeSelector::Raw);
}
#[test]
fn test_parse_conflicts() {
assert_eq!(
parse_selector("conflicts").unwrap(),
LineRangeSelector::Conflicts
);
}
#[test]
fn test_parse_single_line() {
match parse_selector("50").unwrap() {
LineRangeSelector::Lines(ranges) => {
assert_eq!(ranges.len(), 1);
assert_eq!(ranges[0].start_line, 50);
assert_eq!(ranges[0].end_line, 50);
}
_ => panic!("Expected Lines variant"),
}
}
#[test]
fn test_parse_range_start_dash_end() {
match parse_selector("50-200").unwrap() {
LineRangeSelector::Lines(ranges) => {
assert_eq!(ranges.len(), 1);
assert_eq!(ranges[0].start_line, 50);
assert_eq!(ranges[0].end_line, 200);
}
_ => panic!("Expected Lines variant"),
}
}
#[test]
fn test_parse_range_start_plus_count() {
match parse_selector("50+150").unwrap() {
LineRangeSelector::Lines(ranges) => {
assert_eq!(ranges.len(), 1);
assert_eq!(ranges[0].start_line, 50);
assert_eq!(ranges[0].end_line, 199);
}
_ => panic!("Expected Lines variant"),
}
}
#[test]
fn test_parse_multiple_ranges() {
match parse_selector("5-16,960-973").unwrap() {
LineRangeSelector::Lines(ranges) => {
assert_eq!(ranges.len(), 2);
assert_eq!(ranges[0].start_line, 5);
assert_eq!(ranges[0].end_line, 16);
assert_eq!(ranges[1].start_line, 960);
assert_eq!(ranges[1].end_line, 973);
}
_ => panic!("Expected Lines variant"),
}
}
#[test]
fn test_merge_overlapping_ranges() {
// "5-16,10-20" should merge into "5-20"
match parse_selector("5-16,10-20").unwrap() {
LineRangeSelector::Lines(ranges) => {
assert_eq!(ranges.len(), 1);
assert_eq!(ranges[0].start_line, 5);
assert_eq!(ranges[0].end_line, 20);
}
_ => panic!("Expected Lines variant"),
}
}
#[test]
fn test_merge_adjacent_ranges() {
// "5-16,17-20" should merge into "5-20"
match parse_selector("5-16,17-20").unwrap() {
LineRangeSelector::Lines(ranges) => {
assert_eq!(ranges.len(), 1);
assert_eq!(ranges[0].start_line, 5);
assert_eq!(ranges[0].end_line, 20);
}
_ => panic!("Expected Lines variant"),
}
}
#[test]
fn test_invalid_range_start_greater_than_end() {
let result = parse_selector("200-50");
assert!(result.is_err());
}
#[test]
fn test_invalid_zero_line() {
let result = parse_selector("0");
assert!(result.is_err());
}
#[test]
fn test_empty_selector() {
assert_eq!(parse_selector("").unwrap(), LineRangeSelector::None);
}
#[test]
fn test_line_range_zero_indexed() {
let range = LineRange::new(50, 200);
let (start, end) = range.zero_indexed();
assert_eq!(start, 49);
assert_eq!(end, 199);
}
#[test]
fn test_line_range_len() {
assert_eq!(LineRange::new(50, 200).len(), 151);
assert_eq!(LineRange::new(50, 50).len(), 1);
}
}
+47 -3
View File
@@ -20,7 +20,7 @@ use runtime::{
lsp_client::LspRegistry,
mcp_tool_bridge::McpToolRegistry,
permission_enforcer::{EnforcementResult, PermissionEnforcer},
read_file_in_workspace,
read_file_in_workspace_with_selector,
summary_compression::compress_summary_text,
task_registry::TaskRegistry,
team_cron_registry::{CronRegistry, TeamRegistry},
@@ -2569,8 +2569,14 @@ fn branch_divergence_output(
fn run_read_file(input: ReadFileInput) -> Result<String, String> {
let workspace = std::env::current_dir().map_err(|error| error.to_string())?;
to_pretty_json(
read_file_in_workspace(&input.path, input.offset, input.limit, &workspace)
.map_err(io_to_string)?,
read_file_in_workspace_with_selector(
&input.path,
input.offset,
input.limit,
input.selector.as_deref(),
&workspace,
)
.map_err(io_to_string)?,
)
}
@@ -2885,6 +2891,7 @@ struct ReadFileInput {
path: String,
offset: Option<usize>,
limit: Option<usize>,
selector: Option<String>,
}
#[derive(Debug, Deserialize)]
@@ -10690,6 +10697,43 @@ printf 'pwsh:%s' "$1"
let _ = fs::remove_dir_all(root);
}
#[test]
fn given_read_only_enforcer_when_read_file_with_selector_then_succeeds() {
let _guard = env_lock()
.lock()
.unwrap_or_else(std::sync::PoisonError::into_inner);
let root = temp_path("perm-read-selector");
fs::create_dir_all(&root).expect("create root");
let file = root.join("multi-line.txt");
fs::write(&file, "line1\nline2\nline3\nline4\nline5\n").expect("write test file");
let original_dir = std::env::current_dir().expect("cwd");
std::env::set_current_dir(&root).expect("set cwd");
let registry = read_only_registry();
let result = registry.execute(
"read_file",
&json!({
"path": file.display().to_string(),
"selector": "2-4"
}),
);
assert!(
result.is_ok(),
"read_file with selector should succeed: {result:?}"
);
// Verify the output contains the expected lines
let output: serde_json::Value =
serde_json::from_str(&result.unwrap()).expect("json output");
let content = output["file"]["content"].as_str().expect("content field");
assert!(content.contains("line2"));
assert!(content.contains("line3"));
assert!(content.contains("line4"));
std::env::set_current_dir(&original_dir).expect("restore cwd");
let _ = fs::remove_dir_all(root);
}
#[test]
fn given_read_only_enforcer_when_glob_search_then_not_permission_denied() {
let registry = read_only_registry();