Reduce actionlint SC2086 noise in generated AWF run scripts#40620
Conversation
… workflows Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
@copilot include extensive documentation on why these warnings are disabled in the compiler go code that introduces them. |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Updated in f7e3ad5. I added extensive inline documentation in |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Test Quality Sentinel completed test quality analysis. |
|
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #40620 does not have the 'implementation' label and has only 31 new lines of code in business logic directories (≤100 threshold). |
There was a problem hiding this comment.
Pull request overview
This PR reduces ShellCheck SC2086 warnings reported by actionlint in generated workflow run: blocks by adding a scoped ShellCheck suppression directive to the generated AWF invocation line, where intentional word-splitting/argument expansion is required for templated arguments.
Changes:
- Update AWF command generation to emit
# shellcheck disable=SC1003,SC2086immediately before the AWF invocation line in all command-assembly branches. - Extend
TestBuildAWFCommand_AddsToolCacheMountProbeto assert the new suppression marker is present. - Regenerate affected compiled workflow lock files and wasm golden outputs to match the new generator output.
Show a summary per file
| File | Description |
|---|---|
| pkg/workflow/awf_helpers.go | Emits a combined ShellCheck suppression directive (SC1003, SC2086) directly before the generated AWF invocation line. |
| pkg/workflow/awf_config_test.go | Adds an assertion ensuring the new ShellCheck suppression directive is included in generated AWF command output. |
| pkg/workflow/testdata/TestWasmGolden_CompileFixtures/with-imports.golden | Updates golden output to reflect the new combined ShellCheck suppression directive. |
| pkg/workflow/testdata/TestWasmGolden_CompileFixtures/smoke-copilot.golden | Updates golden output to reflect the new combined ShellCheck suppression directive. |
| pkg/workflow/testdata/TestWasmGolden_CompileFixtures/playwright-cli-mode.golden | Updates golden output to reflect the new combined ShellCheck suppression directive. |
| pkg/workflow/testdata/TestWasmGolden_CompileFixtures/basic-copilot.golden | Updates golden output to reflect the new combined ShellCheck suppression directive. |
| pkg/workflow/testdata/TestWasmGolden_AllEngines/pi.golden | Updates golden output to reflect the new combined ShellCheck suppression directive. |
| pkg/workflow/testdata/TestWasmGolden_AllEngines/gemini.golden | Updates golden output to reflect the new combined ShellCheck suppression directive. |
| pkg/workflow/testdata/TestWasmGolden_AllEngines/copilot.golden | Updates golden output to reflect the new combined ShellCheck suppression directive. |
| pkg/workflow/testdata/TestWasmGolden_AllEngines/codex.golden | Updates golden output to reflect the new combined ShellCheck suppression directive. |
| pkg/workflow/testdata/TestWasmGolden_AllEngines/claude.golden | Updates golden output to reflect the new combined ShellCheck suppression directive. |
| .github/workflows/workflow-normalizer.lock.yml | Regenerated lock output with the new combined ShellCheck suppression directive. |
| .github/workflows/video-analyzer.lock.yml | Regenerated lock output with the new combined ShellCheck suppression directive. |
| .github/workflows/update-astro.lock.yml | Regenerated lock output with the new combined ShellCheck suppression directive. |
| .github/workflows/unbloat-docs.lock.yml | Regenerated lock output with the new combined ShellCheck suppression directive. |
| .github/workflows/test-workflow.lock.yml | Regenerated lock output with the new combined ShellCheck suppression directive. |
| .github/workflows/test-project-url-default.lock.yml | Regenerated lock output with the new combined ShellCheck suppression directive. |
| .github/workflows/test-dispatcher.lock.yml | Regenerated lock output with the new combined ShellCheck suppression directive. |
| .github/workflows/spec-enforcer.lock.yml | Regenerated lock output with the new combined ShellCheck suppression directive. |
| .github/workflows/smoke-workflow-call.lock.yml | Regenerated lock output with the new combined ShellCheck suppression directive. |
| .github/workflows/smoke-workflow-call-with-inputs.lock.yml | Regenerated lock output with the new combined ShellCheck suppression directive. |
| .github/workflows/smoke-update-cross-repo-pr.lock.yml | Regenerated lock output with the new combined ShellCheck suppression directive. |
| .github/workflows/smoke-test-tools.lock.yml | Regenerated lock output with the new combined ShellCheck suppression directive. |
| .github/workflows/smoke-temporary-id.lock.yml | Regenerated lock output with the new combined ShellCheck suppression directive. |
| .github/workflows/smoke-service-ports.lock.yml | Regenerated lock output with the new combined ShellCheck suppression directive. |
| .github/workflows/smoke-project.lock.yml | Regenerated lock output with the new combined ShellCheck suppression directive. |
| .github/workflows/smoke-pi.lock.yml | Regenerated lock output with the new combined ShellCheck suppression directive. |
| .github/workflows/smoke-otel-backends.lock.yml | Regenerated lock output with the new combined ShellCheck suppression directive. |
| .github/workflows/smoke-opencode.lock.yml | Regenerated lock output with the new combined ShellCheck suppression directive. |
| .github/workflows/smoke-multi-pr.lock.yml | Regenerated lock output with the new combined ShellCheck suppression directive. |
| .github/workflows/smoke-gemini.lock.yml | Regenerated lock output with the new combined ShellCheck suppression directive. |
| .github/workflows/smoke-crush.lock.yml | Regenerated lock output with the new combined ShellCheck suppression directive. |
| .github/workflows/smoke-create-cross-repo-pr.lock.yml | Regenerated lock output with the new combined ShellCheck suppression directive. |
| .github/workflows/smoke-copilot.lock.yml | Regenerated lock output with the new combined ShellCheck suppression directive. |
| .github/workflows/smoke-copilot-sdk.lock.yml | Regenerated lock output with the new combined ShellCheck suppression directive. |
| .github/workflows/smoke-copilot-arm.lock.yml | Regenerated lock output with the new combined ShellCheck suppression directive. |
| .github/workflows/smoke-copilot-aoai-entra.lock.yml | Regenerated lock output with the new combined ShellCheck suppression directive. |
| .github/workflows/smoke-copilot-aoai-apikey.lock.yml | Regenerated lock output with the new combined ShellCheck suppression directive. |
| .github/workflows/smoke-codex.lock.yml | Regenerated lock output with the new combined ShellCheck suppression directive. |
| .github/workflows/smoke-ci.lock.yml | Regenerated lock output with the new combined ShellCheck suppression directive. |
| .github/workflows/smoke-call-workflow.lock.yml | Regenerated lock output with the new combined ShellCheck suppression directive. |
| .github/workflows/smoke-antigravity.lock.yml | Regenerated lock output with the new combined ShellCheck suppression directive. |
| .github/workflows/security-review.lock.yml | Regenerated lock output with the new combined ShellCheck suppression directive. |
| .github/workflows/schema-consistency-checker.lock.yml | Regenerated lock output with the new combined ShellCheck suppression directive. |
| .github/workflows/research.lock.yml | Regenerated lock output with the new combined ShellCheck suppression directive. |
| .github/workflows/repo-tree-map.lock.yml | Regenerated lock output with the new combined ShellCheck suppression directive. |
| .github/workflows/release.lock.yml | Regenerated lock output with the new combined ShellCheck suppression directive. |
| .github/workflows/poem-bot.lock.yml | Regenerated lock output with the new combined ShellCheck suppression directive. |
| .github/workflows/pdf-summary.lock.yml | Regenerated lock output with the new combined ShellCheck suppression directive. |
| .github/workflows/notion-issue-summary.lock.yml | Regenerated lock output with the new combined ShellCheck suppression directive. |
| .github/workflows/lint-monster.lock.yml | Regenerated lock output with the new combined ShellCheck suppression directive. |
| .github/workflows/issue-monster.lock.yml | Regenerated lock output with the new combined ShellCheck suppression directive. |
| .github/workflows/hippo-embed.lock.yml | Regenerated lock output with the new combined ShellCheck suppression directive. |
| .github/workflows/github-remote-mcp-auth-test.lock.yml | Regenerated lock output with the new combined ShellCheck suppression directive. |
| .github/workflows/firewall.lock.yml | Regenerated lock output with the new combined ShellCheck suppression directive. |
| .github/workflows/example-permissions-warning.lock.yml | Regenerated lock output with the new combined ShellCheck suppression directive. |
| .github/workflows/example-failure-category-filter.lock.yml | Regenerated lock output with the new combined ShellCheck suppression directive. |
| .github/workflows/docs-noob-tester.lock.yml | Regenerated lock output with the new combined ShellCheck suppression directive. |
| .github/workflows/daily-testify-uber-super-expert.lock.yml | Regenerated lock output with the new combined ShellCheck suppression directive. |
| .github/workflows/daily-team-status.lock.yml | Regenerated lock output with the new combined ShellCheck suppression directive. |
| .github/workflows/daily-news.lock.yml | Regenerated lock output with the new combined ShellCheck suppression directive. |
| .github/workflows/daily-multi-device-docs-tester.lock.yml | Regenerated lock output with the new combined ShellCheck suppression directive. |
| .github/workflows/daily-max-ai-credits-test.lock.yml | Regenerated lock output with the new combined ShellCheck suppression directive. |
| .github/workflows/daily-malicious-code-scan.lock.yml | Regenerated lock output with the new combined ShellCheck suppression directive. |
| .github/workflows/daily-hippo-learn.lock.yml | Regenerated lock output with the new combined ShellCheck suppression directive. |
| .github/workflows/daily-function-namer.lock.yml | Regenerated lock output with the new combined ShellCheck suppression directive. |
| .github/workflows/daily-doc-updater.lock.yml | Regenerated lock output with the new combined ShellCheck suppression directive. |
| .github/workflows/daily-credit-limit-test.lock.yml | Regenerated lock output with the new combined ShellCheck suppression directive. |
| .github/workflows/daily-cli-tools-tester.lock.yml | Regenerated lock output with the new combined ShellCheck suppression directive. |
| .github/workflows/daily-byok-ollama-test.lock.yml | Regenerated lock output with the new combined ShellCheck suppression directive. |
| .github/workflows/daily-awf-spec-compiler-surfacing.lock.yml | Regenerated lock output with the new combined ShellCheck suppression directive. |
| .github/workflows/daily-architecture-diagram.lock.yml | Regenerated lock output with the new combined ShellCheck suppression directive. |
| .github/workflows/daily-ambient-context-optimizer.lock.yml | Regenerated lock output with the new combined ShellCheck suppression directive. |
| .github/workflows/daily-agent-of-the-day-blog-writer.lock.yml | Regenerated lock output with the new combined ShellCheck suppression directive. |
| .github/workflows/commit-changes-analyzer.lock.yml | Regenerated lock output with the new combined ShellCheck suppression directive. |
| .github/workflows/codex-github-remote-mcp-test.lock.yml | Regenerated lock output with the new combined ShellCheck suppression directive. |
| .github/workflows/chaos-pr-bundle-fuzzer.lock.yml | Regenerated lock output with the new combined ShellCheck suppression directive. |
| .github/workflows/changeset.lock.yml | Regenerated lock output with the new combined ShellCheck suppression directive. |
| .github/workflows/brave.lock.yml | Regenerated lock output with the new combined ShellCheck suppression directive. |
| .github/workflows/bot-detection.lock.yml | Regenerated lock output with the new combined ShellCheck suppression directive. |
| .github/workflows/auto-triage-issues.lock.yml | Regenerated lock output with the new combined ShellCheck suppression directive. |
| .github/workflows/ai-moderator.lock.yml | Regenerated lock output with the new combined ShellCheck suppression directive. |
| .github/workflows/agentic-token-optimizer.lock.yml | Regenerated lock output with the new combined ShellCheck suppression directive. |
| .github/workflows/agent-persona-explorer.lock.yml | Regenerated lock output with the new combined ShellCheck suppression directive. |
| .github/workflows/ace-editor.lock.yml | Regenerated lock output with the new combined ShellCheck suppression directive. |
| .github/workflows/ab-testing-advisor.lock.yml | Regenerated lock output with the new combined ShellCheck suppression directive. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 227/260 changed files
- Comments generated: 0
🧪 Test Quality Sentinel Report
📊 Metrics & Test Classification (1 test analyzed)
Go: 1 ( Verdict
References:
|
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd and /zoom-out — approving with two minor suggestions (test breadth and constant scoping).
📋 Key Themes & Highlights
Key Themes
- Test branch coverage: The new shellcheck assertion verifies only the
configFileSetup-onlybranch. The remaining three branches all use the sameshellcheckDirectivevariable so they're safe today, but a future fifth branch would lack a guard. See the inline comment onawf_config_test.go:1378. - Constant scoping:
shellcheckDirectiveas a localvaris fine; a package-levelconstwould improve discoverability and allow tests to reference it symbolically. Seeawf_helpers.go:399.
Positive Highlights
- ✅ Generator-level fix is exactly the right approach — 258 lock files update mechanically with zero behavioral change
- ✅ Excellent rationale comment precisely documents the security reasoning: trusted compiler-owned fragments vs user-provided shell input
- ✅ DRY extraction of
shellcheckDirectivefrom four identical format-string literals eliminates split-brain risk on future directive updates - ✅ Regression guard added — the contract that the directive appears in generated output is now tested
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 93.1 AIC · ⌖ 9.66 AIC · ⊞ 6.9K
| @@ -1375,6 +1375,7 @@ func TestBuildAWFCommand_AddsToolCacheMountProbe(t *testing.T) { | |||
| assert.NotContains(t, command, `/home/runner/work/_tool`, "should not assume a self-hosted runner tool-cache path") | |||
| assert.NotContains(t, command, `:-/opt/hostedtoolcache`, "should not fall back to /opt/hostedtoolcache") | |||
| assert.Contains(t, command, `${GH_AW_TOOL_CACHE_MOUNT:+--mount "$GH_AW_TOOL_CACHE_MOUNT"}`, "should inject tool-cache mount args into awf invocation") | |||
| assert.Contains(t, command, "# shellcheck disable=SC1003,SC2086", "should suppress intentional argument splitting in awf invocation") | |||
There was a problem hiding this comment.
[/tdd] The assertion covers only one of the four BuildAWFCommand branches (the configFileSetup-only path). All four branches use the same shellcheckDirective variable today, so correctness is safe — but a future developer adding a fifth branch and forgetting the directive would slip past undetected.
💡 Suggested coverage extension
Consider adding the assertion to TestBuildAWFCommand_ConfigFileWithPathSetup (branch 1: PathSetup + configFileSetup) and to TestBuildAWFCommand_WritesAgentCLIStartTimestamp (already table-driven, covers two variants). Alternatively, a focused parameterized test:
func TestBuildAWFCommand_ContainsShellcheckDirective(t *testing.T) {
// exercise all four if/else branches in BuildAWFCommand
configs := []struct {
name string
cfg AWFCommandConfig
}{
{"PathSetup+configFileSetup", ...},
{"PathSetup only", ...},
{"configFileSetup only", ...},
{"neither", ...},
}
for _, tc := range configs {
t.Run(tc.name, func(t *testing.T) {
assert.Contains(t, BuildAWFCommand(tc.cfg), "# shellcheck disable=SC1003,SC2086")
})
}
}This makes the suppression contract explicit across every generation path.
There was a problem hiding this comment.
Addressed in 5b3217e. I added directive assertions in additional tests (TestBuildAWFCommand_ConfigFileWithPathSetup and TestBuildAWFCommand_WritesAgentCLIStartTimestamp) so this contract is now checked beyond the original single test path.
| // | ||
| // We keep normal quoting for all user-controlled values via shellEscapeArg/shellJoinArgs | ||
| // and scope this suppression to the generated AWF invocation line only. | ||
| shellcheckDirective := "# shellcheck disable=SC1003,SC2086" |
There was a problem hiding this comment.
[/zoom-out] Extracting shellcheckDirective from four identical string literals in the format templates into a single named variable is a clean DRY win — great call. One small follow-up: because this is currently a local var, tests must duplicate the string literal to assert against it (as in awf_config_test.go:1378). Promoting it to a package-level unexported constant would let tests reference it symbolically and would make this suppression easier to find when auditing all shellcheck directives in the package.
💡 Package-level const example
// awfShellcheckDirective suppresses SC1003 (GitHub expression literals with single
// quotes) and SC2086 (intentional unquoted expansion of compiler-owned AWF argument
// fragments). It is scoped to the generated AWF invocation line only.
const awfShellcheckDirective = "# shellcheck disable=SC1003,SC2086"Tests could then use the exported or unexported constant instead of the raw string, making the coupling explicit.
There was a problem hiding this comment.
Addressed in 5b3217e. The suppression marker is now a package-level constant (awfShellcheckDirective) in pkg/workflow/awf_helpers.go, and tests now reference that constant instead of duplicating the literal.
|
@copilot Run pr-finisher skill |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Daily static-analysis reported SC2086 as the dominant actionlint finding, concentrated in generated
.lock.ymlrun:blocks. The issue came from intentional shell argument expansion in the AWF invocation path, which shellcheck flags when not explicitly scoped.Scope of fix (generator-level)
pkg/workflow/awf_helpers.goto use scoped suppression where word-splitting is intentional:# shellcheck disable=SC1003→# shellcheck disable=SC1003,SC2086Coverage for behavior contract
TestBuildAWFCommand_AddsToolCacheMountProbeinpkg/workflow/awf_config_test.goto assert the scoped suppression marker is present in generated command content.Generated artifacts updated