Skip to content

Fix copilot engine shell argument escaping for special characters#1035

Merged
pelikhan merged 9 commits intomainfrom
copilot/fix-69617a65-4a68-440d-8f3a-c6c2b67411aa
Sep 26, 2025
Merged

Fix copilot engine shell argument escaping for special characters#1035
pelikhan merged 9 commits intomainfrom
copilot/fix-69617a65-4a68-440d-8f3a-c6c2b67411aa

Conversation

Copy link
Copy Markdown
Contributor

Copilot AI commented Sep 26, 2025

Fix Copilot Engine Shell Argument Escaping

Problem

The copilot engine was generating shell commands with unescaped special characters in --allow-tool arguments, causing bash syntax errors that prevented workflows from executing.

Plan

  • Create test to reproduce shell escaping issue
  • Implement shell escaping for copilot tool arguments
  • Update computeCopilotToolArguments to properly quote arguments with special characters
  • Ensure generated shell commands are properly escaped
  • Run tests to verify fix
  • Manual testing with make recompile
  • Move shell helper functions to separate shell.go file per review feedback
  • Review claude and codex engines for similar issues
  • Fix over-escaping of already-quoted arguments like "$INSTRUCTION"
  • Fix tests after linting and structural changes

Solution

  • Added shellJoinArgs() and shellEscapeArg() functions in dedicated shell.go file
  • Arguments containing special characters ()[]{}*?$"'|&;<> \t\n` are wrapped in single quotes
  • Single quotes within arguments are properly escaped using '\"'\"' technique
  • Fixed: Already-quoted arguments (like "$INSTRUCTION") are left unchanged to preserve variable expansion
  • Updated GetExecutionSteps() to use shellJoinArgs() instead of strings.Join()

Review of Other Engines

  • Claude engine: Has basic escaping for spaces and commas, sufficient for its use case
  • Codex engine: Uses string formatting instead of argument joining, no issues found
  • Only the copilot engine needed the robust shell escaping solution

Fix for "$INSTRUCTION" Issue

The shell escaping was incorrectly wrapping "$INSTRUCTION" in single quotes, preventing variable expansion:

  • Before: --prompt '"$INSTRUCTION"' (broken - no variable expansion)
  • After: --prompt "$INSTRUCTION" (correct - allows variable expansion)

Testing

  • Enhanced shell helper tests with cases for already-quoted arguments
  • Added specific copilot engine test for $INSTRUCTION handling
  • Fixed: Updated tests to expect 3 steps instead of 2 after engine structure changes
  • All existing tests continue to pass
  • Verified fix by recompiling dev.lock.yml

All tests fixed and passing

Original prompt

In the copilot engine, fix the syntax of the --allow-tool arguments to wrap identifiers with single quotes.

--allow-tool foo

See crash log at https://github.com/githubnext/gh-aw/actions/runs/18036259899/job/51323820311

Run set -o pipefail
  set -o pipefail
  
  INSTRUCTION=$(cat /tmp/aw-prompts/prompt.txt)
  
  # Run copilot CLI with log capture
  copilot --add-dir /tmp/ --log-level debug --log-dir /tmp/.copilot/logs/ --allow-tool safe_outputs --allow-tool shell(git add:*) --allow-tool shell(git branch:*) --allow-tool shell(git checkout:*) --allow-tool shell(git commit:*) --allow-tool shell(git merge:*) --allow-tool shell(git rm:*) --allow-tool shell(git switch:*) --allow-tool write --prompt "$INSTRUCTION" 2>&1 | tee /tmp/code-tidying-agent.log
  shell: /usr/bin/bash -e {0}
  env:
    GITHUB_AW_SAFE_OUTPUTS: /tmp/aw_output_541fdb352def1cf6.txt
    GITHUB_STEP_SUMMARY: 
    GITHUB_TOKEN: ***
    XDG_CONFIG_HOME: /tmp/.copilot/
    XDG_STATE_HOME: /tmp/.copilot/
/home/runner/work/_temp/cc577935-ef7d-476c-bd54-3aa77f43b0dc.sh: line 6: syntax error near unexpected token `('

💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.

Copilot AI and others added 2 commits September 26, 2025 11:35
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Comment thread pkg/workflow/copilot_engine.go Outdated
Copilot AI changed the title In the copilot engine, fix the syntax of the --allow-tool arguments to wrap identifiers with single quotes. --allow-tool foo See crash log at https://github.com/githubnext/gh-aw/actions/runs/18036259899/job/51323820311 ``` Run set -o pipefai... Fix copilot engine shell argument escaping for special characters Sep 26, 2025
Copilot AI requested a review from pelikhan September 26, 2025 11:40
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
@pelikhan
Copy link
Copy Markdown
Collaborator

pelikhan commented Sep 26, 2025

...

@pelikhan
Copy link
Copy Markdown
Collaborator

@copilot Review the generated lock file dev.lock.yml . The "$INSTRUCTIONS" argument should NOT be quoted. Fix this issue, add tests.

    copilot --add-dir /tmp/ --log-level debug --log-dir /tmp/.copilot/logs/ --allow-tool safe_outputs --prompt '"$INSTRUCTION"' 2>&1 | tee /tmp/dev.log

…CTION"

Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Copy link
Copy Markdown
Contributor Author

Copilot AI commented Sep 26, 2025

@copilot Review the generated lock file dev.lock.yml . The "$INSTRUCTIONS" argument should NOT be quoted. Fix this issue, add tests.

...

Fixed! The shell escaping now recognizes already-quoted arguments and preserves them.

Before: --prompt '"$INSTRUCTION"' (broken - no variable expansion)
After: --prompt "$INSTRUCTION" (correct - allows variable expansion)

Enhanced shellEscapeArg() to skip escaping for arguments already wrapped in quotes, and added comprehensive tests for this behavior. Commit: 88d6468

@pelikhan
Copy link
Copy Markdown
Collaborator

@copilot fix tests

pelikhan and others added 2 commits September 26, 2025 12:15
Copy link
Copy Markdown
Contributor Author

Copilot AI commented Sep 26, 2025

@copilot fix tests

Tests fixed! The copilot engine tests were failing because they expected 2 steps but the engine now generates 3 steps (upload config, execute copilot CLI, ensure log file). Updated tests to expect 3 steps and look at the correct step for the copilot command. All tests now pass. Commit: 70430b3

@pelikhan pelikhan marked this pull request as ready for review September 26, 2025 12:21
@pelikhan pelikhan merged commit 85a0f73 into main Sep 26, 2025
14 checks passed
@pelikhan pelikhan deleted the copilot/fix-69617a65-4a68-440d-8f3a-c6c2b67411aa branch September 26, 2025 12:21
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