Comfy-Org / Comfy-Org/github-workflows
[groom] reviewers.yml is parsed by two independent implementations kept in parity by comment only
- Dominant language
- Shell
- Stars
- 6
- Forks
- 1
- Avg merge
- 14h 4m
- Merged PRs (30d)
- 98
Description
**Groom auto-builder** — Comfy-Org/github-workflows: this CONFIRMED finding could not be auto-built (patch modifies a CI-privileged or dataset-of-record path (per .github/groom/patch_policy.py — workflow/action defs, lockfiles, build/test config that executes in pre-review CI, graded eval cases under a suites/**/cases/ tree, or a symlink into such a tree) — a human must author these changes), so it is filed for a human. · [run](https://github.com/Comfy-Org/github-workflows/actions/runs/34210376864)
**Problem (verified).** `refresh-reviewers.yml` WRITES `.github/reviewers.yml` and `assign-reviewers.yml` READS it, and each ships its own hand-rolled parser for the same `default_pool` + `rules[{paths, reviewers}]` schema plus its own glob-to-regex translator. JS: `assign-reviewers.yml:361` (`globToRegExp`) and `:386` (`parseReviewerConfig`). Python: `generate.py:96` (`glob_to_regexp`, whose docstring says it 'Must stay byte-for-byte semantics-equal to the JS original - the map is only correct if it is scored with the same matcher the runtime assigns with') and `:172` (`parse_reviewer_config`, under the 'parity with parseReviewerConfig' banner at `:131`). Parity is asserted in prose only: `.github/assign-reviewers/tests/assignment.test.cjs:5-9` extracts and executes the inline JS, `.github/refresh-reviewers/tests/test_generate.py` drives the Python, and no shared corpus is fed through both.
**A divergence already exists.** The JS `unquote` (`assign-reviewers.yml:398-400`) tests `startsWith`/`endsWith` with no length guard, so a one-character token consisting of a single double-quote is both an opening and a closing quote and it returns the empty string; the Python `_unquote` (`generate.py:152-155`) requires `len(s) >= 2` and returns that character unchanged. Degenerate input, but it is live proof that the 'byte-for-byte equal' comment is unverified rather than checked.
**Honest scope - not a security finding.** `generate.py` is a drift DETECTOR that emits a rewritten config for a human-reviewed PR (see its header, `:36-40`), and assignment is advisory. A divergence would mis-score the committed expertise map or mis-route a review request; it grants no permission and gates no merge.
**Fix.** Keep both implementations - `assign-reviewers.yml` deliberately declares no `workflows_ref` (zero occurrences in the file) and loads nothing at run time, which is what lets a caller pin it by `uses:` alone. Add the missing parity test instead: one shared fixture corpus of `reviewers.yml` documents plus glob/path pairs covering the edge cases each parser documents (comment stripping inside quotes, flow vs block sequences, `*` / `**` / `**/` / `?`, and the degenerate quote above), run through the Python functions and through the inline JS the `.cjs` harness already extracts, asserting identical parse output and identical match verdicts. Path-filter it to both surfaces so an edit to either side runs it.
**Risk.** Low: a test, no runtime change. The cost is one CI job needing both `node` and `python3`, and choosing a corpus that covers the documented edge cases rather than only the happy path.
Contributor guide
Assessment
This issue has not been assessed yet.