fix(labeling): harden labeler rule matching and add tests#430
Conversation
|
Caution Review failedPull request was closed or merged during review Note
|
| Layer / File(s) | Summary |
|---|---|
Import compatibility fix scripts/agents/includes/labeler-utils.js |
@actions/core is now imported as a namespace (import * as core); minimatch is resolved via conditional check supporting both function and object exports instead of destructuring. |
Comprehensive test coverage for labeler functions scripts/agents/includes/__tests__/labeler-utils.test.js |
Jest suite validates matchesBranchPattern with regex and glob patterns, matchesFilePatterns across three rule modes (any-glob-to-any-file, all-globs-to-all-files, any-glob-to-all-files), and determineLabelsFromRules with branch and file-based label application and deduplication. |
🎯 2 (Simple) | ⏱️ ~10 minutes
🚥 Pre-merge checks | ✅ 4 | ❌ 1
❌ Failed checks (1 warning)
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Description check | The PR description provides summary and validation steps, but lacks required sections from the template including linked issues, changelog, risk assessment, testing instructions, and checklist items. | Add missing required sections: link related issues, provide changelog entry per Keep a Changelog format, complete risk assessment with impact and mitigation, include testing instructions with prerequisites and steps, and verify checklist items. |
✅ Passed checks (4 passed)
| Check name | Status | Explanation |
|---|---|---|
| Title check | ✅ Passed | The title directly and clearly describes the main changes: hardening labeler imports for compatibility and adding comprehensive test coverage for labeler utilities. |
| Docstring Coverage | ✅ Passed | No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. |
| Linked Issues check | ✅ Passed | Check skipped because no linked issues were found for this pull request. |
| Out of Scope Changes check | ✅ Passed | Check skipped because no linked issues were found for this pull request. |
✏️ Tip: You can configure your own custom pre-merge checks in the settings.
✨ Finishing Touches
📝 Generate docstrings
- Create stacked PR
- Commit on current branch
🧪 Generate unit tests (beta)
- Create PR with unit tests
- Commit unit tests in branch
codex/issue-419-labeler-hardening
Warning
Review ran into problems
🔥 Problems
Stopped waiting for pipeline failures after 30000ms. One of your pipelines takes longer than our 30000ms fetch window to run, so review may not consider pipeline-failure results for inline comments if any failures occurred after the fetch window. Increase the timeout if you want to wait longer or run a @coderabbit review after the pipeline has finished.
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands and usage tips.
🔍 Reviewer Summary for PR #430CI Status: ✅ Recommendations
|
There was a problem hiding this comment.
Code Review
This pull request introduces a new test suite for the labeler utilities and updates module imports and package resolution in labeler-utils.js. The review feedback identifies two key issues: first, the test file incorrectly uses CommonJS require to load an ES module, which will fail at runtime; second, there is a logical bug in the all-globs-to-all-files test case and its underlying implementation that needs to be corrected to ensure proper glob matching behavior.
| const { | ||
| matchesBranchPattern, | ||
| matchesFilePatterns, | ||
| determineLabelsFromRules, | ||
| } = require("../labeler-utils.js"); |
There was a problem hiding this comment.
Since labeler-utils.js is an ES module (using import/export syntax), using CommonJS require to load it will fail at runtime in standard Node.js environments with ERR_REQUIRE_ESM. To ensure compatibility and consistency with the rest of the codebase, use ES module import syntax instead.
| const { | |
| matchesBranchPattern, | |
| matchesFilePatterns, | |
| determineLabelsFromRules, | |
| } = require("../labeler-utils.js"); | |
| import { | |
| matchesBranchPattern, | |
| matchesFilePatterns, | |
| determineLabelsFromRules, | |
| } from "../labeler-utils.js"; |
| test("supports all-globs-to-all-files", () => { | ||
| const config = { | ||
| "all-globs-to-all-files": [".github/workflows/**", "docs/**"], | ||
| }; | ||
| expect(matchesFilePatterns(changedFiles, config)).toBe(true); | ||
| }); |
There was a problem hiding this comment.
The test for all-globs-to-all-files expects true even though changedFiles contains scripts/agents/labeling.agent.js, which does not match either of the configured glob patterns (.github/workflows/** or docs/**). This test only passes because the underlying implementation of all-globs-to-all-files in labeler-utils.js (line 110) incorrectly uses changedFiles.some instead of changedFiles.every. In a standard GitHub Actions labeler, all-globs-to-all-files requires that all changed files match the glob patterns. To fix this, the test should be updated to use a set of files and patterns where all files actually match the patterns, and the underlying implementation in labeler-utils.js should be corrected to use every instead of some.
| test("supports all-globs-to-all-files", () => { | |
| const config = { | |
| "all-globs-to-all-files": [".github/workflows/**", "docs/**"], | |
| }; | |
| expect(matchesFilePatterns(changedFiles, config)).toBe(true); | |
| }); | |
| test("supports all-globs-to-all-files", () => { | |
| const config = { | |
| "all-globs-to-all-files": ["docs/**"], | |
| }; | |
| const docsOnlyFiles = ["docs/ISSUE_LABELS.md", "docs/ISSUE_TYPES.md"]; | |
| expect(matchesFilePatterns(docsOnlyFiles, config)).toBe(true); | |
| }); |
Summary
labeler-utilsimport compatibility for@actions/coreandminimatchexport shapesValidation
node scripts/validation/validate-labeling-configs.cjsnpm run test:js -- scripts/agents/__tests__/labeling.agent.test.js scripts/agents/includes/__tests__/labeler-utils.test.jsScope
Summary by CodeRabbit
Tests
Chores