Repository navigation
feat: lint workflows during compilation by default - #71349
Conversation
Pass the message through the step environment and preserve literal input in logs and summaries. Add regression coverage for shell syntax, multiline messages, and printf metacharacters. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Run the built-in Actions/AW AST rules before CLI compilation emits a lock file, with --skip-lint and the corresponding MCP option. Preserve compiler validation and reject the opt-out for dry-run compilation. Keep low-level compiler callers opt-in to preserve their existing API contract. Support generated cache-mode in the Actions AST and valid sandbox shorthand forms in authored secret analysis. Add regression coverage, CLI documentation, and a breaking-change migration note. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
pelikhan
changed the title
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality SentinelOutcome: The agent completed and reported no action. A no-op output alone does not verify that all required work was completed or that publication was possible. Agent messages are omitted from this comment. The category uses structured run status, not interpretation of agent prose.
|
|
✅ Ponytail Reviewer completed successfully! Ponytail ReviewerOutcome: The agent completed and reported no action. A no-op output alone does not verify that all required work was completed or that publication was possible. Agent messages are omitted from this comment. The category uses structured run status, not interpretation of agent prose.
|
|
✅ PR Code Quality Reviewer completed the code quality review.
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. See the comment below for the result and any generated ADR draft. Commit pushed:
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review summary
No actionable blocking issues found in the changed lines. The changes consistently wire default AST linting, the explicit opt-out, and the shell-injection regression coverage. This was a static review; tests were not executed. The specialist pass was unavailable because its file-read invocation was rejected, so this verdict is based on the independent review only.
🔎 Code quality review by PR Code Quality Reviewer · copilot · gpt50 · ~2 AIC · ⌖ ~1 AIC · ⊞ 1.6K
Comment /review to run again
🏗️ Design Decision Gate — ADR RequiredThis PR adds 340 new lines in business logic directories ( 📄 Draft ADR committed:
Decision recorded in the draft: CLI and MCP compilation now run the built-in Actions/AW AST lint by default, before lock-file emission and also with 📋 What to do next
Once an ADR is linked in the PR body, this gate will re-run and check that the implementation matches the decision. ❓ Why ADRs MatterADRs keep a searchable, permanent record of why the codebase looks the way it does. This PR makes a breaking change to a default (major changeset), so recording the trade-offs matters. 📋 Michael Nygard ADR Format ReferenceAn ADR must contain these four sections to be considered complete:
All ADRs are stored in
|
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /codebase-design and /tdd to the two most behavior-sensitive hunks: the new default compile-time lint pass and the sandbox-shorthand handling in authored taint analysis. Triage via pr-triage was not invoked (small, well-scoped diff fit a direct read); classification follows the fallback heuristic against pr-meta.json/pr-diff.patch, prioritizing pkg/workflow/compiler_lint.go, pkg/cli/mcp_tools_readonly.go, and pkg/workflow/taint/authored_scripts.go by change size and risk.
📋 Key Themes & Highlights
Key Themes
- Lint coverage parity gap:
lintCompiledWorkflownever passesJavaScriptoptions intoworkflowlint.Input, so JS-driver-aware rules (secret-leak,grader-file-io) run with reduced fidelity at compile time vs. standalonegh aw lint. Not necessarily wrong, but undocumented and worth an explicit decision. - Sandbox shorthand short-circuit: the new early returns in
authoredScriptEnvironmentsilently skip thesandbox.agent.envoverlay forbool-typedagentfields that previously triggered a type error — a quiet behavior widening that should get an explicit regression test.
Positive Highlights
- ✅
--skip-lintis well-guarded: blocked under--dry-runat both the CLI (validateDevelopmentCompileMode) and MCP (registerCompileTool) layers, with matching tests in both places. - ✅ Strong regression coverage for the haiku-printer fix: parameterized tests exercise shell metacharacters, quotes, and empty-string edge cases against the actual generated
run:script. - ✅
compiler_lint_test.gocovers both the vulnerable and fixed workflow shapes, threat-suppression honoring, warning-vs-error severity split, and emission ordering (TestCompilerLintRunsBeforeEmission) — solid TDD-style coverage for the new pass. - ✅ Clear
.changesetand docs updates matching the new default behavior and opt-out flag.
@copilot please address the review comments above.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet50 · ~60 AIC · ⌖ ~16 AIC · ⊞ 10.2K
Comment /matt to run again
There was a problem hiding this comment.
🟡 Changes recommended
Default linting rejects supported workflow configurations and does not consistently honor existing compiler policy and CLI options.
6 open issues
What changed in this PR
This PR moves existing Actions/AW lint checks into gh-aw’s compilation pipeline and fixes shell injection in the haiku dispatch receiver.
Changes:
- Enables built-in linting before lock-file emission, with CLI/MCP opt-outs and a low-level Go opt-in.
- Passes dispatched messages through an environment variable and adds literal-output regressions.
- Extends AST compatibility and documents the breaking default and migration options.
| File | Description |
|---|---|
pkg/workflow/taint/authored_scripts.go |
Handles sandbox shorthands during environment analysis. |
pkg/workflow/taint/authored_scripts_test.go |
Tests shorthand environment handling. |
pkg/workflow/compiler.go |
Runs lint before returning generated YAML. |
pkg/workflow/compiler_types.go |
Stores the compiler lint setting. |
pkg/workflow/compiler_options.go |
Adds WithLint. |
pkg/workflow/compiler_lint.go |
Integrates lint diagnostics and suppressions. |
pkg/workflow/compiler_lint_test.go |
Covers lint failures, warnings, and emission behavior. |
pkg/workflow/actions/workflow.go |
Adds AST storage for cache mode. |
pkg/workflow/actions/parse.go |
Parses cache mode into the AST. |
pkg/workflow/actions/parse_test.go |
Tests cache-mode parsing. |
pkg/workflow/actions/lower.go |
Preserves cache mode during emission. |
pkg/cli/mcp_tools_readonly.go |
Exposes and forwards skip_lint. |
pkg/cli/mcp_tools_output_streams_test.go |
Tests MCP opt-out forwarding and dry-run rejection. |
pkg/cli/compile_development.go |
Rejects skipping lint during dry-run compilation. |
pkg/cli/compile_config.go |
Adds the CLI lint opt-out setting. |
pkg/cli/compile_compiler_setup.go |
Enables linting through shared compiler setup. |
pkg/cli/compile_compiler_setup_test.go |
Tests defaults, opt-out, and retained validation. |
docs/src/content/docs/setup/cli.md |
Documents the new flag and default behavior. |
docs/src/content/docs/reference/compilation-process.md |
Explains lint integration and scope. |
cmd/gh-aw/main.go |
Registers and propagates --skip-lint. |
cmd/gh-aw/compile_flags_test.go |
Tests flag defaults and configuration mapping. |
actions/setup/js/haiku_printer_workflow.test.cjs |
Verifies literal message output through Bash. |
.github/workflows/haiku-printer.yml |
Removes direct message interpolation from shell source. |
.changeset/compile-default-ast-lint.md |
Announces the breaking default and migration guidance. |
Review effort: Max
|
@copilot address the following outstanding work in one pass:
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: b55dccd
|
…lt lint 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>
Pushed 93a1770 and bfd4670 and replied once to each of the eight listed review threads. Focused regressions, impacted Go tests, the five haiku tests, standard Go lint, and schema checks passed. The required gate still fails on the previously reported custom-linter findings in unchanged code; the newly introduced function-length finding was fixed. Supplemental code review found no high-confidence issues. Automated review was unavailable, and CodeQL skipped the oversized database. No CI was triggered. Thread resolution needs a maintainer: this environment can post replies but exposes no resolve-thread operation, and shell GitHub access is unauthenticated. Final HEAD is bfd4670; CI on that revision remains unverified and needs a maintainer re-trigger. |
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Exercise supported workflow shapes under normal, no-emit, and dry-run compilation and preserve malformed-input failures. Finalize ADR-71349 and document the per-compilation migration escape hatch. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
Regarding #71349 (comment): acknowledged as an informational review completion, not a separate code-change request. The two actionable inline findings from this review were evaluated and addressed individually. |
|
Regarding #71349 (comment): no code change is warranted by this informational reported_no_action result. Validation for this batch is based on executed focused regressions, not the sentinel's completion status. |
|
Regarding #71349 (comment): no code change is warranted by this informational reported_no_action result. It is not being treated as evidence that validation or publication succeeded. |
|
Regarding #71349 (comment): acknowledged as an informational review completion with no additional actionable finding. The separate correctness findings were handled through their individual inline threads. |
|
Regarding #71349 (comment): accepted the generated ADR as a starting point and reviewed it against the implementation. The revised ADR records the compilation contract and its coverage and migration trade-offs, and is linked from the PR description. |
|
Regarding #71349 (comment): accepted and completed the ADR's rationale, alternatives, consequences, and status metadata, and linked ADR-71349 from the PR body. It explicitly records limited JavaScript coverage and the compile-only opt-out. |
|
Regarding #71349 (comment): accepted the aggregate request and integrated latest main with make merge-main, checked the eight inline findings, and pushed the combined batch in 46a2d63. Each inline thread has an individual disposition; no workflow dispatch or manual CI trigger was performed. |
|
Regarding #71349 (comment): the earlier fixes and validation report were independently checked rather than treated as proof of completion. This batch supplements the remaining regression and ADR gaps and verifies publication; the required repository gate still reports pre-existing custom-linter findings in unchanged code. |
|
🎉 This pull request is included in a new release. Release: |
Summary
Run built-in Actions/AW AST linting by default during CLI compilation, before emitting lock files. This closes a validation gap exposed by shell injection in the downstream
haiku-printerworkflow, which is also fixed here.--no-emitand recompilation throughadd,update, andvalidate. Lint errors fail compilation; ordinary warnings remain advisory.gh aw compile --skip-lintand MCPskip_lint. These disable only this lint pass, retain existing compiler validation, and are rejected with--dry-run.--allow-action-refswithout accepting malformed references or mutating retained source.imports.aw, and generatedcache-mode, with regressions for normal, no-emit, and dry-run compilation.workflow.WithLint(true).printfformat rather than interpolating them into Bash.Compatibility and coverage
Breaking default: Workflows with lint errors can no longer compile by default. Run
gh aw compile --no-emit, fix findings in the authored source, then rerun the original command.--skip-lintis a per-compilation escape hatch available only throughcompile, not a persistent bypass for other recompiling commands.Compile-time linting does not require a native JavaScript parser. Deeper authored JavaScript driver and custom-grader analysis requires standalone
gh aw lint --javascript-module <module-path>. Secret-leak coverage warnings and unavailable-parser grader-file-io warnings are excluded from compiler warning totals; analysis errors and actual findings remain blocking according to severity. Native Actions YAML and downstream dispatch targets still require standalone linting.ADR: ADR-71349: Run Built-in Actions/AW AST Lint During Compilation by Default
Validation
9084a8c765usingmake merge-main. Build, regeneration, and formatting passed; all 348 workflows compiled without generated lock-file changes.make agent-report-progressreports pre-existing custom Go-linter findings in unchanged code incmd/gh-aw/main.go,pkg/cli/compile_compiler_setup.go, andpkg/cli/compile_development.go.All eight requested inline review threads have been answered and resolved. Latest published commit:
46a2d63098.The full hosted
smoke-copilotrun remains unperformed: its downstream dispatch targets the default branch, so the receiver fix must land there first. No workflow was manually dispatched or CI manually triggered.