Conversation
…ion and …" This reverts commit f901af8.
There was a problem hiding this comment.
Pull request overview
This PR reverts the previously introduced exec-tool enhancements (background execution, PTY support, and session management), restoring a simpler synchronous exec interface and removing the associated session infrastructure and dependencies.
Changes:
- Simplifies
exectool API back to synchronous execution with onlycommand(+ optionalworking_dir) arguments. - Removes session management (types, manager, OS-specific process-group helpers) and associated tests.
- Updates agent/tool tests and module dependencies to reflect the reverted exec behavior (drops direct
creack/ptyusage).
Reviewed changes
Copilot reviewed 10 out of 11 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| pkg/tools/types.go | Removes ExecRequest/ExecResponse types tied to session/PTY features. |
| pkg/tools/shell.go | Reverts exec tool to synchronous-only execution; removes action/session/PTY logic and schema. |
| pkg/tools/shell_test.go | Updates tests to the simplified exec API; removes background/PTY/session test coverage. |
| pkg/tools/shell_timeout_unix_test.go | Updates timeout test invocation to omit action. |
| pkg/tools/session.go | Deletes session manager + session types introduced for background/PTY support. |
| pkg/tools/session_test.go | Deletes session manager unit tests. |
| pkg/tools/session_process_unix.go | Deletes session-era process-group helper. |
| pkg/tools/session_process_windows.go | Deletes session-era process-group helper. |
| pkg/agent/instance_test.go | Updates exec invocation to use working_dir/no action. |
| go.mod | Removes direct dependency on github.com/creack/pty. |
| go.sum | Updates sums consistent with dropping direct PTY usage. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| for _, cmd := range commands { | ||
| ctx, cancel := context.WithTimeout(context.Background(), 2*time.Second) | ||
| result := tool.Execute(ctx, map[string]any{"action": "run", "command": cmd}) | ||
| cancel() | ||
| result := tool.Execute(context.Background(), map[string]any{"command": cmd}) | ||
| if result.IsError && strings.Contains(result.ForLLM, "path outside working dir") { |
There was a problem hiding this comment.
TestShellTool_URLsNotBlocked executes real network-capable commands (curl/wget/git clone). This hunk removed the per-call context.WithTimeout, so these tests can now hang or run up to the tool’s default timeout (60s) if the binaries exist and network stalls. Consider restoring a short context timeout here (as before) and/or change the test commands to non-network no-op commands that still contain URLs (e.g., echo/printf with URL arguments) so the test only validates the guard logic.
huaaudio
left a comment
huaaudio
left a comment
There was a problem hiding this comment.
LGTM, we still need to get a workaround for the cross-platform CGo issue and exec stability.
…-enhancement Revert "feat(tools): add exec tool enhancement with background execution and PTY support"
Reverts #1752