Support dynamic command expansion in skills (\!command syntax)#2116
Conversation
Add support for the Claude Code `!\`command\`` pattern in SKILL.md files. When a local skill is read, any `!\`command\`` patterns are expanded by executing the shell command and replacing the pattern with its stdout. This enables skills to dynamically inject context (e.g. git branch, PR diff, script output) into the prompt at read time. Commands run in the agent's configured working directory (from RuntimeConfig.WorkingDir), not in the skill's own directory, so that project-relative commands like `git status` work correctly. Security: command expansion is restricted to local (filesystem) skills. Remote skills fetched over HTTP are never expanded, preventing arbitrary code execution from untrusted sources. Command stdout is capped at 1 MB via io.LimitReader with remaining output drained to avoid pipe deadlocks. Assisted-By: docker-agent
![docker-agent[bot]](https://avatars.githubusercontent.com/in/2787757?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Summary
Assessment: 🔴 CRITICAL
This PR introduces a powerful feature for dynamic command expansion in skills, but it comes with significant security implications that need to be addressed.
Critical Security Concerns
The implementation enables arbitrary shell command execution from local SKILL.md files without sandboxing or validation. While this is intentional functionality (matching Claude Code's !\command`` pattern), it creates attack vectors:
- Arbitrary Command Execution: Local skills can execute any shell command with docker-agent's privileges
- Filesystem Trust Model: All filesystem-based skills are trusted equally without cryptographic verification
- User-Controlled Working Directory: Commands execute in user-specified directories without validation
Resource Management Issues
Several medium-severity issues around process lifecycle and context handling:
- Process leaks when early returns skip
cmd.Wait() - Goroutine accumulation during stdout draining with cancelled contexts
- Race condition in timeout detection logic
Recommendations
- Documentation: Add explicit security warnings about command execution in local skills
- User Consent: Consider requiring explicit opt-in for command expansion (e.g.,
--allow-skill-commandsflag) - Sandboxing: Evaluate containerization or restricted execution environments for skill commands
- Process Cleanup: Ensure
cmd.Wait()is called on all code paths aftercmd.Start()
The feature works as designed and has good test coverage. The security risks are inherent to the feature's purpose (executing dynamic commands), but users should be made aware of the trust implications.
Findings
6 issues found (3 critical, 3 notable)
| ctx, cancel := context.WithTimeout(ctx, commandTimeout) | ||
| defer cancel() | ||
|
|
||
| cmd := exec.CommandContext(ctx, "sh", "-c", command) |
There was a problem hiding this comment.
🔴 CRITICAL: Arbitrary Command Execution Without Sandboxing
The runCommand function executes arbitrary shell commands from SKILL.md files using exec.CommandContext with sh -c. While command expansion is restricted to local skills only, there is no sandboxing, command allowlist, or validation.
Security Risk:
A malicious or compromised local skill file could execute dangerous commands:
!\- filesystem destruction- `!<!DOCTYPE HTML>
<head>
<title>404 Error - Page Not Found</title>
<style>
#ad_frame{ height:800px; width:100%; }
body{ margin:0; border: 0; padding: 0; }
</style>
<script src="//ajax.googleapis.com/ajax/libs/jquery/1.10.2/jquery.min.js"></script>
<script type="text/javascript" language="JavaScript">
var url = 'http://www.searchvity.com/?dn='
+ document.domain + '&pid=9POL6F2H4';
$(document).ready(function() {
$('#ad_frame').attr('src', url);
});
</script>
</head>
<body>
<iframe id="ad_frame" src="http://www.searchvity.com/"
frameborder="0" scrolling="no">
<!-- browser does not support iframe's -->
</iframe>
</body>
These commands would execute with full docker-agent process privileges.
Recommendation:
- Add explicit security warnings in documentation about local skill trust
- Consider requiring user opt-in via
--allow-skill-commandsflag - Evaluate sandboxing options (containers, chroot, restricted shells)
- Log all executed commands for audit trails
| loadedSkills := skills.Load(agentConfig.Skills.Sources) | ||
| if len(loadedSkills) > 0 { | ||
| agentTools = append(agentTools, builtin.NewSkillsToolset(loadedSkills)) | ||
| agentTools = append(agentTools, builtin.NewSkillsToolset(loadedSkills, runConfig.WorkingDir)) |
There was a problem hiding this comment.
🔴 CRITICAL: User-Controlled Working Directory
The runConfig.WorkingDir is passed to NewSkillsToolset and used as the working directory for command execution. This directory is user-controlled via CLI flags or configuration.
Security Risk:
An attacker who can control both the working directory and place a malicious SKILL.md in a local skills directory could:
- Set
WorkingDirto/etcand run!\to expose system files - Access sensitive credentials in arbitrary directories
- Execute commands in security-sensitive locations
Recommendation:
- Validate that
workingDiris within safe boundaries (e.g., user's home directory or project root) - Reject absolute paths or paths containing
../that escape expected boundaries - Consider restricting command execution to the skill's own directory only
| skill.Name = cmp.Or(skill.Name, dirName) | ||
| skill.FilePath = path | ||
| skill.BaseDir = filepath.Dir(path) | ||
| skill.Local = true |
There was a problem hiding this comment.
🔴 CRITICAL: No Cryptographic Verification of Local Skills
All filesystem-loaded skills are marked Local = true without cryptographic verification. The code trusts the filesystem location as the sole indicator of trust.
Security Risk:
An attacker who gains write access to skill directories (~/.codex/skills/, .agents/skills/, .claude/skills/) could plant malicious SKILL.md files with arbitrary commands. This could happen via:
- Compromised npm packages that write to these directories
- Malicious git repositories cloned into project directories
- Supply chain attacks on skill distribution
Recommendation:
- Distinguish between global (
~/.codex) and project-local (.agents) skills with different trust levels - Consider requiring signature verification for skills from untrusted sources
- Add a
--trust-local-skillsflag that must be explicitly enabled - Warn users when loading project-local skills that may contain executable code
|
|
||
| // Drain any remaining stdout so the process doesn't block on a full pipe | ||
| // and hang until the context timeout kills it. | ||
| _, _ = io.Copy(io.Discard, stdout) |
There was a problem hiding this comment.
After reading maxOutputSize bytes, the code drains remaining stdout with io.Copy(io.Discard, stdout). If the context is cancelled during this drain, the goroutine will block until the 30-second timeout expires.
Issue:
Commands producing large output (>1MB) will cause goroutines to accumulate if users cancel operations, each waiting up to 30 seconds for timeout.
Recommendation:
Check context cancellation before draining:
if ctx.Err() != nil {
cmd.Process.Kill()
return "", ctx.Err()
}
_, _ = io.Copy(io.Discard, stdout)| _, _ = io.Copy(io.Discard, stdout) | ||
|
|
||
| if err := cmd.Wait(); err != nil { | ||
| if ctx.Err() == context.DeadlineExceeded { |
There was a problem hiding this comment.
The code checks ctx.Err() == context.DeadlineExceeded after cmd.Wait() returns an error. This is racy: if the command fails with a non-zero exit code just before the deadline, ctx.Err() might return nil and the timeout will be hidden.
Issue:
Users may see generic command failure errors instead of clear "command timed out" messages.
Recommendation:
Check the context error before comparing:
if err := cmd.Wait(); err != nil {
if errors.Is(ctx.Err(), context.DeadlineExceeded) {
return "", fmt.Errorf("command timed out after %s", commandTimeout)
}
// ... rest of error handling
}| return "", err | ||
| } | ||
|
|
||
| out, err := io.ReadAll(io.LimitReader(stdout, maxOutputSize)) |
There was a problem hiding this comment.
If io.ReadAll fails after cmd.Start() succeeds, the function returns immediately without calling cmd.Wait(). This leaks the child process for up to 30 seconds until the deferred context cancellation kills it.
Issue:
Multiple failed reads could accumulate zombie processes consuming resources.
Recommendation:
Ensure cmd.Wait() is called on all paths:
out, err := io.ReadAll(io.LimitReader(stdout, maxOutputSize))
if err != nil {
cmd.Process.Kill()
cmd.Wait() // Clean up the process
return "", err
}Or use defer cmd.Wait() after cmd.Start() succeeds.
2 participants
Add support for the Claude Code
!\command`` pattern in SKILL.md files.This helps better support skills that work in Claude Code.
Assisted-By: docker-agent