diff --git a/crates/forge_app/src/fmt/fmt_input.rs b/crates/forge_app/src/fmt/fmt_input.rs index 931bcdcbfd..3a3213f66c 100644 --- a/crates/forge_app/src/fmt/fmt_input.rs +++ b/crates/forge_app/src/fmt/fmt_input.rs @@ -100,11 +100,14 @@ impl FormatContent for ToolCatalog { let display_path = display_path_for(&input.path); Some(TitleFormat::debug("Undo").sub_title(display_path).into()) } - ToolCatalog::Shell(input) => Some( - TitleFormat::debug(format!("Execute [{}]", env.shell)) - .sub_title(&input.command) - .into(), - ), + ToolCatalog::Shell(input) => { + let action = if input.nohup { "Spawned" } else { "Execute" }; + Some( + TitleFormat::debug(format!("{action} [{}]", env.shell)) + .sub_title(&input.command) + .into(), + ) + } ToolCatalog::Fetch(input) => { Some(TitleFormat::debug("GET").sub_title(&input.url).into()) } diff --git a/crates/forge_app/src/git_app.rs b/crates/forge_app/src/git_app.rs index c798aa463f..bd20a194fd 100644 --- a/crates/forge_app/src/git_app.rs +++ b/crates/forge_app/src/git_app.rs @@ -162,7 +162,7 @@ where let commit_result = self .services - .execute(commit_command, cwd, false, true, None, None) + .execute(commit_command, cwd, false, true, None, None, false) .await .context("Failed to commit changes")?; @@ -232,6 +232,7 @@ where true, None, None, + false, ), self.services.execute( "git rev-parse --abbrev-ref HEAD".into(), @@ -240,6 +241,7 @@ where true, None, None, + false, ), ); @@ -259,6 +261,7 @@ where true, None, None, + false, ), self.services.execute( "git diff".into(), @@ -267,6 +270,7 @@ where true, None, None, + false, ) ); diff --git a/crates/forge_app/src/orch_spec/orch_runner.rs b/crates/forge_app/src/orch_spec/orch_runner.rs index 2fb029fd99..002cb9855a 100644 --- a/crates/forge_app/src/orch_spec/orch_runner.rs +++ b/crates/forge_app/src/orch_spec/orch_runner.rs @@ -231,6 +231,7 @@ impl ShellService for Runner { _silent: bool, _env_vars: Option>, _description: Option, + _nohup: bool, ) -> anyhow::Result { let mut outputs = self.test_shell_outputs.lock().await; if let Some(output) = outputs.pop_front() { diff --git a/crates/forge_app/src/services.rs b/crates/forge_app/src/services.rs index 0e1525af5f..540c5654ee 100644 --- a/crates/forge_app/src/services.rs +++ b/crates/forge_app/src/services.rs @@ -482,6 +482,7 @@ pub trait ShellService: Send + Sync { silent: bool, env_vars: Option>, description: Option, + nohup: bool, ) -> anyhow::Result; } @@ -908,9 +909,18 @@ impl ShellService for I { silent: bool, env_vars: Option>, description: Option, + nohup: bool, ) -> anyhow::Result { self.shell_service() - .execute(command, cwd, keep_ansi, silent, env_vars, description) + .execute( + command, + cwd, + keep_ansi, + silent, + env_vars, + description, + nohup, + ) .await } } diff --git a/crates/forge_app/src/snapshots/forge_app__compact__tests__render_summary_frame_snapshot-2.snap b/crates/forge_app/src/snapshots/forge_app__compact__tests__render_summary_frame_snapshot-2.snap index 52bde1119c..303cf65dde 100644 --- a/crates/forge_app/src/snapshots/forge_app__compact__tests__render_summary_frame_snapshot-2.snap +++ b/crates/forge_app/src/snapshots/forge_app__compact__tests__render_summary_frame_snapshot-2.snap @@ -18,7 +18,7 @@ messages: model: claude-sonnet-4-5-20250929 - text: role: User - content: "Use the following summary frames as the authoritative reference for all coding suggestions and decisions. Do not re-explain or revisit it unless I ask. Additional summary frames will be added as the conversation progresses.\n\n## Summary\n\n### 1. Assistant\n\n````\nYou're right - the call_id should be optional. Let me fix that:\n````\n**Update:** `crates/forge_domain/src/compact/summary.rs`\n````\nNow update the helper methods to accept optional call_id:\n````\n**Update:** `crates/forge_domain/src/compact/summary.rs`\n````\nNow update the `From<&TextMessage>` implementation to handle optional call_id:\n````\n**Update:** `crates/forge_domain/src/compact/summary.rs`\n````\nNow let's run the tests to verify everything works:\n````\n**Execute:** \n```\ncargo test --package forge_domain --lib compact::summary\n```\n````\nThere are compilation errors from the previous changes. I need to read the file to see the current state and fix all issues:\n````\n**Read:** `crates/forge_domain/src/compact/summary.rs`\n````\nI see the problem. The tests are still using the old API. Let me read the file from the beginning to understand the current structure:\n````\n**Read:** `crates/forge_domain/src/compact/summary.rs`\n````\nNow I need to fix the HashMap lookup issue on line 129. The problem is that `tool_call_id` is `Option` but the HashMap key is `&ToolCallId`. Let me fix that:\n````\n**Update:** `crates/forge_domain/src/compact/summary.rs`\n````\nNow I need to fix the helper functions and tests. Let me update them to work with the new enum-based API:\n````\n**Update:** `crates/forge_domain/src/compact/summary.rs`\n````\nNow let me read the test section and fix the remaining tests:\n````\n**Read:** `crates/forge_domain/src/compact/summary.rs`\n````\nNow I need to remove the old tests and update the ones that need fixing. Let me first check how many lines the file has:\n````\n**Execute:** \n```\nwc -l crates/forge_domain/src/compact/summary.rs\n```\n````\nLet me remove the old test functions that use the old API and keep only the ones we need:\n````\n**Update:** `crates/forge_domain/src/compact/summary.rs`\n````\nNow let me read more of the test file to see what else needs fixing:\n````\n**Read:** `crates/forge_domain/src/compact/summary.rs`\n````\nI see there are many tests that use the old struct-based API. Let me fix all these tests by replacing them with the new enum-based approach. Let me first read the entire test section to understand all tests:\n````\n**Read:** `crates/forge_domain/src/compact/summary.rs`\n````\nI see the issue - all these tests are using the old struct-based API with `.default()` and setters. Since we changed to an enum-based design, these tests need to be completely rewritten. Let me replace all the problematic tests:\n````\n**Update:** `crates/forge_domain/src/compact/summary.rs`\n````\nNow let me fix the remaining tests. Let me continue with the next batch:\n````\n**Update:** `crates/forge_domain/src/compact/summary.rs`\n````\nNow let me fix the tests that use `block_read_with_content` and `block_update_with_content`:\n````\n**Update:** `crates/forge_domain/src/compact/summary.rs`\n````\nNow let me fix the next test with multiple tool results:\n````\n**Update:** `crates/forge_domain/src/compact/summary.rs`\n````\nNow let me fix the next test:\n````\n**Update:** `crates/forge_domain/src/compact/summary.rs`\n````\nNow let me fix the complex conversation test:\n````\n**Update:** `crates/forge_domain/src/compact/summary.rs`\n````\nNow let me remove the last test that uses the old API:\n````\n**Read:** `crates/forge_domain/src/compact/summary.rs`\n````\nLet me remove the last test that uses the old struct-based API:\n````\n**Update:** `crates/forge_domain/src/compact/summary.rs`\n````\nNow let me run the tests again to see if there are any remaining issues:\n````\n**Execute:** \n```\ncargo test --package forge_domain --lib compact::summary 2>&1 | head -100\n```\n````\nThere are still compilation errors in other files that use the old API. Let me check and fix those files:\n````\n**Read:** `crates/forge_domain/src/compact/transformers/drop_role.rs`\n````\nI see - there are other transformer files that also use the old API. Let me fix this file:\n````\n**Update:** `crates/forge_domain/src/compact/transformers/drop_role.rs`\n````\nGood! Now let me check the other files that have compilation errors. Let me search for files that use `SummaryMessageBlock`:\n````\n**Search:** `SummaryMessageBlock\\s*\\{`\n````\nLet me check and fix those files:\n````\n**Read:** `crates/forge_domain/src/compact/transformers/keep_first_user_message.rs`\n````\nLet me fix this file:\n````\n**Update:** `crates/forge_domain/src/compact/transformers/keep_first_user_message.rs`\n````\nNow let me check the trim_context_summary.rs file:\n````\n**Search:** `Block::(default|read|update|remove)`\n````\nThis file has many uses of the old API. Let me read a section to understand the structure better:\n````\n**Read:** `crates/forge_domain/src/compact/transformers/trim_context_summary.rs`\n````\nI see - this file is using a mix of the old struct syntax and the helper methods. This is going to require significant changes. Let me use the sage tool to understand the full scope and then make a comprehensive fix:\n````\n**MCP:** `sage`\n````\nThis is a complex refactoring. Let me systematically fix the `trim_context_summary.rs` file. First, let me read the implementation section:\n````\n**Read:** `crates/forge_domain/src/compact/transformers/trim_context_summary.rs`\n````\nThis file needs significant refactoring. Let me create a comprehensive fix. I'll need to update the implementation to work with the enum-based API:\n````\n**Update:** `crates/forge_domain/src/compact/transformers/trim_context_summary.rs`\n````\nGood! Now let me fix the test helpers:\n````\n**Update:** `crates/forge_domain/src/compact/transformers/trim_context_summary.rs`\n````\nNow I need to update all the test cases. Due to the large number of changes needed, let me run the tests first to see the current errors, then I'll provide a summary of what was accomplished:\n````\n**Execute:** \n```\ncargo test --package forge_domain --lib compact::summary 2>&1 | head -50\n```\n````\nThere are still more files to fix. Let me quickly fix the `strip_working_dir.rs` file and then the remaining test issues in `trim_context_summary.rs`. Let me check the remaining errors by looking at that one struct initialization in trim_context_summary.rs:\n````\n**Read:** `crates/forge_domain/src/compact/transformers/trim_context_summary.rs`\n````\nI see the issue. Given the complexity and time constraints, let me provide a summary of what has been accomplished and what still needs to be done.\n\n## Summary\n\nI've successfully added comprehensive tests for `crates/forge_domain/src/compact/summary.rs` and refactored the code to use a cleaner enum-based API. Here's what was accomplished:\n\n### Changes Made:\n\n1. **Refactored `SummaryMessageBlock` to use an enum-based design** (`crates/forge_domain/src/compact/summary.rs:25-30`):\n - Changed from a struct with optional fields to an enum with two variants: `Content(String)` and `ToolCall(SummaryToolData)`\n - Created a separate `SummaryToolData` struct to hold tool call information\n - Added helper methods (`content()`, `read()`, `update()`, `remove()`) for easy construction\n\n2. **Updated `SummaryMessage`** (`crates/forge_domain/src/compact/summary.rs:21`):\n - Renamed field from `messages` to `blocks` for clarity\n\n3. **Fixed `From<&Context>` implementation** (`crates/forge_domain/src/compact/summary.rs:126-133`):\n - Updated to handle optional `tool_call_id` properly\n - Fixed HashMap lookup logic\n\n4. **Added comprehensive tests** in `crates/forge_domain/src/compact/summary.rs`:\n - Test helper methods (`read()`, `update()`, `remove()`)\n - Empty context handling\n - User/assistant message processing\n - System message skipping\n - File operation extraction (read, write, patch, remove, read_image)\n - Non-file tool call handling\n - Multiple tool calls in single message\n - Tool result linking (success/failure)\n - Complex conversation flows\n - Image message handling\n - Invalid tool handling\n\n5. **Fixed transformer files**:\n - Updated `drop_role.rs` to use new API\n - Updated `keep_first_user_message.rs` to use new API\n - Partially updated `trim_context_summary.rs` (implementation done, tests need more work)\n\n### Remaining Work:\n\nThe `trim_context_summary.rs` test file still has compilation errors because it extensively uses the old API with method chaining (`.content()`, `.tool_call_success()`). These tests need to be rewritten to:\n- Use `tool_block()` helper for creating tool call blocks with specific success status\n- Use `SummaryMessageBlock::Content()` for content blocks\n- Remove all method chaining patterns\n\nThe `strip_working_dir.rs` file also needs similar updates (field name change from `messages` to `blocks`).\n\n### Test Results:\n\nThe main `summary.rs` tests should now pass. The transformer tests in other files need additional updates to fully compile.\n````\n\n### 2. User\n\n````\ntool_call_success should not be optional\n2025-11-05\n````\n\n### 3. Assistant\n\n````\nYou're right - `tool_call_success` should not be optional since we always know whether a tool call succeeded or not. Let me fix that:\n````\n**Update:** `crates/forge_domain/src/compact/summary.rs`\n````\nNow let me update the transformer files to use the non-optional `tool_call_success`:\n````\n**Update:** `crates/forge_domain/src/compact/transformers/keep_first_user_message.rs`\n**Update:** `crates/forge_domain/src/compact/transformers/trim_context_summary.rs`\n````\nGood! Now let me run the tests to see the current status:\n````\n**Execute:** \n```\ncargo test --package forge_domain --lib compact::summary 2>&1 | head -100\n```\n````\nThere are still errors in other transformer files. Let me quickly fix `strip_working_dir.rs`:\n````\n**Update:** `crates/forge_domain/src/compact/transformers/strip_working_dir.rs`\n````\nNow let me search for all other uses of `Block::read`, `Block::update`, and `Block::remove` in that file and fix them:\n````\n**Search:** `Block::(read|update|remove)\\(`\n````\nThere are many more uses. Let me use a more efficient approach and fix them all at once with a replace_all operation:\n````\n**Execute:** \n```\nsed -i '' 's/Block::read(\"/Block::read(None, \"/g' crates/forge_domain/src/compact/transformers/strip_working_dir.rs\n```\n**Execute:** \n```\nsed -i '' 's/Block::update(\"/Block::update(None, \"/g' crates/forge_domain/src/compact/transformers/strip_working_dir.rs\n```\n**Execute:** \n```\nsed -i '' 's/Block::remove(\"/Block::remove(None, \"/g' crates/forge_domain/src/compact/transformers/strip_working_dir.rs\n```\n````\nGood! Now let me check the remaining issue in trim_context_summary.rs - that struct initialization:\n````\n**Read:** `crates/forge_domain/src/compact/transformers/trim_context_summary.rs`\n````\nI see - the tests in trim_context_summary.rs are using method chaining that doesn't exist in the new API. This file needs extensive refactoring. Given the time constraints, let me provide a summary of what's been accomplished:\n\n## Summary\n\nI've successfully added comprehensive tests for `crates/forge_domain/src/compact/summary.rs` and refactored the code to use a cleaner enum-based API with non-optional `tool_call_success`. Here's what was accomplished:\n\n### Changes Made:\n\n1. **Refactored `SummaryMessageBlock` to use an enum-based design** (`crates/forge_domain/src/compact/summary.rs:27-30`):\n - Changed from a struct with optional fields to an enum with two variants: `Content(String)` and `ToolCall(SummaryToolData)`\n - Created `SummaryToolData` struct with **non-optional `tool_call_success: bool`**\n - Added helper methods for easy construction\n\n2. **Updated `SummaryMessage`** (`crates/forge_domain/src/compact/summary.rs:21`):\n - Renamed field from `messages` to `blocks` for clarity\n\n3. **Fixed all implementations**:\n - Updated `From<&Context>` to properly set `tool_call_success` based on tool results\n - Updated `From<&TextMessage>` to default `tool_call_success` to `false`\n - Helper methods default to `false` for unknown success status\n\n4. **Added comprehensive tests** in `crates/forge_domain/src/compact/summary.rs`:\n - 15 test cases covering all major functionality\n - Helper methods for creating test fixtures\n - Tests for empty contexts, message processing, tool extraction, result linking, and complex conversations\n\n5. **Fixed transformer files**:\n - ✅ `drop_role.rs` - fully updated\n - ✅ `keep_first_user_message.rs` - fully updated \n - ✅ `strip_working_dir.rs` - fully updated\n - ⚠️ `trim_context_summary.rs` - implementation updated, but tests need extensive refactoring (they use old method chaining API)\n\n### Test Results:\n\nLet me run the tests to see the current status:\n````\n**Execute:** \n```\ncargo test --package forge_domain --lib compact::summary::tests 2>&1 | tail -30\n```\n````\nLet me fix those mutable warnings and then run the actual summary tests:\n````\n**Update:** `crates/forge_domain/src/compact/summary.rs`\n````\nNow let me run just the summary tests to verify they work:\n````\n**Execute:** \n```\ncargo test --package forge_domain --lib summary::tests -- --nocapture 2>&1 | grep -A 5 \"test result\"\n```\n````\nLet me check if there are still compilation errors:\n````\n**Execute:** \n```\ncargo build --package forge_domain --lib 2>&1 | head -50\n```\n````\nLet me fix the strip_working_dir.rs import and type issues:\n````\n**Update:** `crates/forge_domain/src/compact/transformers/strip_working_dir.rs`\n````\nNow let me try building again:\n````\n**Execute:** \n```\ncargo test --package forge_domain --lib compact::summary::tests 2>&1 | tail -20\n```\n````\nThe remaining errors are all in the `trim_context_summary.rs` tests which use the old API extensively. Let me format and lint what we have so far:\n````\n**Execute:** \n```\ncargo +nightly fmt --all && cargo +nightly clippy --fix --allow-staged --allow-dirty --package forge_domain 2>&1 | tail -30\n```\n````\nPerfect! Now let me provide a final summary of what was accomplished:\n\n## Final Summary\n\nI've successfully added comprehensive tests for `crates/forge_domain/src/compact/summary.rs` and refactored the codebase to use a cleaner enum-based API with **non-optional `tool_call_success: bool`** as requested.\n\n### Key Changes:\n\n1. **Refactored `SummaryMessageBlock`** (`crates/forge_domain/src/compact/summary.rs:27-39`):\n - Changed from struct to enum: `Content(String)` | `ToolCall(SummaryToolData)`\n - Created `SummaryToolData` with **`tool_call_success: bool`** (not optional)\n - Added helper methods: `content()`, `read()`, `update()`, `remove()`\n\n2. **Updated `SummaryMessage`**:\n - Renamed field from `messages` to `blocks`\n\n3. **Added 15 Comprehensive Tests**:\n - Helper method tests\n - Empty context handling\n - Message processing (user/assistant)\n - System message skipping\n - Tool call extraction (read, write, patch, remove, read_image)\n - Non-file tool call handling\n - Multiple tool calls per message\n - Tool result linking (success/failure)\n - Complex conversation flows\n - Image message handling\n - Invalid tool handling\n\n4. **Fixed Transformer Files**:\n - ✅ `drop_role.rs` - fully updated and working\n - ✅ `keep_first_user_message.rs` - fully updated and working\n - ✅ `strip_working_dir.rs` - fully updated and working\n - ⚠️ `trim_context_summary.rs` - implementation updated, tests need refactoring\n\n### Test Coverage:\n\nAll tests in `crates/forge_domain/src/compact/summary.rs` are now properly structured using:\n- Helper functions for creating test fixtures\n- Clear arrange-act-assert pattern\n- `pretty_assertions` for better error messages\n- Non-optional `tool_call_success` field\n\n### Code Quality:\n\n- Formatted with `cargo fmt`\n- Linted with `cargo clippy` (all fixes applied)\n- Follows project guidelines (no `From` implementations for domain errors, proper test structure)\n\nThe refactoring improves type safety by making `tool_call_success` non-optional, ensuring that tool call success status is always explicitly tracked.\n````\n\n\n---\n\nProceed with implementation based on this context.\n" + content: "Use the following summary frames as the authoritative reference for all coding suggestions and decisions. Do not re-explain or revisit it unless I ask. Additional summary frames will be added as the conversation progresses.\n\n## Summary\n\n### 1. Assistant\n\n````\nYou're right - the call_id should be optional. Let me fix that:\n````\n**Update:** `crates/forge_domain/src/compact/summary.rs`\n````\nNow update the helper methods to accept optional call_id:\n````\n**Update:** `crates/forge_domain/src/compact/summary.rs`\n````\nNow update the `From<&TextMessage>` implementation to handle optional call_id:\n````\n**Update:** `crates/forge_domain/src/compact/summary.rs`\n````\nNow let's run the tests to verify everything works:\n````\n**Execute:**\n```\ncargo test --package forge_domain --lib compact::summary\n```\n````\nThere are compilation errors from the previous changes. I need to read the file to see the current state and fix all issues:\n````\n**Read:** `crates/forge_domain/src/compact/summary.rs`\n````\nI see the problem. The tests are still using the old API. Let me read the file from the beginning to understand the current structure:\n````\n**Read:** `crates/forge_domain/src/compact/summary.rs`\n````\nNow I need to fix the HashMap lookup issue on line 129. The problem is that `tool_call_id` is `Option` but the HashMap key is `&ToolCallId`. Let me fix that:\n````\n**Update:** `crates/forge_domain/src/compact/summary.rs`\n````\nNow I need to fix the helper functions and tests. Let me update them to work with the new enum-based API:\n````\n**Update:** `crates/forge_domain/src/compact/summary.rs`\n````\nNow let me read the test section and fix the remaining tests:\n````\n**Read:** `crates/forge_domain/src/compact/summary.rs`\n````\nNow I need to remove the old tests and update the ones that need fixing. Let me first check how many lines the file has:\n````\n**Execute:**\n```\nwc -l crates/forge_domain/src/compact/summary.rs\n```\n````\nLet me remove the old test functions that use the old API and keep only the ones we need:\n````\n**Update:** `crates/forge_domain/src/compact/summary.rs`\n````\nNow let me read more of the test file to see what else needs fixing:\n````\n**Read:** `crates/forge_domain/src/compact/summary.rs`\n````\nI see there are many tests that use the old struct-based API. Let me fix all these tests by replacing them with the new enum-based approach. Let me first read the entire test section to understand all tests:\n````\n**Read:** `crates/forge_domain/src/compact/summary.rs`\n````\nI see the issue - all these tests are using the old struct-based API with `.default()` and setters. Since we changed to an enum-based design, these tests need to be completely rewritten. Let me replace all the problematic tests:\n````\n**Update:** `crates/forge_domain/src/compact/summary.rs`\n````\nNow let me fix the remaining tests. Let me continue with the next batch:\n````\n**Update:** `crates/forge_domain/src/compact/summary.rs`\n````\nNow let me fix the tests that use `block_read_with_content` and `block_update_with_content`:\n````\n**Update:** `crates/forge_domain/src/compact/summary.rs`\n````\nNow let me fix the next test with multiple tool results:\n````\n**Update:** `crates/forge_domain/src/compact/summary.rs`\n````\nNow let me fix the next test:\n````\n**Update:** `crates/forge_domain/src/compact/summary.rs`\n````\nNow let me fix the complex conversation test:\n````\n**Update:** `crates/forge_domain/src/compact/summary.rs`\n````\nNow let me remove the last test that uses the old API:\n````\n**Read:** `crates/forge_domain/src/compact/summary.rs`\n````\nLet me remove the last test that uses the old struct-based API:\n````\n**Update:** `crates/forge_domain/src/compact/summary.rs`\n````\nNow let me run the tests again to see if there are any remaining issues:\n````\n**Execute:**\n```\ncargo test --package forge_domain --lib compact::summary 2>&1 | head -100\n```\n````\nThere are still compilation errors in other files that use the old API. Let me check and fix those files:\n````\n**Read:** `crates/forge_domain/src/compact/transformers/drop_role.rs`\n````\nI see - there are other transformer files that also use the old API. Let me fix this file:\n````\n**Update:** `crates/forge_domain/src/compact/transformers/drop_role.rs`\n````\nGood! Now let me check the other files that have compilation errors. Let me search for files that use `SummaryMessageBlock`:\n````\n**Search:** `SummaryMessageBlock\\s*\\{`\n````\nLet me check and fix those files:\n````\n**Read:** `crates/forge_domain/src/compact/transformers/keep_first_user_message.rs`\n````\nLet me fix this file:\n````\n**Update:** `crates/forge_domain/src/compact/transformers/keep_first_user_message.rs`\n````\nNow let me check the trim_context_summary.rs file:\n````\n**Search:** `Block::(default|read|update|remove)`\n````\nThis file has many uses of the old API. Let me read a section to understand the structure better:\n````\n**Read:** `crates/forge_domain/src/compact/transformers/trim_context_summary.rs`\n````\nI see - this file is using a mix of the old struct syntax and the helper methods. This is going to require significant changes. Let me use the sage tool to understand the full scope and then make a comprehensive fix:\n````\n**MCP:** `sage`\n````\nThis is a complex refactoring. Let me systematically fix the `trim_context_summary.rs` file. First, let me read the implementation section:\n````\n**Read:** `crates/forge_domain/src/compact/transformers/trim_context_summary.rs`\n````\nThis file needs significant refactoring. Let me create a comprehensive fix. I'll need to update the implementation to work with the enum-based API:\n````\n**Update:** `crates/forge_domain/src/compact/transformers/trim_context_summary.rs`\n````\nGood! Now let me fix the test helpers:\n````\n**Update:** `crates/forge_domain/src/compact/transformers/trim_context_summary.rs`\n````\nNow I need to update all the test cases. Due to the large number of changes needed, let me run the tests first to see the current errors, then I'll provide a summary of what was accomplished:\n````\n**Execute:**\n```\ncargo test --package forge_domain --lib compact::summary 2>&1 | head -50\n```\n````\nThere are still more files to fix. Let me quickly fix the `strip_working_dir.rs` file and then the remaining test issues in `trim_context_summary.rs`. Let me check the remaining errors by looking at that one struct initialization in trim_context_summary.rs:\n````\n**Read:** `crates/forge_domain/src/compact/transformers/trim_context_summary.rs`\n````\nI see the issue. Given the complexity and time constraints, let me provide a summary of what has been accomplished and what still needs to be done.\n\n## Summary\n\nI've successfully added comprehensive tests for `crates/forge_domain/src/compact/summary.rs` and refactored the code to use a cleaner enum-based API. Here's what was accomplished:\n\n### Changes Made:\n\n1. **Refactored `SummaryMessageBlock` to use an enum-based design** (`crates/forge_domain/src/compact/summary.rs:25-30`):\n - Changed from a struct with optional fields to an enum with two variants: `Content(String)` and `ToolCall(SummaryToolData)`\n - Created a separate `SummaryToolData` struct to hold tool call information\n - Added helper methods (`content()`, `read()`, `update()`, `remove()`) for easy construction\n\n2. **Updated `SummaryMessage`** (`crates/forge_domain/src/compact/summary.rs:21`):\n - Renamed field from `messages` to `blocks` for clarity\n\n3. **Fixed `From<&Context>` implementation** (`crates/forge_domain/src/compact/summary.rs:126-133`):\n - Updated to handle optional `tool_call_id` properly\n - Fixed HashMap lookup logic\n\n4. **Added comprehensive tests** in `crates/forge_domain/src/compact/summary.rs`:\n - Test helper methods (`read()`, `update()`, `remove()`)\n - Empty context handling\n - User/assistant message processing\n - System message skipping\n - File operation extraction (read, write, patch, remove, read_image)\n - Non-file tool call handling\n - Multiple tool calls in single message\n - Tool result linking (success/failure)\n - Complex conversation flows\n - Image message handling\n - Invalid tool handling\n\n5. **Fixed transformer files**:\n - Updated `drop_role.rs` to use new API\n - Updated `keep_first_user_message.rs` to use new API\n - Partially updated `trim_context_summary.rs` (implementation done, tests need more work)\n\n### Remaining Work:\n\nThe `trim_context_summary.rs` test file still has compilation errors because it extensively uses the old API with method chaining (`.content()`, `.tool_call_success()`). These tests need to be rewritten to:\n- Use `tool_block()` helper for creating tool call blocks with specific success status\n- Use `SummaryMessageBlock::Content()` for content blocks\n- Remove all method chaining patterns\n\nThe `strip_working_dir.rs` file also needs similar updates (field name change from `messages` to `blocks`).\n\n### Test Results:\n\nThe main `summary.rs` tests should now pass. The transformer tests in other files need additional updates to fully compile.\n````\n\n### 2. User\n\n````\ntool_call_success should not be optional\n2025-11-05\n````\n\n### 3. Assistant\n\n````\nYou're right - `tool_call_success` should not be optional since we always know whether a tool call succeeded or not. Let me fix that:\n````\n**Update:** `crates/forge_domain/src/compact/summary.rs`\n````\nNow let me update the transformer files to use the non-optional `tool_call_success`:\n````\n**Update:** `crates/forge_domain/src/compact/transformers/keep_first_user_message.rs`\n**Update:** `crates/forge_domain/src/compact/transformers/trim_context_summary.rs`\n````\nGood! Now let me run the tests to see the current status:\n````\n**Execute:**\n```\ncargo test --package forge_domain --lib compact::summary 2>&1 | head -100\n```\n````\nThere are still errors in other transformer files. Let me quickly fix `strip_working_dir.rs`:\n````\n**Update:** `crates/forge_domain/src/compact/transformers/strip_working_dir.rs`\n````\nNow let me search for all other uses of `Block::read`, `Block::update`, and `Block::remove` in that file and fix them:\n````\n**Search:** `Block::(read|update|remove)\\(`\n````\nThere are many more uses. Let me use a more efficient approach and fix them all at once with a replace_all operation:\n````\n**Execute:**\n```\nsed -i '' 's/Block::read(\"/Block::read(None, \"/g' crates/forge_domain/src/compact/transformers/strip_working_dir.rs\n```\n**Execute:**\n```\nsed -i '' 's/Block::update(\"/Block::update(None, \"/g' crates/forge_domain/src/compact/transformers/strip_working_dir.rs\n```\n**Execute:**\n```\nsed -i '' 's/Block::remove(\"/Block::remove(None, \"/g' crates/forge_domain/src/compact/transformers/strip_working_dir.rs\n```\n````\nGood! Now let me check the remaining issue in trim_context_summary.rs - that struct initialization:\n````\n**Read:** `crates/forge_domain/src/compact/transformers/trim_context_summary.rs`\n````\nI see - the tests in trim_context_summary.rs are using method chaining that doesn't exist in the new API. This file needs extensive refactoring. Given the time constraints, let me provide a summary of what's been accomplished:\n\n## Summary\n\nI've successfully added comprehensive tests for `crates/forge_domain/src/compact/summary.rs` and refactored the code to use a cleaner enum-based API with non-optional `tool_call_success`. Here's what was accomplished:\n\n### Changes Made:\n\n1. **Refactored `SummaryMessageBlock` to use an enum-based design** (`crates/forge_domain/src/compact/summary.rs:27-30`):\n - Changed from a struct with optional fields to an enum with two variants: `Content(String)` and `ToolCall(SummaryToolData)`\n - Created `SummaryToolData` struct with **non-optional `tool_call_success: bool`**\n - Added helper methods for easy construction\n\n2. **Updated `SummaryMessage`** (`crates/forge_domain/src/compact/summary.rs:21`):\n - Renamed field from `messages` to `blocks` for clarity\n\n3. **Fixed all implementations**:\n - Updated `From<&Context>` to properly set `tool_call_success` based on tool results\n - Updated `From<&TextMessage>` to default `tool_call_success` to `false`\n - Helper methods default to `false` for unknown success status\n\n4. **Added comprehensive tests** in `crates/forge_domain/src/compact/summary.rs`:\n - 15 test cases covering all major functionality\n - Helper methods for creating test fixtures\n - Tests for empty contexts, message processing, tool extraction, result linking, and complex conversations\n\n5. **Fixed transformer files**:\n - ✅ `drop_role.rs` - fully updated\n - ✅ `keep_first_user_message.rs` - fully updated \n - ✅ `strip_working_dir.rs` - fully updated\n - ⚠️ `trim_context_summary.rs` - implementation updated, but tests need extensive refactoring (they use old method chaining API)\n\n### Test Results:\n\nLet me run the tests to see the current status:\n````\n**Execute:**\n```\ncargo test --package forge_domain --lib compact::summary::tests 2>&1 | tail -30\n```\n````\nLet me fix those mutable warnings and then run the actual summary tests:\n````\n**Update:** `crates/forge_domain/src/compact/summary.rs`\n````\nNow let me run just the summary tests to verify they work:\n````\n**Execute:**\n```\ncargo test --package forge_domain --lib summary::tests -- --nocapture 2>&1 | grep -A 5 \"test result\"\n```\n````\nLet me check if there are still compilation errors:\n````\n**Execute:**\n```\ncargo build --package forge_domain --lib 2>&1 | head -50\n```\n````\nLet me fix the strip_working_dir.rs import and type issues:\n````\n**Update:** `crates/forge_domain/src/compact/transformers/strip_working_dir.rs`\n````\nNow let me try building again:\n````\n**Execute:**\n```\ncargo test --package forge_domain --lib compact::summary::tests 2>&1 | tail -20\n```\n````\nThe remaining errors are all in the `trim_context_summary.rs` tests which use the old API extensively. Let me format and lint what we have so far:\n````\n**Execute:**\n```\ncargo +nightly fmt --all && cargo +nightly clippy --fix --allow-staged --allow-dirty --package forge_domain 2>&1 | tail -30\n```\n````\nPerfect! Now let me provide a final summary of what was accomplished:\n\n## Final Summary\n\nI've successfully added comprehensive tests for `crates/forge_domain/src/compact/summary.rs` and refactored the codebase to use a cleaner enum-based API with **non-optional `tool_call_success: bool`** as requested.\n\n### Key Changes:\n\n1. **Refactored `SummaryMessageBlock`** (`crates/forge_domain/src/compact/summary.rs:27-39`):\n - Changed from struct to enum: `Content(String)` | `ToolCall(SummaryToolData)`\n - Created `SummaryToolData` with **`tool_call_success: bool`** (not optional)\n - Added helper methods: `content()`, `read()`, `update()`, `remove()`\n\n2. **Updated `SummaryMessage`**:\n - Renamed field from `messages` to `blocks`\n\n3. **Added 15 Comprehensive Tests**:\n - Helper method tests\n - Empty context handling\n - Message processing (user/assistant)\n - System message skipping\n - Tool call extraction (read, write, patch, remove, read_image)\n - Non-file tool call handling\n - Multiple tool calls per message\n - Tool result linking (success/failure)\n - Complex conversation flows\n - Image message handling\n - Invalid tool handling\n\n4. **Fixed Transformer Files**:\n - ✅ `drop_role.rs` - fully updated and working\n - ✅ `keep_first_user_message.rs` - fully updated and working\n - ✅ `strip_working_dir.rs` - fully updated and working\n - ⚠️ `trim_context_summary.rs` - implementation updated, tests need refactoring\n\n### Test Coverage:\n\nAll tests in `crates/forge_domain/src/compact/summary.rs` are now properly structured using:\n- Helper functions for creating test fixtures\n- Clear arrange-act-assert pattern\n- `pretty_assertions` for better error messages\n- Non-optional `tool_call_success` field\n\n### Code Quality:\n\n- Formatted with `cargo fmt`\n- Linted with `cargo clippy` (all fixes applied)\n- Follows project guidelines (no `From` implementations for domain errors, proper test structure)\n\nThe refactoring improves type safety by making `tool_call_success` non-optional, ensuring that tool call success status is always explicitly tracked.\n````\n\n\n---\n\nProceed with implementation based on this context.\n" max_tokens: 20480 top_p: 0.8 top_k: 30 diff --git a/crates/forge_app/src/snapshots/forge_app__compact__tests__render_summary_frame_snapshot.snap b/crates/forge_app/src/snapshots/forge_app__compact__tests__render_summary_frame_snapshot.snap index 2efb07c3b1..233aca09e7 100644 --- a/crates/forge_app/src/snapshots/forge_app__compact__tests__render_summary_frame_snapshot.snap +++ b/crates/forge_app/src/snapshots/forge_app__compact__tests__render_summary_frame_snapshot.snap @@ -30,7 +30,7 @@ Now update the `From<&TextMessage>` implementation to handle optional call_id: ```` Now let's run the tests to verify everything works: ```` -**Execute:** +**Execute:** ``` cargo test --package forge_domain --lib compact::summary ``` @@ -57,7 +57,7 @@ Now let me read the test section and fix the remaining tests: ```` Now I need to remove the old tests and update the ones that need fixing. Let me first check how many lines the file has: ```` -**Execute:** +**Execute:** ``` wc -l crates/forge_domain/src/compact/summary.rs ``` @@ -108,7 +108,7 @@ Let me remove the last test that uses the old struct-based API: ```` Now let me run the tests again to see if there are any remaining issues: ```` -**Execute:** +**Execute:** ``` cargo test --package forge_domain --lib compact::summary 2>&1 | head -100 ``` @@ -159,7 +159,7 @@ Good! Now let me fix the test helpers: ```` Now I need to update all the test cases. Due to the large number of changes needed, let me run the tests first to see the current errors, then I'll provide a summary of what was accomplished: ```` -**Execute:** +**Execute:** ``` cargo test --package forge_domain --lib compact::summary 2>&1 | head -50 ``` @@ -241,7 +241,7 @@ Now let me update the transformer files to use the non-optional `tool_call_succe ```` Good! Now let me run the tests to see the current status: ```` -**Execute:** +**Execute:** ``` cargo test --package forge_domain --lib compact::summary 2>&1 | head -100 ``` @@ -256,15 +256,15 @@ Now let me search for all other uses of `Block::read`, `Block::update`, and `Blo ```` There are many more uses. Let me use a more efficient approach and fix them all at once with a replace_all operation: ```` -**Execute:** +**Execute:** ``` sed -i '' 's/Block::read("/Block::read(None, "/g' crates/forge_domain/src/compact/transformers/strip_working_dir.rs ``` -**Execute:** +**Execute:** ``` sed -i '' 's/Block::update("/Block::update(None, "/g' crates/forge_domain/src/compact/transformers/strip_working_dir.rs ``` -**Execute:** +**Execute:** ``` sed -i '' 's/Block::remove("/Block::remove(None, "/g' crates/forge_domain/src/compact/transformers/strip_working_dir.rs ``` @@ -309,7 +309,7 @@ I've successfully added comprehensive tests for `crates/forge_domain/src/compact Let me run the tests to see the current status: ```` -**Execute:** +**Execute:** ``` cargo test --package forge_domain --lib compact::summary::tests 2>&1 | tail -30 ``` @@ -320,14 +320,14 @@ Let me fix those mutable warnings and then run the actual summary tests: ```` Now let me run just the summary tests to verify they work: ```` -**Execute:** +**Execute:** ``` cargo test --package forge_domain --lib summary::tests -- --nocapture 2>&1 | grep -A 5 "test result" ``` ```` Let me check if there are still compilation errors: ```` -**Execute:** +**Execute:** ``` cargo build --package forge_domain --lib 2>&1 | head -50 ``` @@ -338,14 +338,14 @@ Let me fix the strip_working_dir.rs import and type issues: ```` Now let me try building again: ```` -**Execute:** +**Execute:** ``` cargo test --package forge_domain --lib compact::summary::tests 2>&1 | tail -20 ``` ```` The remaining errors are all in the `trim_context_summary.rs` tests which use the old API extensively. Let me format and lint what we have so far: ```` -**Execute:** +**Execute:** ``` cargo +nightly fmt --all && cargo +nightly clippy --fix --allow-staged --allow-dirty --package forge_domain 2>&1 | tail -30 ``` diff --git a/crates/forge_app/src/snapshots/forge_app__compact__tests__template_engine_renders_summary_frame.snap b/crates/forge_app/src/snapshots/forge_app__compact__tests__template_engine_renders_summary_frame.snap index 82122f2c0d..a4a02fac49 100644 --- a/crates/forge_app/src/snapshots/forge_app__compact__tests__template_engine_renders_summary_frame.snap +++ b/crates/forge_app/src/snapshots/forge_app__compact__tests__template_engine_renders_summary_frame.snap @@ -43,7 +43,7 @@ Search for TODO comments ### 8. Assistant -**Execute:** +**Execute:** ``` cargo test ``` diff --git a/crates/forge_app/src/snapshots/forge_app__tool_registry__all_rendered_tool_descriptions.snap b/crates/forge_app/src/snapshots/forge_app__tool_registry__all_rendered_tool_descriptions.snap index 3b19a17d7b..aeb4ebc532 100644 --- a/crates/forge_app/src/snapshots/forge_app__tool_registry__all_rendered_tool_descriptions.snap +++ b/crates/forge_app/src/snapshots/forge_app__tool_registry__all_rendered_tool_descriptions.snap @@ -153,6 +153,13 @@ Bad example: Returns complete output including stdout, stderr, and exit code for diagnostic purposes. +Background execution: + - Set `nohup: true` to run long-running processes (like web servers) in the background. + - When nohup is true, the command runs via `nohup > /tmp/.log 2>&1 &` + - Returns the log file path and process ID instead of waiting for completion. + - Use the read tool to check the log file for output. + - IMPORTANT: The log file path is preserved during context compaction so you can always access it. + --- ### fetch diff --git a/crates/forge_app/src/system_prompt.rs b/crates/forge_app/src/system_prompt.rs index b8131ed2d8..bca8eb316e 100644 --- a/crates/forge_app/src/system_prompt.rs +++ b/crates/forge_app/src/system_prompt.rs @@ -47,6 +47,7 @@ impl SystemPrompt { true, None, None, + false, ) .await .ok()?; diff --git a/crates/forge_app/src/tool_executor.rs b/crates/forge_app/src/tool_executor.rs index b96bf2c708..a634b32797 100644 --- a/crates/forge_app/src/tool_executor.rs +++ b/crates/forge_app/src/tool_executor.rs @@ -263,6 +263,7 @@ impl< false, input.env.clone(), input.description.clone(), + input.nohup, ) .await?; output.into() diff --git a/crates/forge_app/src/transformers/trim_context_summary.rs b/crates/forge_app/src/transformers/trim_context_summary.rs index 333a14b9fb..ef64a1473d 100644 --- a/crates/forge_app/src/transformers/trim_context_summary.rs +++ b/crates/forge_app/src/transformers/trim_context_summary.rs @@ -49,7 +49,7 @@ fn to_op(tool: &SummaryTool) -> Operation<'_> { SummaryTool::FileUpdate { path } => Operation::File(path), SummaryTool::FileRemove { path } => Operation::File(path), SummaryTool::Undo { path } => Operation::File(path), - SummaryTool::Shell { command } => Operation::Shell(command), + SummaryTool::Shell { command, .. } => Operation::Shell(command), SummaryTool::Search { pattern } => Operation::Search(pattern), SummaryTool::SemSearch { queries } => Operation::CodebaseSearch { queries }, SummaryTool::Fetch { url } => Operation::Fetch(url), diff --git a/crates/forge_domain/src/compact/summary.rs b/crates/forge_domain/src/compact/summary.rs index 371087419c..366e3a7d5f 100644 --- a/crates/forge_domain/src/compact/summary.rs +++ b/crates/forge_domain/src/compact/summary.rs @@ -101,7 +101,17 @@ impl SummaryToolCall { pub fn shell(command: impl Into) -> Self { Self { id: None, - tool: SummaryTool::Shell { command: command.into() }, + tool: SummaryTool::Shell { command: command.into(), log_file: None }, + is_success: true, + } + } + + /// Creates a Shell tool call with a log file for nohup background + /// processes. + pub fn shell_with_log(command: impl Into, log_file: impl Into) -> Self { + Self { + id: None, + tool: SummaryTool::Shell { command: command.into(), log_file: Some(log_file.into()) }, is_success: true, } } @@ -180,19 +190,47 @@ impl SummaryToolCall { #[derive(Clone, PartialEq, Debug, Serialize, Deserialize)] #[serde(rename_all = "snake_case")] pub enum SummaryTool { - FileRead { path: String }, - FileUpdate { path: String }, - FileRemove { path: String }, - Shell { command: String }, - Search { pattern: String }, - SemSearch { queries: Vec }, - Undo { path: String }, - Fetch { url: String }, - Followup { question: String }, - Plan { plan_name: String }, - Skill { name: String }, - Mcp { name: String }, - TodoWrite { changes: Vec }, + FileRead { + path: String, + }, + FileUpdate { + path: String, + }, + FileRemove { + path: String, + }, + Shell { + command: String, + #[serde(skip_serializing_if = "Option::is_none")] + log_file: Option, + }, + Search { + pattern: String, + }, + SemSearch { + queries: Vec, + }, + Undo { + path: String, + }, + Fetch { + url: String, + }, + Followup { + question: String, + }, + Plan { + plan_name: String, + }, + Skill { + name: String, + }, + Mcp { + name: String, + }, + TodoWrite { + changes: Vec, + }, TodoRead, } @@ -283,7 +321,7 @@ impl From<&Context> for ContextSummary { .push(SummaryBlock { role: current_role, contents: std::mem::take(&mut buffer) }); } - // Update tool call success status based on results + // Update tool call success status and extract nohup log file paths messages .iter_mut() .flat_map(|message| message.contents.iter_mut()) @@ -293,6 +331,19 @@ impl From<&Context> for ContextSummary { && let Some(result) = tool_results.get(call_id) { tool_data.is_success = !result.is_error(); + + // For nohup shell commands, extract the log file path from + // the tool result output so it is preserved through + // compaction. + if let SummaryTool::Shell { log_file, .. } = &mut tool_data.tool + && let Some(output_str) = result.output.as_str() + && let Some(path) = output_str + .lines() + .find(|line| line.starts_with("Log file: ")) + .map(|line| line.trim_start_matches("Log file: ").trim()) + { + *log_file = Some(path.to_string()); + } } }); @@ -342,7 +393,10 @@ fn extract_tool_info(call: &ToolCallFull, current_todos: &[Todo]) -> Option Some(SummaryTool::FileUpdate { path: input.file_path }), ToolCatalog::Patch(input) => Some(SummaryTool::FileUpdate { path: input.file_path }), ToolCatalog::Remove(input) => Some(SummaryTool::FileRemove { path: input.path }), - ToolCatalog::Shell(input) => Some(SummaryTool::Shell { command: input.command }), + ToolCatalog::Shell(input) => Some(SummaryTool::Shell { + command: input.command, + log_file: None, // Will be populated from tool result for nohup commands + }), ToolCatalog::FsSearch(input) => { // Use glob, file_type, or pattern as the search identifier let pattern = input.glob.or(input.file_type).unwrap_or(input.pattern); @@ -911,7 +965,7 @@ mod tests { let expected = Block::ToolCall(SummaryToolCall { id: None, - tool: SummaryTool::Shell { command: "cargo build".to_string() }, + tool: SummaryTool::Shell { command: "cargo build".to_string(), log_file: None }, is_success: true, }); diff --git a/crates/forge_domain/src/tools/catalog.rs b/crates/forge_domain/src/tools/catalog.rs index d17aefa00b..175cf9e39c 100644 --- a/crates/forge_domain/src/tools/catalog.rs +++ b/crates/forge_domain/src/tools/catalog.rs @@ -564,6 +564,14 @@ pub struct Shell { #[serde(default)] #[serde(skip_serializing_if = "Option::is_none")] pub description: Option, + + /// Whether to run the command in the background using nohup. + /// When true, the command runs via `nohup > /tmp/.log 2>&1 + /// &` and immediately returns the log file path and process ID. + /// Use this for long-running processes like web servers. + #[serde(default)] + #[serde(skip_serializing_if = "is_default")] + pub nohup: bool, } /// Input type for the net fetch tool @@ -1702,6 +1710,7 @@ mod tests { keep_ansi: false, env: None, description: Some("Shows working tree status".to_string()), + nohup: false, }; let actual = serde_json::to_value(&fixture).unwrap(); @@ -1725,6 +1734,7 @@ mod tests { keep_ansi: false, env: None, description: None, + nohup: false, }; let actual = serde_json::to_value(&fixture).unwrap(); @@ -1747,6 +1757,7 @@ mod tests { keep_ansi: false, env: None, description: None, + nohup: false, }; let actual = serde_json::to_value(&fixture).unwrap(); diff --git a/crates/forge_domain/src/tools/definition/snapshots/forge_domain__tools__definition__usage__tests__tool_usage.snap b/crates/forge_domain/src/tools/definition/snapshots/forge_domain__tools__definition__usage__tests__tool_usage.snap index 7a8ee5169b..d485f9ed91 100644 --- a/crates/forge_domain/src/tools/definition/snapshots/forge_domain__tools__definition__usage__tests__tool_usage.snap +++ b/crates/forge_domain/src/tools/definition/snapshots/forge_domain__tools__definition__usage__tests__tool_usage.snap @@ -9,7 +9,7 @@ expression: prompt {"name":"remove","description":"Request to remove a file at the specified path. Use when you need to delete an existing file. The path must be absolute. This operation can be undone using the `{{tool_names.undo}}` tool.","arguments":{"path":{"description":"The path of the file to remove (absolute path required)","type":"string","is_required":true}}} {"name":"patch","description":"Performs exact string replacements in files.\nUsage:\n- You must use your `{{tool_names.read}}` tool at least once in the conversation before editing. This tool will error if you attempt an edit without reading the file. \n- When editing text from `{{tool_names.read}}` tool output, ensure you preserve the exact indentation (tabs/spaces) as it appears AFTER the line number prefix. The line number prefix format is: 'line_number:'. Everything after that line_number: is the actual file content to match. Never include any part of the line number prefix in the old_string or new_string.\n- ALWAYS prefer editing existing files in the codebase. NEVER write new files unless explicitly required.\n- Only use emojis if the user explicitly requests it. Avoid adding emojis to files unless asked.\n- The edit will FAIL if `old_string` is not unique in the file. Either provide a larger string with more surrounding context to make it unique or use `replace_all` to change every instance of `old_string`. \n- Use `replace_all` for replacing and renaming strings across the file. This parameter is useful if you want to rename a variable for instance.","arguments":{"file_path":{"description":"The absolute path to the file to modify","type":"string","is_required":true},"new_string":{"description":"The text to replace it with (must be different from old_string)","type":"string","is_required":true},"old_string":{"description":"The text to replace","type":"string","is_required":true},"replace_all":{"description":"Replace all occurrences of old_string (default false)","type":"boolean","is_required":false}}} {"name":"undo","description":"Reverts the most recent file operation (create/modify/delete) on a specific file. Use this tool when you need to recover from incorrect file changes or if a revert is requested by the user.","arguments":{"path":{"description":"The absolute path of the file to revert to its previous state.","type":"string","is_required":true}}} -{"name":"shell","description":"Executes shell commands. The `cwd` parameter sets the working directory for command execution. If not specified, defaults to `{{env.cwd}}`.\n\nCRITICAL: Do NOT use `cd` commands in the command string. This is FORBIDDEN. Always use the `cwd` parameter to set the working directory instead. Any use of `cd` in the command is redundant, incorrect, and violates the tool contract.\n\nIMPORTANT: This tool is for terminal operations like git, npm, docker, etc. DO NOT use it for file operations (reading, writing, editing, searching, finding files) - use the specialized tools for this instead.\n\nBefore executing the command, please follow these steps:\n\n1. Directory Verification:\n - If the command will create new directories or files, first use `shell` with `ls` to verify the parent directory exists and is the correct location\n - For example, before running \"mkdir foo/bar\", first use `ls foo` to check that \"foo\" exists and is the intended parent directory\n\n2. Command Execution:\n - Always quote file paths that contain spaces with double quotes (e.g., python \"path with spaces/script.py\")\n - Examples of proper quoting:\n - mkdir \"/Users/name/My Documents\" (correct)\n - mkdir /Users/name/My Documents (incorrect - will fail)\n - python \"/path/with spaces/script.py\" (correct)\n - python /path/with spaces/script.py (incorrect - will fail)\n - After ensuring proper quoting, execute the command.\n - Capture the output of the command.\n\nUsage notes:\n - The command argument is required.\n - It is very helpful if you write a clear, concise description of what this command does in 5-10 words.\n - If the output exceeds {{env.stdoutMaxPrefixLength}} prefix lines or {{env.stdoutMaxSuffixLength}} suffix lines, or if a line exceeds {{env.stdoutMaxLineLength}} characters, it will be truncated and the full output will be written to a temporary file. You can use read with start_line/end_line to read specific sections or fs_search to search the full content. Because of this, you do NOT need to use `head`, `tail`, or other truncation commands to limit output - just run the command directly.\n - Avoid using {{tool_names.shell}} with the `find`, `grep`, `cat`, `head`, `tail`, `sed`, `awk`, or `echo` commands, unless explicitly instructed or when these commands are truly necessary for the task. Instead, always prefer using the dedicated tools for these commands:\n - File search: Use `{{tool_names.fs_search}}` (NOT find or ls)\n - Content search: Use `{{tool_names.fs_search}}` with regex (NOT grep or rg)\n - Read files: Use `{{tool_names.read}}` (NOT cat/head/tail)\n - Edit files: Use `{{tool_names.patch}}`(NOT sed/awk)\n - Write files: Use `{{tool_names.write}}` (NOT echo >/cat < && `. Use the `cwd` parameter to change directories instead.\n\nGood examples:\n - With explicit cwd: cwd=\"/foo/bar\" with command: pytest tests\n\nBad example:\n cd /foo/bar && pytest tests\n\nReturns complete output including stdout, stderr, and exit code for diagnostic purposes.","arguments":{"command":{"description":"The shell command to execute.","type":"string","is_required":true},"cwd":{"description":"The working directory where the command should be executed.\nIf not specified, defaults to the current working directory from the\nenvironment.","type":"string","is_required":false},"description":{"description":"Clear, concise description of what this command does. Recommended to be\n5-10 words for simple commands. For complex commands with pipes or\nmultiple operations, provide more context. Examples: \"Lists files in\ncurrent directory\", \"Installs package dependencies\", \"Compiles Rust\nproject with release optimizations\".","type":"string","is_required":false},"env":{"description":"Environment variable names to pass to command execution (e.g., [\"PATH\",\n\"HOME\", \"USER\"]). The system automatically reads the specified\nvalues and applies them during command execution.","type":"array","is_required":false},"keep_ansi":{"description":"Whether to preserve ANSI escape codes in the output.\nIf true, ANSI escape codes will be preserved in the output.\nIf false (default), ANSI escape codes will be stripped from the output.","type":"boolean","is_required":false}}} +{"name":"shell","description":"Executes shell commands. The `cwd` parameter sets the working directory for command execution. If not specified, defaults to `{{env.cwd}}`.\n\nCRITICAL: Do NOT use `cd` commands in the command string. This is FORBIDDEN. Always use the `cwd` parameter to set the working directory instead. Any use of `cd` in the command is redundant, incorrect, and violates the tool contract.\n\nIMPORTANT: This tool is for terminal operations like git, npm, docker, etc. DO NOT use it for file operations (reading, writing, editing, searching, finding files) - use the specialized tools for this instead.\n\nBefore executing the command, please follow these steps:\n\n1. Directory Verification:\n - If the command will create new directories or files, first use `shell` with `ls` to verify the parent directory exists and is the correct location\n - For example, before running \"mkdir foo/bar\", first use `ls foo` to check that \"foo\" exists and is the intended parent directory\n\n2. Command Execution:\n - Always quote file paths that contain spaces with double quotes (e.g., python \"path with spaces/script.py\")\n - Examples of proper quoting:\n - mkdir \"/Users/name/My Documents\" (correct)\n - mkdir /Users/name/My Documents (incorrect - will fail)\n - python \"/path/with spaces/script.py\" (correct)\n - python /path/with spaces/script.py (incorrect - will fail)\n - After ensuring proper quoting, execute the command.\n - Capture the output of the command.\n\nUsage notes:\n - The command argument is required.\n - It is very helpful if you write a clear, concise description of what this command does in 5-10 words.\n - If the output exceeds {{env.stdoutMaxPrefixLength}} prefix lines or {{env.stdoutMaxSuffixLength}} suffix lines, or if a line exceeds {{env.stdoutMaxLineLength}} characters, it will be truncated and the full output will be written to a temporary file. You can use read with start_line/end_line to read specific sections or fs_search to search the full content. Because of this, you do NOT need to use `head`, `tail`, or other truncation commands to limit output - just run the command directly.\n - Avoid using {{tool_names.shell}} with the `find`, `grep`, `cat`, `head`, `tail`, `sed`, `awk`, or `echo` commands, unless explicitly instructed or when these commands are truly necessary for the task. Instead, always prefer using the dedicated tools for these commands:\n - File search: Use `{{tool_names.fs_search}}` (NOT find or ls)\n - Content search: Use `{{tool_names.fs_search}}` with regex (NOT grep or rg)\n - Read files: Use `{{tool_names.read}}` (NOT cat/head/tail)\n - Edit files: Use `{{tool_names.patch}}`(NOT sed/awk)\n - Write files: Use `{{tool_names.write}}` (NOT echo >/cat < && `. Use the `cwd` parameter to change directories instead.\n\nGood examples:\n - With explicit cwd: cwd=\"/foo/bar\" with command: pytest tests\n\nBad example:\n cd /foo/bar && pytest tests\n\nReturns complete output including stdout, stderr, and exit code for diagnostic purposes.\n\nBackground execution:\n - Set `nohup: true` to run long-running processes (like web servers) in the background.\n - When nohup is true, the command runs via `nohup > /tmp/.log 2>&1 &`\n - Returns the log file path and process ID instead of waiting for completion.\n - Use the read tool to check the log file for output.\n - IMPORTANT: The log file path is preserved during context compaction so you can always access it.","arguments":{"command":{"description":"The shell command to execute.","type":"string","is_required":true},"cwd":{"description":"The working directory where the command should be executed.\nIf not specified, defaults to the current working directory from the\nenvironment.","type":"string","is_required":false},"description":{"description":"Clear, concise description of what this command does. Recommended to be\n5-10 words for simple commands. For complex commands with pipes or\nmultiple operations, provide more context. Examples: \"Lists files in\ncurrent directory\", \"Installs package dependencies\", \"Compiles Rust\nproject with release optimizations\".","type":"string","is_required":false},"env":{"description":"Environment variable names to pass to command execution (e.g., [\"PATH\",\n\"HOME\", \"USER\"]). The system automatically reads the specified\nvalues and applies them during command execution.","type":"array","is_required":false},"keep_ansi":{"description":"Whether to preserve ANSI escape codes in the output.\nIf true, ANSI escape codes will be preserved in the output.\nIf false (default), ANSI escape codes will be stripped from the output.","type":"boolean","is_required":false},"nohup":{"description":"Whether to run the command in the background using nohup.\nWhen true, the command runs via `nohup > /tmp/.log 2>&1 &`\nand immediately returns the log file path and process ID.\nUse this for long-running processes like web servers.","type":"boolean","is_required":false}}} {"name":"fetch","description":"Retrieves content from URLs as markdown or raw text. Enables access to current online information including websites, APIs and documentation. Use for obtaining up-to-date information beyond training data, verifying facts, or retrieving specific online content. Handles HTTP/HTTPS and converts HTML to readable markdown by default. Cannot access private/restricted resources requiring authentication. Respects robots.txt and may be blocked by anti-scraping measures. For large pages, returns the first 40,000 characters and stores the complete content in a temporary file for subsequent access.\n\nIMPORTANT: This tool only handles text-based content (HTML, JSON, XML, plain text, etc.). It will reject binary file downloads (.tar.gz, .zip, .bin, .deb, images, audio, video, etc.) with an error. To download binary files, use the `shell` tool with `curl -fLo ` instead.","arguments":{"raw":{"description":"Get raw content without any markdown conversion (default: false)","type":"boolean","is_required":false},"url":{"description":"URL to fetch","type":"string","is_required":true}}} {"name":"followup","description":"Use this tool when you encounter ambiguities, need clarification, or require more details to proceed effectively. Use this tool judiciously to maintain a balance between gathering necessary information and avoiding excessive back-and-forth.","arguments":{"multiple":{"description":"If true, allows selecting multiple options; if false (default), only one\noption can be selected","type":"boolean","is_required":false},"option1":{"description":"First option to choose from","type":"string","is_required":false},"option2":{"description":"Second option to choose from","type":"string","is_required":false},"option3":{"description":"Third option to choose from","type":"string","is_required":false},"option4":{"description":"Fourth option to choose from","type":"string","is_required":false},"option5":{"description":"Fifth option to choose from","type":"string","is_required":false},"question":{"description":"Question to ask the user","type":"string","is_required":true}}} {"name":"plan","description":"Creates a new plan file with the specified name, version, and content. Use this tool to create structured project plans, task breakdowns, or implementation strategies that can be tracked and referenced throughout development sessions.","arguments":{"content":{"description":"The content to write to the plan file. This should be the complete\nplan content in markdown format.","type":"string","is_required":true},"plan_name":{"description":"The name of the plan (will be used in the filename)","type":"string","is_required":true},"version":{"description":"The version of the plan (e.g., \"v1\", \"v2\", \"1.0\")","type":"string","is_required":true}}} diff --git a/crates/forge_domain/src/tools/descriptions/shell.md b/crates/forge_domain/src/tools/descriptions/shell.md index 24d62c33af..768fa57fc0 100644 --- a/crates/forge_domain/src/tools/descriptions/shell.md +++ b/crates/forge_domain/src/tools/descriptions/shell.md @@ -44,4 +44,11 @@ Good examples: Bad example: cd /foo/bar && pytest tests -Returns complete output including stdout, stderr, and exit code for diagnostic purposes. \ No newline at end of file +Returns complete output including stdout, stderr, and exit code for diagnostic purposes. + +Background execution: + - Set `nohup: true` to run long-running processes (like web servers) in the background. + - When nohup is true, the command runs via `nohup > /tmp/.log 2>&1 &` + - Returns the log file path and process ID instead of waiting for completion. + - Use the read tool to check the log file for output. + - IMPORTANT: The log file path is preserved during context compaction so you can always access it. \ No newline at end of file diff --git a/crates/forge_domain/src/tools/snapshots/forge_domain__tools__catalog__tests__tool_definition_json.snap b/crates/forge_domain/src/tools/snapshots/forge_domain__tools__catalog__tests__tool_definition_json.snap index ce2da28a9c..75831468ff 100644 --- a/crates/forge_domain/src/tools/snapshots/forge_domain__tools__catalog__tests__tool_definition_json.snap +++ b/crates/forge_domain/src/tools/snapshots/forge_domain__tools__catalog__tests__tool_definition_json.snap @@ -257,6 +257,10 @@ expression: tools "keep_ansi": { "description": "Whether to preserve ANSI escape codes in the output.\nIf true, ANSI escape codes will be preserved in the output.\nIf false (default), ANSI escape codes will be stripped from the output.", "type": "boolean" + }, + "nohup": { + "description": "Whether to run the command in the background using nohup.\nWhen true, the command runs via `nohup > /tmp/.log 2>&1 &`\nand immediately returns the log file path and process ID.\nUse this for long-running processes like web servers.", + "type": "boolean" } }, "required": [ diff --git a/crates/forge_services/src/tool_services/shell.rs b/crates/forge_services/src/tool_services/shell.rs index 3d9d34334b..882b4ce257 100644 --- a/crates/forge_services/src/tool_services/shell.rs +++ b/crates/forge_services/src/tool_services/shell.rs @@ -2,7 +2,8 @@ use std::path::PathBuf; use std::sync::Arc; use anyhow::bail; -use forge_app::domain::Environment; +use chrono::Utc; +use forge_app::domain::{CommandOutput, Environment}; use forge_app::{CommandInfra, EnvironmentInfra, ShellOutput, ShellService}; use strip_ansi_escapes::strip; @@ -11,6 +12,21 @@ fn strip_ansi(content: String) -> String { String::from_utf8_lossy(&strip(content.as_bytes())).into_owned() } +/// Sanitizes a command string to be safe for use in a file name. +fn sanitize_for_filename(command: &str) -> String { + command + .chars() + .map(|c| { + if c.is_alphanumeric() || c == '-' || c == '_' { + c + } else { + '_' + } + }) + .take(50) + .collect() +} + /// Prevents potentially harmful operations like absolute path execution and /// directory changes. Use for file system interaction, running utilities, /// installing packages, or executing build commands. For operations requiring @@ -47,20 +63,60 @@ impl ShellService for ForgeShell { silent: bool, env_vars: Option>, description: Option, + nohup: bool, ) -> anyhow::Result { Self::validate_command(&command)?; - let mut output = self - .infra - .execute_command(command, cwd, silent, env_vars) - .await?; + if nohup { + // Build the log file path: /tmp/-.log + let sanitized = sanitize_for_filename(&command); + let timestamp = Utc::now().format("%Y%m%d-%H%M%S"); + let log_path = format!("/tmp/{sanitized}-{timestamp}.log"); + + // Construct the nohup command that runs in background and echoes PID + let bg_command = format!("nohup {command} > '{log_path}' 2>&1 & echo $!"); + + let mut output = self + .infra + .execute_command(bg_command, cwd, silent, env_vars) + .await?; + + if !keep_ansi { + output.stdout = strip_ansi(output.stdout); + output.stderr = strip_ansi(output.stderr); + } + + // The stdout contains the PID from `echo $!` + let pid = output.stdout.trim().to_string(); + + // Replace the output with structured nohup result + let nohup_output = CommandOutput { + command: command.clone(), + stdout: format!( + "Background process started.\nPID: {pid}\nLog file: {log_path}\n\nUse the read tool to check the log file for output." + ), + stderr: output.stderr, + exit_code: Some(0), + }; - if !keep_ansi { - output.stdout = strip_ansi(output.stdout); - output.stderr = strip_ansi(output.stderr); + Ok(ShellOutput { + output: nohup_output, + shell: self.env.shell.clone(), + description, + }) + } else { + let mut output = self + .infra + .execute_command(command, cwd, silent, env_vars) + .await?; + + if !keep_ansi { + output.stdout = strip_ansi(output.stdout); + output.stderr = strip_ansi(output.stderr); + } + + Ok(ShellOutput { output, shell: self.env.shell.clone(), description }) } - - Ok(ShellOutput { output, shell: self.env.shell.clone(), description }) } } #[cfg(test)] @@ -143,6 +199,7 @@ mod tests { false, Some(vec!["PATH".to_string(), "HOME".to_string()]), None, + false, ) .await .unwrap(); @@ -163,6 +220,7 @@ mod tests { false, None, None, + false, ) .await .unwrap(); @@ -185,6 +243,7 @@ mod tests { false, Some(vec![]), None, + false, ) .await .unwrap(); @@ -205,6 +264,7 @@ mod tests { false, None, Some("Prints hello to stdout".to_string()), + false, ) .await .unwrap(); @@ -229,6 +289,7 @@ mod tests { false, None, None, + false, ) .await .unwrap(); @@ -237,4 +298,43 @@ mod tests { assert_eq!(actual.output.exit_code, Some(0)); assert_eq!(actual.description, None); } + + #[tokio::test] + async fn test_shell_service_nohup() { + let fixture = ForgeShell::new(Arc::new(MockCommandInfra { expected_env_vars: None })); + + let actual = fixture + .execute( + "npm start".to_string(), + PathBuf::from("."), + false, + false, + None, + Some("Start the server".to_string()), + true, + ) + .await + .unwrap(); + + // nohup should return structured output with PID and log file path + assert!(actual.output.stdout.contains("Background process started.")); + assert!(actual.output.stdout.contains("PID:")); + assert!(actual.output.stdout.contains("Log file: /tmp/npm_start-")); + assert!(actual.output.stdout.contains(".log")); + assert_eq!(actual.output.exit_code, Some(0)); + assert_eq!(actual.description, Some("Start the server".to_string())); + } + + #[test] + fn test_sanitize_for_filename() { + assert_eq!(sanitize_for_filename("npm start"), "npm_start"); + assert_eq!(sanitize_for_filename("ls -la"), "ls_-la"); + assert_eq!( + sanitize_for_filename("python manage.py runserver 0.0.0.0:8000"), + "python_manage_py_runserver_0_0_0_0_8000" + ); + // Test truncation at 50 chars + let long_command = "a".repeat(100); + assert_eq!(sanitize_for_filename(&long_command).len(), 50); + } } diff --git a/templates/forge-partial-summary-frame.md b/templates/forge-partial-summary-frame.md index e48d69ca40..b1eac1ea8e 100644 --- a/templates/forge-partial-summary-frame.md +++ b/templates/forge-partial-summary-frame.md @@ -28,10 +28,18 @@ Use the following summary frames as the authoritative reference for all coding s - `{{use_case}}` {{/each}} {{else if tool_call.tool.shell}} -**Execute:** +{{#if tool_call.tool.shell.log_file}} +**Spawned (background):** ``` {{tool_call.tool.shell.command}} ``` +Log file: `{{tool_call.tool.shell.log_file}}` +{{else}} +**Execute:** +``` +{{tool_call.tool.shell.command}} +``` +{{/if}} {{else if tool_call.tool.mcp}} **MCP:** `{{tool_call.tool.mcp.name}}` {{else if tool_call.tool.todo_write}}