[GH-ISSUE #286] [groom] reviewers.yml has two hand-ported parsers whose parity is asserted by two independent suites and no shared corpus #7

New issue

Open

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 14, 2026).
Original GitHub issue: https://github.com/Comfy-Org/github-workflows/issues/286

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

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)

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.
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.

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

bsakaguchi added the

groom

label

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

bsakaguchi commented

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

Author

Owner

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

Triage: still valid, tracked internally. Leaving this open.

Re-verified every claim against origin/main @ cf7b8d9c2614ad9f8ee41acf245381ad76fcdc8b. All five hold, and the line numbers in the body are still exact:

parseReviewerConfig at assign-reviewers.yml:424 and parse_reviewer_config at generate.py:172 are both present, with the "parity with parseReviewerConfig" banner still at generate.py:131 and the "byte-for-byte semantics-equal" docstring at generate.py:99-101.
No shared corpus exists. git ls-tree -r origin/main .github/assign-reviewers .github/refresh-reviewers returns only the two READMEs, generate.py, assignment.test.cjs, test_generate.py and test_inputs_docs.py. Each suite carries its own inline literals with no overlap.
The unquote divergence is live. assign-reviewers.yml:436-440 has no length guard; generate.py:152-156 requires len(s) >= 2. For a one-character ", JS returns '' and Python returns ".
The harness anchor is unchanged. assignment.test.cjs:7 still splits on the 10-space script: | and the step name Publish reviewer history manifest. Those are still unique (:166 and :710 respectively), which is exactly why it works today and why it would silently capture the wrong span if either changed.
Confirmed zero actions/checkout and zero workflows_ref in assign-reviewers.yml, so the reasoning for not unifying the two implementations stands.

Nothing has fixed this since filing: the only commits touching these paths since 2026-09-14 are a8ecb81 and 0501c01, both adding the unrelated workflow-inputs-vs-docs drift test.

Two additions from the sweep, now carried into the tracked work:

A second divergence the body does not mention: assign-reviewers.yml:449 splits lines on /\r?\n/, generate.py:180 on a bare "\n". Latent rather than live — Python's .strip() absorbs the \r at every consumer traced — but worth a corpus case.
The two CI path filters are disjoint, which #270 flagged and this issue does not. test-assign-reviewers.yml watches only .github/assign-reviewers/** + assign-reviewers.yml; test-refresh-reviewers.yml watches only .github/refresh-reviewers/** + its own workflow + its caller guide. A corpus added under one directory would run only on the surface you edited, defeating the point — so both filters need the corpus path.

#270 describes the same defect from the other anchor file and has been closed as a duplicate of this one, with its path-filter requirement folded into the tracked work.

This is being tracked internally and the issue stays open until the fix lands. Reopen or comment if you can still reproduce something here that this misses.

Created by Claude Code

@mattmillerai commented on GitHub (Sep 18, 2026): **Triage: still valid, tracked internally. Leaving this open.** Re-verified every claim against `origin/main` @ `cf7b8d9c2614ad9f8ee41acf245381ad76fcdc8b`. All five hold, and the line numbers in the body are still exact: - `parseReviewerConfig` at `assign-reviewers.yml:424` and `parse_reviewer_config` at `generate.py:172` are both present, with the "parity with parseReviewerConfig" banner still at `generate.py:131` and the "byte-for-byte semantics-equal" docstring at `generate.py:99-101`. - **No shared corpus exists.** `git ls-tree -r origin/main .github/assign-reviewers .github/refresh-reviewers` returns only the two READMEs, `generate.py`, `assignment.test.cjs`, `test_generate.py` and `test_inputs_docs.py`. Each suite carries its own inline literals with no overlap. - **The `unquote` divergence is live.** `assign-reviewers.yml:436-440` has no length guard; `generate.py:152-156` requires `len(s) >= 2`. For a one-character `"`, JS returns `''` and Python returns `"`. - **The harness anchor is unchanged.** `assignment.test.cjs:7` still splits on the 10-space `script: |` and the step name `Publish reviewer history manifest`. Those are still unique (`:166` and `:710` respectively), which is exactly why it works today and why it would silently capture the wrong span if either changed. - Confirmed zero `actions/checkout` and zero `workflows_ref` in `assign-reviewers.yml`, so the reasoning for not unifying the two implementations stands. Nothing has fixed this since filing: the only commits touching these paths since 2026-09-14 are `a8ecb81` and `0501c01`, both adding the unrelated workflow-inputs-vs-docs drift test. Two additions from the sweep, now carried into the tracked work: 1. **A second divergence** the body does not mention: `assign-reviewers.yml:449` splits lines on `/\r?\n/`, `generate.py:180` on a bare `"\n"`. Latent rather than live — Python's `.strip()` absorbs the `\r` at every consumer traced — but worth a corpus case. 2. **The two CI path filters are disjoint**, which #270 flagged and this issue does not. `test-assign-reviewers.yml` watches only `.github/assign-reviewers/**` + `assign-reviewers.yml`; `test-refresh-reviewers.yml` watches only `.github/refresh-reviewers/**` + its own workflow + its caller guide. A corpus added under one directory would run only on the surface you edited, defeating the point — so both filters need the corpus path. #270 describes the same defect from the other anchor file and has been closed as a duplicate of this one, with its path-filter requirement folded into the tracked work. This is being tracked internally and the issue stays open until the fix lands. Reopen or comment if you can still reproduce something here that this misses. *Created by Claude Code*

bsakaguchi referenced this issue

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

[PR #7] [MERGED] chore: drop committed __pycache__ and add Python .gitignore #16