fix(labeler): honor matchCondition: AND and support brace patterns (my pick — matches the commit, scoped, descriptive)#439
Merged
Conversation
github-actions
Bot
added
toc
github-actions
Bot
added
needs-group
processLabelRule previously computed `shouldApply = !foundNamespace`
unconditionally, so a `kind: label` rule with `matchCondition: AND`
behaved identically to a `NOT` rule. Paired NOT/AND rules (e.g. apply
`needs-triage` when no `triage/*` exists, remove it when one does) ended
up firing in exactly the wrong situations: on a fresh PR with no labels
the labeler would add `needs-triage`/`needs-kind`/`needs-group` and
immediately remove them in the same run, and when a `triage/*` label was
later added manually via the UI the paired `needs-*` label would never
be cleared.
Also teach the label-rule `match` parser to understand a single level of
comma-separated brace alternation such as `{toc,tag/*,sub/*}`.
`filepath.Match` on its own does not support braces, so previously such
a pattern only matched a literal label whose name began with `{`.
Adds focused tests for both behaviors, including a regression test that
mirrors the cncf/toc#2164 scenario.
Co-authored-by: Cursor <cursoragent@cursor.com>
Signed-off-by: Riaan Kleinhans <riaankleinhans@gmail.com>
riaankleinhans
force-pushed
the
fix/labeler-honor-match-condition
branch
from
14b2827 to
33000fc
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes labeler rule evaluation in utilities/labeler to prevent contradictory add/remove label actions in the same run, and adds support for brace-alternation patterns used in .github/labels.yaml.
Changes:
- Fix
processLabelRuleto honormatchCondition: ANDby firing actions when the matched label namespace is present. - Add brace-alternation matching (
{a,b/*,c}) for label patterns viamatchAnyPattern+expandBraces. - Add regression/unit tests covering
matchCondition: ANDbehavior and brace pattern expansion/matching.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| utilities/labeler/labeler.go | Fixes AND semantics for label rules and adds brace pattern expansion/matching helper functions. |
| utilities/labeler/labeler_test.go | Adds regression tests for AND behavior and brace pattern matching, plus unit tests for brace expansion. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Previously `processLabelRule` discarded the error from `matchAnyPattern` (and from `filepath.Match` before that). A malformed pattern such as `triage/[` would silently leave `foundNamespace=false`, causing the rule to fire in the wrong direction. Return the error (with the rule name and offending pattern) so it is surfaced by `processRules`, matching the convention already used by `processFilePathRule`. Add a regression test that exercises a known-bad pattern. Addresses Copilot review feedback on PR #439. Co-authored-by: Cursor <cursoragent@cursor.com> Signed-off-by: Riaan Kleinhans <riaankleinhans@gmail.com>
Two follow-ups from Copilot review on PR #439. 1. matchAnyPattern now uses path.Match instead of filepath.Match. Labels are not filesystem paths; path.Match treats "/" as the separator on every platform so "*" cannot accidentally cross "/" on Windows (where filepath.Match uses "\" as the separator). The labeler runs in a Linux container so production behavior is unchanged, but tests now behave the same regardless of the developer's OS, and the choice matches semantic intent. 2. expandBraces now returns an error for patterns it cannot safely expand: more than one brace group (e.g. "foo-{a,b}-{c,d}"), nested braces (e.g. "{a,{b,c}}"), or unbalanced braces. Previously such inputs were silently partially expanded ("foo-a-{c,d}", ...) and path.Match treated the leftover "{...}" as literal characters, producing confusing non-matches with no diagnostic. The error propagates through matchAnyPattern → processLabelRule → processRules, so an operator sees the rule name and offending pattern in the log. Adds table coverage in TestExpandBraces for the new error paths and a TestLabeler_ProcessLabelRule_UnsupportedBracePattern regression test. Co-authored-by: Cursor <cursoragent@cursor.com> Signed-off-by: Riaan Kleinhans <riaankleinhans@gmail.com>
Two follow-ups from Copilot review on PR #439. 1. Lift pattern validation out of the existing-labels loop. processLabelRule previously called matchAnyPattern inside `for _, lbl := range existingLabels`, which meant a malformed `spec.match` was only surfaced when the issue already had at least one label. On a freshly-opened PR with zero labels the loop never ran, validation never happened, and the rule still fired based on foundNamespace=false — exactly the silent-mis-fire mode the previous fix was meant to close, just down a different path. Split the helper into compilePatterns (expand + validate, called once per rule) and matchAny (cheap per-label check). This also stops re-expanding braces for every label. 2. Fix a stale code-path reference in the InvalidPattern test comment that still mentioned filepath.Match; the helper now uses path.Match. Tests updated to use empty existing-labels slices so they cover the zero-label regression case directly. Co-authored-by: Cursor <cursoragent@cursor.com> Signed-off-by: Riaan Kleinhans <riaankleinhans@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes two bugs in
utilities/labelerthat together cause the symptom on cncf/toc#2164: the bot addsneeds-triage/needs-kind/needs-groupto a fresh PR and immediately removes them in the same run.matchCondition: ANDwas ignored.processLabelRulealways computedshouldApply := !foundNamespace, so anANDrule behaved identically to its pairedNOTrule — exactly inverted from intent. On a fresh PR, both rules fired: one addedneeds-*, the other removed it. Conversely, when atriage/*label was added via the UI, theANDrule never fired, soneeds-triagenever auto-cleared.Brace patterns like
{toc,tag/*,sub/*}never matched.filepath.Matchdoesn't understand brace alternation, so the cncf/tocneeds-grouprule's pattern only matched a literal label starting with{. Hidden today by bug #1; would surface asneeds-groupgetting permanently applied if #1 were fixed alone.Fix
matchCondition: ANDinprocessLabelRule.matchAnyPattern/expandBraceshelper so{a,b/*,c}-style patterns work.Tests
TestLabeler_ProcessLabelRule_RemoveWithMatchConditionAND— regression for the Add Cloud Native AI Scheduling Challenges Whitepaper toc#2164 scenario.TestLabeler_ProcessLabelRule_BracePattern— table test across each brace alternative.TestExpandBraces— unit test for the expander.go vetandgo test ./utilities/labeler/...both pass.