Comfy-Org / Comfy-Org/github-workflows

[groom] reviewers.yml is parsed by two independent implementations kept in parity by comment only

Open
#270 0 comments 0 reactions 0 assignees View on GitHub
groom
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

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.