fix: compile zizmor fails by default on high severity findings - #50872
Conversation
Previously, high/critical severity zizmor findings were silently swallowed in the compile pipeline when not in strict mode. The runZizmorOnFiles function already correctly returns errors for high/critical findings regardless of strict mode, but the pipeline discarded those errors. Now high severity findings always fail compilation. Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ PR Code Quality Reviewer completed the code quality review. Warning Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding. What happenedThe threat detection engine failed to produce results. Review the workflow run logs for details. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ Warning Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding. What happenedThe threat detection engine failed to produce results. Review the workflow run logs for details. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. Warning Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding. What happenedThe threat detection engine failed to produce results. Review the workflow run logs for details. No ADR enforcement needed: PR #50872 does not have the 'implementation' label and has only 4 new lines of code in business logic directories (threshold: 100). |
|
✅ Test Quality Sentinel completed test quality analysis. Warning Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding. What happenedThe threat detection engine failed to produce results. Review the workflow run logs for details. No new or modified test functions in this PR. The single modified test file (eslint-factory/src/rules/require-fetch-response-body-try-catch.test.ts) contains only line deletions (net: -3 lines). Test Quality Sentinel skipped. |
There was a problem hiding this comment.
Pull request overview
Attempts to make compilation fail on high/critical zizmor findings without strict mode.
Changes:
- Unconditionally propagates zizmor wrapper errors.
- Adds a patch changeset.
- Reformats an ESLint test fixture.
Show a summary per file
| File | Description |
|---|---|
pkg/cli/compile_pipeline.go |
Changes zizmor error propagation. |
eslint-factory/src/rules/require-fetch-response-body-try-catch.test.ts |
Reformats test data. |
.changeset/zizmor-fail-high-severity.md |
Documents the intended fix. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Suppressed comments (1)
pkg/cli/compile_pipeline.go:517
- The directory path has the same ineffective propagation:
RunZizmorOnFilesconverts all non-strict errors tonilthroughhandleBatchToolError(pkg/cli/compile_external_tools.go:113,123-136). As written, a high/critical finding with strict mode off still reaches this line asnil, so compile-all continues to succeed. Preserve the high/critical error before the generic non-strict suppression.
if err := RunZizmorOnFiles(lockFilesForZizmor, config.Verbose && !config.JSONOutput, config.Strict); err != nil {
return workflowDataList, err
- Files reviewed: 3/3 changed files
- Comments generated: 2
- Review effort level: Balanced
| if err := RunZizmorOnFiles(lockFilesForZizmor, config.Verbose && !config.JSONOutput, config.Strict); err != nil { | ||
| if config.Strict { | ||
| return workflowDataList, err | ||
| } | ||
| // Always fail on high/critical severity findings (zizmor returns errors for those | ||
| // regardless of strict mode). In strict mode, all findings are errors. | ||
| return workflowDataList, err |
| // Always fail on high/critical severity findings (zizmor returns errors for those | ||
| // regardless of strict mode). In strict mode, all findings are errors. | ||
| return workflowDataList, err |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Warning
Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding.
What happened
The threat detection engine failed to produce results.
Review the workflow run logs for details.
Skills-Based Review 🧠
Applied /diagnosing-bugs — the fix is correct and well-described, but lacks a regression test.
📋 Key Themes & Highlights
Key Themes
- Missing regression test: The bug was silent (no compile error). Without a test, the
if config.Strictguard could be reintroduced accidentally incompileSpecificFilesorcompileAllFilesInDirectorywithout detection.
Positive Highlights
- ✅ Root cause correctly identified and fixed in both callsites symmetrically
- ✅ The comment added at line 204–205 clearly explains the invariant (
runZizmorOnFilesalready gates severity; theStrictguard was redundant) - ✅ Changeset entry is accurate and matches the behaviour table in the PR description
- ✅ PR description includes a clear before/after behaviour matrix — excellent documentation
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 19.1 AIC · ⊞ 7.1K
Comment /matt to run again
| } | ||
| // Always fail on high/critical severity findings (zizmor returns errors for those | ||
| // regardless of strict mode). In strict mode, all findings are errors. | ||
| return workflowDataList, err |
There was a problem hiding this comment.
[/diagnosing-bugs] No regression test guards this fix — the bug could silently reappear if the RunZizmorOnFiles error-propagation logic changes.
Add a unit or integration test that calls compileSpecificFiles with config.Strict = false and a high/critical zizmor finding, asserting a non-nil error.
@copilot please address this.
There was a problem hiding this comment.
Warning
Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding.
What happened
The threat detection engine failed to produce results.
Review the workflow run logs for details.
The fix is correct. RunZizmorOnFiles already returns errors only for high/critical findings when strict is off, so the if config.Strict guard was incorrectly swallowing them. Removing the guard in both compileSpecificFiles and compileAllFilesInDirectory is the right approach. Changeset and test reformatting are both appropriate.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 14.3 AIC · ⊞ 5.3K
There was a problem hiding this comment.
Warning
Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding.
What happened
The threat detection engine failed to produce results.
Review the workflow run logs for details.
Verdict: Request changes — the fix does not actually work
The diff removes the if config.Strict guard around RunZizmorOnFiles errors at both call sites, but RunZizmorOnFiles funnels through the shared handleBatchToolError helper, which unconditionally returns nil for any error when strict is false. I confirmed this experimentally with a throwaway unit test. As a result, high/critical zizmor findings still will not fail compilation in non-strict mode — the PR's stated goal is not met, and the change is effectively a no-op for the non-strict path. See inline comments for the call chain and a suggested fix (bypass or special-case handleBatchToolError for severity-forced failures, plus add a regression test).
💡 Additional notes
- The changeset description overclaims: "zizmor now fails by default on high severity findings, regardless of strict mode" is not true given the current code path.
- No test was added to cover the non-strict + high-severity failure scenario; such a test would have caught this immediately.
- The
eslint-factorytest formatting change is a trivial no-op (line collapse), not flagged as an issue.
🔎 Code quality review by PR Code Quality Reviewer · auto · 64.2 AIC · ⊞ 7.8K
Comment /review to run again
| @@ -202,9 +202,9 @@ func compileSpecificFiles( | |||
| return workflowDataList, err | |||
| } | |||
| if err := RunZizmorOnFiles(lockFilesForZizmor, config.Verbose && !config.JSONOutput, config.Strict); err != nil { | |||
There was a problem hiding this comment.
This change is a no-op for the stated goal: RunZizmorOnFiles returning an error here still gets swallowed by handleBatchToolError (in pkg/cli/compile_external_tools.go) whenever strict is false, so high/critical zizmor findings will not fail compilation in non-strict mode despite this diff.
💡 Why this doesn't fix the bug
The call chain is:
RunZizmorOnFiles(...) -> runBatchLockFileTool(...) -> handleBatchToolError(toolName, err, strict, verbose)and handleBatchToolError is:
func handleBatchToolError(toolName string, err error, strict, verbose bool) error {
if err == nil {
return nil
}
if strict {
return fmt.Errorf("%s failed: %w", toolName, err)
}
// In non-strict mode, errors are warnings
if verbose {
fmt.Fprintln(os.Stderr, ...)
}
return nil
}Regardless of why runZizmorOnFiles returned an error (including the new high/critical-severity error path), handleBatchToolError unconditionally returns nil when strict is false. Removing the if config.Strict { return ... } guard at the call site in compile_pipeline.go therefore has no effect: the error never reaches this call site to begin with when strict is false, because it was already discarded one layer down.
I verified this experimentally by adding a throwaway unit test that calls runBatchLockFileTool("zizmor", ..., strict=false, ...) with a runner returning a high-severity-style error — it returns nil, confirming the swallow happens before RunZizmorOnFiles returns to compile_pipeline.go.
Suggested fix: handleBatchToolError needs to distinguish "must always fail" errors (e.g. wrap them in a sentinel type or check severity) from ordinary warnings, or RunZizmorOnFiles/runZizmorOnFiles needs to bypass handleBatchToolError entirely for the high/critical path and return the raw error directly to the caller instead of going through the shared warning-swallowing helper.
This should also get a regression test asserting that a high-severity zizmor error surfaces as a real error when strict=false.
| @@ -514,9 +514,7 @@ func compileAllFilesInDirectory( | |||
| return workflowDataList, err | |||
| } | |||
| if err := RunZizmorOnFiles(lockFilesForZizmor, config.Verbose && !config.JSONOutput, config.Strict); err != nil { | |||
There was a problem hiding this comment.
Same broken pattern as the other call site: handleBatchToolError still swallows this error whenever strict is false, so compileAllFilesInDirectory also fails to fail on high/critical zizmor findings.
💡 Details
See the companion comment on line 204 for the full call-chain explanation (RunZizmorOnFiles -> runBatchLockFileTool -> handleBatchToolError, which unconditionally returns nil for any error when strict is false). Fixing only one of the two call sites would not have been sufficient either way — the actual defect is in the shared handleBatchToolError helper, not at these call sites.
|
🎉 This pull request is included in a new release. Release: |
compile_pipeline.gowas swallowing zizmor errors in non-strict mode, even for high/critical severity findings.runZizmorOnFilesalready returns errors only for high/critical when strict is off, but the pipeline gated propagation onconfig.Strict.Ref: https://github.com/github/gh-aw/actions/runs/31103313197/job/92623368579
if config.Strictguard in bothcompileSpecificFiles()andcompileAllFilesInDirectory()so zizmor errors always propagate