Fix static-analysis scan pipeline short-circuiting and add scanner output completeness assertion#58717
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
@copilot resolve the merge conflicts on this branch. |
There was a problem hiding this comment.
🟡 Changes recommended
Moderate issues leave scanner completeness detection, empty-input logging, and regression coverage unreliable.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Prevents scanner failures from short-circuiting later static-analysis tools and adds output-completeness monitoring.
Changes:
- Runs batch scanners sequentially while preserving errors.
- Adds execution logging for empty scan inputs.
- Adds workflow completeness checks and regression coverage.
File summaries
| File | Description | Review notes |
|---|---|---|
pkg/cli/syft.go |
Logs zero-image scans. | No unresolved comments. |
pkg/cli/shellcheck.go |
Refactors helpers and logs zero-step scans. | Nit (2 votes): A new comment names the wrong function and contains an empty priority list. |
pkg/cli/grype.go |
Logs zero-image scans. | No unresolved comments. |
pkg/cli/compile_pipeline.go |
Adds non-short-circuiting scanner orchestration. | Moderate (1 vote): Actionlint still emits no marker with zero lock files. Nit (1 vote): A helper comment names the wrong function. |
pkg/cli/compile_external_tools_test.go |
Adds orchestration regression coverage. | Moderate (2 votes): Empty inputs do not test continued execution after an early scanner error. |
pkg/cli/actionlint.go |
Adds actionlint execution logging. | No unresolved comments. |
.github/workflows/static-analysis-report.md |
Adds scanner-output completeness assertions. | Moderate (2 votes): The shellcheck substring can be satisfied by actionlint output; use scanner-specific markers and regenerate the lock file. |
.github/workflows/static-analysis-report.lock.yml |
Regenerates the compiled workflow. | Must be regenerated after correcting the completeness assertion. |
Review details
Suppressed comments (1)
pkg/cli/compile_pipeline.go:499
- This comment names
runBatchExternalTools, but it is attached torunBatchLinters; the actual orchestration function is declared later at line 646. Document the helper that follows here so generated documentation and code navigation are accurate.
// runBatchExternalTools executes all enabled batch analysis tools sequentially without short-circuiting
// when individual tools report findings or errors.
- Files reviewed: 8/8 changed files
- Comments generated: 4
- Review effort level: Balanced
|
|
||
| MISSING_TOOLS=0 | ||
| for tool in zizmor poutine actionlint runner-guard syft grype yamllint shellcheck; do | ||
| if ! grep -qi "$tool" "$COMPILE_LOG"; then |
There was a problem hiding this comment.
Fixed in 32f97f7: replaced the bare grep -qi "$tool" check with a per-scanner marker map (e.g. Running actionlint (, Running shellcheck on) so actionlint's "with shellcheck/pyflakes" message can no longer satisfy the dedicated shellcheck check. Verified locally that a log containing actionlint's message but missing the shellcheck marker now fails the assertion.
There was a problem hiding this comment.
Correction: the commit hash is ffd49a9, not 32f97f7.
| stats := &CompilationStats{} | ||
| var validationResults []ValidationResult | ||
|
|
||
| strictGrantErr, batchToolErr := runBatchExternalTools(ctx, config, opts, stats, &validationResults) |
There was a problem hiding this comment.
Fixed in 32f97f7: added package-level function-variable seams for all 9 batch scanner entry points and rewrote the test to make actionlint return an error, then assert all scanners still run in pipeline order and the first error is preserved. Confirmed this test fails if the pipeline is reverted to short-circuit on the first error.
There was a problem hiding this comment.
Correction: the commit hash is ffd49a9, not 32f97f7.
| func runBatchLinters(ctx context.Context, config CompileConfig, opts batchToolsOptions) error { | ||
| var firstErr error | ||
|
|
||
| if config.Actionlint && !config.NoEmit && len(opts.lockFilesForActionlint) > 0 { |
There was a problem hiding this comment.
Fixed in 32f97f7: removed the len(lockFiles) > 0 guards for all lock-file batch tools (actionlint, zizmor, syft, grype, grant, yamllint) and for the directory scanners (poutine, runner-guard), so they're always invoked when enabled. runBatchLockFileTool and RunShellcheckOnLockFilesAndResources now emit an explicit zero-input stderr marker before returning, so the completeness check can tell "ran with zero input" apart from "never invoked".
There was a problem hiding this comment.
Correction: the commit hash is ffd49a9, not 32f97f7.
| // extractRunStepsFromLockFile parses a compiled lock file and returns all | ||
| // run: steps whose effective shell is lintable by shellcheck. | ||
| // | ||
| // The effective shell for a step is resolved in priority order: |
There was a problem hiding this comment.
Fixed in 32f97f7: rewrote the doc comment on extractRunStepsFromJob to describe its actual behavior (job-level step extraction with the resolved default shell), removing the mislabeled/duplicated reference to extractRunStepsFromLockFile.
There was a problem hiding this comment.
Correction: the commit hash is ffd49a9, not 32f97f7.
…nce-static-analysis-fix # Conflicts: # .github/workflows/static-analysis-report.lock.yml Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
@copilot Please take the next forward-progress pass on this PR.
|
…zero-input logging, doc fix Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Addressed all four outstanding review threads in 32f97f7:
Full |
The static-analysis compilation pipeline aborted prematurely when an early scanner (e.g.,
zizmor) returned a finding error, silently skipping 6 of 8 security scanners (poutine,runner-guard,syft,grype,yamllint,shellcheck). This caused security scan runs to complete successfully while masking findings from 75% of the configured toolchain.Changes
Non-short-circuiting batch scanner pipeline
Refactored
compileSpecificFilesandcompileAllFilesInDirectoryinpkg/cli/compile_pipeline.goto userunBatchExternalTools. Batch scanners now execute sequentially to completion regardless of individual tool findings or errors, accumulating failures to return only after post-processing and results output complete.Guaranteed scanner invocation logging
Updated
actionlint.go,syft.go,grype.go, andshellcheck.goto ensure every enabled tool emits an execution log line to stderr even when operating on zero lock files, container images, or script steps.Output completeness assertion step
Added an
Assert static analysis output completenessstep to.github/workflows/static-analysis-report.mdthat checkscompile-output.txtfor output from all 8 tools and triggers a hard failure if any scanner produces zero output.Implementation