Skip to content

fix(codemod): skip shell comment lines when scanning run blocks for expression hoisting #38682

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
pelikhan merged 4 commits into from
Jun 11, 2026
+132 −0
Merged

fix(codemod): skip shell comment lines when scanning run blocks for expression hoisting #38682

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
0a76345
Initial plan
Copilot Jun 11, 2026
a179f04
fix: skip bash comment lines in run block expression scanning
Copilot Jun 11, 2026
209c091
fix: address review feedback – shell-agnostic comment, stronger test …
Copilot Jun 11, 2026
fdcf4e5
Merge branch 'main' into copilot/move-step-run-expressions-to-env-bin…
pelikhan Jun 11, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions pkg/cli/codemod_steps_run_secrets_env.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -231,6 +231,13 @@ func rewriteStepRunSecretsToEnv(stepLines []string, stepIndent string) ([]string
if effectiveStepLineIndentLen(t, getIndentation(stepLines[j]), stepIndent) <= runKeyIndentLen {
break
}
// Skip shell comment lines – expressions inside # comments are
// documentation-only and must not generate env bindings.
// NOTE: heredoc boundaries are not tracked; lines starting with
// '#' inside a heredoc body are also skipped (follow-up needed).
if strings.HasPrefix(t, "#") {

@github-actions github-actions Bot Jun 11, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/diagnose] This guard handles full-line comments (# ...) but not trailing inline comments — e.g. echo "hello" # ${{ steps.foo.outputs.val }}. The expression on that line would still be hoisted and rewritten, which may produce unexpected env bindings or break glob-containing expressions.

💡 Context

This is likely out of scope for this fix (the PR title targets "bash comment lines"), but documenting the known limitation prevents future confusion. Consider adding a TODO comment or a follow-up issue so the inline-comment case is tracked.

// NOTE: trailing inline comments (e.g. `cmd # ${{ expr }}`) are not yet skipped;
// they continue to generate env bindings. Tracked as a separate follow-up.

@github-actions github-actions Bot Jun 11, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Heredoc lines starting with # will silently lose expression hoisting: if a run: | block contains a heredoc whose lines begin with # (e.g., cat <<EOF\n# ${{ env.CONFIG }}\nEOF), those expressions are now silently skipped. Before this fix they were accidentally hoisted — which was the right outcome for strict-mode compliance.

💡 Details

The scanner has no heredoc-boundary tracking — it does not detect <<EOF / <<'EOF' opener/closer pairs. Any block-scalar line whose trimmed content begins with # is treated as a comment, even inside a heredoc body.

GitHub Actions evaluates ${{ expr }} before handing the script to the shell, so ${{ env.CONFIG }} inside a heredoc IS live and should be hoisted:

run: |
  cat <<EOF
  # config: ${{ env.CONFIG }}
  EOF

After this change the codemod silently passes over that expression, leaving the run block non-strict.

Minimal mitigation: add an explicit comment above the guard stating the heredoc limitation, or track heredoc open/close state before applying the skip.

continue
}
updatedLine, bindings := replaceStepExpressionRefs(stepLines[j], shellIsPowerShell, bindingExprs)
if len(bindings) > 0 {
stepLines[j] = updatedLine
Expand Down
125 changes: 125 additions & 0 deletions pkg/cli/codemod_steps_run_secrets_env_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -569,6 +569,131 @@ steps:
assert.NotContains(t, result, "$env:EXPR_GITHUB_ACTOR", "bash step must not use $env:VARNAME")
})

t.Run("ignores expressions inside shell comment lines in run block", func(t *testing.T) {
// Expressions inside shell comments are documentation-only and must not
// generate env bindings or be rewritten.
content := `---
on: workflow_dispatch
steps:
- name: Use parsed value
env:
VALUE: ${{ steps.parse.outputs.value }}
run: |
echo "Got: $VALUE"
# Note: the prompt placeholders ${{ steps.parse.outputs.* }} resolve to
# empty strings because they're evaluated in a different context.
---
`
frontmatter := map[string]any{
"on": "workflow_dispatch",
"steps": []any{
map[string]any{
"name": "Use parsed value",
"env": map[string]any{
"VALUE": "${{ steps.parse.outputs.value }}",
},
"run": "echo \"Got: $VALUE\"\n# Note: the prompt placeholders ${{ steps.parse.outputs.* }} resolve to\n# empty strings because they're evaluated in a different context.",
},
},
}

result, applied, err := codemod.Apply(content, frontmatter)
require.NoError(t, err, "codemod should apply cleanly")
assert.False(t, applied, "codemod should not modify content when only expression is in a bash comment")
assert.Equal(t, content, result, "content should be unchanged")
assert.NotContains(t, result, "EXPR_", "no EXPR_ bindings should be generated for comment-only expressions")

@github-actions github-actions Bot Jun 11, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing test coverage for folded-block-scalar (>, >-) run blocks: the implementation branch at line 225 covers |, |-, >, and >-, but every new test uses only run: |. The comment-skip logic is identical for folded scalars in the code, but there is no regression guard for that path.

💡 Suggested addition

Add a minimal test variant:

t.Run("ignores comment lines in folded run block", func(t *testing.T) {
	content := `---
on: workflow_dispatch
steps:
  - name: S
    run: >
      echo "${{ github.actor }}"
      # doc ${{ steps.x.outputs.y }}
---
`
	// frontmatter omitted for brevity
	result, applied, err := codemod.Apply(content, frontmatter)
	require.NoError(t, err)
	assert.True(t, applied, "real expression should be hoisted")
	assert.Contains(t, result, "EXPR_GITHUB_ACTOR")
	assert.Contains(t, result, "${{ steps.x.outputs.y }}", "comment expression must be preserved verbatim")
})

assert.Contains(t, result, "${{ steps.parse.outputs.* }}", "original comment text should be preserved verbatim")
})

