Skip to content

feat: add nohup parameter for background shell job execution#2706

Open
chengyixu wants to merge 2 commits intoantinomyhq:mainfrom
chengyixu:feat/background-shell-job-2635
Open

feat: add nohup parameter for background shell job execution#2706
chengyixu wants to merge 2 commits intoantinomyhq:mainfrom
chengyixu:feat/background-shell-job-2635

Conversation

@chengyixu
Copy link
Copy Markdown

Summary

Implements #2635 - adds the ability to run shell commands as background jobs using nohup.

Changes

  • nohup: bool parameter added to the Shell tool input schema (default false)
  • When nohup=true, the command runs via nohup <command> > /tmp/<sanitized>-<timestamp>.log 2>&1 &
  • Returns the log file path and PID instead of blocking for output
  • Console displays * [HH:MM:SS] [Spawned] <command> for background jobs
  • Critical: Background job log file paths are preserved through context compaction by extracting them from tool results and storing in SummaryTool::Shell's new log_file field
  • Updated tool description documentation to explain nohup usage

Files changed

File Description
forge_domain/src/tools/catalog.rs Added nohup: bool field to Shell struct
forge_domain/src/tools/descriptions/shell.md Documented nohup behavior
forge_domain/src/compact/summary.rs Added log_file to SummaryTool::Shell + extraction from tool results
forge_app/src/services.rs Updated ShellService trait signature with nohup param
forge_app/src/tool_executor.rs Pass nohup parameter through
forge_app/src/fmt/fmt_input.rs Show "Spawned" instead of "Execute" for nohup commands
forge_app/src/git_app.rs Updated all internal shell calls with nohup: false
forge_app/src/system_prompt.rs Updated shell call with nohup: false
forge_app/src/orch_spec/orch_runner.rs Updated test mock with nohup param
forge_app/src/transformers/trim_context_summary.rs Use .. pattern for new field
forge_services/src/tool_services/shell.rs Core nohup implementation + tests
templates/forge-partial-summary-frame.md Show background log path in compaction summary

Testing

  • All existing tests pass (539 forge_domain, 530 forge_app, 200 forge_services, 69 forge_infra)
  • Added test_shell_service_nohup — verifies nohup returns PID and log file path
  • Added test_sanitize_for_filename — verifies command sanitization for log filenames

Design Decisions

  1. Compaction preservation: The log file path is extracted from the tool result output (which contains Log file: /tmp/...) and stored in SummaryTool::Shell { log_file }. During compaction, the template renders it as **Spawned (background):** ... Log file: ... so the agent always knows about running background processes.

  2. Log file path quoting: The nohup command uses single quotes around the log path to prevent shell injection when paths contain spaces or special characters.

  3. Backwards compatibility: The nohup field defaults to false and is skipped during serialization when false, so existing tool calls are unaffected.

/claim #2635

Add `nohup: bool` parameter to the Shell tool that allows running
commands in the background using nohup. When enabled, the command
runs via `nohup <command> > /tmp/<log>.log 2>&1 &` and returns the
log file path and PID immediately.

Key changes:
- Shell struct gains `nohup: bool` field (default false)
- When nohup=true, command is wrapped in nohup and returns PID + log path
- Console displays "Spawned" instead of "Execute" for background commands
- Log file paths are extracted from tool results and preserved through
  context compaction via SummaryTool::Shell's new `log_file` field
- Compaction template shows "Spawned (background)" with log file path

Fixes antinomyhq#2635

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@CLAassistant
Copy link
Copy Markdown

CLAassistant commented Mar 26, 2026

CLA assistant check
All committers have signed the CLA.

Comment on lines +67 to +68
let timestamp = Utc::now().format("%Y%m%d-%H%M%S");
let log_path = format!("/tmp/{sanitized}-{timestamp}.log");
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Path Traversal Risk: The log file path uses /tmp/ with a sanitized command name, but doesn't handle potential collisions or validate the directory exists. If multiple processes run the same command within the same second, they'll overwrite each other's log files due to timestamp resolution.

Fix: Add a random component or use process ID:

let timestamp = Utc::now().format("%Y%m%d-%H%M%S");
let log_path = format!("/tmp/{sanitized}-{timestamp}-{}.log", std::process::id());

Spotted by Graphite

Fix in Graphite


Is this helpful? React 👍 or 👎 to let us know.

This comment came from an experimental review—please leave feedback if it was helpful/unhelpful. Learn more about experimental comments here.

@chengyixu
Copy link
Copy Markdown
Author

recheck

@amitksingh1490 amitksingh1490 enabled auto-merge (squash) March 27, 2026 01:50
@amitksingh1490 amitksingh1490 disabled auto-merge March 27, 2026 01:50
@amitksingh1490 amitksingh1490 requested review from amitksingh1490 and tusharmath and removed request for tusharmath March 27, 2026 04:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants