Fix zizmor output gap and eliminate SC2016 shellcheck violations in generated lock files#48094
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Fixes static-analysis blind spots by making zizmor findings visible without completion logs and suppressing false-positive SC2016 warnings.
Changes:
- Adds deterministic zizmor JSON fallback output and regression coverage.
- Adds SC2016 suppressions for generated AWF configuration and legitimate awk/GraphQL literals.
- Regenerates affected workflow locks and golden fixtures.
Show a summary per file
| File | Description |
|---|---|
pkg/cli/zizmor.go |
Adds fallback finding-file selection. |
pkg/cli/zizmor_test.go |
Tests missing completion logs. |
pkg/workflow/awf_helpers.go |
Generates SC2016 directives. |
.github/workflows/cli-consistency-checker.md |
Suppresses awk false positive. |
.github/workflows/daily-community-attribution.md |
Suppresses GraphQL false positive. |
.github/workflows/ai-moderator.lock.yml |
Regenerates suppression output. |
.github/workflows/chaos-pr-bundle-fuzzer.lock.yml |
Regenerates suppression output. |
.github/workflows/codex-github-remote-mcp-test.lock.yml |
Regenerates suppression output. |
.github/workflows/constraint-solving-potd.lock.yml |
Regenerates suppression output. |
.github/workflows/daily-assign-issue-to-user.lock.yml |
Regenerates suppression output. |
.github/workflows/daily-awf-spec-compiler-surfacing.lock.yml |
Regenerates suppression output. |
.github/workflows/daily-byok-ollama-test.lock.yml |
Regenerates suppression output. |
.github/workflows/daily-credit-limit-test.lock.yml |
Regenerates suppression output. |
.github/workflows/daily-formal-spec-verifier.lock.yml |
Regenerates suppression output. |
.github/workflows/daily-max-ai-credits-test.lock.yml |
Regenerates suppression output. |
.github/workflows/daily-model-resolution.lock.yml |
Regenerates suppression output. |
.github/workflows/daily-multi-device-docs-tester.lock.yml |
Regenerates suppression output. |
.github/workflows/daily-testify-uber-super-expert.lock.yml |
Regenerates suppression output. |
.github/workflows/example-permissions-warning.lock.yml |
Regenerates suppression output. |
.github/workflows/firewall.lock.yml |
Regenerates suppression output. |
.github/workflows/github-remote-mcp-auth-test.lock.yml |
Regenerates suppression output. |
.github/workflows/hippo-embed.lock.yml |
Regenerates suppression output. |
.github/workflows/release.lock.yml |
Regenerates suppression output. |
.github/workflows/smoke-ci.lock.yml |
Regenerates suppression output. |
.github/workflows/test-quality-sentinel.lock.yml |
Regenerates suppression output. |
pkg/workflow/testdata/wasm_golden/WasmBinary/basic-copilot.golden |
Updates WASM golden output. |
pkg/workflow/testdata/wasm_golden/WasmBinary/playwright-cli-mode.golden |
Updates WASM golden output. |
pkg/workflow/testdata/wasm_golden/WasmBinary/smoke-copilot.golden |
Updates WASM golden output. |
pkg/workflow/testdata/wasm_golden/WasmBinary/with-imports.golden |
Updates WASM golden output. |
pkg/workflow/testdata/TestWasmGolden_CompileFixtures/basic-copilot.golden |
Updates compiler fixture. |
pkg/workflow/testdata/TestWasmGolden_CompileFixtures/playwright-cli-mode.golden |
Updates compiler fixture. |
pkg/workflow/testdata/TestWasmGolden_CompileFixtures/smoke-copilot.golden |
Updates compiler fixture. |
pkg/workflow/testdata/TestWasmGolden_CompileFixtures/with-imports.golden |
Updates compiler fixture. |
pkg/workflow/testdata/TestWasmGolden_AllEngines/claude.golden |
Updates Claude golden output. |
pkg/workflow/testdata/TestWasmGolden_AllEngines/codex.golden |
Updates Codex golden output. |
pkg/workflow/testdata/TestWasmGolden_AllEngines/copilot.golden |
Updates Copilot golden output. |
pkg/workflow/testdata/TestWasmGolden_AllEngines/gemini.golden |
Updates Gemini golden output. |
pkg/workflow/testdata/TestWasmGolden_AllEngines/pi.golden |
Updates Pi golden output. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 76/280 changed files
- Comments generated: 2
- Review effort level: Medium
| displayFiles := completedFiles | ||
| if len(displayFiles) == 0 && len(fileFindings) > 0 { | ||
| for filePath := range fileFindings { | ||
| displayFiles = append(displayFiles, filePath) | ||
| } | ||
| sort.Strings(displayFiles) | ||
| } |
There was a problem hiding this comment.
Addressed in the latest commit. The logic now always starts with completedFiles for order preservation, then builds a sorted extraFiles slice for any paths in fileFindings that weren't in completedFiles, and appends them. A new test case (partial_stderr_markers: file absent from completed list still shows findings) covers the partial-parse scenario.
| // "$schema" (a JSON Schema key) as a literal dollar sign inside single-quoted | ||
| // shell arguments when no runtime variables are injected. No shell expansion | ||
| // is intended; shellcheck incorrectly warns that $schema won't expand. | ||
| "# shellcheck disable=SC2016\nprintf '%%s\\n' %s > %q", |
There was a problem hiding this comment.
Fixed in the latest commit. The # shellcheck disable=SC2016 directive is now only emitted when strings.HasPrefix(printfArg, "'") — i.e., only when the argument is single-quoted (no runtime variables injected). Double-quoted args from shellEscapeArgWithVarsPreserved already escape $schema as \$schema, so shellcheck doesn't warn there, and the suppression was unnecessary and could mask real quoting regressions.
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR does not have the 'implementation' label and has ≤100 new lines of code in business logic directories (73 additions detected). |
|
✅ Test Quality Sentinel completed test quality analysis. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: Fix zizmor output gap and SC2016 shellcheck violations
The two core changes are sound.
Zizmor fallback (pkg/cli/zizmor.go): The completedFiles → fileFindings keys fallback correctly guards against log-format changes. However, there is a pre-existing partial-completion edge case: if stderr parsing yields some but not all file markers, completedFiles is non-empty and the fallback is skipped, leaving findings for uncompleted files invisible. The condition len(displayFiles) == 0 should arguably be len(displayFiles) < len(fileFindings) to handle that case.
SC2016 suppression (pkg/workflow/awf_helpers.go): The disable comment is emitted unconditionally before printf, including for the double-quoted variant where $schema is escaped as \$schema and SC2016 would not trigger. The suppression is harmless but over-broad.
Workflow markdown changes (cli-consistency-checker.md, daily-community-attribution.md): Placement and rationale are correct.
No blocking issues — both concerns above are pre-existing or cosmetic.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 26.1 AIC · ⌖ 4.57 AIC · ⊞ 5K
🧪 Test Quality Sentinel Report✅ Test Quality Score: 90/100 — Excellent
📊 Metrics (1 test)
Verdict
|
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd — no blocking issues; two observations worth addressing.
📋 Key Themes & Highlights
Key Themes
- Partial
completedFilesgap: The fallback only triggers whencompletedFilesis entirely empty. If stderr parsing yields some but not all completed-file markers (e.g. a transient truncation mid-run), findings for the un-listed files are still silently dropped — the same blind spot as before, just narrower. - Unconditional SC2016 suppression:
awf_helpers.goprepends# shellcheck disable=SC2016for every AWF config write, including double-quoted forms where shellcheck would never fire. Harmless but broader than necessary. - Regression test added ✅ — the new
"fallback to sorted fileFindings"test case covers the primary fix path.
Positive Highlights
- ✅ Clean, well-commented fallback logic in
parseAndDisplayZizmorOutput - ✅ SC2016 suppressions in
.mdworkflow sources are precisely placed with good inline explanations - ✅ All 262 lock files recompiled consistently
- ✅ Root cause (fragile stderr parsing dependency) correctly identified
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 55.7 AIC · ⌖ 4.77 AIC · ⊞ 6.7K
Comment /matt to run again
| // is displayed even if the zizmor log format has changed (e.g., after | ||
| // a Docker image upgrade that drops or reformats the "completed" log line). | ||
| displayFiles := completedFiles | ||
| if len(displayFiles) == 0 && len(fileFindings) > 0 { |
There was a problem hiding this comment.
[/diagnosing-bugs] The fallback only fires when completedFiles is entirely empty — a partial parse (some markers received, some not) still silently drops findings for un-listed files.
💡 Suggested fix
Use fileFindings as the authoritative source and treat completedFiles as an ordering hint rather than a gate:
// Always display every file that has findings; use completedFiles for ordering when available.
displayFiles := make([]string, 0, len(fileFindings))
for fp := range fileFindings {
displayFiles = append(displayFiles, fp)
}
// Stable sort: files present in completedFiles come first (in their original order),
// then any remaining files sorted alphabetically.
completed := map[string]int{}
for i, fp := range completedFiles {
completed[fp] = i
}
sort.Slice(displayFiles, func(i, j int) bool {
oi, iOK := completed[displayFiles[i]]
oj, jOK := completed[displayFiles[j]]
if iOK && jOK { return oi < oj }
if iOK { return true }
if jOK { return false }
return displayFiles[i] < displayFiles[j]
})Also worth adding a test case where completedFiles has one file but fileFindings has two, to lock in the partial-parse behaviour.
@copilot please address this.
There was a problem hiding this comment.
Fixed in the latest commit. The logic now always starts from completedFiles for ordering, then collects any finding paths absent from that list into a sorted extraFiles slice and appends them — so partial stderr markers no longer drop findings for unlisted files. A new test case (partial_stderr_markers: file absent from completed list still shows findings) covers this scenario.
There was a problem hiding this comment.
Two blocking issues need to be fixed before merge.
### Findings
Blocking — pkg/cli/zizmor.go (the fallback is incomplete): The fallback to fileFindings keys only fires when completedFiles is completely empty (len == 0). If stderr parsing succeeds for even one file out of ten (partial log capture, Docker image that emits the marker inconsistently), completedFiles is non-empty, the fallback is skipped, and all unmatched findings are silently dropped — the same partial-output blind spot the PR claims to fix.
Blocking — pkg/workflow/awf_helpers.go (SC2016 suppression emitted unconditionally): # shellcheck disable=SC2016 is always prepended regardless of which quoting path is taken. When len(preservedVars) > 0, shellEscapeArgWithVarsPreserved produces a double-quoted argument ($schema → \$schema), so SC2016 cannot fire — yet the suppression is still emitted. This is misleading and will silently mask real SC2016 violations in future double-quoted configs. The directive should only be emitted in the else branch where shellEscapeArg (single-quoting) is used.
🔎 Code quality review by PR Code Quality Reviewer · sonnet46 · 76.2 AIC · ⌖ 4.59 AIC · ⊞ 5.7K
Comment /review to run again
|
@copilot please run the Unresolved review feedback:
Failed checks:
|
…low.md sync - pkg/cli/zizmor.go: append missing finding paths to completedFiles in sorted order instead of only falling back when completedFiles is empty; handles partial stderr markers (addresses reviewer feedback on PR #48094) - pkg/cli/zizmor_test.go: add test case for partial stderr markers scenario - pkg/workflow/awf_helpers.go: emit SC2016 suppression only when printfArg is single-quoted (no runtime vars injected); double-quoted args already escape bare $ signs so SC2016 does not fire there (addresses reviewer feedback) - pkg/cli/data/agentic_workflows_fallback_aw_files.json: add release-workflow.md to sync with .github/aw/ directory (fixes TestFallbackAWFilesMatchesLocalAWDirectory) - pkg/workflow/testdata/*/: regenerate golden files (remove SC2016 comment from double-quoted printf lines) - .github/workflows/*.lock.yml: recompile 262 workflows (SC2016 suppression only on single-quoted printf lines now) Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
🤖 PR Triage — §30192186368
Why: Fixes zizmor output blind spot (monitoring gap) and eliminates 35 SC2016 shellcheck violations across 262 recompiled lock files. CI status unknown. Large file count (29 files) warrants brief review. Next: Include in batch review with other static-analysis/chore items. Verify CI passes.
|
|
@copilot please run the Unresolved review feedback:
|
The unresolved feedback in discussion_r3651693853 was already addressed in commit
The three review threads are all |
Static analysis report surfaced two persistent issues: zizmor security scanner output had gone dark for 10+ consecutive runs (monitoring blind spot), and 35 SC2016 shellcheck violations in generated lock files were unaddressed.
Zizmor output gap (
pkg/cli/zizmor.go)parseAndDisplayZizmorOutput()only rendered findings for files present incompletedFiles, which was populated by parsing a specific log line from zizmor's stderr (INFO audit: zizmor: 🌈 completed). If the Docker image changed its log format,completedFilesstayed empty and nothing was displayed — even when valid JSON findings existed.Fix: fall back to iterating sorted
fileFindingskeys whencompletedFilesis empty:SC2016 violations (35 → 0)
All violations were
printf '%s\n' '{"$schema":...}'lines in generated.lock.ymlfiles — shellcheck incorrectly treats$schema(a JSON Schema vocabulary key) as an unexpanded shell variable reference inside single quotes.pkg/workflow/awf_helpers.go: prepend# shellcheck disable=SC2016to theconfigFileSetupprintf line in the compiler, propagating the suppression to all generated lock files..github/workflows/cli-consistency-checker.md: suppress SC2016 for an awk program literal ($1is an awk field variable, not a shell variable)..github/workflows/daily-community-attribution.md: suppress SC2016 for a GraphQL query literal ($owner,$name,$endCursorare GraphQL syntax).All 262 workflows recompiled; golden test files updated accordingly.
run: https://github.com/github/gh-aw/actions/runs/30187302401