Skip to content

Ignore ${{ ... }} in bash # comments during run-script expression harvesting#35777

Merged
pelikhan merged 3 commits into
mainfrom
copilot/issue-1234-fix-expression-harvesting
May 29, 2026
Merged

Ignore ${{ ... }} in bash # comments during run-script expression harvesting#35777
pelikhan merged 3 commits into
mainfrom
copilot/issue-1234-fix-expression-harvesting

Conversation

Copy link
Copy Markdown
Contributor

Copilot AI commented May 29, 2026

gh aw compile regressed in v0.76+ by harvesting ${{ ... }} tokens from bash comments inside run: blocks, hoisting them into step env, and generating invalid lock workflows when those expressions resolve to non-scalars (e.g. secrets.*). This PR limits harvesting/guardrail scans to executable script content.

  • Scope of fix: executable-only expression scanning

    • Added shell line-comment stripping (# ...) before run-script expression extraction.
    • Kept heredoc exclusion intact; scanning now ignores both heredoc bodies and bash comments.
  • Compiler paths updated

    • sanitizeRunStepExpressions now scans stripShellLineComments(removeHeredocContent(run)).
    • Template-injection/regression guardrail run-script scans apply the same preprocessing in parsed workflow validation paths.
  • Behavioral coverage added

    • New sanitizer cases:
      • comment-only expression is not extracted
      • mixed comment + executable expression extracts only executable expression
    • New helper tests for comment stripping behavior (quoted #, escaped \#, trailing comments).
    • Both validation paths now include a comment-expression case that should pass.
// Before scanning run: content for ${{ ... }}
scanContent := stripShellLineComments(removeHeredocContent(runVal))
matches := ExpressionPattern.FindAllStringSubmatch(scanContent, -1)

Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Copilot AI changed the title [WIP] Fix regression in expression-harvesting pass for bash comments Ignore ${{ ... }} in bash # comments during run-script expression harvesting May 29, 2026
Copilot AI requested a review from pelikhan May 29, 2026 20:21
@pelikhan pelikhan marked this pull request as ready for review May 29, 2026 20:57
Copilot AI review requested due to automatic review settings May 29, 2026 20:57
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR narrows GitHub expression harvesting and validation for workflow run: scripts so ${{ ... }} inside bash # comments is treated like non-executable content, similar to existing heredoc exclusions.

Changes:

  • Adds stripShellLineComments to remove bash-style comments while preserving quoted and escaped #.
  • Applies heredoc + comment stripping before sanitizer and validation scans.
  • Adds unit and compiler-path regression tests for comment-only expressions.
Show a summary per file
File Description
pkg/workflow/template_injection_validation.go Excludes shell comments from parsed workflow expression validation scans.
pkg/workflow/template_injection_validation_test.go Adds unit coverage for shell comment stripping behavior.
pkg/workflow/template_injection_utils.go Adds the comment-stripping helper and comment-start detection.
pkg/workflow/run_step_sanitizer.go Updates run-step expression scanning to ignore comments.
pkg/workflow/run_step_sanitizer_test.go Adds sanitizer cases for comment-only and mixed comment/executable expressions.
pkg/workflow/compiler_template_injection_both_paths_test.go Adds both-path validation coverage for expressions inside bash comments.

Copilot's findings

Tip

Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

  • Files reviewed: 6/6 changed files
  • Comments generated: 1

// Only scan executable script content:
// - strip heredoc bodies (written to files/stdin, not executed)
// - strip bash line comments (not executed)
scanContent := stripShellLineComments(removeHeredocContent(runVal))
@pelikhan
Copy link
Copy Markdown
Collaborator

@copilot review all comments and reviews from copilot and github-actions actor. Address and resolve.

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

Copilot AI commented May 29, 2026

@copilot review all comments and reviews from copilot and github-actions actor. Address and resolve.

Addressed the actionable Copilot review feedback in commit be2cf6c: replacement now skips bash # comments (while still replacing executable script occurrences), and regression coverage was added for the same expression appearing in both comment and executable lines. Targeted workflow sanitizer tests pass.

@github-actions
Copy link
Copy Markdown
Contributor

``
@copilot review all comments and address unresolved review feedback, then rerun the targeted sanitizer tests.

Generated by 👨‍🍳 PR Sous Chef · gpt54 15.5M ·

@pelikhan
Copy link
Copy Markdown
Collaborator

@pelikhan pelikhan merged commit 6b2cc89 into main May 29, 2026
25 of 27 checks passed
@pelikhan pelikhan deleted the copilot/issue-1234-fix-expression-harvesting branch May 29, 2026 22:09
Copilot stopped work on behalf of pelikhan due to an error May 29, 2026 22:09
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.

Expression-harvesting pass hoists ${{ ... }} tokens from inside bash # comments into step env, breaking workflow load (regression since v0.76)

3 participants