Add compile-time glob validation for workflow trigger filter patterns#22873
Add compile-time glob validation for workflow trigger filter patterns#22873
Conversation
- Add glob_validation.go with self-contained ValidateRefGlob/ValidatePathGlob logic adapted from github.com/rhysd/actionlint (MIT License, stdlib-only deps) - Add ValidateGlobPatterns() to compiler_filters_validation.go - Wire ValidateGlobPatterns into compiler_orchestrator_frontmatter.go - Add 18-case TestValidateGlobPatterns table-driven test Catches invalid branch/tag/path globs at compile time, including the notable ./ prefix edge case that actionlint v1.7.11 checks. Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> Agent-Logs-Url: https://github.com/github/gh-aw/sessions/75dde239-a5d4-4e93-bbd9-d24252279a4e
|
@copilot add more tests |
There was a problem hiding this comment.
Pull request overview
Adds compile-time validation for GitHub Actions workflow trigger filter glob patterns so invalid branch/tag/path filters are caught during frontmatter compilation (instead of only at lint time).
Changes:
- Introduces a stdlib-only glob validator (adapted from actionlint) for ref and path patterns.
- Adds
ValidateGlobPatternsto validate branches/tags/paths filters across supported trigger events. - Wires glob validation into the frontmatter compilation pipeline and adds table-driven tests.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| pkg/workflow/glob_validation.go | New glob validation implementation for ref and path filter patterns (including rejecting ./ / ../ path prefixes). |
| pkg/workflow/compiler_filters_validation.go | Adds ValidateGlobPatterns and supporting helpers to validate filter glob lists in the on: section. |
| pkg/workflow/compiler_orchestrator_frontmatter.go | Hooks glob validation into the existing frontmatter validation pipeline. |
| pkg/workflow/compiler_filters_validation_test.go | Adds table-driven tests covering valid/invalid ref/path patterns and list typing. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
pkg/workflow/glob_validation.go
Outdated
| case '[', '?', '*': | ||
| c = v.scan.Next() | ||
| if v.isRef { | ||
| v.invalidRefChar(v.scan.Peek(), "ref name cannot contain spaces, ~, ^, :, [, ?, *") |
There was a problem hiding this comment.
In the \\ escape handling for ref patterns, the invalid character reported is taken from v.scan.Peek() after consuming the escaped rune. This points at the next character (or EOF), producing incorrect/misleading diagnostics (and potentially odd output when at end of string). Use the consumed escaped rune (c) when calling invalidRefChar so the error reports the actual invalid character being escaped.
| v.invalidRefChar(v.scan.Peek(), "ref name cannot contain spaces, ~, ^, :, [, ?, *") | |
| v.invalidRefChar(c, "ref name cannot contain spaces, ~, ^, :, [, ?, *") |
pkg/workflow/glob_validation.go
Outdated
| // Reject '.', '..', './<path>', and '../<path>' (#521 in actionlint) | ||
| stripped := strings.TrimPrefix(p, "!") | ||
| if stripped == "." || stripped == ".." || strings.HasPrefix(stripped, "./") || strings.HasPrefix(stripped, "../") { | ||
| errs = append(errs, invalidGlobPattern{"'.' and '..' are not allowed in glob path", 0}) |
There was a problem hiding this comment.
validatePathGlob rejects patterns starting with ./ and ../, but the error message says only "'.' and '..' are not allowed". This is likely to confuse users hitting the ./src/** case called out in the PR description. Consider updating the message to explicitly mention that ./ and ../ prefixes (and ./.. themselves) are not allowed.
| errs = append(errs, invalidGlobPattern{"'.' and '..' are not allowed in glob path", 0}) | |
| errs = append(errs, invalidGlobPattern{"'.', '..', and paths starting with './' or '../' are not allowed in glob path", 0}) |
pkg/workflow/glob_validation.go
Outdated
| Column int | ||
| } | ||
|
|
||
| func (e *invalidGlobPattern) Error() string { |
There was a problem hiding this comment.
invalidGlobPattern is stored and returned as a value type ([]invalidGlobPattern), but Error() is defined on *invalidGlobPattern. If the intent is for invalidGlobPattern itself to satisfy the error interface, consider changing Error() to a value receiver (or otherwise clarify/remove the method) to avoid surprising behavior when used without taking an address.
| func (e *invalidGlobPattern) Error() string { | |
| func (e invalidGlobPattern) Error() string { |
|
/cloclo apply comments |
|
🎤 Bravo! /cloclo has delivered a stunning performance! Standing ovation! 🌟 |
…keys, and low-level helpers - TestValidateGlobPatternsExtendedEvents: pull_request_target, workflow_run, branches-ignore, tags-ignore, second invalid pattern in list, non-map event value - TestValidateRefGlob: 17 cases for the low-level validateRefGlob helper - TestValidatePathGlob: 16 cases for the low-level validatePathGlob helper - TestToStringSlice: 8 cases for the toStringSlice conversion helper Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> Agent-Logs-Url: https://github.com/github/gh-aw/sessions/8a412c5c-7f0e-441d-a835-0947a3066501
Added in ad1f719. The new tests cover:
|
|
@copilot apply comments |
- Use value receiver on invalidGlobPattern.Error() so the type satisfies the error interface when used as a value (not just pointer) - Fix invalidRefChar call in escape handling to use the consumed rune c instead of v.scan.Peek() (which pointed at the next character) - Improve error message for ./ and ../ path prefix rejection to explicitly mention both the bare dot forms and the prefix forms Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> Agent-Logs-Url: https://github.com/github/gh-aw/sessions/49256cc9-5d91-4606-b170-0f1fdb6ed8fd
Applied all three in 383025d:
|
…idation - cli.md: expand gh aw audit description with the new report sections added in #22932 (engine config, prompt analysis, session & agent performance, MCP server health, safe output summary) - github-tools.md: document toolsets shorthand values (default, all) and add a note that toolsets: [all] excludes dependabot (#22851) - triggers.md: document compile-time glob pattern validation for push/ pull_request/pull_request_target/workflow_run filter fields (#22873) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…idation (#22972) - cli.md: expand gh aw audit description with the new report sections added in #22932 (engine config, prompt analysis, session & agent performance, MCP server health, safe output summary) - github-tools.md: document toolsets shorthand values (default, all) and add a note that toolsets: [all] excludes dependabot (#22851) - triggers.md: document compile-time glob pattern validation for push/ pull_request/pull_request_target/workflow_run filter fields (#22873) Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
Workflow trigger filter patterns (branches, tags, paths) were only validated by Docker-based actionlint at lint time. Invalid globs — including the always-invalid
./src/**path prefix — could silently produce broken.lock.ymlfiles.Changes
pkg/workflow/glob_validation.go(new)Self-contained glob validator adapted from
actionlintglob.go (MIT License, stdlib-only). The fullactionlintpackage cannot be imported directly due to a transitivego.yaml.in/yaml/v4rc.3→rc.4 API incompatibility.validateRefGlob(pat)— Git ref pattern validation (branches, tags)validatePathGlob(pat)— path pattern validation; explicitly rejects./-prefixed patternspkg/workflow/compiler_filters_validation.goValidateGlobPatterns(frontmatter)— iterates overpush,pull_request,pull_request_target,workflow_runevents and validates all branch/tag/path filter patternspkg/workflow/compiler_orchestrator_frontmatter.goValidateGlobPatternsin the frontmatter pipeline, immediately after the existingValidateEventFilterscheckpkg/workflow/compiler_filters_validation_test.goTestValidateGlobPatterns: 18 table-driven cases covering valid patterns, invalid ref/path globs, the./prefix edge case, and[]anylist inputsTestValidateGlobPatternsExtendedEvents: additional cases forpull_request_targetandworkflow_runevents, all filter keys (branches-ignore,tags-ignore), second-invalid-pattern-in-list detection, and non-map event value handlingTestValidateRefGlob: 17 direct cases for the low-level ref glob helper (space, tilde, caret, colon, slash prefix/suffix, trailing dot, empty/unclosed brackets, bare!)TestValidatePathGlob: 16 direct cases for the path glob helper (./and../prefixes, bare./.., whitespace, empty string, unclosed brackets)TestToStringSlice: 8 cases for the conversion helper covering all supported input types and error pathsExample — caught at compile time:
💬 Send tasks to Copilot coding agent from Slack and Teams to turn conversations into code. Copilot posts an update in your thread when it's finished.