[GH-ISSUE #270] [groom] reviewers.yml is parsed by two independent implementations kept in parity by comment only #8

New issue

Closed

opened 2026-10-26 22:34:28 +00:00 by bsakaguchi · 1 comment

bsakaguchi commented

2026-10-26 22:34:28 +00:00

Owner

Originally created by @cloud-code-bot[bot] on GitHub (Sep 8, 2026).
Original GitHub issue: https://github.com/Comfy-Org/github-workflows/issues/270

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

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.

Originally created by @cloud-code-bot[bot] on GitHub (Sep 8, 2026). Original GitHub issue: https://github.com/Comfy-Org/github-workflows/issues/270 **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.

bsakaguchi

2026-10-26 22:34:28 +00:00

closed this issue
added the
groom
label

bsakaguchi commented

2026-10-26 22:34:30 +00:00

Author

Owner

@mattmillerai commented on GitHub (Sep 18, 2026):

Triage: closing as a duplicate of #286. Not fixed, not stale — the defect is real and unchanged; it is simply tracked in the other issue.

Verified against origin/main @ cf7b8d9c2614ad9f8ee41acf245381ad76fcdc8b. Every claim here still holds, with the JS line numbers now stale (the workflow grew via #283): globToRegExp is at assign-reviewers.yml:304 (body says :361) and parseReviewerConfig at :424 (body says :386). The Python citations are still exact. The unquote/_unquote divergence you documented is live: assign-reviewers.yml:436-440 has no length guard, generate.py:152-156 requires len(s) >= 2.

Why #286 rather than this one, given this issue is the earlier of the two: it states the same defect — same two ported function pairs, same parity-by-comment root cause, same two suites with no shared corpus, same unquote divergence, same "keep both implementations" conclusion — and adds three things this one lacks:

A second divergence: /\r?\n/ vs "\n" line splitting.
A whole second scope item: re-anchoring the fragile assignment.test.cjs extraction on a sentinel comment instead of on indentation plus an unrelated downstream step name.
A stronger argument for not unifying: parse_reviewer_config returns a (config, locations) pair the JS has no equivalent for, so they genuinely are not the same function.

Its citations are also current, where this issue's JS ones have drifted.

The two were filed six days apart by the same sweep and did not self-dedupe because the signature keys on the anchor file — generate.py here, assignment.test.cjs there.

Your requirement that #286 does not have has been carried forward, not dropped: "Path-filter it to both surfaces so an edit to either side runs it." Confirmed necessary — test-assign-reviewers.yml and test-refresh-reviewers.yml watch disjoint paths today, so a corpus added under one directory would only ever run on the surface you edited. That is now part of the tracked work.

Follow along on #286. Reopen this if you think it carries something #286 genuinely misses.

Created by Claude Code

@mattmillerai commented on GitHub (Sep 18, 2026): **Triage: closing as a duplicate of #286.** Not fixed, not stale — the defect is real and unchanged; it is simply tracked in the other issue. Verified against `origin/main` @ `cf7b8d9c2614ad9f8ee41acf245381ad76fcdc8b`. Every claim here still holds, with the JS line numbers now stale (the workflow grew via #283): `globToRegExp` is at `assign-reviewers.yml:304` (body says `:361`) and `parseReviewerConfig` at `:424` (body says `:386`). The Python citations are still exact. The `unquote`/`_unquote` divergence you documented is live: `assign-reviewers.yml:436-440` has no length guard, `generate.py:152-156` requires `len(s) >= 2`. **Why #286 rather than this one**, given this issue is the earlier of the two: it states the same defect — same two ported function pairs, same parity-by-comment root cause, same two suites with no shared corpus, same `unquote` divergence, same "keep both implementations" conclusion — and adds three things this one lacks: 1. A second divergence: `/\r?\n/` vs `"\n"` line splitting. 2. A whole second scope item: re-anchoring the fragile `assignment.test.cjs` extraction on a sentinel comment instead of on indentation plus an unrelated downstream step name. 3. A stronger argument for not unifying: `parse_reviewer_config` returns a `(config, locations)` pair the JS has no equivalent for, so they genuinely are not the same function. Its citations are also current, where this issue's JS ones have drifted. The two were filed six days apart by the same sweep and did not self-dedupe because the signature keys on the anchor file — `generate.py` here, `assignment.test.cjs` there. **Your requirement that #286 does not have has been carried forward**, not dropped: *"Path-filter it to both surfaces so an edit to either side runs it."* Confirmed necessary — `test-assign-reviewers.yml` and `test-refresh-reviewers.yml` watch disjoint paths today, so a corpus added under one directory would only ever run on the surface you edited. That is now part of the tracked work. Follow along on #286. Reopen this if you think it carries something #286 genuinely misses. *Created by Claude Code*

bsakaguchi referenced this issue

2026-10-26 22:35:26 +00:00

[PR #8] [MERGED] feat(cursor-review): auto-label on PR open/ready, not just assignment #17

bsakaguchi referenced this issue

2026-10-26 22:38:18 +00:00

[PR #306] [MERGED] test(workflow-pins): table-driven declared-vs-documented input drift harness for all 16 reusables #307