Repository navigation
Use Actions and AW ASTs for precise scanner differentials - #70868
Conversation
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
🧠 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.
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. See the comment below for the result and any generated ADR draft. Commit pushed:
|
|
✅ 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 — ADR RequiredThis PR makes significant changes to core business logic (596 new lines in 📄 Draft ADR committed: The draft records the decision to stop using nearest-preceding raw YAML nodes and raw line text to identify scanner findings, and to use semantic Actions/AW AST anchors instead. Steps are matched by ID or unique name, otherwise by a content fingerprint. Unmapped or unparseable sources fall back to coordinates plus a file digest.
📋 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 MatterADRs 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 ReferenceAn ADR must contain these four sections to be considered complete:
All ADRs are stored in
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Verdict: COMMENT
No actionable blocking issues found in the changed lines. The review was static; repository code, tests, and build commands were not executed.
🔎 Code quality review by PR Code Quality Reviewer · copilot · gpt50 · 3.13 AIC · ⌖ 0.598 AIC · ⊞ 1.6K
Comment /review to run again
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /codebase-design and /diagnosing-bugs. The new scannerSource abstraction (pkg/cli/compile_scanner_source.go) is a well-designed, deep module — single load/locate/identity interface hiding substantial AST-walking complexity, with strong regression coverage for the hard cases (aliases, flow mappings, duplicate names, AW frontmatter offsets). Two issues worth addressing, left as inline comments.
📋 Key Themes & Highlights
Key Themes
- Caching defeated at one call site:
withActionlintFindingsinpkg/cli/actionlint.gouses the standalonescannerFindingIdentity/actionlintShellcheckIdentitywrappers per-issue, each allocating a freshscannerSourcesmap and re-parsing the file, unlikewithScannerFindingswhich shares one map across all findings. This undercuts the per-file caching this PR specifically introduces. - Ambiguity short-circuit in
locate: thecolumn <= 0conflict branch returnsnilas soon as it sees any two conflicting candidates, which could discard a later unambiguous match depending on iteration order. Worth a 3+-location test case to confirm intended semantics.
Positive Highlights
- ✅ Clear separation:
indexAW/indexActionsfeed a singleindexNodeAST walker, keeping YAML-vs-Actions-vs-AW concerns well isolated. - ✅ Excellent regression test matrix covering 15 realistic edge cases (reordered flow maps, YAML aliases, duplicate step names, AW post-steps, dotted job IDs) — this is textbook
/tddcoverage for a precision-sensitive matcher. - ✅ Conservative fallback (coordinate + file digest) when parsing fails, rather than guessing a match, matches the stated design goal.
- ✅ Docs updated to explain the new matching semantics for future readers.
@copilot please address the review comments above.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet50 · 72.3 AIC · ⌖ 15 AIC · ⊞ 10.2K
Comment /matt to run again
| return make(scannerSources).findingIdentity(finding) | ||
| } | ||
|
|
||
| func (sources scannerSources) findingIdentity(finding scanfindings.Finding) string { |
There was a problem hiding this comment.
[/codebase-design] Per-finding path reconstructs the file map from scratch: withActionlintFindings (pkg/cli/actionlint.go) calls scannerFindingIdentity and actionlintShellcheckIdentity separately for every issue, and each wrapper does make(scannerSources).findingIdentity(...), discarding the parsed/cached scannerSource after a single lookup. For a lock file with many findings this re-reads and re-parses the YAML/AW AST once per finding instead of once per file.
💡 Suggested fix
Plumb a single scannerSources map through the withActionlintFindings loop (as withScannerFindings already does) instead of relying on the standalone wrapper functions:
sources := make(scannerSources)
for _, issue := range native {
...
identity := sources.findingIdentity(finding)
if issue.Kind == "shellcheck" {
if stableIdentity, ok := sources.shellcheckIdentity(issue); ok {
identity = stableIdentity
}
}
}This keeps the new per-file caching (sources.load) effective across the whole actionlint result set, which is exactly the optimization the PR introduces elsewhere.
@copilot please address this.
There was a problem hiding this comment.
withActionlintFindings now reuses one scannerSources map across the full actionlint result, retaining parsed source per file. Fixed in 9afe60d.
| if line == location.line && column > 0 && (column < location.column || line == location.endLine && column >= location.endColumn) { | ||
| continue | ||
| } | ||
| if found != nil && column <= 0 && found.path != location.path { |
There was a problem hiding this comment.
[/diagnosing-bugs] Ambiguity handling in locate looks fragile: when column <= 0 and two overlapping locations disagree on path, it returns nil for the whole call, discarding found even if a later, unambiguous location would have matched. Loop order (map iteration order of source.locations, which is a slice so it is deterministic, but the set of locations considered “found so far” depends on where in the loop the conflict is detected) makes the early-return behavior order-sensitive rather than based on the full candidate set.
💡 Why this matters
Consider three overlapping locations A, B, C at the same unresolved column where A and B conflict but C (visited later) is actually the correct unambiguous match — the current code bails out on the A/B conflict before ever considering C, even though the regression tests only exercise two-location conflicts (see TestScannerFindingIdentityAST/flow_mapping_columns_distinguish_fields). A test with 3+ overlapping scalars at column 0 would help pin down the intended semantics here (should ambiguity be “any conflicting pair” or “the final candidate set still conflicts”?).
@copilot please address this.
There was a problem hiding this comment.
locate now evaluates the full set of best-ranked candidates before deciding whether their paths are ambiguous; added regressions for a later specific candidate and a genuinely tied conflict. Fixed in 9afe60d.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🟡 Changes recommended
Identity-matching errors can suppress new scanner findings or incorrectly fail the differential gate.
5 open issues
What changed in this PR
Updates gh-aw’s scanner differentials to match semantic workflow content rather than raw YAML positions.
Changes:
- Add Actions/AW source anchors and logical step identities.
- Introduce source snapshots, conservative fallbacks, and stricter ShellCheck coordinate handling.
- Add regression coverage and document matching behavior.
| File | Description |
|---|---|
pkg/cli/compile_scanner_source.go |
Adds semantic source indexing and identity generation. |
pkg/cli/compile_scanner_differential.go |
Integrates source-aware matching and ShellCheck handling. |
pkg/cli/compile_scanner_differential_test.go |
Adds identity and fallback regression cases. |
docs/src/content/docs/reference/compilation-process.md |
Documents differential matching behavior. |
Review effort: Max
| if !found { | ||
| return "", false | ||
| } | ||
| message := issue.Message |
There was a problem hiding this comment.
The known actionlint ShellCheck prefix is stripped before parsing and identity construction. Added a prefixed-message regression through withActionlintFindings that preserves identity across a comment insertion. Fixed in 9afe60d.
| return source.indexNode(value.Value, child, field) | ||
| case *ast.SequenceNode: | ||
| for index, child := range value.Values { | ||
| item, err := scannerSourceItem(semantic, index) |
There was a problem hiding this comment.
The source index now validates authored needs order against lowered values and falls back to coordinate/file-digest identity when they differ. Added an unsorted-needs regression. Fixed in 9afe60d.
| } | ||
| location := scannerSourceLocation{ | ||
| path: path, value: value, digest: scannerValueDigest(string(content)), line: tk.Position.Line, column: tk.Position.Column, | ||
| endLine: tk.Position.Line, endColumn: tk.Position.Column + len([]rune(tk.Value)), |
There was a problem hiding this comment.
Scalar endpoints now use the trimmed authored token extent, including quotes. Regressions cover one-character and empty quoted strings, their characters, and exclusive endpoints. Fixed in 9afe60d.
| location.endLine = index + 1 | ||
| } | ||
| } else if !key { | ||
| location.endLine += strings.Count(strings.TrimRight(tk.Origin, "\r\n"), "\n") |
There was a problem hiding this comment.
The scalar source origin is trimmed before calculating its line span; a following indented comment is no longer attributed to the preceding scalar. Fixed in 9afe60d.
| if line < location.line || line > location.endLine { | ||
| continue | ||
| } | ||
| if line == location.line && column > 0 && (column < location.column || line == location.endLine && column >= location.endColumn) { |
There was a problem hiding this comment.
Poutine and runner-guard now preserve unknown columns as zero; native adapter regressions verify it. Fixed in 9afe60d.
Preserve fail-closed scanner gating alongside AST identity and partial-failure hardening. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
@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: ab8814f
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
|
🎉 This pull request is included in a new release. Release: |
The differential scanner previously used the nearest preceding YAML node and raw line text. This could misattribute findings, conflate distinct steps, or report harmless formatting changes as new findings. Adversarial analysis also reproduced source/sink identity collisions and fail-open handling of partial scanner results.
Summary
Main integration
Merged
origin/mainat27cc912e2d, preserving its stricter policy: existing and added medium-or-higher or unclassified findings fail the gate. Baseline matching only classifies findings; it does not waive them. Also preserved the concurrently added draft ADR and aligned it with the final behavior.Validation
Passed:
make buildandmake fmtgo test ./pkg/cli -run '^(TestDryRunZizmor|TestDryRunScanner|TestScanner|TestActionlintShellcheckIdentity|TestZizmor|TestParseAndDisplayZizmor|TestDevelopmentCompileScanner|TestCollectDryRunBaseline)' -count=1go test ./pkg/scanfindings ./pkg/workflow/actions ./pkg/workflow/aw -count=1Added=1, Existing=0.Validation limitations:
timepackage. There is no passing full-lint claim for the final revision.go test ./pkg/workflow -run 'DryRun|Development' -count=1fails inTestDryRunCompiledCacheModeandTestRepositoryWorkflowsCompileDryRun: incoming main's fail-closed policy rejects pre-steps/custom steps these tests expect to compile. The workflow sources and tests are unchanged fromorigin/main; this PR does not relax that policy.