[PR #310] [MERGED] fix(reviewers): converge both glob translators on full-string anchoring, code-point `?` and line-terminator-agnostic `**` #311

New issue

Closed

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

bsakaguchi commented

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

Owner

📋 Pull Request Information

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

Base: main ← Head: matt/be-15311-glob-translator-convergence

📝 Commits (2)

f11b5ad fix(reviewers): converge both glob translators on full-string anchoring, code-point ? and line-terminator-agnostic **
ef83485 docs(reviewers): scope the ? code-point claim to precomposed forms, and pin NFD in the corpus

📊 Changes

5 files changed (+85 additions, -20 deletions)

View changed files

📝 .github/assign-reviewers/parser-corpus.json (+41 -3)
📝 .github/refresh-reviewers/README.md (+1 -1)
📝 .github/refresh-reviewers/generate.py (+9 -9)
📝 .github/workflows/assign-reviewers.yml (+27 -5)
📝 docs/callers/assign-reviewers.md (+7 -2)

📄 Description

STACKED — merging lands on matt/be-15247-shared-reviewers-parser-corpus (owned by @mattmillerai, PR #305), NOT main. Review the net diff (4 files) rather than the two-dot diff, and do not merge this before #305.

ELI-5

Two copies of the same "does this file path match this pattern?" code exist — one in JavaScript inside assign-reviewers.yml (the thing that actually picks reviewers) and one in Python inside refresh-reviewers/generate.py (the thing that proposes the reviewer map). They were supposed to behave identically. They didn't, in three small ways involving newlines and emoji. This makes them identical and adds test cases so they can't drift apart again.

What changed

generate.py glob_to_regexp: re.compile("^" + out + "$") → re.compile("^" + out + r"\Z", re.DOTALL). Python's $ also matches just before a single trailing newline; \Z does not, so the anchoring is genuinely full-string and now matches the JS RegExp. re.DOTALL is the other half — ** (the only construct that compiles to a bare .) spans line terminators. The docstring's "deliberately-unfixed divergence" paragraph is deleted and the full-string-anchored claim restored.
assign-reviewers.yml globToRegExp: new RegExp('^' + re + '$') → new RegExp('^' + re + '$', 'su'). s makes ** span line terminators; u makes ? one Unicode code point rather than one UTF-16 code unit, so an astral path character (an emoji) counts once instead of as its two surrogate halves. The comment above it now documents all three semantics and names the Python counterpart.
parser-corpus.json: every formerly-divergent case pinned, so both suites enforce it from the one fixture. Four new globs[] entries (src/a.js, src/**, src/*, **/x?) plus two cases on the existing docs/?.md.
docs/callers/assign-reviewers.md: one sentence under the glob-semantics paragraph stating ? is one Unicode character. matchesAny also backs REVIEWER_SKIP_BASE_BRANCHES, and while git refnames cannot contain control characters (so no base-branch decision changes on the s flag), they can contain astral characters — which is exactly what the u flag changes, so the doc sentence is load-bearing there and not only for path rules.

Non-vacuity by mutation

Each of the four edits reverted one at a time, with the corpus cases in place. Every revert reds the relevant suite on a specific new case:

Reverted	Suite	Case that goes red
Python `\Z` → `$` (DOTALL kept)	`test_generate.py`	`glob='src/a.js', path='src/a.js\n'`
Python `re.DOTALL` dropped (`\Z` kept)	`test_generate.py`	`glob='src/'` on `'src/a\nb'` and** `'src/a/b\n'`
JS `s` dropped (`'u'`)	`assignment.test.cjs`	`corpus glob — src/`, first on `globToRegExp("src/").test("src/a\rb")`
JS `u` dropped (`'s'`)	`assignment.test.cjs`	`corpus glob — docs/?.md` on `"docs/😀.md"` and `corpus glob — **/x?` on `"a/b/x😀"`

Differential sweep — the half that is not being changed

Rather than assert parity, I measured it. 1,165 generated globs × 489 generated paths = 569,685 (glob, path) pairs, both translators run over the same space (the generator built the glob and path alphabets out of *, **, **/, ?, /, ., every regex metacharacter the translator escapes, \n, \r, U+2028, U+2029, é and an astral emoji):

Before: 6,454 pairs where the two ports disagreed.
After: 0. They agree on all 569,685.
Blast radius on the portion this does not set out to change: of every path containing no line terminator and no astral code point, 0 change verdict on either side, and 0 diverged between the ports before the fix either. Every behaviour change this PR makes is confined to paths holding a line terminator or an astral character.
Separately, a compile-safety sweep for the u flag: 262,161 probe globs (every UTF-16 code unit as a lone glob character — lone surrogates included — each of those followed by and preceded by a literal, each adjacent to */?, plus astral code points and realistic shapes). 0 patterns that compiled before and throw under su, and 0 whose self-match verdict changed. That is the empirical check behind the "every escape is a legal identity escape under u" claim, not a restatement of it.

Judgment calls

The precondition is not met yet, so this is stacked rather than parked. The corpus file this builds on is not on main — it arrives with #305, which is open, green and mergeable but unmerged. Since the blocker is an unmerged PR that already carries the foundation code on a branch, this stacks on that branch instead of waiting. GitHub retargets to main when #305 merges.
Two of the src/** cases were ambiguous in the source text. The list of expected paths carried two visually identical "src/a b" entries. I read them as U+2028 and U+2029 — the two line terminators a JS RegExp honours that Python's . does not, which is the only reading that makes them two distinct cases and makes them formerly-divergent in the direction the change fixes. Verified: both were JS-false / Python-true before, both true after.
U+2028/U+2029 stay \u-escaped in the JSON while é and the emoji are written raw, matching the file's existing style. They are invisible characters, and a raw one in a fixture is a review hazard — a raw U+2028 is also a line terminator to a JS parser. The why field says so.
Not marked Skip-caller-bump. This is a behavioural change inside a watched surface, so the assign-reviewers fleet should bump.

Residual

validPath means the cached-history path never exercises the s flag. assign-reviewers.yml:285 rejects any path containing \x00-\x1f when validating a downloaded history manifest, so a cached record holding a newline in a filename throws and falls back to a live fetch rather than reaching globToRegExp. Live PR file paths are not filtered that way and do reach it, so the change is not vacuous at runtime — but the two paths into the matcher now have different reachable input sets, and nothing tests that asymmetry. U+2028/U+2029 are outside \x00-\x1f and pass validPath, so the cached path is only partly insulated. Worth its own look at whether validPath should be the shared entry gate for both.
The caller doc has no drift test. docs/callers/refresh-reviewers.md is watched by a two-way input/docs drift test and by test-refresh-reviewers.yml's path filter; docs/callers/assign-reviewers.md — the file this PR edits — is watched by neither, so the sentence added here can rot silently. check_workflow_pins.py's docs cross-check covers workflows_ref rows only.
? semantics are documented for base-branch globs but not corpus-pinned for them. The corpus drives globToRegExp/matchesAny directly, which is the same function the base-branch gate calls, so the semantics are covered; what is not covered is a case shaped like a refname (stack/…) going through REVIEWER_SKIP_BASE_BRANCHES end to end. The stated reason is that refnames cannot hold control characters — true for control characters, but the u-flag half applies to refnames and is asserted only indirectly.
Unexercised artifacts. The referenced spike, the sibling config-parser ticket, the separately-tracked CRLF / multi-line-flow work, and the PR review-thread discussion this was deferred from were all named to me by reference only; I could not read any of them and did not act on anything in them. The review comment that motivated this change is on #305, which I did read as a PR but whose review threads I did not enumerate. The sibling config-parser items (duplicate key, comment whitespace, BOM) are deliberately untouched here.
The parity claim is bounded by the sweep's alphabet. "0 divergent pairs" is 0 over a generated space built from the characters listed above, not a proof over all strings. A divergence needing a character class the generator never emitted would not have been seen.

Provenance

Authored by: agent-work loop
Verified: node --test .github/assign-reviewers/tests/*.test.cjs: 104 pass, 0 fail (100 before; +4 corpus glob entries); python3 -m unittest discover -s .github/refresh-reviewers/tests -p 'test_*.py': 56 tests, OK; python3 .github/workflow-pins/check_workflow_pins.py: OK, 11 workflows, 0 exempt; python3 .github/agents-md-integrity/check_agents_md.py --root .: passed (2 pre-existing warnings, untouched by this diff); yaml.safe_load on the edited workflow: parses; 4/4 mutation reverts red the expected suite on the expected case; differential sweep 569,685 pairs 6,454 → 0 divergent; u-flag compile sweep 262,161 probes, 0 regressions.
Deviations: none — all five items delivered. The one substitution is that the two ambiguous src/** path literals were resolved to U+2028/U+2029 as described under Judgment calls.

_{🔄 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/310 **Author:** [@mattmillerai](https://github.com/mattmillerai) **Created:** 9/18/2026 **Status:** ✅ Merged **Merged:** 9/19/2026 **Merged by:** [@mattmillerai](https://github.com/mattmillerai) **Base:** `main` ← **Head:** `matt/be-15311-glob-translator-convergence` --- ### 📝 Commits (2) - [`f11b5ad`](https://github.com/Comfy-Org/github-workflows/commit/f11b5ad9651d5424e3b7f845eef7172abefd12bd) fix(reviewers): converge both glob translators on full-string anchoring, code-point `?` and line-terminator-agnostic `**` - [`ef83485`](https://github.com/Comfy-Org/github-workflows/commit/ef834854f06b6e03bf442ec2b9a876b7959a699a) docs(reviewers): scope the `?` code-point claim to precomposed forms, and pin NFD in the corpus ### 📊 Changes **5 files changed** (+85 additions, -20 deletions) <details> <summary>View changed files</summary> 📝 `.github/assign-reviewers/parser-corpus.json` (+41 -3) 📝 `.github/refresh-reviewers/README.md` (+1 -1) 📝 `.github/refresh-reviewers/generate.py` (+9 -9) 📝 `.github/workflows/assign-reviewers.yml` (+27 -5) 📝 `docs/callers/assign-reviewers.md` (+7 -2) </details> ### 📄 Description **STACKED — merging lands on `matt/be-15247-shared-reviewers-parser-corpus` (owned by @mattmillerai, PR #305), NOT `main`.** Review the net diff (4 files) rather than the two-dot diff, and do not merge this before #305. ## ELI-5 Two copies of the same "does this file path match this pattern?" code exist — one in JavaScript inside `assign-reviewers.yml` (the thing that actually picks reviewers) and one in Python inside `refresh-reviewers/generate.py` (the thing that proposes the reviewer map). They were supposed to behave identically. They didn't, in three small ways involving newlines and emoji. This makes them identical and adds test cases so they can't drift apart again. ## What changed - **`generate.py` `glob_to_regexp`:** `re.compile("^" + out + "$")` → `re.compile("^" + out + r"\Z", re.DOTALL)`. Python's `$` also matches just *before* a single trailing newline; `\Z` does not, so the anchoring is genuinely full-string and now matches the JS `RegExp`. `re.DOTALL` is the other half — `**` (the only construct that compiles to a bare `.`) spans line terminators. The docstring's "deliberately-unfixed divergence" paragraph is deleted and the full-string-anchored claim restored. - **`assign-reviewers.yml` `globToRegExp`:** `new RegExp('^' + re + '$')` → `new RegExp('^' + re + '$', 'su')`. `s` makes `**` span line terminators; `u` makes `?` one Unicode **code point** rather than one UTF-16 code unit, so an astral path character (an emoji) counts once instead of as its two surrogate halves. The comment above it now documents all three semantics and names the Python counterpart. - **`parser-corpus.json`:** every formerly-divergent case pinned, so both suites enforce it from the one fixture. Four new `globs[]` entries (`src/a.js`, `src/**`, `src/*`, `**/x?`) plus two cases on the existing `docs/?.md`. - **`docs/callers/assign-reviewers.md`:** one sentence under the glob-semantics paragraph stating `?` is one Unicode character. `matchesAny` also backs `REVIEWER_SKIP_BASE_BRANCHES`, and while git refnames cannot contain control characters (so no base-branch decision changes on the `s` flag), they *can* contain astral characters — which is exactly what the `u` flag changes, so the doc sentence is load-bearing there and not only for path rules. ## Non-vacuity by mutation Each of the four edits reverted one at a time, with the corpus cases in place. Every revert reds the relevant suite on a specific new case: | Reverted | Suite | Case that goes red | | --- | --- | --- | | Python `\Z` → `$` (DOTALL kept) | `test_generate.py` | `glob='src/a.js', path='src/a.js\n'` | | Python `re.DOTALL` dropped (`\Z` kept) | `test_generate.py` | `glob='src/**'` on `'src/a\nb'` **and** `'src/a/b\n'` | | JS `s` dropped (`'u'`) | `assignment.test.cjs` | `corpus glob — src/**`, first on `globToRegExp("src/**").test("src/a\rb")` | | JS `u` dropped (`'s'`) | `assignment.test.cjs` | `corpus glob — docs/?.md` on `"docs/😀.md"` **and** `corpus glob — **/x?` on `"a/b/x😀"` | ## Differential sweep — the half that is not being changed Rather than assert parity, I measured it. 1,165 generated globs × 489 generated paths = **569,685 (glob, path) pairs**, both translators run over the same space (the generator built the glob and path alphabets out of `*`, `**`, `**/`, `?`, `/`, `.`, every regex metacharacter the translator escapes, `\n`, `\r`, U+2028, U+2029, `é` and an astral emoji): - **Before:** 6,454 pairs where the two ports disagreed. - **After:** **0**. They agree on all 569,685. - Blast radius on the portion this does *not* set out to change: of every path containing **no** line terminator and **no** astral code point, **0** change verdict on either side, and **0** diverged between the ports before the fix either. Every behaviour change this PR makes is confined to paths holding a line terminator or an astral character. - Separately, a compile-safety sweep for the `u` flag: 262,161 probe globs (every UTF-16 code unit as a lone glob character — lone surrogates included — each of those followed by and preceded by a literal, each adjacent to `*`/`?`, plus astral code points and realistic shapes). **0** patterns that compiled before and throw under `su`, and **0** whose self-match verdict changed. That is the empirical check behind the "every escape is a legal identity escape under `u`" claim, not a restatement of it. ## Judgment calls - **The precondition is not met yet, so this is stacked rather than parked.** The corpus file this builds on is not on `main` — it arrives with #305, which is open, green and mergeable but unmerged. Since the blocker is an unmerged PR that already carries the foundation code on a branch, this stacks on that branch instead of waiting. GitHub retargets to `main` when #305 merges. - **Two of the `src/**` cases were ambiguous in the source text.** The list of expected paths carried two visually identical `"src/a b"` entries. I read them as U+2028 and U+2029 — the two line terminators a JS `RegExp` honours that Python's `.` does not, which is the only reading that makes them two distinct cases *and* makes them formerly-divergent in the direction the change fixes. Verified: both were JS-false / Python-true before, both true after. - **U+2028/U+2029 stay `\u`-escaped in the JSON** while `é` and the emoji are written raw, matching the file's existing style. They are invisible characters, and a raw one in a fixture is a review hazard — a raw U+2028 is also a line terminator to a JS parser. The `why` field says so. - **Not marked `Skip-caller-bump`.** This is a behavioural change inside a watched surface, so the assign-reviewers fleet should bump. ## Residual - **`validPath` means the cached-history path never exercises the `s` flag.** `assign-reviewers.yml:285` rejects any path containing `\x00-\x1f` when validating a downloaded history manifest, so a cached record holding a newline in a filename throws and falls back to a live fetch rather than reaching `globToRegExp`. Live PR file paths are *not* filtered that way and do reach it, so the change is not vacuous at runtime — but the two paths into the matcher now have different reachable input sets, and nothing tests that asymmetry. U+2028/U+2029 are outside `\x00-\x1f` and pass `validPath`, so the cached path is only partly insulated. Worth its own look at whether `validPath` should be the shared entry gate for both. - **The caller doc has no drift test.** `docs/callers/refresh-reviewers.md` is watched by a two-way input/docs drift test and by `test-refresh-reviewers.yml`'s path filter; `docs/callers/assign-reviewers.md` — the file this PR edits — is watched by neither, so the sentence added here can rot silently. `check_workflow_pins.py`'s docs cross-check covers `workflows_ref` rows only. - **`?` semantics are documented for base-branch globs but not corpus-pinned for them.** The corpus drives `globToRegExp`/`matchesAny` directly, which is the same function the base-branch gate calls, so the semantics are covered; what is not covered is a case shaped like a refname (`stack/…`) going through `REVIEWER_SKIP_BASE_BRANCHES` end to end. The stated reason is that refnames cannot hold control characters — true for control characters, but the `u`-flag half applies to refnames and is asserted only indirectly. - **Unexercised artifacts.** The referenced spike, the sibling config-parser ticket, the separately-tracked CRLF / multi-line-flow work, and the PR review-thread discussion this was deferred from were all named to me by reference only; I could not read any of them and did not act on anything in them. The review comment that motivated this change is on #305, which I did read as a PR but whose review threads I did not enumerate. The sibling config-parser items (duplicate key, comment whitespace, BOM) are deliberately untouched here. - **The parity claim is bounded by the sweep's alphabet.** "0 divergent pairs" is 0 over a generated space built from the characters listed above, not a proof over all strings. A divergence needing a character class the generator never emitted would not have been seen. ## Provenance - **Authored by:** agent-work loop - **Verified:** `node --test .github/assign-reviewers/tests/*.test.cjs`: 104 pass, 0 fail (100 before; +4 corpus glob entries); `python3 -m unittest discover -s .github/refresh-reviewers/tests -p 'test_*.py'`: 56 tests, OK; `python3 .github/workflow-pins/check_workflow_pins.py`: OK, 11 workflows, 0 exempt; `python3 .github/agents-md-integrity/check_agents_md.py --root .`: passed (2 pre-existing warnings, untouched by this diff); `yaml.safe_load` on the edited workflow: parses; 4/4 mutation reverts red the expected suite on the expected case; differential sweep 569,685 pairs 6,454 → 0 divergent; `u`-flag compile sweep 262,161 probes, 0 regressions. - **Deviations:** none — all five items delivered. The one substitution is that the two ambiguous `src/**` path literals were resolved to U+2028/U+2029 as described under Judgment calls. --- <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:21 +00:00

bsakaguchi referenced this issue

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

[PR #311] [MERGED] fix(reviewers): converge both reviewers.yml parsers on last-wins duplicate keys, s-white comments and a leading BOM #312

bsakaguchi closed this issue

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

bsakaguchi referenced this issue

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

[PR #312] [MERGED] fix(reviewers): trim tokens with one explicit space-and-tab set in both reviewers.yml parsers #313