feat: fail compile --actionlint on high severity errors - #50874
Conversation
In non-strict mode, actionlint now fails only on high-severity errors (non-shellcheck errors, or shellcheck errors with 'error'/'warning' severity). Low-severity shellcheck findings (info/style) no longer cause failure. Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
🧠 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. |
|
✅ 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. Test Quality Sentinel: Analysis complete. Score 100/100. All 3 new tests are design-contract tests with edge-case coverage. No violations detected. |
|
✅ 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. |
|
✅ 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. |
|
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. Great work! 🎉 This PR looks solid and ready for review. The fix for actionlint severity classification is well-focused, includes comprehensive tests (91 new test lines), and has a clear description of the problem and solution. What's working well:
This is ready for maintainer review!
|
There was a problem hiding this comment.
Pull request overview
Adds severity-aware actionlint gating so non-strict compilation fails only for high-severity findings.
Changes:
- Classifies ShellCheck findings by embedded severity.
- Gates non-strict compilation on high-severity findings.
- Adds severity parsing and counting tests.
Show a summary per file
| File | Description |
|---|---|
pkg/cli/actionlint.go |
Implements severity classification and gating. |
pkg/cli/actionlint_test.go |
Tests severity extraction and counting. |
eslint-factory/src/rules/require-fetch-response-body-try-catch.test.ts |
Reformats an existing test case. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 3/3 changed files
- Comments generated: 2
- Review effort level: Balanced
| if stdout == "" || strings.TrimSpace(stdout) == "" { | ||
| return 0 | ||
| } | ||
| var errors []actionlintError | ||
| if err := json.Unmarshal([]byte(stdout), &errors); err != nil { | ||
| return 0 |
| func extractShellcheckSeverity(message string) string { | ||
| // Find the "SC" code prefix | ||
| scIdx := strings.Index(message, "SC") | ||
| if scIdx < 0 { | ||
| return "" | ||
| } | ||
| // After "SC<digits>:" we expect the severity | ||
| rest := message[scIdx:] | ||
| parts := strings.SplitN(rest, ":", 3) | ||
| if len(parts) < 2 { | ||
| return "" | ||
| } | ||
| return parts[1] | ||
| } |
![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.
Review: feat: fail compile --actionlint on high severity errors
The overall approach is correct and well-tested. One blocking issue needs to be addressed before merging.
Blocking issue: countHighSeverityErrors silently returns 0 on JSON parse failure, which contradicts the PR's stated fail-safe behaviour. If actionlint outputs non-JSON (e.g. a plain-text error message on tool misconfiguration), the function treats it as "no high-severity errors" and allows compilation to succeed. This is the exact failure mode the PR is trying to prevent.
Non-blocking observations:
- The severity classification logic (
extractShellcheckSeverity) and tests are clean and correct. - Defaulting unrecognised shellcheck severity strings to
true(high) inisHighSeverityActionlintErrorcorrectly applies the fail-safe principle there. - The
strictvs non-strict refactoring is a clear improvement.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 21.2 AIC · ⊞ 5.3K
| if stdout == "" || strings.TrimSpace(stdout) == "" { | ||
| return 0 | ||
| } | ||
| var errors []actionlintError |
There was a problem hiding this comment.
Silent failure on JSON parse error violates the fail-safe principle
The PR description states "unrecognized formats default to high severity (fail-safe)", but countHighSeverityErrors returns 0 when JSON unmarshalling fails:
if err := json.Unmarshal([]byte(stdout), &errors); err != nil {
return 0
}If stdout is non-empty but not valid JSON (e.g. actionlint emits a plain-text error or partial output on failure), this silently classifies all findings as low-severity and lets compilation succeed — the opposite of fail-safe.
Suggested fix:
if err := json.Unmarshal([]byte(stdout), &errors); err != nil {
actionlintLog.Printf("countHighSeverityErrors: failed to parse actionlint output: %v", err)
return 1 // fail-safe: treat unparseable output as high severity
}@copilot please address this.
|
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. Blocked: Design Decision Gate - ADR RequiredThis PR makes significant changes to core business logic (165 new lines in pkg/) but does not have a linked Architecture Decision Record (ADR). Draft ADR committed: docs/adr/50874-actionlint-severity-based-failure-in-non-strict-mode.md - review and complete it before merging. This PR cannot merge until an ADR is linked in the PR body. What to do next:
Once an ADR is linked in the PR body, this gate will re-run and verify the implementation matches the decision. Why ADRs matter: ADRs create a searchable permanent record of why the codebase looks the way it does. Future contributors and your future self will thank you. Michael Nygard ADR Format - an ADR must contain: Context, Decision, Alternatives Considered, and Consequences sections. All ADRs are stored in docs/adr/ as Markdown files numbered by PR number.
|
|
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. ✅ Test Quality Sentinel ReportPR: #50874 — "feat: fail compile --actionlint on high severity errors" Metrics
New Tests
Verdict✅ Quality passes — no violations. The PR adds focused, well-designed unit tests that comprehensively cover the new severity classification feature.
|
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 — one high-severity parsing bug plus a fail-open severity check undermine this PR's own goal.
Themes
- High:
extractShellcheckSeverityuses a barestrings.Index(message, "SC")to locate the shellcheck code prefix, which can match unrelated "SC" substrings inside free-text messages and misclassify severity. - Medium:
countHighSeverityErrorssilently returns 0 on JSON unmarshal failure, so an unparseable actionlint payload is treated as "no high-severity findings" and compile passes — contradicting the stated fail-safe design. - Medium:
stdoutis parsed twice (once inparseAndDisplayActionlintOutput, again incountHighSeverityErrors) — wasted work and a source of future divergence between the two decoders. - Medium: The core new behavior (
handleActionlintFindingsgating non-strict mode on severity) has no direct test; only the lower-level helper functions are tested in isolation.
Grumpy-coder sub-agent findings were reviewed and all four were confirmed as valid on independent inspection of the diff.
🔎 Code quality review by PR Code Quality Reviewer · auto · 59.5 AIC · ⊞ 7.8K
Comment /review to run again
| fileDescription := actionlintFileDescription(lockFiles) | ||
|
|
||
| // In non-strict mode, fail only on high-severity errors. | ||
| highSeverityCount := countHighSeverityErrors(stdout) |
There was a problem hiding this comment.
countHighSeverityErrors re-parses the same stdout JSON that parseAndDisplayActionlintOutput already unmarshaled a few lines earlier in the caller — wasted work and a maintenance hazard if the two parsers ever diverge.
💡 Duplicate JSON parsing of the same actionlint output
runActionlintOnFilesWithOptions already calls parseAndDisplayActionlintOutput(runResult.stdout, verbose), which unmarshals stdout into []actionlintError and computes totalErrors/errorsByKind. handleActionlintFindings now performs a second, independent json.Unmarshal of the identical payload via countHighSeverityErrors(stdout).
Besides the redundant allocation/CPU cost on every compile, this creates two separate code paths decoding the same JSON — if actionlintError's tags or unmarshal semantics change in one place but not the other, severity counting and display counting can silently diverge.
Fix: thread the already-parsed []actionlintError slice from parseAndDisplayActionlintOutput through to handleActionlintFindings instead of re-parsing stdout as a raw string.
| return 0 | ||
| } | ||
| var errors []actionlintError | ||
| if err := json.Unmarshal([]byte(stdout), &errors); err != nil { |
There was a problem hiding this comment.
countHighSeverityErrors silently returns 0 on JSON unmarshal failure, so a corrupted/unparseable actionlint payload is treated as "zero high-severity errors" and compile passes — the opposite of the fail-safe design this PR claims to provide.
💡 Parse failures are swallowed instead of surfaced
If json.Unmarshal fails inside countHighSeverityErrors, the function returns 0 with no logging and no propagated error. This is distinct from parseErr, which is already computed by parseAndDisplayActionlintOutput earlier in the call chain and tracked as an IntegrationErrors stat.
In handleActionlintFindings, this means: when parseErr != nil (payload already known to be malformed), highSeverityCount will also be 0 because the second json.Unmarshal in countHighSeverityErrors fails identically — so the function falls through to return nil, silently passing compile even though actionlint genuinely exited with errors that could not be parsed. The doc comment for extractShellcheckSeverity explicitly says unrecognized formats default to high severity as "fail-safe", but this top-level unmarshal failure path does the opposite and fails open.
Fix: reuse the already-computed parseErr/parsed errors from parseAndDisplayActionlintOutput instead of re-parsing, or at minimum log the unmarshal error and treat unparseable output as high severity (consistent with the fail-safe intent stated elsewhere in this diff).
| // Expected format: "shellcheck reported issue in this script: SC<code>:<severity>:<line>:<col>: <message>" | ||
| func extractShellcheckSeverity(message string) string { | ||
| // Find the "SC" code prefix | ||
| scIdx := strings.Index(message, "SC") |
There was a problem hiding this comment.
strings.Index(message, "SC") can match the literal substring "SC" anywhere in the free-text shellcheck message, not just the intended SC<code>: prefix — misclassifying severity.
💡 Fragile substring search for the shellcheck code prefix
The expected format is shellcheck reported issue in this script: SC2086:error:4:13: msg, but strings.Index(message, "SC") finds the first occurrence of the literal two-character substring "SC" anywhere in the message — including inside arbitrary shellcheck wording that happens to contain "SC" (e.g. words like "SCRIPT", "DESCRIPTION", "DISCARD", or even a message that legitimately starts with different casing/wording before the code). If any such text precedes the real SC<code> token, scIdx points at the wrong location and the subsequent strings.SplitN(rest, ":", 3) extracts garbage as the "severity", silently misclassifying a finding as high or low severity.
Fix: parse severity with an anchored regex (e.g. regexp.MustCompile(SC\d+:(\w+):)) or split on the known prefix "in this script: " first, rather than a bare substring search for "SC".
|
|
||
| func handleActionlintFindings(strict bool, lockFiles []string, totalErrors int, parseErr error) error { | ||
| if !strict { | ||
| func handleActionlintFindings(strict bool, lockFiles []string, totalErrors int, parseErr error, stdout string) error { |
There was a problem hiding this comment.
handleActionlintFindings is the function whose behavior actually changed (non-strict mode now fails on high severity), yet no test exercises this integration path end-to-end.
💡 Missing integration test for the core new behavior
The new tests (TestIsHighSeverityActionlintError, TestExtractShellcheckSeverity, TestCountHighSeverityErrors) only cover the lower-level helpers in isolation. handleActionlintFindings itself — the function that decides whether compile --actionlint fails in non-strict mode — has no direct test verifying:
- it returns a non-nil error when
stdoutcontains high-severity findings andstrict=false - it returns
nilwhenstdoutcontains only low-severity (info/style) findings andstrict=false - the strict-mode branch is unaffected by the new
stdoutparameter - behavior when
stdoutis unparseable in non-strict mode (see the fail-open concern noted elsewhere)
This is the actual regression surface the PR is meant to fix (per the linked CI run); testing only the helper functions leaves the wiring/branch logic unverified.
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 and /tdd — requesting changes on two correctness issues.
📋 Key Themes & Highlights
Key Themes
- Dual JSON parse:
countHighSeverityErrorsre-decodesstdoutthatparseAndDisplayActionlintOutputalready decoded. The decoded[]actionlintErrorslice should be threaded through the call chain instead. - Silent fail-open on parse error: when the second parse in
countHighSeverityErrorsfails it returns 0, silently passing the gate with no visible warning. The existingparseErrpath already handles this correctly if the refactor above is adopted. - Test gap: the intentional fail-open behaviour on malformed JSON in
countHighSeverityErrorsis not covered by a test.
Positive Highlights
- ✅ Fail-safe default (unknown message format → high severity) is a solid design choice
- ✅ Well-structured unit tests for
extractShellcheckSeverityandisHighSeverityActionlintError - ✅ Clear PR description with the shellcheck format documented inline
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 47.9 AIC · ⊞ 7.1K
Comment /matt to run again
| fileDescription := actionlintFileDescription(lockFiles) | ||
|
|
||
| // In non-strict mode, fail only on high-severity errors. | ||
| highSeverityCount := countHighSeverityErrors(stdout) |
There was a problem hiding this comment.
[/diagnosing-bugs] countHighSeverityErrors re-parses stdout JSON a second time, even though parseAndDisplayActionlintOutput already decoded the same data into []actionlintError above. This means two unmarshal passes on the same string, and the count can silently return 0 if the second parse fails while the first succeeded.
💡 Suggested refactor
Pass the already-decoded slice through the call chain instead of stdout:
// handleActionlintFindings(strict, lockFiles, totalErrors, parseErr, errors []actionlintError)
highSeverityCount := countHighSeverityErrors(errors)This removes the dual-parse and aligns with the existing totalErrors pattern.
@copilot please address this.
| return fmt.Errorf("actionlint found %d high severity error(s) in %s", highSeverityCount, fileDescription) | ||
| } | ||
|
|
||
| if parseErr != nil { |
There was a problem hiding this comment.
[/diagnosing-bugs] When countHighSeverityErrors returns 0 due to a JSON parse failure, the function silently returns nil — treating a parse error as "no high-severity errors". The parseErr check below (line 461) logs a debug message but still returns nil, so a real failure in the output format would silently pass the gate.
💡 Suggested fix
Consider promoting the parse-failure path to a user-visible warning before returning nil, or — if the refactor above is adopted — rely on the existing parseErr from parseAndDisplayActionlintOutput which already surfaces this via a warning message to stderr.
@copilot please address this.
| // All low severity | ||
| input2 := `[ | ||
| {"message":"shellcheck reported issue in this script: SC2016:info:4:13: msg","filepath":"a.yml","line":1,"column":1,"kind":"shellcheck"}, | ||
| {"message":"shellcheck reported issue in this script: SC2034:style:1:1: msg","filepath":"b.yml","line":2,"column":1,"kind":"shellcheck"} |
There was a problem hiding this comment.
[/tdd] TestCountHighSeverityErrors doesn't cover the JSON-parse-failure path, which is the fail-safe that silently returns 0. Without a test for malformed input, it's easy for someone to inadvertently change the fail-safe behaviour (e.g. to return a non-zero count) without a failing test.
💡 Suggested test case
// malformed JSON returns 0 (fail-open)
got3 := countHighSeverityErrors(`not valid json`)
if got3 != 0 {
t.Errorf("countHighSeverityErrors() = %d, want 0 for malformed input", got3)
}A brief comment explaining the intentional fail-open behaviour here would also help future readers.
@copilot please address this.
|
@copilot run pr-finisher skill |
|
🎉 This pull request is included in a new release. Release: |
compile --actionlintin non-strict mode was ignoring all findings, including genuine high-severity errors. Only strict mode would fail, making the--actionlintflag ineffective as a gate for real problems.Ref: https://github.com/github/gh-aw/actions/runs/31103313197/job/92623368612#step:5:1
Changes
error/warning= high,info/style= low).handleActionlintFindingsreturns an error when high-severity findings exist, even without--strict.Severity extraction
Shellcheck messages from actionlint follow the format:
extractShellcheckSeverityparses this field; unrecognized formats default to high severity (fail-safe).