t.Run("ignores expressions in shell comments but still hoists real run expressions", func(t *testing.T) {
content := `---
on: workflow_dispatch
steps:
- name: Use parsed value
run: |
echo "${{ steps.parse.outputs.value }}"
# See also ${{ steps.parse.outputs.* }} for all outputs
---
`
frontmatter := map[string]any{
"on": "workflow_dispatch",
"steps": []any{
map[string]any{
"name": "Use parsed value",
"run": "echo \"${{ steps.parse.outputs.value }}\"\n# See also ${{ steps.parse.outputs.* }} for all outputs",
},
},
}

result, applied, err := codemod.Apply(content, frontmatter)
require.NoError(t, err, "codemod should apply cleanly")
assert.True(t, applied, "codemod should apply for real run expression")
assert.Contains(t, result, "EXPR_STEPS_PARSE_OUTPUTS_VALUE: ${{ steps.parse.outputs.value }}", "real expression should be hoisted")
assert.Contains(t, result, `echo "$EXPR_STEPS_PARSE_OUTPUTS_VALUE"`, "real expression reference should be rewritten")
assert.Contains(t, result, "${{ steps.parse.outputs.* }}", "comment expression should be preserved verbatim")
assert.NotContains(t, result, ": ${{ steps.parse.outputs.* }}", "wildcard comment expression must not generate an env binding")
})

t.Run("ignores expressions inside pwsh comment lines in run block", func(t *testing.T) {
// Expressions inside # comments are documentation-only for all shells,
// including PowerShell.
content := `---
on: workflow_dispatch
steps:
- name: PS step
shell: pwsh
run: |
Write-Output "hello"
# ${{ steps.parse.outputs.value }} is documented here
---
`
frontmatter := map[string]any{
"on": "workflow_dispatch",
"steps": []any{
map[string]any{
"name": "PS step",
"shell": "pwsh",
"run": "Write-Output \"hello\"\n# ${{ steps.parse.outputs.value }} is documented here",
},
},
}

result, applied, err := codemod.Apply(content, frontmatter)
require.NoError(t, err, "codemod should apply cleanly")
assert.False(t, applied, "codemod should not modify content when only expression is in a comment")
assert.Equal(t, content, result, "content should be unchanged")
assert.NotContains(t, result, "EXPR_", "no EXPR_ bindings should be generated for comment-only expressions")
})

t.Run("ignores expressions in shell comments in folded run block", func(t *testing.T) {
// The comment-skip logic applies to folded-scalar (>) run blocks as well.
content := `---
on: workflow_dispatch
steps:
- name: Use value
run: >
echo "${{ steps.parse.outputs.value }}"
# doc ${{ steps.parse.outputs.* }}
---
`
frontmatter := map[string]any{
"on": "workflow_dispatch",
"steps": []any{
map[string]any{
"name": "Use value",
"run": "echo \"${{ steps.parse.outputs.value }}\"\n# doc ${{ steps.parse.outputs.* }}",
},
},
}

result, applied, err := codemod.Apply(content, frontmatter)
require.NoError(t, err, "codemod should apply cleanly")
assert.True(t, applied, "real expression should be hoisted")
assert.Contains(t, result, "EXPR_STEPS_PARSE_OUTPUTS_VALUE: ${{ steps.parse.outputs.value }}", "real expression should be hoisted to env")
assert.NotContains(t, result, ": ${{ steps.parse.outputs.* }}", "comment expression must not generate an env binding")
assert.Contains(t, result, "${{ steps.parse.outputs.* }}", "comment expression should be preserved verbatim")
})

t.Run("uses distinct bindings when different bodies collide to the same EXPR_ name", func(t *testing.T) {
// inputs.my-input and inputs.my_input both sanitize to EXPR_INPUTS_MY_INPUT.
// The second one must fall back to a hash-based name to avoid being silently
Expand Down
Loading