Lock in tag-scoped on.push as a valid scoped trigger#45362
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
on.push as a valid scoped trigger
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR does not have the 'implementation' label and has ≤100 new lines of code in business logic directories (48 additions detected across 4 files). |
|
✅ Test Quality Sentinel completed test quality analysis. |
There was a problem hiding this comment.
Pull request overview
This PR fixes cross-repo scope auditing/validation for GitHub Actions on.push by treating tag-scoped push triggers (on.push.tags / on.push.tags-ignore) as valid “scoped” triggers (not “unscoped”), and updates related logs/tests to prevent regressions.
Changes:
- Expand push-scope validation to consider
tags/tags-ignoreas valid ref scoping (in addition tobranches/branches-ignore). - Update orchestrator/validator log wording to consistently describe “branch/tag” scoping.
- Add regression tests covering tag-scoped push filters (unit-level and compile-level).
Show a summary per file
| File | Description |
|---|---|
| pkg/workflow/push_branch_scope_test.go | Adds compile-level coverage that tag-scoped push workflows compile cleanly (no unscoped warning/error). |
| pkg/workflow/compiler_orchestrator_frontmatter.go | Updates strict/non-strict logging to reflect branch-or-tag scoping. |
| pkg/workflow/compiler_filters_validation.go | Updates push-scope validation messaging and accepts tag ref filters as scoping. |
| pkg/workflow/compiler_filters_validation_test.go | Adds unit tests ensuring tags and tags-ignore are treated as scoped push filters. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 4/4 changed files
- Comments generated: 2
- Review effort level: Low
| // ValidatePushBranchScope ensures that any push event in the on: section specifies a | ||
| // branches or branches-ignore filter. An unscoped push trigger fires on every push to | ||
| // every branch, which causes unintended workflow fan-out on feature branches (the | ||
| // workflows activate immediately after new lock files are first pushed to the branch, | ||
| // producing zero-turn failures for every agentic workflow in the repository). | ||
| // branch or tag ref filter. An unscoped push trigger fires on every push to every | ||
| // branch, which causes unintended workflow fan-out on feature branches (the workflows | ||
| // activate immediately after new lock files are first pushed to the branch, producing | ||
| // zero-turn failures for every agentic workflow in the repository). |
There was a problem hiding this comment.
Updated in c975eda: the ValidatePushBranchScope doc comment now states that unscoped push fires for both branches and tags.
| "push (no branch or tag filter)", | ||
| "push event must specify a 'branches', 'branches-ignore', 'tags', or 'tags-ignore' filter; an unscoped push trigger fires on every push to every branch and causes unintended workflow fan-out on feature branches", | ||
| "Add a branch or tag filter to the push trigger:\n\non:\n push:\n branches:\n - main", |
There was a problem hiding this comment.
Updated in c975eda: the validation error message now says unscoped push applies to every branch and tag, not branches only.
There was a problem hiding this comment.
Review: Lock in tag-scoped on.push as a valid scoped trigger
The fix is correct and complete.
What was wrong: The validation logic already accepted tags/tags-ignore filters in the map lookup, but the surrounding comments, log messages, and error messages all said "branch" only — creating a confusing mismatch between behaviour and documentation.
What changed:
- Comment, log, and error message wording updated to say "branch or tag" throughout
- Two new unit test cases added for
tagsandtags-ignorefilters - One compile-level test added confirming tag-scoped workflows pass without warnings
No logic bugs, no security concerns, no regressions. The changes are minimal, focused, and well-tested.
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
proxy.golang.org
To allow these domains, add them to the
network.allowedlist in your workflow frontmatter:
network:
allowed:
- defaults
- "proxy.golang.org"See Network Configuration for more information.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 16.4 AIC · ⌖ 4.96 AIC · ⊞ 4.8K
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /codebase-design — changes are accurate and well-tested; approving with one minor suggestion.
📋 Key Themes & Highlights
Key Themes
- Correct scope fix:
tagsandtags-ignoreare now recognised as valid ref-scope filters. The logic at lines 126-134 ofcompiler_filters_validation.gowas already handling tags correctly; this PR aligns all messaging and documentation to match. - Good regression coverage: both unit table-driven cases and the compile-level integration case are included.
One Minor Suggestion
- The remediation hint in
newUnScopedPushErrorstill shows only abranchesexample (see inline comment). Adding a shorttagsexample would make the hint consistent with the expanded accepted scope.
Positive Highlights
- ✅ Minimal, surgical diff — only wording changes, no logic changes
- ✅ Test names read as specifications
- ✅ PR description clearly explains the motivation
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 19.2 AIC · ⌖ 4.44 AIC · ⊞ 6.6K
Comment /matt to run again
| "push (no branch filter)", | ||
| "push (no branch or tag filter)", | ||
| "push event must specify a 'branches', 'branches-ignore', 'tags', or 'tags-ignore' filter; an unscoped push trigger fires on every push to every branch and causes unintended workflow fan-out on feature branches", | ||
| "Add a branch or tag filter to the push trigger:\n\non:\n push:\n branches:\n - main", |
There was a problem hiding this comment.
[/codebase-design] The hint example only shows a branches filter, but now that tags and tags-ignore are first-class accepted filters, users hitting this error for a tag-only workflow won't see the idiomatic pattern.
💡 Suggested addition
"Add a branch or tag filter to the push trigger:\n\non:\n push:\n branches:\n - main\n\n# or for tag-based releases:\n\non:\n push:\n tags:\n - 'v*.*.*'\n",A short tags example alongside the branches example keeps the hint aligned with the now-expanded accepted scope.
@copilot please address this.
There was a problem hiding this comment.
Updated in c975eda: the remediation hint now includes both a branch-scoped example and a tag-scoped release example (on.push.tags).
There was a problem hiding this comment.
The functional fix is correct — adding tags and tags-ignore to the scope check (lines 126–134) is the right change, and the tests cover the new cases well.
The two existing review comments capture the only remaining issue: the newUnScopedPushError() message still describes the problem in branch-only terms, which will confuse users whose tag-scoped push is flagged as unscoped. Once that message is updated to mention both branches and tags, this PR is ready.
🔎 Code quality review by PR Code Quality Reviewer · 22.8 AIC · ⌖ 4.29 AIC · ⊞ 5.4K
Comment /review to run again
🧪 Test Quality Sentinel Report✅ Test Quality Score: 80/100 — Acceptable
📊 Metrics (3 tests)
|
|
@copilot run pr-finisher skill |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Ran pr-finisher pass and pushed c975eda. Addressed all in-scope review feedback with replies, ran local validation ( |
Tag-only push triggers were being reported as unscoped in cross-repo compatibility auditing, even though
on.push.tagsis a valid ref-scoped trigger and should not require branch filters. This surfaced against first-party release workflows and exposed a gap between the scope validator and the existing filter validation logic.Scope validation
Regression coverage
on.push.tagson.push.tags-ignoreWhy this matters