Fix SC2086: Quote ${GITHUB_WORKSPACE} in generated workflow steps#3288
Fix SC2086: Quote ${GITHUB_WORKSPACE} in generated workflow steps#3288
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
Agentic Changeset Generator triggered by this pull request. |
|
Agentic Changeset Generator triggered by this pull request. |
There was a problem hiding this comment.
Pull Request Overview
This PR addresses ShellCheck warning SC2086 by adding quotes around ${GITHUB_WORKSPACE} variable references in generated shell commands. The changes update both the code generation logic and test expectations to ensure proper shell variable quoting throughout the workflow compilation process.
Key changes:
- Modified cleanup script path generation to use quoted
${GITHUB_WORKSPACE}references - Updated agent file path handling to quote the
${GITHUB_WORKSPACE}portion - Added single-quote wrapping around cleanup script commands to prevent word splitting
- Updated all test assertions to verify the new quoted format
Reviewed Changes
Copilot reviewed 23 out of 23 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
pkg/workflow/copilot_engine.go |
Updated agent path and cleanup script path generation to quote ${GITHUB_WORKSPACE} |
pkg/workflow/claude_engine.go |
Updated agent path generation to quote ${GITHUB_WORKSPACE} |
pkg/workflow/codex_engine.go |
Updated agent path generation to quote ${GITHUB_WORKSPACE} |
pkg/workflow/firewall_args_test.go |
Updated test assertions to expect quoted ${GITHUB_WORKSPACE} |
pkg/workflow/firewall_args_integration_test.go |
Updated integration test to expect quoted cleanup script paths |
pkg/workflow/engine_agent_import_test.go |
Updated test assertions to expect quoted agent file paths |
.github/workflows/*.lock.yml |
Regenerated workflow lock files with quoted variable references |
.changeset/patch-fix-sc2086-quote-variables.md |
Added changeset documenting the SC2086 fix |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if workflowData.AgentFile != "" { | ||
| // Agent file path is relative to repository root, so prefix with $GITHUB_WORKSPACE | ||
| agentPath := fmt.Sprintf("${GITHUB_WORKSPACE}/%s", workflowData.AgentFile) | ||
| agentPath := fmt.Sprintf("\"${GITHUB_WORKSPACE}\"/%s", workflowData.AgentFile) |
There was a problem hiding this comment.
The agent path quoting creates nested quotes that will be escaped incorrectly. The variable agentPath includes literal double quotes around ${GITHUB_WORKSPACE}, which will then be wrapped in single quotes by shellEscapeArg() (called via shellJoinArgs()), resulting in '\"${GITHUB_WORKSPACE}\"/.github/agents/file.md'. This prevents variable expansion because the $ is inside single quotes. Instead, construct the path without pre-quoting and let shellJoinArgs handle the escaping, or use a different quoting strategy that preserves variable expansion.
| agentPath := fmt.Sprintf("\"${GITHUB_WORKSPACE}\"/%s", workflowData.AgentFile) | |
| agentPath := fmt.Sprintf("${GITHUB_WORKSPACE}/%s", workflowData.AgentFile) |
| if workflowData.AgentFile != "" { | ||
| // Agent file path is relative to repository root, so prefix with $GITHUB_WORKSPACE | ||
| agentPath := fmt.Sprintf("${GITHUB_WORKSPACE}/%s", workflowData.AgentFile) | ||
| agentPath := fmt.Sprintf("\"${GITHUB_WORKSPACE}\"/%s", workflowData.AgentFile) |
There was a problem hiding this comment.
The agent path quoting creates nested quotes. When this path is used in the shell command at line 178, it's passed directly to awk without additional quoting. The literal double quotes around ${GITHUB_WORKSPACE} will cause shell parsing issues. The variable expansion will work here because it's not being passed through shellEscapeArg(), but the double quotes will be interpreted literally by awk, potentially causing the file path to be incorrect.
| agentPath := fmt.Sprintf("\"${GITHUB_WORKSPACE}\"/%s", workflowData.AgentFile) | |
| agentPath := fmt.Sprintf("${GITHUB_WORKSPACE}/%s", workflowData.AgentFile) |
| if workflowData.AgentFile != "" { | ||
| // Agent file path is relative to repository root, so prefix with $GITHUB_WORKSPACE | ||
| agentPath := fmt.Sprintf("${GITHUB_WORKSPACE}/%s", workflowData.AgentFile) | ||
| agentPath := fmt.Sprintf("\"${GITHUB_WORKSPACE}\"/%s", workflowData.AgentFile) |
There was a problem hiding this comment.
The agent path quoting creates nested quotes. When this path is used in the shell command at line 112, it's passed directly to awk without additional quoting. The literal double quotes around ${GITHUB_WORKSPACE} will cause shell parsing issues. The variable expansion will work here because it's not being passed through shellEscapeArg(), but the double quotes will be interpreted literally by awk, potentially causing the file path to be incorrect.
| agentPath := fmt.Sprintf("\"${GITHUB_WORKSPACE}\"/%s", workflowData.AgentFile) | |
| agentPath := fmt.Sprintf("${GITHUB_WORKSPACE}/%s", workflowData.AgentFile) |
| func generateAWFCleanupStep(scriptPath string) GitHubActionStep { | ||
| if scriptPath == "" { | ||
| scriptPath = "${GITHUB_WORKSPACE}/scripts/ci/cleanup.sh" | ||
| scriptPath = "\"${GITHUB_WORKSPACE}\"/scripts/ci/cleanup.sh" |
There was a problem hiding this comment.
The cleanup script path includes literal double quotes around ${GITHUB_WORKSPACE}, which will become part of the actual path string. Combined with the outer single quotes added at line 901 ('%s || true'), the resulting command will be '"${GITHUB_WORKSPACE}"/scripts/ci/cleanup.sh || true', where the double quotes are literal characters preventing proper variable expansion. The path should either not include the quotes (relying on the outer single quotes) or use a different quoting mechanism.
| scriptPath = "\"${GITHUB_WORKSPACE}\"/scripts/ci/cleanup.sh" | |
| scriptPath = "${GITHUB_WORKSPACE}/scripts/ci/cleanup.sh" |
| func generateAWFPostExecutionCleanupStep(scriptPath string) GitHubActionStep { | ||
| if scriptPath == "" { | ||
| scriptPath = "${GITHUB_WORKSPACE}/scripts/ci/cleanup.sh" | ||
| scriptPath = "\"${GITHUB_WORKSPACE}\"/scripts/ci/cleanup.sh" |
There was a problem hiding this comment.
The cleanup script path includes literal double quotes around ${GITHUB_WORKSPACE}, which will become part of the actual path string. Combined with the outer single quotes added at line 981 ('%s || true'), the resulting command will be '"${GITHUB_WORKSPACE}"/scripts/ci/cleanup.sh || true', where the double quotes are literal characters preventing proper variable expansion. The path should either not include the quotes (relying on the outer single quotes) or use a different quoting mechanism.
| scriptPath = "\"${GITHUB_WORKSPACE}\"/scripts/ci/cleanup.sh" | |
| scriptPath = "${GITHUB_WORKSPACE}/scripts/ci/cleanup.sh" |
The workflow compiler was generating shell commands with unquoted
${GITHUB_WORKSPACE}variables, triggering 108 SC2086 shellcheck warnings across 19 workflows.Changes
Compiler fixes (5 files):
pkg/workflow/copilot_engine.go- Quote cleanup script paths and agent file pathspkg/workflow/claude_engine.go- Quote agent file paths in awk commandspkg/workflow/codex_engine.go- Quote agent file paths in awk commandsTest updates (3 files):
Example
Before (SC2086 warning):
After (compliant):
All affected workflows recompiled. Zero GITHUB_WORKSPACE SC2086 errors remain.
Original prompt
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.