Comfy-Org / Comfy-Org/github-workflows
[groom] reviewers.yml has two hand-ported parsers whose parity is asserted by two independent suites and no shared corpus
- 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/34828869870)
## Problem
parseReviewerConfig (JS, inline at assign-reviewers.yml:424) and parse_reviewer_config (Python, generate.py:172, under a header at 131 reading "parity with parseReviewerConfig") are a line-by-line port: same comment-stripping quote-state machine, same unquote, same flow-sequence parser, same indent helper, same dash/list-key walk. globToRegExp is ported the same way (generate.py:96, docstring "Port of assign-reviewers.yml's globToRegExp"). Parity is asserted only by two independent hand-written suites (.github/assign-reviewers/tests/assignment.test.cjs and .github/refresh-reviewers/tests/test_generate.py); there is no shared corpus, so a case exercised on one side proves nothing about the other.
Candidate divergences visible on a read, offered as corpus seeds rather than as claims: the JS splits lines on a CR-optional newline regex while the Python splits on a bare newline; the JS unquote has no length guard, so a one-character double-quote would slice to empty, while the Python _unquote requires length >= 2.
## The harness is fragile, exactly as described
assignment.test.cjs:7 recovers the shipped script by splitting the workflow text on a literal 10-space-indented "script: |" line and on the exact downstream step name "Publish reviewer history manifest", then stripping 12 leading spaces per line. Verified: assign-reviewers.yml contains exactly ONE "script: |" (line 166) and the anchor step is at line 710, so it works today -- but it is load-bearing on there never being a second "script: |" at that indentation, on that step name never changing, and on the indentation staying at 12. Any of those would silently capture the wrong span rather than fail.
## The unification half is correctly dropped
Grepping assign-reviewers.yml for actions/checkout or workflows_ref returns NOTHING -- this workflow deliberately has no checkout at all, so moving the JS to a file would add a workflows_ref pin to every existing caller, a breaking contract change for a cosmetic gain. And the Python genuinely computes something the JS does not: parse_reviewer_config returns a (config, locations) pair recording flow/block line positions so the drift generator can rewrite only those bytes. They are not the same function. Do NOT collapse them.
## Scope (test-only)
1. Add one shared fixture corpus under .github/assign-reviewers/ -- JSON mapping config text to expected parsed shape, and glob+path to expected match -- and have BOTH assignment.test.cjs and test_generate.py iterate it. The Python side asserts only the config half of its return; locations stays covered by its own suite.
2. Re-anchor the extraction in assignment.test.cjs on a sentinel comment emitted inside the inline script, rather than on indentation plus an unrelated downstream step name, and fail loudly if the sentinel is not found.
## Value and risk
The config format is a contract read by the runtime and REWRITTEN by the drift generator, so a divergence could mean the generator edits lists that the runtime reads differently -- nothing today could catch that. Risk is low: test-only, no shipped code changes. Not marked security: this decides who is ASSIGNED to review, not who may approve or what may execute.
Contributor guide
Assessment
This issue has not been assessed yet.