Comfy-Org/github-workflows

Fork 0

mirror of https://github.com/Comfy-Org/github-workflows.git synced 2026-11-05 00:17:37 +00:00

[PR #305] [MERGED] test(reviewers): one shared parser corpus for both reviewers.yml ports, plus the JS unquote length guard #306

New issue

Closed

opened 2026-10-26 22:38:18 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

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

Owner

📋 Pull Request Information

Original PR: https://github.com/Comfy-Org/github-workflows/pull/305
Author: @mattmillerai
Created: 9/18/2026
Status: ✅ Merged
Merged: 9/18/2026
Merged by: @mattmillerai

Base: main ← Head: matt/be-15247-shared-reviewers-parser-corpus

📝 Commits (2)

1ab8b60 test(reviewers): drive both reviewers.yml parsers from one shared corpus
02f501c test(reviewers): bracket the whole script scalar, frame nested spans, pin multi-line flow

📊 Changes

9 files changed (+529 additions, -7 deletions)

View changed files

📝 .github/assign-reviewers/README.md (+39 -0)
➕ .github/assign-reviewers/parser-corpus.json (+239 -0)
📝 .github/assign-reviewers/tests/assignment.test.cjs (+137 -2)
📝 .github/refresh-reviewers/README.md (+8 -1)
📝 .github/refresh-reviewers/generate.py (+16 -3)
📝 .github/refresh-reviewers/tests/test_generate.py (+53 -0)
📝 .github/workflows/assign-reviewers.yml (+23 -1)
📝 .github/workflows/test-assign-reviewers.yml (+6 -0)
📝 .github/workflows/test-refresh-reviewers.yml (+8 -0)

📄 Description

ELI-5

.github/reviewers.yml is written by one workflow and read by another, and each one has its own hand-written mini-YAML parser plus its own glob matcher. They were supposed to behave identically, but the only thing saying so was a comment, and they had already drifted. This adds one fixture file that both parsers are run against, so drift now turns a suite red instead of quietly rewriting a list the runtime reads differently.

What changed

.github/assign-reviewers/parser-corpus.json (new) — the shared corpus: 11 config documents mapped to their expected parsed shape, and 34 glob/path verdicts across 10 globs. Language-neutral JSON, no assembly logic on either side (a second text-building implementation would itself be a new parity surface).
assignment.test.cjs — iterates the corpus through the inline JS. globToRegExp, matchesAny and parseReviewerConfig are evaluated straight out of the shipped workflow bytes, not copied, so the corpus drives exactly what the runtime runs.
test_generate.py — new TestSharedParserCorpus iterates the SAME file through parse_reviewer_config and glob_to_regexp. Only the config half of the (config, locations) return is compared; locations has no JS counterpart and stays covered by TestSurgicalRewrite.
Sentinel-anchored extraction — assignment.test.cjs now splits on // PARITY-HARNESS:<name>:{begin,end} comment lines emitted inside the inline script (script, plus the nested glob-matcher and config-parser regions), replacing the split on a 10-space script: | plus the name of an unrelated downstream step. Missing, duplicated, inverted and out-dented markers each throw naming the marker; five tests cover those paths.
One shipped-code fix — the JS unquote gains the s.length >= 2 guard the Python port has had since #63. Without it a lone " slices to ''.
Path filters — test-refresh-reviewers.yml now names .github/assign-reviewers/parser-corpus.json in both its pull_request and push filters; test-assign-reviewers.yml already covers it via .github/assign-reviewers/** and now says why that glob must stay. Editing the corpus runs both suites.
Docs: both directory READMEs, the glob_to_regexp docstring and the parity with parseReviewerConfig banner now point at the corpus rather than promising parity in prose.

Why the corpus is non-vacuous

Replaying the corpus through the pre-fix JS (extracted from origin/main) fails exactly one case, in both the flow and the block path:

one-character quote is not a quoted empty string
  got  {"default_pool":["alice"],"rules":[{"paths":[],"reviewers":["","bob"]}]}
  want {"default_pool":["\"","alice"],"rules":[{"paths":[],"reviewers":["\"","bob"]}]}

The empty-string reviewer in the block sequence is the half nothing downstream filters.

The riskiest line

The unquote guard is the only shipped-behaviour change, and it narrows an existing condition — so, per the Chesterton's-fence rule, the history: the unguarded form landed in the original workflow (#18, feat(assign-reviewers)); the guard was introduced on the Python side in #63 when the parser was hand-ported, and was never back-ported. The JS is the drifted side, not the intentional one.

Routing is unaffected either way: both '' and " are rejected by eligible() (/^[a-z\d](?:[a-z\d-]{0,38})$/i) before any candidate is created, so no assignment outcome changes. That matches the ticket's own reading that neither divergence is currently user-visible.

Note this makes the PR not strictly test-only, which is what issue #286 scoped; the driving ticket's acceptance criteria explicitly require the JS fix, and that is the authority I followed.

Divergence sweep — the part this does NOT fix

The corpus only pins cases where the two ports agree. To size what it leaves behind, I ran a differential fuzz of both ports (20,000 generated reviewers.yml-shaped documents; 20,000 generated glob/path pairs) and classified every mismatch.

Globs: 0 mismatches / 20,000 pairs. Configs: 12,366 / 20,000 mismatch, 0 unclassified — every one attributable to one of two classes, both left unfixed here and both detailed under ## Residual. Restricted to the 3,237 fuzz documents containing no exotic whitespace, 102 mismatch and all 102 carry a duplicate top-level default_pool: key; the 3,135 with neither trait mismatch 0 times. The 1,647 exotic-free CRLF documents contribute 0 mismatches of their own, which corroborates the ticket's call that the /\r?\n/-vs-"\n" line-split asymmetry is latent — the corpus pins it anyway.

The fuzzer deliberately over-samples exotic whitespace, so the headline 12,366 is a property of the generator, not a defect rate on real configs.

Review round (commit `02f501c`)

The four-model panel raised ten findings. Four were fixed, five deferred to two recorded
follow-ups, one closed as unreachable. Each claim was checked against the code before acting
on it rather than taken on faith.

Fixed:

The sentinels never proved they bracketed the scalar. region checked that each marker
occurred exactly once and in order, but not that the pair spanned the whole script: |
block — so JavaScript added above :begin or below :end would still ship while being
excluded from the bytes runScript and helpers execute. That is the same silent
wrong-span failure this PR exists to eliminate, pointing the other way.
assertBracketsBlockScalar now pins both boundaries at import time (outer pair only; the
nested regions are sub-spans by design). The indent scan also stopped at end - 1, so
out-denting :end itself stayed green; it now covers the marker line.
Nested-region errors reported line numbers into an intermediate string. glob-matcher
and config-parser are extracted from the already-de-indented script, so their failures
quoted an offset into that string as if it were a workflow line. region/regionSpan now
take an origin reporting frame ({file, offset}) and the nested calls pass the script's
real position, making every reported line file-absolute.
glob_to_regexp's docstring claimed "Full-string anchored", which is false in Python
($ also matches before a trailing newline; the JS RegExp has no m flag and does not).
Promoting that to an executable-parity claim was the opposite of this PR's purpose; it now
names the divergence instead. The matcher itself is unchanged — see ## Residual.
The unquote guard's justification was wrong in both halves. eligible() does reject
'', and the guard does not prevent an empty entry anyway (- "" is two characters). The
comment now rests on the reason that holds — parity with the Python port — and names both
things the guard is not, so the next reader who checks the claim finds it true.

Corpus gained one case. A multi-line flow sequence (default_pool: [alice, / bob]):
both ports fall back to s[1:] and drop the continuation, so they agree — verified through
both before adding it — and the corpus now pins that they keep agreeing.

Every new guard is mutation-checked. Reverting <= end, dropping the origin offset, or
corrupting the new corpus expectation each reds the relevant suite; the offset test asserts
both the framed and unframed line numbers so the frame cannot be silently dropped. One test
also asserts the real workflow file satisfies the bracket check, so the import-time
assertion cannot pass vacuously.

Closed without a ticket: the **/-backtracking finding on matchesAny. Globs come from
reviewers.yml at the reviewed base SHA and from repo vars — both trusted, neither
fork-controllable — so the pathological input is not reachable today, and the reviewer's own
framing was "robustness limit rather than a fork-reachable DoS". Answered on the thread.

Residual

Three parser divergences remain live on main after this PR, and the review round added two
more in the glob translator plus two defects in the rewrite path. All are recorded as
follow-ups (the divergences deliberately without an autonomous-work label — each needs a
human call on which side is authoritative before code moves). None is reachable from a reviewers.yml that refresh-reviewers itself emits, and each needs a decision about which side is correct — which is why none is fixed here (the ticket names only the unquote guard, and its out-of-scope list forbids touching the locations return / rewrite path). They cannot be added to the corpus as-is, because a corpus case requires both ports to agree.

Duplicate top-level default_pool: key — JS accumulates, Python replaces. The JS block branch does out.default_pool.push(...) onto whatever an earlier default_pool: section produced; the Python collects into a local items and then assigns config["default_pool"] = items, so a later section (including an empty one) wipes the earlier values. Minimal repro — 'default_pool: [alice]\ndefault_pool: bob\n' → JS {"default_pool":["alice"]}, Python {"default_pool":[]}. 102 of the 390 exotic-free fuzz documents with a duplicate key diverge. A duplicate key is malformed input that a real YAML parser would reject or last-wins, so severity is low, but the two ports disagree on it today.
Comment detection uses different whitespace classes. stripComment tests /\s/.test(s[i - 1]); _strip_comment tests s[i - 1].isspace(). Those disagree on 6 of the 1,112,064 Unicode scalar values (measured, not estimated): U+001C–U+001F and U+0085 are whitespace to Python only, U+FEFF to JS only. Both directions are reachable, and I ran both minimal repros: with U+0085 immediately before the #, default_pool: [alice\^E#, bob] parses to ["alice\^E#", "bob"] in JS and ["alice"] in Python; substituting U+FEFF flips which side strips, giving ["alice"] in JS and ["alice\ufeff#", "bob"] in Python. A BOM-prefixed reviewers.yml is the realistic trigger.
The $ anchor differs on a trailing newline. glob_to_regexp compiles ^…$ and matches with re.match, and Python's $ also matches immediately before a final \n; JS's $ (no m flag) does not. glob_to_regexp('src/a.js').match('src/a.js\n') is truthy in Python and false in JS. Needs a path containing a newline to reach, which git can produce in quoted form; re.fullmatch or an explicit \Z would be the fix, but changing the matcher is a scoring-behaviour change the ticket puts out of scope.
? and non-BMP characters (glob). JS builds [^/] without the u flag, so ? consumes one UTF-16 code unit, while Python's re matches one code point — docs/?.md matches a non-BMP filename in Python only. Raised by the panel; not in the original sweep, which was ASCII/BMP-only.
. and line terminators (glob). JS . (from */**) excludes \r, U+2028 and U+2029; Python's excludes only \n. Same origin as 4.

Two rewrite-path defects were also raised and reproduced. Neither is a parser divergence, so neither is corpus-reachable (locations has no JS counterpart), and both sit on this ticket's explicit out-of-scope list:

A multi-line flow sequence is rewritten into invalid YAML. locations records ("flow", i) — one line — so rewrite_config replaces the opening line and orphans the continuation. Reproduced: default_pool: [alice,\n bob] rewritten to ['zoe'] yields default_pool: [zoe]\n bob], which is invalid YAML committed to the caller's reviewers.yml.
A CRLF config has every line ending rewritten. generate.py reads the committed config with subprocess.run(..., text=True), whose universal-newline translation strips every \r before rewrite_config runs — so changing one list rewrites the whole file, defeating the byte-preserving guarantee TestSurgicalRewrite asserts for LF files. The corpus's CRLF case still stands: it pins that both ports parse CRLF identically, which is a separate property.

Also unfixed, deliberately, and already named by the ticket: the line-split asymmetry (JS /\r?\n/, Python "\n" at generate.py and again in the rewrite path). The corpus's CRLF case pins that both ports agree on the parsed config today; the fuzz found no CRLF-attributable mismatch. It is left as-is because Python's .strip() absorbs the trailing \r at every traced consumer, and rewriting the split touches the locations line bookkeeping this ticket puts out of scope.

Artifacts exercised. Both linked issues were readable and were read: #286 (OPEN — the dedup identity, whose two-item scope list this implements) and #270 (CLOSED as a duplicate — its CI path-filter requirement is the test-refresh-reviewers.yml change above). The groom auto-builder Actions run named in the provenance chain was not fetched; it is a build-refusal record with no acceptance content, and #286's body reproduces its refusal reason verbatim. No unexercised artifact affects the change.

Judgment call on acceptance criterion 5. test-assign-reviewers.yml covers the corpus through its existing .github/assign-reviewers/** glob rather than a redundant exact-path entry; a comment at the top of that file records that the glob is load-bearing for this reason. test-refresh-reviewers.yml names the file explicitly, since nothing there would otherwise match it.

Not a capability denial. The diff adds no deny/dead-end path and no "unsupported" string; the unquote change makes a one-character token survive as itself instead of collapsing to '', so the negative-claim falsification trigger does not fire.

Provenance

Authored by: agent-work loop
Verified: node --test .github/assign-reviewers/tests/*.test.cjs: 100 tests, 100 pass, 0 fail (76 before this PR, 94 before the review round); python3 -m unittest discover -s .github/refresh-reviewers/tests -p 'test_*.py' -v: 56 tests, OK (was 53) — the count is unchanged by the new corpus case because the corpus is driven under subTest; python3 .github/workflow-pins/check_workflow_pins.py: OK, 11 workflows, 0 defaults, 0 exempt; python3 .github/agents-md-integrity/check_agents_md.py --root .: passed, 2 pre-existing warnings (AGENTS.md 155 lines, no CODEOWNERS); yaml.safe_load on the edited workflow: parses; python3 -W error::SyntaxWarning on generate.py: clean; pre-fix replay of the corpus against origin/main's inline JS still fails exactly the one intended config case. Non-vacuity checked by mutation, not assumed — the new corpus case reds BOTH suites when its expectation is corrupted (FAIL: test_config_cases [multi-line flow sequence stops at the opening line] and the matching node failure); reverting <= end to < end reds the END-marker test; the origin-offset test asserts the framed (line 102) and unframed (line 3) reports in the same assertion. Both deferred rewrite-path defects were reproduced locally before being filed, not inferred from the review.
Deviations: acceptance criterion 5 satisfied via an existing glob on the assign-reviewers side rather than an added exact path — see the judgment call above. Parser/glob divergences are left unfixed and itemised under ## Residual: three from the original sweep, two more surfaced by the review round. Two rewrite-path defects surfaced by the review round are also deferred — both reproduced, both on the ticket's explicit out-of-scope list. All are recorded as follow-ups; the divergence follow-up is deliberately left for human triage rather than autonomous pickup, because each item needs a which-side-is-authoritative decision before any code moves. One panel finding (**/ backtracking in matchesAny) was closed on its thread without a ticket: the input is not attacker-reachable under current code.

_{🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.}

## 📋 Pull Request Information **Original PR:** https://github.com/Comfy-Org/github-workflows/pull/305 **Author:** [@mattmillerai](https://github.com/mattmillerai) **Created:** 9/18/2026 **Status:** ✅ Merged **Merged:** 9/18/2026 **Merged by:** [@mattmillerai](https://github.com/mattmillerai) **Base:** `main` ← **Head:** `matt/be-15247-shared-reviewers-parser-corpus` --- ### 📝 Commits (2) - [`1ab8b60`](https://github.com/Comfy-Org/github-workflows/commit/1ab8b60ab71e65413b60d2c09eb9a81660af1995) test(reviewers): drive both reviewers.yml parsers from one shared corpus - [`02f501c`](https://github.com/Comfy-Org/github-workflows/commit/02f501c4e8ecfe12f59a528e703b545a1f647b7c) test(reviewers): bracket the whole script scalar, frame nested spans, pin multi-line flow ### 📊 Changes **9 files changed** (+529 additions, -7 deletions) <details> <summary>View changed files</summary> 📝 `.github/assign-reviewers/README.md` (+39 -0) ➕ `.github/assign-reviewers/parser-corpus.json` (+239 -0) 📝 `.github/assign-reviewers/tests/assignment.test.cjs` (+137 -2) 📝 `.github/refresh-reviewers/README.md` (+8 -1) 📝 `.github/refresh-reviewers/generate.py` (+16 -3) 📝 `.github/refresh-reviewers/tests/test_generate.py` (+53 -0) 📝 `.github/workflows/assign-reviewers.yml` (+23 -1) 📝 `.github/workflows/test-assign-reviewers.yml` (+6 -0) 📝 `.github/workflows/test-refresh-reviewers.yml` (+8 -0) </details> ### 📄 Description ## ELI-5 `.github/reviewers.yml` is written by one workflow and read by another, and each one has its own hand-written mini-YAML parser plus its own glob matcher. They were supposed to behave identically, but the only thing saying so was a comment, and they had already drifted. This adds one fixture file that both parsers are run against, so drift now turns a suite red instead of quietly rewriting a list the runtime reads differently. ## What changed - **`.github/assign-reviewers/parser-corpus.json` (new)** — the shared corpus: 11 config documents mapped to their expected parsed shape, and 34 glob/path verdicts across 10 globs. Language-neutral JSON, no assembly logic on either side (a second `text`-building implementation would itself be a new parity surface). - **`assignment.test.cjs`** — iterates the corpus through the inline JS. `globToRegExp`, `matchesAny` and `parseReviewerConfig` are evaluated straight out of the shipped workflow bytes, not copied, so the corpus drives exactly what the runtime runs. - **`test_generate.py`** — new `TestSharedParserCorpus` iterates the SAME file through `parse_reviewer_config` and `glob_to_regexp`. Only the `config` half of the `(config, locations)` return is compared; `locations` has no JS counterpart and stays covered by `TestSurgicalRewrite`. - **Sentinel-anchored extraction** — `assignment.test.cjs` now splits on `// PARITY-HARNESS:<name>:{begin,end}` comment lines emitted inside the inline script (`script`, plus the nested `glob-matcher` and `config-parser` regions), replacing the split on a 10-space `script: |` plus the name of an unrelated downstream step. Missing, duplicated, inverted and out-dented markers each throw naming the marker; five tests cover those paths. - **One shipped-code fix** — the JS `unquote` gains the `s.length >= 2` guard the Python port has had since #63. Without it a lone `"` slices to `''`. - **Path filters** — `test-refresh-reviewers.yml` now names `.github/assign-reviewers/parser-corpus.json` in both its `pull_request` and `push` filters; `test-assign-reviewers.yml` already covers it via `.github/assign-reviewers/**` and now says why that glob must stay. Editing the corpus runs both suites. - Docs: both directory READMEs, the `glob_to_regexp` docstring and the `parity with parseReviewerConfig` banner now point at the corpus rather than promising parity in prose. ## Why the corpus is non-vacuous Replaying the corpus through the **pre-fix** JS (extracted from `origin/main`) fails exactly one case, in both the flow and the block path: ``` one-character quote is not a quoted empty string got {"default_pool":["alice"],"rules":[{"paths":[],"reviewers":["","bob"]}]} want {"default_pool":["\"","alice"],"rules":[{"paths":[],"reviewers":["\"","bob"]}]} ``` The empty-string reviewer in the block sequence is the half nothing downstream filters. ## The riskiest line The `unquote` guard is the only shipped-behaviour change, and it narrows an existing condition — so, per the Chesterton's-fence rule, the history: the unguarded form landed in the original workflow (#18, `feat(assign-reviewers)`); the guard was introduced on the Python side in #63 when the parser was hand-ported, and was never back-ported. The JS is the drifted side, not the intentional one. Routing is unaffected either way: both `''` and `"` are rejected by `eligible()` (`/^[a-z\d](?:[a-z\d-]{0,38})$/i`) before any candidate is created, so no assignment outcome changes. That matches the ticket's own reading that neither divergence is currently user-visible. Note this makes the PR **not strictly test-only**, which is what issue #286 scoped; the driving ticket's acceptance criteria explicitly require the JS fix, and that is the authority I followed. ## Divergence sweep — the part this does NOT fix The corpus only pins cases where the two ports agree. To size what it leaves behind, I ran a differential fuzz of both ports (20,000 generated reviewers.yml-shaped documents; 20,000 generated glob/path pairs) and classified every mismatch. **Globs: 0 mismatches / 20,000 pairs.** **Configs: 12,366 / 20,000 mismatch, 0 unclassified** — every one attributable to one of two classes, both left unfixed here and both detailed under `## Residual`. Restricted to the 3,237 fuzz documents containing no exotic whitespace, 102 mismatch and all 102 carry a duplicate top-level `default_pool:` key; the 3,135 with neither trait mismatch 0 times. The 1,647 exotic-free CRLF documents contribute 0 mismatches of their own, which corroborates the ticket's call that the `/\r?\n/`-vs-`"\n"` line-split asymmetry is latent — the corpus pins it anyway. The fuzzer deliberately over-samples exotic whitespace, so the headline 12,366 is a property of the generator, not a defect rate on real configs. ## Review round (commit `02f501c`) The four-model panel raised ten findings. Four were fixed, five deferred to two recorded follow-ups, one closed as unreachable. Each claim was checked against the code before acting on it rather than taken on faith. **Fixed:** - **The sentinels never proved they bracketed the scalar.** `region` checked that each marker occurred exactly once and in order, but not that the pair spanned the whole `script: |` block — so JavaScript added above `:begin` or below `:end` would still ship while being excluded from the bytes `runScript` and `helpers` execute. That is the same silent wrong-span failure this PR exists to eliminate, pointing the other way. `assertBracketsBlockScalar` now pins both boundaries at import time (outer pair only; the nested regions are sub-spans by design). The indent scan also stopped at `end - 1`, so out-denting `:end` itself stayed green; it now covers the marker line. - **Nested-region errors reported line numbers into an intermediate string.** `glob-matcher` and `config-parser` are extracted from the already-de-indented script, so their failures quoted an offset into that string as if it were a workflow line. `region`/`regionSpan` now take an `origin` reporting frame (`{file, offset}`) and the nested calls pass the script's real position, making every reported line file-absolute. - **`glob_to_regexp`'s docstring claimed "Full-string anchored", which is false in Python** (`$` also matches before a trailing newline; the JS `RegExp` has no `m` flag and does not). Promoting that to an executable-parity claim was the opposite of this PR's purpose; it now names the divergence instead. The matcher itself is unchanged — see `## Residual`. - **The `unquote` guard's justification was wrong in both halves.** `eligible()` *does* reject `''`, and the guard does not prevent an empty entry anyway (`- ""` is two characters). The comment now rests on the reason that holds — parity with the Python port — and names both things the guard is not, so the next reader who checks the claim finds it true. **Corpus gained one case.** A multi-line flow sequence (`default_pool: [alice,` / ` bob]`): both ports fall back to `s[1:]` and drop the continuation, so they *agree* — verified through both before adding it — and the corpus now pins that they keep agreeing. **Every new guard is mutation-checked.** Reverting `<= end`, dropping the origin offset, or corrupting the new corpus expectation each reds the relevant suite; the offset test asserts both the framed and unframed line numbers so the frame cannot be silently dropped. One test also asserts the *real* workflow file satisfies the bracket check, so the import-time assertion cannot pass vacuously. **Closed without a ticket:** the `**/`-backtracking finding on `matchesAny`. Globs come from `reviewers.yml` at the reviewed base SHA and from repo vars — both trusted, neither fork-controllable — so the pathological input is not reachable today, and the reviewer's own framing was "robustness limit rather than a fork-reachable DoS". Answered on the thread. ## Residual Three parser divergences remain live on `main` after this PR, and the review round added two more in the glob translator plus two defects in the rewrite path. All are recorded as follow-ups (the divergences deliberately without an autonomous-work label — each needs a human call on which side is authoritative before code moves). None is reachable from a `reviewers.yml` that `refresh-reviewers` itself emits, and each needs a decision about which side is correct — which is why none is fixed here (the ticket names only the `unquote` guard, and its out-of-scope list forbids touching the `locations` return / rewrite path). They cannot be added to the corpus as-is, because a corpus case requires both ports to agree. 1. **Duplicate top-level `default_pool:` key — JS accumulates, Python replaces.** The JS block branch does `out.default_pool.push(...)` onto whatever an earlier `default_pool:` section produced; the Python collects into a local `items` and then assigns `config["default_pool"] = items`, so a later section (including an empty one) wipes the earlier values. Minimal repro — `'default_pool: [alice]\ndefault_pool: bob\n'` → JS `{"default_pool":["alice"]}`, Python `{"default_pool":[]}`. 102 of the 390 exotic-free fuzz documents with a duplicate key diverge. A duplicate key is malformed input that a real YAML parser would reject or last-wins, so severity is low, but the two ports disagree on it today. 2. **Comment detection uses different whitespace classes.** `stripComment` tests `/\s/.test(s[i - 1])`; `_strip_comment` tests `s[i - 1].isspace()`. Those disagree on **6 of the 1,112,064 Unicode scalar values** (measured, not estimated): U+001C–U+001F and U+0085 are whitespace to Python only, U+FEFF to JS only. Both directions are reachable, and I ran both minimal repros: with U+0085 immediately before the `#`, `default_pool: [alice\^E#, bob]` parses to `["alice\^E#", "bob"]` in JS and `["alice"]` in Python; substituting U+FEFF flips which side strips, giving `["alice"]` in JS and `["alice\ufeff#", "bob"]` in Python. A BOM-prefixed `reviewers.yml` is the realistic trigger. 3. **The `$` anchor differs on a trailing newline.** `glob_to_regexp` compiles `^…$` and matches with `re.match`, and Python's `$` also matches immediately before a final `\n`; JS's `$` (no `m` flag) does not. `glob_to_regexp('src/a.js').match('src/a.js\n')` is truthy in Python and `false` in JS. Needs a path containing a newline to reach, which git can produce in quoted form; `re.fullmatch` or an explicit `\Z` would be the fix, but changing the matcher is a scoring-behaviour change the ticket puts out of scope. 4. **`?` and non-BMP characters (glob).** JS builds `[^/]` without the `u` flag, so `?` consumes one UTF-16 code unit, while Python's `re` matches one code point — `docs/?.md` matches a non-BMP filename in Python only. Raised by the panel; not in the original sweep, which was ASCII/BMP-only. 5. **`.` and line terminators (glob).** JS `.` (from `*`/`**`) excludes `\r`, U+2028 and U+2029; Python's excludes only `\n`. Same origin as 4. Two **rewrite-path** defects were also raised and reproduced. Neither is a parser divergence, so neither is corpus-reachable (`locations` has no JS counterpart), and both sit on this ticket's explicit out-of-scope list: - **A multi-line flow sequence is rewritten into invalid YAML.** `locations` records `("flow", i)` — one line — so `rewrite_config` replaces the opening line and orphans the continuation. Reproduced: `default_pool: [alice,\n bob]` rewritten to `['zoe']` yields `default_pool: [zoe]\n bob]`, which is invalid YAML committed to the caller's `reviewers.yml`. - **A CRLF config has every line ending rewritten.** `generate.py` reads the committed config with `subprocess.run(..., text=True)`, whose universal-newline translation strips every `\r` before `rewrite_config` runs — so changing one list rewrites the whole file, defeating the byte-preserving guarantee `TestSurgicalRewrite` asserts for LF files. The corpus's CRLF case still stands: it pins that both ports *parse* CRLF identically, which is a separate property. Also unfixed, deliberately, and already named by the ticket: the **line-split asymmetry** (JS `/\r?\n/`, Python `"\n"` at `generate.py` and again in the rewrite path). The corpus's CRLF case pins that both ports agree on the parsed config today; the fuzz found no CRLF-attributable mismatch. It is left as-is because Python's `.strip()` absorbs the trailing `\r` at every traced consumer, and rewriting the split touches the `locations` line bookkeeping this ticket puts out of scope. **Artifacts exercised.** Both linked issues were readable and were read: [#286](https://github.com/Comfy-Org/github-workflows/issues/286) (OPEN — the dedup identity, whose two-item scope list this implements) and [#270](https://github.com/Comfy-Org/github-workflows/issues/270) (CLOSED as a duplicate — its CI path-filter requirement is the `test-refresh-reviewers.yml` change above). The groom auto-builder Actions run named in the provenance chain was not fetched; it is a build-refusal record with no acceptance content, and #286's body reproduces its refusal reason verbatim. No unexercised artifact affects the change. **Judgment call on acceptance criterion 5.** `test-assign-reviewers.yml` covers the corpus through its existing `.github/assign-reviewers/**` glob rather than a redundant exact-path entry; a comment at the top of that file records that the glob is load-bearing for this reason. `test-refresh-reviewers.yml` names the file explicitly, since nothing there would otherwise match it. **Not a capability denial.** The diff adds no deny/dead-end path and no "unsupported" string; the `unquote` change makes a one-character token survive as itself instead of collapsing to `''`, so the negative-claim falsification trigger does not fire. ## Provenance - **Authored by:** agent-work loop - **Verified:** `node --test .github/assign-reviewers/tests/*.test.cjs`: 100 tests, 100 pass, 0 fail (76 before this PR, 94 before the review round); `python3 -m unittest discover -s .github/refresh-reviewers/tests -p 'test_*.py' -v`: 56 tests, OK (was 53) — the count is unchanged by the new corpus case because the corpus is driven under `subTest`; `python3 .github/workflow-pins/check_workflow_pins.py`: OK, 11 workflows, 0 defaults, 0 exempt; `python3 .github/agents-md-integrity/check_agents_md.py --root .`: passed, 2 pre-existing warnings (AGENTS.md 155 lines, no CODEOWNERS); `yaml.safe_load` on the edited workflow: parses; `python3 -W error::SyntaxWarning` on `generate.py`: clean; pre-fix replay of the corpus against `origin/main`'s inline JS still fails exactly the one intended config case. Non-vacuity checked by mutation, not assumed — the new corpus case reds BOTH suites when its expectation is corrupted (`FAIL: test_config_cases [multi-line flow sequence stops at the opening line]` and the matching node failure); reverting `<= end` to `< end` reds the END-marker test; the origin-offset test asserts the framed (`line 102`) and unframed (`line 3`) reports in the same assertion. Both deferred rewrite-path defects were reproduced locally before being filed, not inferred from the review. - **Deviations:** acceptance criterion 5 satisfied via an existing glob on the assign-reviewers side rather than an added exact path — see the judgment call above. Parser/glob divergences are left unfixed and itemised under `## Residual`: three from the original sweep, two more surfaced by the review round. Two rewrite-path defects surfaced by the review round are also deferred — both reproduced, both on the ticket's explicit out-of-scope list. All are recorded as follow-ups; the divergence follow-up is deliberately left for human triage rather than autonomous pickup, because each item needs a which-side-is-authoritative decision before any code moves. One panel finding (`**/` backtracking in `matchesAny`) was closed on its thread without a ticket: the input is not attacker-reachable under current code. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi added the

pull-request

label

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

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

bsakaguchi closed this issue

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