Merge branch 'main' into micn/goose-swarm

* main:
  Fix #4550: Correct row/column transposition in xlsx tool (#4622)
  fix: #4634 #4636 do not always encourage absolute paths (#4641)
  fix(1718): make --dir work when launching Goose.app (#4642)
This commit is contained in:
Michael Neale
2025-09-15 17:23:59 -07:00
3 changed files with 170 additions and 19 deletions
@@ -97,7 +97,7 @@ impl XlsxTool {
}
pub fn get_range(&self, worksheet: &Worksheet, range: &str) -> Result<RangeData> {
let (start_col, start_row, end_col, end_row) = parse_range(range)?;
let (start_row, start_col, end_row, end_col) = parse_range(range)?;
let mut values = Vec::new();
// Iterate through rows first, then columns
@@ -206,7 +206,7 @@ impl XlsxTool {
}
fn parse_range(range: &str) -> Result<(u32, u32, u32, u32)> {
// Handle ranges like "A1:B10"
// Handle ranges like "A1:B10" and return (start_row, start_col, end_row, end_col)
let parts: Vec<&str> = range.split(':').collect();
if parts.len() != 2 {
anyhow::bail!("Invalid range format. Expected format: 'A1:B10'");
@@ -215,11 +215,12 @@ fn parse_range(range: &str) -> Result<(u32, u32, u32, u32)> {
let start = parse_cell_reference(parts[0])?;
let end = parse_cell_reference(parts[1])?;
// parse_cell_reference returns (row, col), so start.0 is row, start.1 is col
Ok((start.0, start.1, end.0, end.1))
}
fn parse_cell_reference(reference: &str) -> Result<(u32, u32)> {
// Parse Excel cell reference (e.g., "A1") and return (column, row)
// Parse Excel cell reference (e.g., "A1") and return (row, column) to match umya_spreadsheet's expectation
let mut col_str = String::new();
let mut row_str = String::new();
let mut parsing_row = false;
@@ -241,7 +242,7 @@ fn parse_cell_reference(reference: &str) -> Result<(u32, u32)> {
let col = column_letter_to_number(&col_str)?;
let row = row_str.parse::<u32>().context("Invalid row number")?;
Ok((col, row))
Ok((row, col))
}
fn column_letter_to_number(column: &str) -> Result<u32> {
@@ -328,4 +329,77 @@ mod tests {
);
Ok(())
}
#[test]
fn test_coordinate_mapping() -> Result<()> {
let xlsx = XlsxTool::new(get_test_file())?;
let worksheet = xlsx.get_worksheet_by_index(0)?;
// Verify the coordinate system mapping
// A1 should be row=1, col=1
let a1 = xlsx.get_cell_value(worksheet, 1, 1)?;
println!("A1 (1,1): {}", a1.value);
assert_eq!(a1.value, "Segment");
// A2 should be row=2, col=1
let a2 = xlsx.get_cell_value(worksheet, 2, 1)?;
println!("A2 (2,1): {}", a2.value);
assert_eq!(a2.value, "Country");
// B1 should be row=1, col=2
let b1 = xlsx.get_cell_value(worksheet, 1, 2)?;
println!("B1 (1,2): {}", b1.value);
assert_eq!(b1.value, "Government");
// B2 should be row=2, col=2
let b2 = xlsx.get_cell_value(worksheet, 2, 2)?;
println!("B2 (2,2): {}", b2.value);
assert_eq!(b2.value, "Canada");
// Verify that parse_cell_reference works correctly (row, col)
assert_eq!(parse_cell_reference("A1")?, (1, 1));
assert_eq!(parse_cell_reference("A2")?, (2, 1));
assert_eq!(parse_cell_reference("B1")?, (1, 2));
assert_eq!(parse_cell_reference("B2")?, (2, 2));
assert_eq!(parse_cell_reference("Z1")?, (1, 26));
assert_eq!(parse_cell_reference("AA1")?, (1, 27));
Ok(())
}
#[test]
fn test_issue_4550_row_column_transposition() -> Result<()> {
// This test specifically addresses issue #4550 where A2 was returning B1's value
let xlsx = XlsxTool::new(get_test_file())?;
let worksheet = xlsx.get_worksheet_by_index(0)?;
// Test that A2 (row 2, column 1) returns the correct value
let a2_value = xlsx.get_cell_value(worksheet, 2, 1)?;
assert_eq!(a2_value.value, "Country", "A2 should contain 'Country'");
// Test that B1 (row 1, column 2) returns its own value, not A2's
let b1_value = xlsx.get_cell_value(worksheet, 1, 2)?;
assert_eq!(
b1_value.value, "Government",
"B1 should contain 'Government'"
);
// Additional verification with ranges
let range = xlsx.get_range(worksheet, "A1:B2")?;
assert_eq!(
range.values[0][0].value, "Segment",
"A1 should be 'Segment'"
);
assert_eq!(
range.values[0][1].value, "Government",
"B1 should be 'Government'"
);
assert_eq!(
range.values[1][0].value, "Country",
"A2 should be 'Country'"
);
assert_eq!(range.values[1][1].value, "Canada", "B2 should be 'Canada'");
Ok(())
}
}
@@ -319,7 +319,6 @@ impl ServerHandler for DeveloperServer {
**Important**: Each shell command runs in its own process. Things like directory changes or
sourcing files do not persist between tool calls. So you may need to repeat them each time by
stringing together commands.
- Pathnames: Use absolute paths and avoid cd unless explicitly requested
"#};
let windows_specific = indoc! {r#"
@@ -1166,19 +1165,12 @@ impl DeveloperServer {
let expanded = expand_path(path_str);
let path = Path::new(&expanded);
let suggestion = cwd.join(path);
match is_absolute_path(&expanded) {
true => Ok(path.to_path_buf()),
false => Err(ErrorData::new(
ErrorCode::INVALID_PARAMS,
format!(
"The path {} is not an absolute path, did you possibly mean {}?",
path_str,
suggestion.to_string_lossy(),
),
None,
)),
// If the path is absolute, return it as-is
if is_absolute_path(&expanded) {
Ok(path.to_path_buf())
} else {
// For relative paths, resolve them relative to the current working directory
Ok(cwd.join(path))
}
}
@@ -3317,4 +3309,88 @@ Additional instructions here.
assert!(!instructions.contains(".goosehints")); // Make sure it's not loading the default
std::env::remove_var("CONTEXT_FILE_NAMES");
}
#[tokio::test]
#[serial]
async fn test_resolve_path_absolute() {
let temp_dir = tempfile::tempdir().unwrap();
std::env::set_current_dir(&temp_dir).unwrap();
let server = create_test_server();
let absolute_path = temp_dir.path().join("test.txt");
let absolute_path_str = absolute_path.to_str().unwrap();
let resolved = server.resolve_path(absolute_path_str).unwrap();
assert_eq!(resolved, absolute_path);
}
#[tokio::test]
#[serial]
async fn test_resolve_path_relative() {
let temp_dir = tempfile::tempdir().unwrap();
std::env::set_current_dir(&temp_dir).unwrap();
let server = create_test_server();
let relative_path = "subdir/test.txt";
let resolved = server.resolve_path(relative_path).unwrap();
let expected = std::env::current_dir().unwrap().join("subdir/test.txt");
assert_eq!(resolved, expected);
}
#[tokio::test]
#[serial]
async fn test_text_editor_with_absolute_path() {
let temp_dir = tempfile::tempdir().unwrap();
std::env::set_current_dir(&temp_dir).unwrap();
let server = create_test_server();
let absolute_path = temp_dir.path().join("absolute_test.txt");
let absolute_path_str = absolute_path.to_str().unwrap();
let write_params = Parameters(TextEditorParams {
path: absolute_path_str.to_string(),
command: "write".to_string(),
view_range: None,
file_text: Some("Absolute path test".to_string()),
old_str: None,
new_str: None,
insert_line: None,
diff: None,
});
let result = server.text_editor(write_params).await;
assert!(result.is_ok());
let content = fs::read_to_string(&absolute_path).unwrap();
assert_eq!(content.trim(), "Absolute path test");
}
#[tokio::test]
#[serial]
async fn test_text_editor_with_relative_path() {
let temp_dir = tempfile::tempdir().unwrap();
std::env::set_current_dir(&temp_dir).unwrap();
let server = create_test_server();
let relative_path = "relative_test.txt";
let write_params = Parameters(TextEditorParams {
path: relative_path.to_string(),
command: "write".to_string(),
view_range: None,
file_text: Some("Relative path test".to_string()),
old_str: None,
new_str: None,
insert_line: None,
diff: None,
});
let result = server.text_editor(write_params).await;
assert!(result.is_ok());
let absolute_path = temp_dir.path().join(relative_path);
let content = fs::read_to_string(&absolute_path).unwrap();
assert_eq!(content.trim(), "Relative path test");
}
}
+2 -1
View File
@@ -434,9 +434,10 @@ let envToggles: EnvToggles = loadSettings().envToggles;
// Parse command line arguments
const parseArgs = () => {
const args = process.argv.slice(2); // Remove first two elements (electron and script path)
let dirPath = null;
// Remove first two elements in dev mode (electron and script path)
const args = !dirPath && app.isPackaged ? process.argv : process.argv.slice(2);
for (let i = 0; i < args.length; i++) {
if (args[i] === '--dir' && i + 1 < args.length) {
dirPath = args[i + 1];