-
Notifications
You must be signed in to change notification settings - Fork 424
Support glob patterns in allowed label filters for safe-outputs
#32027
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -192,6 +192,21 @@ describe("safe_output_validator.cjs", () => { | |
| expect(result.value).not.toContain("custom"); | ||
| }); | ||
|
|
||
| it("should filter labels based on allowed glob patterns", () => { | ||
| const result = validator.validateLabels( | ||
| ["team-backend", "priority-high", "area/ui", "bug"], | ||
| ["team-*", "priority-*", "area/*"], | ||
| 10 | ||
| ); | ||
|
|
||
| expect(result.valid).toBe(true); | ||
| expect(result.value).toHaveLength(3); | ||
| expect(result.value).toContain("team-backend"); | ||
| expect(result.value).toContain("priority-high"); | ||
| expect(result.value).toContain("area/ui"); | ||
| expect(result.value).not.toContain("bug"); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [/tdd] The new test covers the happy path well, but there is no test for the it("should reject labels matching blocked pattern even if they match allowed glob", () => {
const result = validator.validateLabels(
["team-backend", "team-[bot]"],
["team-*"], // allowed glob
10,
["*[bot]"] // blocked pattern
);
expect(result.valid).toBe(true);
expect(result.value).toContain("team-backend");
expect(result.value).not.toContain("team-[bot]");
}); |
||
| }); | ||
|
|
||
| it("should limit labels to max count", () => { | ||
| const result = validator.validateLabels(["a", "b", "c", "d", "e"], undefined, 3); | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[/tdd]
matchesSimpleGlobis case-insensitive by default (seeglob_pattern_helpers.cjs:caseSensitive = false), but the previousallowedLabels.includes(label)was case-sensitive. This is a silent behavior change: anallowedlist of["Bug"]will now accept the label"bug", which was previously rejected.This may be intentional to align with
blocked(which is also case-insensitive), but there is no test or documentation covering this. Consider adding a test:And documenting that
allowedpatterns are matched case-insensitively.