fix(codemod): skip shell comment lines when scanning run blocks for expression hoisting#38682
Conversation
Expressions inside bash comment lines (lines whose first non-whitespace character is `#`) inside `run: |` blocks are documentation-only and must not trigger env-binding extraction. Add a skip check in `rewriteStepRunSecretsToEnv` and two regression tests. Closes #38681 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Fixes the steps-run-secrets-to-env codemod so it won’t treat ${{ ... }} inside full-line # comments within multi-line run: | blocks as real expressions, preventing spurious EXPR_<hash> env bindings and unintended rewrites of comment text. This is aimed at avoiding invalid workflow output when commented expressions include glob/wildcard syntax.
Changes:
- Skip comment-only script lines (trimmed lines starting with
#) when scanningrun: |block bodies for expression hoisting. - Add regression tests covering (1) comment-only expressions (no-op) and (2) mixed real+comment expressions (hoist real, preserve comment).
Show a summary per file
| File | Description |
|---|---|
pkg/cli/codemod_steps_run_secrets_env.go |
Skips # comment lines when scanning run block bodies before attempting expression replacement/hoisting. |
pkg/cli/codemod_steps_run_secrets_env_test.go |
Adds regression tests to ensure comment-only expressions are ignored and mixed cases still hoist real expressions. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 2/2 changed files
- Comments generated: 2
| // Skip bash comment lines – expressions inside comments are | ||
| // documentation-only and must not generate env bindings. | ||
| if strings.HasPrefix(t, "#") { |
| 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") |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #38682 does not have the implementation label (has_implementation_label=false) and has only 70 new lines in business logic directories (≤100 threshold, requires_adr_by_default_volume=false). |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
🧪 Test Quality Sentinel completed test quality analysis. |
|
@copilot run pr-finisher skill |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnose and /tdd — no blocking issues; a few suggestions for completeness.
📋 Key Themes & Highlights
Key Themes
- Code comment accuracy: The inline comment says "bash comment lines" but
#is the comment character for all shells targeted here (bash, sh, pwsh, powershell). A quick wording update avoids future confusion. - Known limitation – inline trailing comments:
cmd # ${{ expr }}lines are not yet excluded. The bug report is scoped to full-line comments, so this is out of scope, but worth aTODOor follow-up issue to prevent silent regressions later. - Test coverage gaps: Two narrow scenarios lack explicit tests — PowerShell
#comment lines, and>folded-scalar run blocks. The fix is correct in both cases; the tests would just lock in the guarantee.
Positive Highlights
- ✅ Minimal, surgical fix — one
continueguard in exactly the right place - ✅ Two regression tests added, covering both the no-op case and the mixed (real + commented) expression case
- ✅ Test names read as behavioural specs; Arrange/Act/Assert structure is clear throughout
- ✅ Uses the already-trimmed
tvariable for the prefix check, so both flush (# comment) and indented (# comment) forms are handled without extra allocation
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 216.7 AIC · ⌖ 13.8 AIC
| if effectiveStepLineIndentLen(t, getIndentation(stepLines[j]), stepIndent) <= runKeyIndentLen { | ||
| break | ||
| } | ||
| // Skip bash comment lines – expressions inside comments are |
There was a problem hiding this comment.
[/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.
| } | ||
| // Skip bash comment lines – expressions inside comments are | ||
| // documentation-only and must not generate env bindings. | ||
| if strings.HasPrefix(t, "#") { |
There was a problem hiding this comment.
[/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.| 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) { |
There was a problem hiding this comment.
[/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_")
})| 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) { |
There was a problem hiding this comment.
[/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.
🧪 Test Quality Sentinel Report✅ Test Quality Score: 90/100 — Excellent
📊 Metrics & Test Classification (2 tests analyzed)
Test Classification Details
Language SupportTests analyzed:
i️ Test Inflation Note (13:1 ratio)The test file grew at a 13:1 ratio versus the production fix (65 lines added in test vs 5 lines in production). This is above the 2:1 threshold and costs 10 points in the score. However, this is expected and healthy for this PR: each subtest requires a substantial YAML fixture block, frontmatter map literal, and multiple descriptive assertions — all of which are necessary setup for a thorough behavioral test. The inflation does not indicate low-quality tests; it reflects the fixture-heavy nature of YAML-processing codemods. Verdict
📖 Understanding Test ClassificationsDesign Tests (High Value) verify what the system does:
Implementation Tests (Low Value) verify how the system does it:
Goal: Shift toward tests that describe the system's behavioral contract — the promises it makes to its users and collaborators. References: §27366532151
|
There was a problem hiding this comment.
The core fix is correct: full-line # comments in run: | blocks should not trigger env-binding extraction. Two medium issues and one low issue worth addressing.
### Findings summary
Medium — heredoc lines starting with # silently skip expression hoisting (codemod_steps_run_secrets_env.go line 236): The strings.HasPrefix(t, "#") guard has no heredoc-boundary awareness. A run: | block containing cat <<EOF\n# ${{ env.CONFIG }}\nEOF will now leave that expression un-hoisted. Before this fix it was accidentally hoisted (correct for strict-mode); after, it is silently dropped. Affects only the unusual pattern of heredocs with #-prefixed content lines that contain expressions.
Medium — no test coverage for folded-block-scalar run blocks (codemod_steps_run_secrets_env_test.go line 604): Both > and >- are handled by the same branch as |/|-, but neither style is exercised by the new tests.
Low — NotContains assertion tests an impossible condition (codemod_steps_run_secrets_env_test.go line 634): "steps.parse.outputs.*:" can never appear as a raw YAML key; env bindings are always EXPR_-prefixed or hash-based. This assertion would pass regardless of the fix.
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
proxy.golang.org
To allow these domains, add them to the
network.allowedlist in your workflow frontmatter:
network:
allowed:
- defaults
- "proxy.golang.org"See Network Configuration for more information.
🔎 Code quality review by PR Code Quality Reviewer · 14.7 AIC · ⌖ 13.5 AIC
| } | ||
| // Skip bash comment lines – expressions inside comments are | ||
| // documentation-only and must not generate env bindings. | ||
| if strings.HasPrefix(t, "#") { |
There was a problem hiding this comment.
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 }}
EOFAfter 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.
| 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") |
There was a problem hiding this comment.
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.
| 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") |
There was a problem hiding this comment.
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")
})…assertions, pwsh & folded-scalar tests Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Done in 209c091. Addressed all in-scope review feedback:
|
pelikhan
deleted the
copilot/move-step-run-expressions-to-env-bindings-fix
branch
The
steps-run-secrets-to-envcodemod incorrectly processes${{ ... }}expressions inside shell comment lines withinrun: |blocks, generating spuriousEXPR_<hash>env bindings and rewriting comment text. This produces invalid workflows when the commented expression contains glob syntax (e.g.steps.foo.outputs.*). The fix applies to all shells that use#for comments (bash, sh, pwsh, powershell, etc.) and to all block-scalar styles (|,|-,>,>-).Changes
codemod_steps_run_secrets_env.go: In the multi-linerun:block scanning loop insiderewriteStepRunSecretsToEnv, skip any line whose trimmed content starts with#before callingreplaceStepExpressionRefs. This is shell-agnostic and handles both flush and indented comment forms. A note is included documenting the known heredoc limitation (lines starting with#inside a heredoc body are also skipped).codemod_steps_run_secrets_env_test.go: Four regression tests:shell: pwsh) step with a#comment containing an expression → no env binding generated.run: >) block with a#comment line → real expression is hoisted, comment expression is not.Before:
After: