Skip to content

Commit a3529db

Browse files
Copilotalexec
andauthored
Fix double expansion in slash command arguments (#152)
* Initial plan * Add tests to verify single expansion issue Co-authored-by: alexec <1142830+alexec@users.noreply.github.com> * Fix double expansion: parse task before expanding Co-authored-by: alexec <1142830+alexec@users.noreply.github.com> * Improve comment clarity per code review feedback Co-authored-by: alexec <1142830+alexec@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: alexec <1142830+alexec@users.noreply.github.com>
1 parent ff888eb commit a3529db

2 files changed

Lines changed: 78 additions & 8 deletions

File tree

integration_test.go

Lines changed: 66 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1230,3 +1230,69 @@ This rule is from a remote directory.
12301230
t.Errorf("task not found in stdout. Output:\n%s", output)
12311231
}
12321232
}
1233+
1234+
// TestSingleExpansion verifies that content is expanded only once in the full flow
1235+
func TestSingleExpansion(t *testing.T) {
1236+
dirs := setupTestDirs(t)
1237+
1238+
// Create a task that uses a parameter with expansion syntax
1239+
taskFile := filepath.Join(dirs.tasksDir, "test-expand.md")
1240+
taskContent := `Task with parameter: ${param1}
1241+
1242+
And a value that looks like expansion syntax but should not be expanded: ${"nested"}`
1243+
if err := os.WriteFile(taskFile, []byte(taskContent), 0644); err != nil {
1244+
t.Fatalf("failed to create task file: %v", err)
1245+
}
1246+
1247+
// Run with param1 set to a value that contains expansion syntax
1248+
output := runTool(t, "-C", dirs.tmpDir, "-p", "param1=!`echo hello`", "test-expand")
1249+
1250+
// The param1 should be replaced with the literal string "!`echo hello`"
1251+
// It should NOT be expanded again (that would execute the command)
1252+
if !strings.Contains(output, "!`echo hello`") {
1253+
t.Errorf("Expected param1 to be replaced with literal value, got: %s", output)
1254+
}
1255+
1256+
// Verify "hello" is not in output (which would indicate the command was executed)
1257+
// Note: there may be other "hello" strings, so check for the specific context
1258+
if strings.Contains(output, "Task with parameter: hello") {
1259+
t.Errorf("Parameter value was re-expanded (command was executed), got: %s", output)
1260+
}
1261+
}
1262+
1263+
// TestCommandExpansionOnce verifies that command files are expanded only once
1264+
func TestCommandExpansionOnce(t *testing.T) {
1265+
dirs := setupTestDirs(t)
1266+
commandsDir := filepath.Join(dirs.tmpDir, ".agents", "commands")
1267+
if err := os.MkdirAll(commandsDir, 0o755); err != nil {
1268+
t.Fatalf("failed to create commands dir: %v", err)
1269+
}
1270+
1271+
// Create a command file with a parameter
1272+
commandFile := filepath.Join(commandsDir, "test-cmd.md")
1273+
commandContent := `Command param: ${cmd_param}`
1274+
if err := os.WriteFile(commandFile, []byte(commandContent), 0644); err != nil {
1275+
t.Fatalf("failed to create command file: %v", err)
1276+
}
1277+
1278+
// Create a task that calls the command with a param containing expansion syntax
1279+
taskFile := filepath.Join(dirs.tasksDir, "test-cmd-task.md")
1280+
taskContent := `/test-cmd cmd_param="!` + "`echo injected`" + `"`
1281+
if err := os.WriteFile(taskFile, []byte(taskContent), 0644); err != nil {
1282+
t.Fatalf("failed to create task file: %v", err)
1283+
}
1284+
1285+
// Run the task
1286+
output := runTool(t, "-C", dirs.tmpDir, "test-cmd-task")
1287+
1288+
// The command parameter should be replaced with the literal string "!`echo injected`"
1289+
// It should NOT be expanded again (that would execute the command)
1290+
if !strings.Contains(output, "!`echo injected`") {
1291+
t.Errorf("Expected command param to be replaced with literal value, got: %s", output)
1292+
}
1293+
1294+
// Verify "injected" is not in output (which would indicate the command was executed)
1295+
if strings.Contains(output, "Command param: injected") {
1296+
t.Errorf("Command parameter value was re-expanded (command was executed), got: %s", output)
1297+
}
1298+
}

pkg/codingcontext/context.go

Lines changed: 12 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -193,21 +193,25 @@ func (cc *Context) findTask(taskName string) error {
193193
cc.agent = agent
194194
}
195195

196-
// Expand parameters only if expand_params is not explicitly set to false
197-
contentToProcess := md.Content
198-
if shouldExpandParams(frontMatter.ExpandParams) {
199-
contentToProcess = cc.expandParams(md.Content, nil)
200-
}
201-
202-
task, err := ParseTask(contentToProcess)
196+
// Parse the task content first to separate text blocks from slash commands
197+
task, err := ParseTask(md.Content)
203198
if err != nil {
204199
return err
205200
}
206201

202+
// Build the final content by processing each block
203+
// Text blocks are expanded if expand_params is not false
204+
// Slash command arguments are NOT expanded here - they are passed as literals
205+
// to command files where they may be substituted via ${param} templates
207206
finalContent := strings.Builder{}
208207
for _, block := range task {
209208
if block.Text != nil {
210-
finalContent.WriteString(block.Text.Content())
209+
textContent := block.Text.Content()
210+
// Expand parameters in text blocks only if expand_params is not explicitly set to false
211+
if shouldExpandParams(frontMatter.ExpandParams) {
212+
textContent = cc.expandParams(textContent, nil)
213+
}
214+
finalContent.WriteString(textContent)
211215
} else if block.SlashCommand != nil {
212216
commandContent, err := cc.findCommand(block.SlashCommand.Name, block.SlashCommand.Params())
213217
if err != nil {

0 commit comments

Comments
 (0)