Conversation
This comment was marked as outdated.
This comment was marked as outdated.
Contributor
There was a problem hiding this comment.
Pull request overview
This is a major architectural refactoring that extracts runner implementations to a dedicated package and removes agent functionality (now in the Don project).
Changes:
- Extracted all runner implementations (exec, sandbox-exec, firejail, docker) from
pkg/commandto newpkg/runnerpackage - Removed entire agent package and related code (agent functionality moved to Don project)
- Added shell completion command for better CLI usability
- Updated Go version and dependencies
- Comprehensive documentation updates reflecting the changes
Reviewed changes
Copilot reviewed 75 out of 76 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| go.mod | Updated Go version to 1.25.5 and removed agent dependencies (OpenAI SDK, cagent, etc.) |
| pkg/runner/*.go | New dedicated runner package with exec, sandbox-exec, firejail, and docker implementations |
| pkg/command/*.go | Updated imports to use new runner package |
| pkg/server/server.go | Removed agent-related methods (GetOpenAITools, ExecuteTool) |
| pkg/utils/home.go | Added MCPShellAgentConfigEnv constant |
| cmd/agent*.go | Deleted agent command and subcommands |
| cmd/completion.go | New shell completion command |
| tests/agent/*.sh | Removed agent test files |
| docs/*.md | Updated documentation to remove agent references and point to Don project |
| examples/README.md | Fixed typo ("paramters" → "parameters") |
| mcpshell.code-workspace | Added VS Code workspace with Don project reference |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Signed-off-by: Alvaro Saurin <saurin@adobe.com>
Runner options (Docker image, user, network settings, etc.) are now only taken from the server-side tool configuration. External callers (MCP clients, CLI users) cannot override these options to prevent privilege escalation attacks. This addresses the security concern raised in PR review where a malicious client could specify a different Docker image or user.
The agent functionality was removed, so this constant is no longer needed.
The function handles multiple config paths, so the plural form is more semantically correct.
Restore tests that were removed during the runner package extraction: - TestDocker_Run_Networking: tests network isolation - TestDocker_Run_PrepareCommand: tests prepare_command option - TestExec_Optimization_SingleExecutable: tests single executable optimization - Expanded TestNewDockerOptions assertions for all fields - Expanded TestSandboxExec_Run with template and env variable tests
085b2b6 to
e620370
Compare
Alpine Linux uses ash (via /bin/sh) and doesn't have bash installed. The Docker runner test was failing because it tried to execute 'bash -c' inside the Alpine container.
Environment variables with spaces in their values were causing the docker command to be parsed incorrectly. For example, 'TEST_MESSAGE=Hello from Docker' was being split, causing 'from' to be interpreted as the image name. Using %q format specifier ensures proper quoting.
The previous fix using Go's %q format didn't work correctly when the docker command was passed through sh -c, as the nested quoting caused issues. Added shellQuote() helper function that uses single quotes with proper escaping for shell-safe quoting of values containing spaces or special characters.
When environment variables with spaces (e.g., 'Hello from Docker container') were passed to the Docker runner, the script was generating unquoted export statements like: export TEST_MESSAGE=Hello from Docker container This caused the shell to only assign 'Hello' to TEST_MESSAGE. Now using shellQuote() to properly quote values: export TEST_MESSAGE='Hello from Docker container' Fixes CI failure in test_runner_docker.sh environment variable test.
…ne issues YAML literal blocks (|) include trailing newlines, causing commands like 'grep --version\n' to be passed to the shell. When quoted with %q, this becomes 'grep --version\n' which the shell interprets as '--versionn'. Now trimming whitespace from commands before embedding in the script.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Major refactoring that extracts the runner implementations into a dedicated \ package and removes the agent functionality (now available in the Don project).
Changes
Architecture
Agent Removal
New Features
Documentation
Maintenance
Testing