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

Open
#286 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/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

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.