Skip to content

Harden fetch-script portability and structured lint reporting - #71932

Merged
pelikhan merged 14 commits into
mainfrom
copilot/close-downstream-lint-regression
Oct 25, 2026
Merged

pelikhan merged 14 commits into
mainfrom
copilot/close-downstream-lint-regression

Conversation

Copilot AI commented Oct 25, 2026 •

Copy link
Copy Markdown
Contributor

Downstream comparisons exposed Bash 3.2-incompatible fetch scripts and optional linter findings missing from successful compile JSON. These changes address both gaps while preserving default exit behavior and legitimate credential handoffs.

  • Fetch portability: Generate balanced case patterns without changing intentional glob matching. Add the native Actions/AW bash-case-pattern rule, independent syntax checks, and macOS system-Bash CI coverage. Regenerate three affected locks.
    case "$remote_ref" in ($source_ref) found=1 ;; esac
  • Structured diagnostics: Preserve actionlint/ShellCheck findings, severity, rule IDs, and locations—including nonfatal findings and tooling failures. Distinguish script-relative coordinates from YAML positions; retain strict, fatal-finding, and dry-run gates.
  • Comparison discipline: Document source/binary provenance, explicit release action mode, matching action SHAs and linter versions, and normalized comparisons that include stderr-only findings. Existing CAO-shaped taint coverage remains unchanged.

Verification boundary: All 348 local workflows compile without lock drift. The 243 downstream comparisons and local Bash 3.2 execution remain unverified because required downloads were inaccessible.

…nostics

Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
@pelikhan
pelikhan marked this pull request as ready for review October 25, 2026 18:45
Copilot AI max review requested due to automatic review settings October 25, 2026 18:45
@pelikhan

Copy link
Copy Markdown
Collaborator

@copilot resolve the merge conflicts on this branch.

Copilot AI changed the title [WIP] Close downstream lint regression findings and harden compiler validation Oct 25, 2026
Copilot AI requested a review from pelikhan October 25, 2026 18:51
@pelikhan

Copy link
Copy Markdown
Collaborator

@copilot resolve the merge conflicts on this branch.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

Default-enabled linting and strict/dry-run reporting have unresolved correctness and MCP integration issues.

6 open issues
What changed in this PR

This PR improves Bash 3.2 portability and machine-readable lint reporting in gh-aw’s compilation pipeline.

Changes:

  • Uses balanced fetch-script case patterns, adds compatibility linting and syntax tests, and adds macOS CI coverage.
  • Preserves external scanner findings, severity, rule IDs, and script locations in compile JSON.
  • Documents reproducible compiler comparisons and regenerates three affected workflow locks.
File Description
pkg/​workflow/​lint/​rule_contract_test.go Adds the case-pattern rule contract.
pkg/​workflow/​lint/​lint.go Registers the compatibility rule by default.
pkg/​workflow/​lint/​lint_test.go Updates rule-registration expectations.
pkg/​workflow/​lint/​bash_case_pattern.go Implements Bash case-pattern compatibility checking.
pkg/​workflow/​lint/​bash_case_pattern_test.go Covers patterns, comments, expressions, and shell selection.
pkg/​workflow/​checkout_step_generator.go Generates balanced fetch-script case patterns.
pkg/​workflow/​checkout_manager_test.go Adds syntax checks independent of ShellCheck availability.
pkg/​cli/​validation_issue.go Adds rule IDs, columns, and script-location metadata.
pkg/​cli/​shellcheck.go Reports structured findings with step and snippet locations.
pkg/​cli/​compile_validation.go Preserves extended metadata during sanitization.
pkg/​cli/​compile_scanner_reporting.go Converts scanner results into batch diagnostics.
pkg/​cli/​compile_scanner_reporting_test.go Tests reporting modes, failures, severity, and locations.
pkg/​cli/​compile_scanner_differential.go Retains script metadata during dry-run filtering.
pkg/​cli/​compile_pipeline.go Integrates scanner reporting into batch compilation.
pkg/​cli/​compile_external_tools.go Forwards scanner results to the compile reporter.
pkg/​cli/​compile_external_tools_test.go Updates batch-diagnostic expectations.
pkg/​cli/​compile_development.go Adjusts warning accounting and scanner diagnostics.
pkg/​cli/​actionlint.go Captures findings and parsing failures for reporting.
docs/​src/​content/​docs/​reference/​workflow-linters.md Documents compatibility linting, JSON diagnostics, and comparison procedures.
.github/​workflows/​smoke-update-cross-repo-pr.lock.yml Regenerates balanced fetch patterns.
.github/​workflows/​smoke-claude.lock.yml Regenerates the balanced fetch pattern.
.github/​workflows/​daily-safeoutputs-git-simulator.lock.yml Regenerates balanced fetch patterns.
.github/​workflows/​cgo.yml Adds macOS system-Bash syntax checking.

