From 70c40a10739b076f05e93196d5464beff75c197c Mon Sep 17 00:00:00 2001 From: Alex Hancock Date: Mon, 15 Sep 2025 12:41:45 -0400 Subject: [PATCH 1/3] fix(1718): make --dir work when launching Goose.app (#4642) --- ui/desktop/src/main.ts | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/ui/desktop/src/main.ts b/ui/desktop/src/main.ts index 5361ec01cb..d321cfcfbf 100644 --- a/ui/desktop/src/main.ts +++ b/ui/desktop/src/main.ts @@ -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]; From 989c1bb1b015a797aa8ac73d2c86cc8cc7e6f1b1 Mon Sep 17 00:00:00 2001 From: Alex Hancock Date: Mon, 15 Sep 2025 13:29:43 -0400 Subject: [PATCH 2/3] fix: #4634 #4636 do not always encourage absolute paths (#4641) --- .../goose-mcp/src/developer/rmcp_developer.rs | 104 +++++++++++++++--- 1 file changed, 90 insertions(+), 14 deletions(-) diff --git a/crates/goose-mcp/src/developer/rmcp_developer.rs b/crates/goose-mcp/src/developer/rmcp_developer.rs index 4bbe87b16d..33c2fe4915 100644 --- a/crates/goose-mcp/src/developer/rmcp_developer.rs +++ b/crates/goose-mcp/src/developer/rmcp_developer.rs @@ -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"); + } } From e7a767528c101d06996aa17ce6495263952e7fa8 Mon Sep 17 00:00:00 2001 From: Jim Alateras Date: Tue, 16 Sep 2025 04:03:51 +1000 Subject: [PATCH 3/3] Fix #4550: Correct row/column transposition in xlsx tool (#4622) Signed-off-by: jalateras Co-authored-by: Claude --- .../src/computercontroller/xlsx_tool.rs | 82 ++++++++++++++++++- 1 file changed, 78 insertions(+), 4 deletions(-) diff --git a/crates/goose-mcp/src/computercontroller/xlsx_tool.rs b/crates/goose-mcp/src/computercontroller/xlsx_tool.rs index ee5ff5d1d6..f67026c3b3 100644 --- a/crates/goose-mcp/src/computercontroller/xlsx_tool.rs +++ b/crates/goose-mcp/src/computercontroller/xlsx_tool.rs @@ -97,7 +97,7 @@ impl XlsxTool { } pub fn get_range(&self, worksheet: &Worksheet, range: &str) -> Result { - 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::().context("Invalid row number")?; - Ok((col, row)) + Ok((row, col)) } fn column_letter_to_number(column: &str) -> Result { @@ -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(()) + } }