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 2 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
5 changes: 5 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,11 @@ func rewriteStepRunSecretsToEnv(stepLines []string, stepIndent string) ([]string
if effectiveStepLineIndentLen(t, getIndentation(stepLines[j]), stepIndent) <= runKeyIndentLen {
break
}
// Skip bash comment lines – expressions inside comments are

@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] Comment says "bash comment lines" but # is also the comment character for PowerShell (pwsh/powershell), sh, and most other shells used in Actions. The wording overstates the scope.

💡 Suggested wording 
// Skip comment lines – expressions inside shell comments are
// documentation-only and must not generate env bindings.

This is a minor documentation accuracy issue; the logic itself is correct for all shell types.

// documentation-only and must not generate env bindings.
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
65 changes: 65 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,71 @@ steps:
assert.NotContains(t, result, "$env:EXPR_GITHUB_ACTOR", "bash step must not use $env:VARNAME")
})

t.Run("ignores expressions inside bash comment lines in run block", func(t *testing.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.

[/tdd] No test covers a PowerShell step (shell: pwsh) with a # comment line containing an expression. While the fix is shell-agnostic (it does not branch on shellIsPowerShell), an explicit test would lock in that guarantee.

💡 Suggested test skeleton 
t.Run("ignores expressions inside pwsh comment lines in run block", func(t *testing.T) {
    content := `---
on: push
steps:
  - name: PS step
    shell: pwsh
    run: |
      Write-Output "hello"
      # ${{ steps.parse.outputs.value }} is documented here
---
`
    // ... frontmatter ...
    result, applied, err := codemod.Apply(content, frontmatter)
    require.NoError(t, err)
    assert.False(t, applied)
    assert.NotContains(t, result, "EXPR_")
})

// Expressions inside bash 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 bash comments but still hoists real run expressions", func(t *testing.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.

[/tdd] The fix applies to > and >- (folded scalar) run blocks as well (runValue == ">" || runValue == ">-" at line 225 of the production file), but only | (literal scalar) is tested. A folded-scalar variant would ensure coverage stays complete if that branch is touched in the future.

💡 Suggested addition

Add a parallel test using run: > with a #-prefixed comment line to confirm the guard fires in the folded-scalar path too.

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")

@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.

Assertion tests an impossible condition: "steps.parse.outputs.*:" can never appear as a YAML key — mapRunExpressionToEnvBinding always produces an EXPR_-prefixed or hash-based name. This NotContains would pass even without the fix.

💡 Suggested replacement

Replace with an assertion that directly validates the invariant — that only one EXPR_ binding was generated (the real expression, not the wildcard from the comment):

assert.Equal(t, 1, strings.Count(result, "EXPR_"),
	"only the real expression should produce an EXPR_ binding")

Or more narrowly, assert the comment line was not rewritten:

assert.Contains(t, result, "# See also ${{ steps.parse.outputs.* }} for all outputs",
	"comment line must not be rewritten in place")

The assert.Contains(t, result, "${{ steps.parse.outputs.* }}", ...) assertion above does provide real coverage; this one just gives false confidence.

})

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