Review effort: Max

Comment thread pkg/cli/actionlint.go
err = handleActionlintExecutionError(runResult.err, strict, lockFiles, totalErrors, parseErr, runResult.stdout)
if shouldParseOutput && parseErr == nil && totalErrors > 0 {
return withActionlintFindings(err, runResult.stdout)
return reportActionlintResult(ctx, err, runResult.stdout, parseErr, shouldParseOutput && totalErrors > 0)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Strict mode now returns actionlint JSON parse errors even when the process exits 0; regression coverage exercises malformed successful output. Fixed in 9012715.

Comment thread pkg/cli/compile_external_tools.go Outdated
Comment on lines +100 to +103
err := runShellcheckOnLockFilesAndResources(ctx, lockFiles, resources, verbose, strict)
if err != nil {
reportCompileScannerResult(ctx, "shellcheck", err)
}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Compile reporting now captures missing ShellCheck/Docker and lock extraction failures while preserving non-strict return behavior; reporting regressions cover both paths. Fixed in 9012715.

Comment on lines +21 to +24
diagnostics, failure := collectScannerDiagnostics(err)
if len(diagnostics) == 0 {
failure = err
}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wrapped operational errors now retain the Docker failure context when combined with findings; the mixed-result regression verifies the reported message. Fixed in 9012715.

Comment thread pkg/cli/compile_scanner_reporting.go Outdated
Comment on lines +63 to +65
result := ValidationResult{
Scope: "batch", Workflow: tool, Valid: gateErr == nil,
Errors: scannerValidationIssues(tool, gateErr), Warnings: []ValidationIssue{},

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MCP stderr fallback now runs only when structured ShellCheck findings are absent, avoiding duplicates while retaining fallback for operational-only results. Added regression coverage. Fixed in 9012715.

Comment thread pkg/cli/compile_scanner_reporting.go Resolved
Comment thread pkg/workflow/lint/bash_case_pattern.go Outdated
Comment on lines +35 to +39
shell, shellKnown := step.Fields["shell"].(string)
if !shellKnown && step.Fields["shell"] != nil {
continue
}
if shell != "" && shell != "bash" && !strings.HasPrefix(shell, "bash ") {

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Bash-case rule now resolves step, job, then workflow defaults for both Actions and authored AW inputs; inherited non-Bash shells have regression coverage. Fixed in 9012715.

…eam-lint-regression

# Conflicts:
#	pkg/workflow/lint/rule_contract_test.go

Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>

Copilot AI commented Oct 25, 2026 •

Copy link
Copy Markdown
Contributor Author

@copilot resolve the merge conflicts on this branch.

Resolved the conflicts and merged origin/main in 0c1e282a6c, preserving both lint-rule contracts.

@copilot resolve the merge conflicts on this branch.

Resolved the conflicts and merged origin/main in 0c1e282a6c, preserving both lint-rule contracts.

@gh-aw-bot

Copy link
Copy Markdown
Collaborator

@copilot address the following outstanding work in one pass:

  1. Update this branch with the latest main using make merge-main, resolving any conflicts and preserving the intended changes.
  2. Review (pkg/cli/actionlint.go:287): If actionlint exits 0 with malformed JSON, this returns nil despite parseErr: --strict leaves the batch valid, and --dry-run drops the failure entirely. Return the parse error in strict mode while preserving the non-strict return policy, and add an exit-0 malformed-output regression case. - Harden fetch-script portability and structured lint reporting #71932 (comment)
  3. Review (pkg/cli/compile_external_tools.go:103): When neither native ShellCheck nor Docker is available, the runner returns nil without reporting anything, so --shellcheck --json is indistinguishable from a clean scan. Lock-file extraction failures likewise remain stderr-only, contrary to the new documentation's guarantee. Report both early failure paths through the compile reporter while preserving non-strict return behavior, and cover them in the reporting tests. - Harden fetch-script portability and structured lint reporting #71932 (comment)
  4. Review (pkg/cli/compile_scanner_reporting.go:24): When ShellCheck returns both lint findings and a wrapped Docker failure, compile JSON reports only exit status 125, losing Docker's stderr and failure context. Preserve the wrapped operational branches in collectScannerDiagnostics, and cover a mixed findings-plus-wrapped-failure result. - Harden fetch-script portability and structured lint reporting #71932 (comment)
  5. Review (pkg/cli/compile_scanner_reporting.go:65): In MCP dry-run compilation, injectDevelopmentShellcheckDiagnostics still appends raw stderr findings to this new batch result. Low-only results become valid: true with nonempty errors, while blocking findings are duplicated. Skip the stderr fallback when structured ShellCheck diagnostics are already present, and cover both cases in the MCP tests. - Harden fetch-script portability and structured lint reporting #71932 (comment)
  6. Review (pkg/cli/compile_scanner_reporting.go:66): When a ShellCheck batch contains both a low/info finding and an operational failure, dryRunScannerBaseline.filter returns the original mixed error. This adds the low-severity finding to Errors, then adds the same finding again to Warnings below, inflating stats.Errors and misclassifying an advisory as blocking. Filter low/info findings out of the error list while retaining the operational failure and the existing gate validity. - Harden fetch-script portability and structured lint reporting #71932 (comment)
  7. Review (pkg/workflow/lint/bash_case_pattern.go:39): Steps inheriting a non-Bash defaults.run.shell are treated as Bash because this filter only reads the step-level shell. A Python string containing $(case ...) can therefore trigger a spurious compatibility warning and reject --dry-run. Resolve the effective shell in step → job → workflow order for both Actions and authored AW inputs, and add inherited-shell regression coverage. - Harden fetch-script portability and structured lint reporting #71932 (comment)

Push the necessary fixes, reply to each listed review thread and resolve it when addressed. Ignore feedback already answered or resolved. Use the pr-finisher skill and stop when only human review or CI remains; do not trigger CI.

Sous-chef head: 0c1e282
Sous-chef work: 3f026a707fa7eedf75e419c4da3a3978f292bba4366e3f7b3f8526614cfc950b 55f30892c8b7c848c919aab3642f9b3da29566f07edc41b80abede4a2960a71a 79be1e0e487ea4a05ea95abed97039e9e35dacefafd74045e5e6fd08571822b7 855864260cc8b5d20757a6938dcf351c39e6e95ae76e8f706c6c41ec319ae69f c5a4f64078c4aa2615328cbfc4e70a160d289f3acd13eeac3e4cf6296ca98cd3 f377723bc0d8bc8df22e8ebb9e4a92cca0d464a7a4e4ec1cdd58babca71e2d42
Sous-chef state: 5e7b64d06c2bf16b316e0dce58464d67b9375545f324534a8b54401dbe9dbee7

Generated by 👨‍🍳 PR Sous Chef · pi · haiku45 · ~6 AIC · ⌖ ~8 AIC · ⊞ 1.1K · ◷
Comment /souschef to run again

@pelikhan

Copy link
Copy Markdown
Collaborator
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Comment thread pkg/cli/compile_scanner_reporting.go Outdated
for _, diagnostic := range diagnostics {
finding := diagnostic.Finding
issues = append(issues, ValidationIssue{
Type: tool + "_finding", Message: finding.Message,

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Scanner findings now use a data-only prefix with normalized, control-character-safe, bounded text; regression coverage was added. Fixed in 0a85bcc.

@pelikhan

Copy link
Copy Markdown
Collaborator

@copilot run pr-finisher skill

Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Comment thread pkg/cli/mcp_tools_readonly.go Outdated
if err := json.Unmarshal([]byte(outputStr), &results); err != nil {
return outputStr
}
for _, result := range results {

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MCP ShellCheck stderr diagnostics are now neutralized and bounded in both development and standard output paths, with regression coverage. Fixed in 0a85bcc.

Line: e.Line,
File: e.File,
Severity: e.Severity,
e.Message = stringutil.SanitizeErrorMessage(e.Message)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The JSON-boundary sanitizer now also covers Script.JobID, Script.StepID, and Script.Source, with regression coverage. Fixed in 0a85bcc.

@pelikhan

Copy link
Copy Markdown
Collaborator

@copilot fix security bot comments

…gression' into copilot/close-downstream-lint-regression

# Conflicts:
#	pkg/cli/compile_scanner_reporting_test.go
#	pkg/cli/shellcheck.go
#	pkg/workflow/lint/bash_case_pattern.go
#	pkg/workflow/lint/bash_case_pattern_test.go

Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
…eam-lint-regression

Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
results = append(results, ValidationResult{Scope: "batch", Workflow: "shellcheck", Valid: false, Warnings: []ValidationIssue{}})
index = len(results) - 1
} else if index >= 0 && index < len(results) {
for _, issue := range append(results[index].Errors, results[index].Warnings...) {

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The shared sanitizer now emits single-line, bounded diagnostics and neutralizes controls/markdown-like markers for both ShellCheck paths. Covered by regressions. Fixed in af50e96.

if index == -1 {
results = append(results, ValidationResult{Scope: "batch", Workflow: "shellcheck", Valid: false, Warnings: []ValidationIssue{}})
index = len(results) - 1
} else if index >= 0 && index < len(results) {

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The shared sanitizer now redacts common secret assignments and secret-like identifiers before adding ShellCheck messages to JSON; both MCP paths have regression coverage. Fixed in af50e96.

Copilot AI and others added 2 commits October 25, 2026 21:39
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Comment thread pkg/cli/shellcheck.go
File: info.LockFile,
}
location := &ValidationScriptLocation{
JobID: info.JobID, StepID: info.ID, StepName: info.Name, StepIndex: info.Index,

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in b675bb1 by omitting all five free-text script identity fields from JSON. Raw identities remain internal so differential matching is unchanged; numeric locations remain available. Regression coverage verifies omission from both errors and warnings, and local validation passes.

Comment thread pkg/cli/validation_issue.go Resolved
@gh-aw-bot

Copy link
Copy Markdown
Collaborator

@copilot address the following outstanding work in one pass:

  1. Update this branch with the latest main using make merge-main, resolving any conflicts and preserving the intended changes.
  2. Review (pkg/cli/shellcheck.go:644): Prompt injection risk: workflow-controlled step/job identifiers (job_id, step_id, step_name, source/resource) are copied into ValidationIssue.Script (e.g., StepName: info.Name) and can be returned in MCP compile JSON. An attacker who controls workflow content can embed instruction-like text in these fields and potentially steer an agent consuming the JSON. Consider applying the same neutralization/demotion used for scanner messages (e.g., strip/escape prompt-like markers and mentions, bound length), or omit these fields from machine output unless explicitly requested. - Harden fetch-script portability and structured lint reporting #71932 (comment)
  3. Review (pkg/cli/validation_issue.go:20): Prompt injection risk: ValidationIssue.Script introduces multiple free-text identity fields (job_id, step_id, step_name, source/resource) that may carry attacker-controlled workflow strings into MCP-visible JSON. Sanitizing for control chars alone is not a deterministic safeguard against instruction-following by an agent; prefer neutralization/demotion similar to scanner output or avoid emitting these fields by default. - Harden fetch-script portability and structured lint reporting #71932 (comment)

Push the necessary fixes, reply to each listed review thread and resolve it when addressed. Ignore feedback already answered or resolved. Use the pr-finisher skill and stop when only human review or CI remains; do not trigger CI.

Sous-chef head: af50e96
Sous-chef work: 0485d0f71431e2534b4cf0fd35d3702e5ef7f0247eff3f041ba584a93927170e c9e94a7ca5843e467876fd74ea3f5574068adbe0234097e8382216b4704adc96
Sous-chef state: 9d3fe237e7644b0095e82fc6720e506e05f1e331eec5628e3feca25c2c974f8a

Generated by 👨‍🍳 PR Sous Chef · pi · haiku45 · ~4 AIC · ⌖ ~8 AIC · ⊞ 1.1K · ◷
Comment /souschef to run again

@pelikhan

Copy link
Copy Markdown
Collaborator

@copilot merge main, fix ci failures

Copilot AI and others added 2 commits October 25, 2026 22:35
…eam-lint-regression

Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
@pelikhan

Copy link
Copy Markdown
Collaborator

@copilot fix ci failure

Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>

Copilot AI commented Oct 25, 2026

Copy link
Copy Markdown
Contributor Author

@copilot merge main, fix ci failures

Ran make merge-main; latest main is already included, with no conflicts. Fixed the remaining CGO failures in 251be7d: updated scanner-message expectations, supplied a valid unavailable-tool fixture, removed the Docker stub’s PATH dependency, and tested non-strict suppression through the public runner. Focused tests and make agent-report-progress pass, including lint, impacted tests, schema freshness, and all 348 workflow drift checks. No CI triggered; current-HEAD workflows marked action_required still require maintainer action.

Copilot AI commented Oct 25, 2026

Copy link
Copy Markdown
Contributor Author

@copilot fix ci failure

The CGO failures are fixed in the already-published 251be7d: valid unavailable-tool fixture, PATH-independent Docker stub, and non-strict policy tested through the public runner. Verified the three failing tests and make agent-report-progress; all pass. make merge-main found no pending merge, and all 348 workflows recompiled without drift. No additional changes or CI runs were needed; CI on 251be7d still needs maintainer verification.

@pelikhan
pelikhan merged commit 6f802ca into main Oct 25, 2026
3 checks passed
@pelikhan
pelikhan deleted the copilot/close-downstream-lint-regression branch October 25, 2026 23:47
@github-actions

Copy link
Copy Markdown
Contributor

🎉 This pull request is included in a new release.

Release: v0.93.6

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

5 participants