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 #312] [MERGED] fix(reviewers): trim tokens with one explicit space-and-tab set in both reviewers.yml parsers #313

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/312
Author: @mattmillerai
Created: 9/18/2026
Status: ✅ Merged
Merged: 9/18/2026
Merged by: @mattmillerai

Base: matt/be-15312-reviewers-parser-convergence ← Head: matt/be-15334-reviewers-parser-explicit-swhite-trim

📝 Commits (2)

c1e2a92 fix(reviewers): trim tokens with one explicit space-and-tab set in both parsers
2a8c65a fix(reviewers): keep the rewriter, the rule-key regex and the config read in step with s-white trimming

📊 Changes

6 files changed (+397 additions, -54 deletions)

View changed files

📝 .github/assign-reviewers/README.md (+29 -5)
📝 .github/assign-reviewers/parser-corpus.json (+154 -2)
📝 .github/refresh-reviewers/generate.py (+87 -26)
📝 .github/refresh-reviewers/tests/test_generate.py (+48 -0)
📝 .github/workflows/assign-reviewers.yml (+58 -18)
📝 docs/callers/assign-reviewers.md (+21 -3)

📄 Description

STACKED — merging lands on matt/be-15312-reviewers-parser-convergence (owned by Matt Miller, PR #311), NOT main. #311 is itself stacked on #305. Both were still open when this was written and this edits the same two parser regions, so it is stacked rather than branched off main. Review it after those land; if they merge first, rebase this onto main before merging.

ELI-5

There are two copies of the same small reviewers.yml parser — one in Python (the bot that rewrites the file) and one in JavaScript (the bot that reads it and assigns people). Each was using its own language's idea of "whitespace" to tidy up a name, and the two languages disagree about which invisible characters count. So the same file could produce alice in one copy and <invisible>alice in the other. This pins both to exactly two characters — a space and a tab — and adds twelve test documents so neither can drift back.

What changed

One trim set, spelled out. S_WHITE = " \t" + _trim() in .github/refresh-reviewers/generate.py; trimSWhite in the PARITY-HARNESS:config-parser region of .github/workflows/assign-reviewers.yml. Applied at all 11 Python and 12 JS trim sites inside the parser regions.
CR handled explicitly at the split. .strip() was also what removed the CR of a CRLF document. CR is not s-white, so the split now consumes it: re.split(r"\r?\n", ...).
12 boundary documents added to .github/assign-reviewers/parser-corpus.json (21 → 33 configs). Both CI path filters already watch that file, so every case runs on both ports; no workflow change needed.
Docs: the "dialect rules" paragraph in .github/assign-reviewers/README.md and docs/callers/assign-reviewers.md goes from three rules to four.

Deliberately untouched: _indent_of / indentOf (already space-only and already agreeing) and the committed .github/reviewers.yml. (The two rstrip() calls in _rewrite_flow_line were also on this list; review showed that reasoning was wrong and they are now rstrip(S_WHITE) — see "Review round".)

Judgment calls

One deviation from the planned implementation, and it fixes a regression that plan would have shipped. The plan specified removing the CR by stripping it off each piece of a split("\n"). That is not what the JS split(/\r?\n/) does: a CR is a line break only when a newline follows it, so the comprehension also eats a bare CR ending the last line of a document with no final newline, which JS keeps as data. Measured on this branch before the fix — a new divergence introduced by the change itself, on a document where the two ports previously agreed:

document: "default_pool:\n  - alice\r"
  ports BEFORE this change:      py=["alice"]     js=["alice"]        agree
  ports with the planned form:   py=["alice"]     js=["alice\r"]      DIVERGE
  ports as shipped (re.split):   py=["alice\r"]   js=["alice\r"]      agree

So the Python side uses re.split(r"\r?\n", ...), which is the JS regex character for character, and a 12th corpus case (a bare CR ending the last line is data, not a line break) pins it. That case reds the Python suite — and only that case — when the comprehension form is substituted back in; verified.

This is a behaviour change for a login padded with U+00A0, not only a parity fix. NBSP is the one character both ports used to absorb, so an NBSP-padded login used to route on both sides and now routes on neither. That is the intended direction: it is the reading a real YAML parser gives (see below), and the alternative is a config that silently means something other than it says. It is also the one user-visible regression risk in this PR — see Residual.

The plan's mutation-check prediction was slightly off on the JS side, in a way worth recording. It expected the JS-reverted suite to red on U+0085 and U+001C. It does not, and should not: those are Cc characters, and ECMAScript's trim() removes only WhiteSpace (which is Zs plus tab/VT/FF/NBSP/U+FEFF) and LineTerminator (LF, CR, U+2028, U+2029). Neither NEL nor FS is in either set, so JS never trimmed them. Those corpus cases pin the Python side; the JS side is where they were already correct.

.trim() outside the sentinel region: there are none. The plan said to leave them alone ("env-var parsing etc."). grep finds zero .trim() calls in assign-reviewers.yml outside the parser region, so that instruction was vacuous — noted so a reviewer does not go looking for an omission.

README/caller-guide "Three" → "Four". The plan asked only for one added sentence in the caller guide. Both files introduce the list with a count, so the count moved too rather than shipping a paragraph that says "three" and lists four.

Review round

Five panel findings, all verified by reproducing them before changing anything. Four fixed in 2a8c65a, one pinned and deferred.

1. The rewriter and the parser disagreed about where a value starts — a routing bug (5 of 6 reviewers). _rewrite_flow_line located the flow span with a BARE lstrip()/rstrip(), which still absorb U+00A0 and U+001C, while _parse_flow now trims s-white only. Measured:

doc: `reviewers:<NBSP>[alice]`
  parsed reviewers = ["\xa0[alice]"]         <- a bare SCALAR to the parser
  rewritten line   = `reviewers:<NBSP>[new-a, new-b]`    <- but the BRACKET branch in the rewriter
  re-parsed        = ["\xa0[new-a, new-b]"]  <- ONE ineligible login: the refreshed rule routes NOBODY

This was introduced by this branch: before it, .strip() absorbed the NBSP on both sides and the two agreed. Both ends now use S_WHITE. The new test asserts the parse -> rewrite -> parse ROUND TRIP rather than the bytes.

2. set_key's . means different things in the two languages (2 of 6). Python's . excludes only LF; JS's also excludes CR, U+2028 and U+2029. On a rule line ending in a bare CR — data, since this branch stopped trimming it — Python matched and JS returned null, so the runtime dropped the key and routed nobody while this generator modelled those reviewers as routing:

python re.match(r"^(paths|reviewers):(.*)$", "reviewers: alice\r") -> <Match ...>
js     "reviewers: alice\r".match(/^(paths|reviewers):(.*)$/)      -> null

Both ports now spell it [^\n]*. The Python half is equivalent there and ships as intent-documentation; the JS half is the fix. Direction chosen to match the reading the corpus already pins for the default_pool: block arm.

3. text=True made the whole CR story vacuous in production (2 of 6). main() read the committed config with universal-newline translation, which rewrites CRLF and a bare CR to LF before the parser runs, while the runtime reads the base64 blob untranslated. Demonstrated on a real CRLF blob:

committed bytes:           "default_pool:\n  - alice\r\n  - bob\r"
git show ... text=True  -> "default_pool:\n  - alice\n  - bob\n"     != runtime bytes
git show ... (bytes)    -> "default_pool:\n  - alice\r\n  - bob\r"  == runtime bytes

Now reads bytes and decodes explicitly. Two consequences handled in the same commit, since CRs reach rewrite_config for the first time: block item lines carry the replaced line's CR so a CRLF config is not rewritten into MIXED endings, and the output file is opened newline="".

4. A configured login that can never route now warns (1 of 6). The padding this PR stops absorbing is invisible in every editor, and the failure mode was completely silent. configuredCandidate() wraps candidate() on the two CONFIGURED paths and warns once per distinct bad token, codepoint-escaped. Keyed on the login-SHAPE regex and not on eligible(): the exclude set legitimately holds the PR author and vars.REVIEWER_EXCLUDE, and warning there would fire on nearly every run. This directly mitigates the first Residual item below.

5. A whitespace-only line is no longer blank — pinned here, behaviour change DEFERRED (2 of 6). Narrowing the blank-line test means a line whose only content is non-s-white whitespace reads as column 0 and ends the block above it:

default_pool:\n  - alice\n<U+00A0>\n  - bob\n   ->  ["alice"]   (bob dropped)
rules: <rule A>\n<U+00A0>\n<rule B>             ->  [A]         (EVERY later rule dropped)

Both ports move together, so this is a behaviour change rather than a parity break. Three corpus cases now pin it — both truncations plus an indented negative control bounding it to column 0 — and both docs describe it. Widening the test is not done here: it cannot delegate to isspace() / /\s/ (those disagree, which is what this PR removes), so it means adding a THIRD spelled-out character class to the dialect on both ports. Filed as a follow-up with the three options.

Verification

Suites (both green):

node --test .github/assign-reviewers/tests/*.test.cjs   130 tests, 130 pass, 0 fail   (was 114)
python3 -m unittest discover -s .github/refresh-reviewers/tests -p 'test_*.py'
                                                        65 tests, OK

All 21 pre-existing corpus cases stay green on both ports. The corpus is now 37 configs (21 -> 33 in the first round, -> 37 in the review round).

Review-round mutation checks: reverting the three S_WHITE arguments in _rewrite_flow_line reds 3 Python tests; reverting the JS [^\n]* alone reds exactly the new bare-CR rule-key case (129/130) while reverting the Python one alone stays green, as expected since it is equivalent there; reverting the CR-carrying block-item line reds the CRLF round-trip test. The script region was re-parsed as an async function body after the edits, and the new warning helper was exercised head-to-head: U+00A0 / trailing CR / U+FEFF each warn once, a repeat of the same token does not re-warn, plain alice routes silently, and an excluded PR author is silent.

Repo-wide lints: check_workflow_pins.py OK (11 workflows, 0 defaults, all checkouts guarded) + its 341-test suite OK; check_agents_md.py --root . passed (2 pre-existing warnings, untouched by this PR) + its 46-test suite OK; assign-reviewers.yml still parses as YAML; generate.py compiles.

Mutation check (both hunks, run against the pushed head):

reverted hunk	suite	result
Python only	`test_generate.py`	7 failures: U+0085 flow · U+001C flow · U+00A0 · U+0085 block item · U+001C block reviewer · U+0085 before a top-level key · bare CR
JS only	`assignment.test.cjs`	6 failures: U+FEFF flow · U+00A0 · U+FEFF block item · U+FEFF rule reviewers · two leading BOMs · bare CR
Python, CR hunk only (planned comprehension form)	`test_generate.py`	1 failure: bare CR — and nothing else

Corpus sweep, both ports executed head-to-head across all 33 documents (pre-change ports read via git show, post-change from the worktree):

	port-vs-port disagreements	a port ≠ the corpus answer
before	9 / 33	11 / 33
after	0 / 33	0 / 33

The 2-document gap is the pair where both ports agreed and both were wrong: U+00A0 (both trimmed NBSP) and the bare CR (both trimmed CR).

Negative-claim falsification. The docs added here state that a padded login "will not route" — a capability denial, so it was attempted rather than assumed. Every padding character (U+00A0, U+0085, U+FEFF, U+001C, CR) in every position (leading, trailing, both) was parsed by both post-change ports and the resulting token pushed through the runtime's own gate, normalizeLogin + eligible() lifted verbatim out of assign-reviewers.yml:

all 16 documents produce a byte-identical token on both ports;
all 15 padded tokens return eligible() === false and never reach addAssignees, while the unpadded alice control returns true;
normalizeLogin only strips a leading @ and lowercases — it does not trim, so nothing downstream rescues the padding. The gate is /^[a-z\d](?:[a-z\d-]{0,38})$/i, anchored, which no padded token can satisfy. A GitHub login cannot contain any of these characters, so there is no path on which a padded login routes.

Is the new reading correct, or only self-consistent? Checked against PyYAML as the reference (a probe, not a repo dependency — this repo's Python is stdlib-only outside coderabbit-config/). For the two padding characters a real YAML parser admits into a document, the new behaviour is what PyYAML produces and the old behaviour was not: [<NBSP>alice<NBSP>] → '\xa0alice\xa0' and [<BOM>alice<BOM>] → 'alice', in both flow and block form. PyYAML rejects a U+001C document outright and reads U+0085 as a line break, and errors on a tab in these positions; these minimal ports diverge there by design, and no routing outcome turns on it since none of those tokens can be a GitHub login.

Residual

A consumer reviewers.yml with a U+00A0-padded login stops routing that person after the caller SHA bump, silently and invisibly. This is the one behaviour change here that can remove routing that works today, and the character is invisible in every editor. I byte-scanned the only reviewers.yml reachable from this repo, .github/reviewers.yml — 6076 chars, no CRLF, zero occurrences of U+FEFF, U+0085, U+00A0, U+001C-U+001F, U+000B-U+000D, U+2028 or U+2029 — so nothing in this repo routes differently. The consumer repos' own reviewers.yml files were not scanned and could not be: the ASSIGN_REVIEWERS_CALLERS roster is a write-only repo secret with no read path from here, and the repos are private. The source ticket asserts three such files were byte-scanned in the preceding investigation; only one of the three is reachable from this repo, so two remain unexercised by this PR. Worth a one-off scan of each enrolled caller's reviewers.yml for U+00A0 before the SHA bump reaches them — a grep -P '\xc2\xa0' .github/reviewers.yml per caller is the whole check.
The upstream PRs this is stacked on are unmerged. #311 (on #305) was open at authoring time; #309, which the plan said to rebase over "if it has landed", had not landed and was not incorporated. If #309 lands first it touches generate.py's rewrite path and this will need a rebase — the conflict surface is parse_reviewer_config's line-splitting, which #309 also reads.
A stray non-s-white whitespace character at column 0 truncates the block above it, silently — inside rules: it drops every later rule. Pinned by three corpus cases and documented in both the README and the caller guide, but the behaviour itself is unchanged and deferred to a follow-up (see "Review round" item 5). Same "used to work, now does not" class as the NBSP item above, and the same consumer-side scan would find it.
The bare-CR case changes behaviour on both ports, not just Python. A document whose last line ends in a lone CR with no final newline previously routed (trim() and strip() both removed the CR); now the CR is part of the login on both sides and it does not route. This follows from trimming s-white only and is pinned by a corpus case, but it is a second, smaller instance of the same "used to route, now does not" class as the NBSP item above. No file in this repo is affected; the same consumer-side scan would cover it.
Comment/docs prose in the parser regions was rewritten, not just added. Two existing intent comments (the Python BOM comment, the JS BOM comment) made claims that this change falsifies — "trim() below already tolerated it here" is no longer true — so both were corrected in place. A reviewer comparing against the pre-change text should read those two hunks as corrections rather than drive-by edits.

Provenance

Authored by: agent-work loop
Verified: node --test .github/assign-reviewers/tests/*.test.cjs: 130 passed, 0 failed; python3 -m unittest discover -s .github/refresh-reviewers/tests -p 'test_*.py': 65 passed, 0 failed; python3 .github/workflow-pins/check_workflow_pins.py: OK, 11 workflows, 0 defaults; python3 -m unittest discover -s .github/workflow-pins/tests -p 'test_*.py': 341 passed, 0 failed; python3 .github/agents-md-integrity/check_agents_md.py --root .: passed (2 pre-existing warnings); python3 -m unittest discover -s .github/agents-md-integrity/tests -p 'test_*.py': 46 passed, 0 failed; assign-reviewers.yml parses as YAML and its script region parses as an async function body; generate.py compiles. First round: three mutation reverts red exactly the expected corpus cases (7 / 6 / 1 as tabulated above); a both-ports head-to-head sweep of all 33 corpus documents: 9 pre-change disagreements → 0; a falsification probe of all 15 padded-login shapes through the runtime's own normalizeLogin+eligible() gate: none route. Review round: each of the five findings reproduced before being changed, and four further mutation reverts red exactly the tests named under "Review round".
Deviations: the CR-removal form deviates from the planned implementation (re.split(r"\r?\n", ...) instead of a trailing-CR strip per split("\n") piece) because the planned form introduced a fresh port divergence on a bare-CR final line; evidence and the pinning corpus case are under "Judgment calls". A 12th corpus case was added beyond the 11 the plan tabulated, for that boundary. The plan's predicted JS mutation failures (U+0085, U+001C) do not occur and cannot — ECMAScript trim() never removed those characters; the observed JS mutation set is the 6 tabulated above. Both .github/assign-reviewers/README.md and docs/callers/assign-reviewers.md had their rule count updated alongside the added rule, slightly beyond the planned one-sentence caller-guide edit. No acceptance criterion was skipped. Review round (2a8c65a): the planned "deliberately untouched" treatment of the two rstrip() calls in _rewrite_flow_line was reversed — review showed the rewriter and the parser then disagree about the value span, which drops routing, so both ends now trim S_WHITE. Reading the committed config as bytes, carrying the CR onto rewritten block item lines, opening the output newline="", and the new invalid-configured-login warning are all beyond the original plan; each is justified under "Review round". One finding (the whitespace-only line) was pinned and documented but deliberately NOT fixed, and is filed as a follow-up.

_{🔄 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/312 **Author:** [@mattmillerai](https://github.com/mattmillerai) **Created:** 9/18/2026 **Status:** ✅ Merged **Merged:** 9/18/2026 **Merged by:** [@mattmillerai](https://github.com/mattmillerai) **Base:** `matt/be-15312-reviewers-parser-convergence` ← **Head:** `matt/be-15334-reviewers-parser-explicit-swhite-trim` --- ### 📝 Commits (2) - [`c1e2a92`](https://github.com/Comfy-Org/github-workflows/commit/c1e2a928897969447d0cf0acf67d64823b023d65) fix(reviewers): trim tokens with one explicit space-and-tab set in both parsers - [`2a8c65a`](https://github.com/Comfy-Org/github-workflows/commit/2a8c65a7517f160529ed4de31d8854fb624f4174) fix(reviewers): keep the rewriter, the rule-key regex and the config read in step with s-white trimming ### 📊 Changes **6 files changed** (+397 additions, -54 deletions) <details> <summary>View changed files</summary> 📝 `.github/assign-reviewers/README.md` (+29 -5) 📝 `.github/assign-reviewers/parser-corpus.json` (+154 -2) 📝 `.github/refresh-reviewers/generate.py` (+87 -26) 📝 `.github/refresh-reviewers/tests/test_generate.py` (+48 -0) 📝 `.github/workflows/assign-reviewers.yml` (+58 -18) 📝 `docs/callers/assign-reviewers.md` (+21 -3) </details> ### 📄 Description **STACKED — merging lands on `matt/be-15312-reviewers-parser-convergence` (owned by Matt Miller, PR #311), NOT `main`.** #311 is itself stacked on #305. Both were still open when this was written and this edits the same two parser regions, so it is stacked rather than branched off `main`. Review it after those land; if they merge first, rebase this onto `main` before merging. ## ELI-5 There are two copies of the same small `reviewers.yml` parser — one in Python (the bot that rewrites the file) and one in JavaScript (the bot that reads it and assigns people). Each was using its own language's idea of "whitespace" to tidy up a name, and the two languages disagree about which invisible characters count. So the same file could produce `alice` in one copy and `<invisible>alice` in the other. This pins both to exactly two characters — a space and a tab — and adds twelve test documents so neither can drift back. ## What changed - **One trim set, spelled out.** `S_WHITE = " \t"` + `_trim()` in `.github/refresh-reviewers/generate.py`; `trimSWhite` in the `PARITY-HARNESS:config-parser` region of `.github/workflows/assign-reviewers.yml`. Applied at all **11** Python and **12** JS trim sites inside the parser regions. - **CR handled explicitly at the split.** `.strip()` was also what removed the CR of a CRLF document. CR is not s-white, so the split now consumes it: `re.split(r"\r?\n", ...)`. - **12 boundary documents** added to `.github/assign-reviewers/parser-corpus.json` (21 → 33 configs). Both CI path filters already watch that file, so every case runs on both ports; no workflow change needed. - **Docs:** the "dialect rules" paragraph in `.github/assign-reviewers/README.md` and `docs/callers/assign-reviewers.md` goes from three rules to four. Deliberately untouched: `_indent_of` / `indentOf` (already space-only and already agreeing) and the committed `.github/reviewers.yml`. (The two `rstrip()` calls in `_rewrite_flow_line` were also on this list; review showed that reasoning was wrong and they are now `rstrip(S_WHITE)` — see "Review round".) ## Judgment calls **One deviation from the planned implementation, and it fixes a regression that plan would have shipped.** The plan specified removing the CR by stripping it off each piece of a `split("\n")`. That is not what the JS `split(/\r?\n/)` does: a CR is a line break only when a newline *follows* it, so the comprehension also eats a **bare CR ending the last line of a document with no final newline**, which JS keeps as data. Measured on this branch before the fix — a new divergence introduced by the change itself, on a document where the two ports previously agreed: ``` document: "default_pool:\n - alice\r" ports BEFORE this change: py=["alice"] js=["alice"] agree ports with the planned form: py=["alice"] js=["alice\r"] DIVERGE ports as shipped (re.split): py=["alice\r"] js=["alice\r"] agree ``` So the Python side uses `re.split(r"\r?\n", ...)`, which is the JS regex character for character, and a 12th corpus case (`a bare CR ending the last line is data, not a line break`) pins it. That case reds the Python suite — and only that case — when the comprehension form is substituted back in; verified. **This is a behaviour change for a login padded with U+00A0, not only a parity fix.** NBSP is the one character *both* ports used to absorb, so an NBSP-padded login used to route on both sides and now routes on neither. That is the intended direction: it is the reading a real YAML parser gives (see below), and the alternative is a config that silently means something other than it says. It is also the one user-visible regression risk in this PR — see Residual. **The plan's mutation-check prediction was slightly off on the JS side, in a way worth recording.** It expected the JS-reverted suite to red on U+0085 and U+001C. It does not, and should not: those are `Cc` characters, and ECMAScript's `trim()` removes only `WhiteSpace` (which is `Zs` plus tab/VT/FF/NBSP/U+FEFF) and `LineTerminator` (LF, CR, U+2028, U+2029). Neither NEL nor FS is in either set, so JS never trimmed them. Those corpus cases pin the **Python** side; the JS side is where they were already correct. **`.trim()` outside the sentinel region: there are none.** The plan said to leave them alone ("env-var parsing etc."). `grep` finds zero `.trim()` calls in `assign-reviewers.yml` outside the parser region, so that instruction was vacuous — noted so a reviewer does not go looking for an omission. **README/caller-guide "Three" → "Four".** The plan asked only for one added sentence in the caller guide. Both files introduce the list with a count, so the count moved too rather than shipping a paragraph that says "three" and lists four. ## Review round Five panel findings, all verified by reproducing them before changing anything. Four fixed in `2a8c65a`, one pinned and deferred. **1. The rewriter and the parser disagreed about where a value starts — a routing bug (5 of 6 reviewers).** `_rewrite_flow_line` located the flow span with a BARE `lstrip()`/`rstrip()`, which still absorb U+00A0 and U+001C, while `_parse_flow` now trims s-white only. Measured: ``` doc: `reviewers:<NBSP>[alice]` parsed reviewers = ["\xa0[alice]"] <- a bare SCALAR to the parser rewritten line = `reviewers:<NBSP>[new-a, new-b]` <- but the BRACKET branch in the rewriter re-parsed = ["\xa0[new-a, new-b]"] <- ONE ineligible login: the refreshed rule routes NOBODY ``` This was introduced by this branch: before it, `.strip()` absorbed the NBSP on both sides and the two agreed. Both ends now use `S_WHITE`. The new test asserts the parse -> rewrite -> parse ROUND TRIP rather than the bytes. **2. `set_key`'s `.` means different things in the two languages (2 of 6).** Python's `.` excludes only LF; JS's also excludes CR, U+2028 and U+2029. On a rule line ending in a bare CR — data, since this branch stopped trimming it — Python matched and JS returned `null`, so the runtime dropped the key and routed nobody while this generator modelled those reviewers as routing: ``` python re.match(r"^(paths|reviewers):(.*)$", "reviewers: alice\r") -> <Match ...> js "reviewers: alice\r".match(/^(paths|reviewers):(.*)$/) -> null ``` Both ports now spell it `[^\n]*`. The Python half is equivalent there and ships as intent-documentation; the JS half is the fix. Direction chosen to match the reading the corpus already pins for the `default_pool:` block arm. **3. `text=True` made the whole CR story vacuous in production (2 of 6).** `main()` read the committed config with universal-newline translation, which rewrites CRLF *and* a bare CR to LF before the parser runs, while the runtime reads the base64 blob untranslated. Demonstrated on a real CRLF blob: ``` committed bytes: "default_pool:\n - alice\r\n - bob\r" git show ... text=True -> "default_pool:\n - alice\n - bob\n" != runtime bytes git show ... (bytes) -> "default_pool:\n - alice\r\n - bob\r" == runtime bytes ``` Now reads bytes and decodes explicitly. Two consequences handled in the same commit, since CRs reach `rewrite_config` for the first time: block item lines carry the replaced line's CR so a CRLF config is not rewritten into MIXED endings, and the output file is opened `newline=""`. **4. A configured login that can never route now warns (1 of 6).** The padding this PR stops absorbing is invisible in every editor, and the failure mode was completely silent. `configuredCandidate()` wraps `candidate()` on the two CONFIGURED paths and warns once per distinct bad token, codepoint-escaped. Keyed on the login-SHAPE regex and **not** on `eligible()`: the exclude set legitimately holds the PR author and `vars.REVIEWER_EXCLUDE`, and warning there would fire on nearly every run. This directly mitigates the first Residual item below. **5. A whitespace-only line is no longer blank — pinned here, behaviour change DEFERRED (2 of 6).** Narrowing the blank-line test means a line whose only content is non-s-white whitespace reads as column 0 and ends the block above it: ``` default_pool:\n - alice\n<U+00A0>\n - bob\n -> ["alice"] (bob dropped) rules: <rule A>\n<U+00A0>\n<rule B> -> [A] (EVERY later rule dropped) ``` Both ports move together, so this is a behaviour change rather than a parity break. Three corpus cases now pin it — both truncations plus an **indented** negative control bounding it to column 0 — and both docs describe it. Widening the test is not done here: it cannot delegate to `isspace()` / `/\s/` (those disagree, which is what this PR removes), so it means adding a THIRD spelled-out character class to the dialect on both ports. Filed as a follow-up with the three options. ## Verification **Suites (both green):** ``` node --test .github/assign-reviewers/tests/*.test.cjs 130 tests, 130 pass, 0 fail (was 114) python3 -m unittest discover -s .github/refresh-reviewers/tests -p 'test_*.py' 65 tests, OK ``` All 21 pre-existing corpus cases stay green on both ports. The corpus is now **37** configs (21 -> 33 in the first round, -> 37 in the review round). **Review-round mutation checks:** reverting the three `S_WHITE` arguments in `_rewrite_flow_line` reds 3 Python tests; reverting the JS `[^\n]*` alone reds exactly the new bare-CR rule-key case (129/130) while reverting the Python one alone stays green, as expected since it is equivalent there; reverting the CR-carrying block-item line reds the CRLF round-trip test. The script region was re-parsed as an async function body after the edits, and the new warning helper was exercised head-to-head: U+00A0 / trailing CR / U+FEFF each warn once, a repeat of the same token does not re-warn, plain `alice` routes silently, and an excluded PR author is silent. **Repo-wide lints:** `check_workflow_pins.py` OK (11 workflows, 0 defaults, all checkouts guarded) + its 341-test suite OK; `check_agents_md.py --root .` passed (2 pre-existing warnings, untouched by this PR) + its 46-test suite OK; `assign-reviewers.yml` still parses as YAML; `generate.py` compiles. **Mutation check (both hunks, run against the pushed head):** | reverted hunk | suite | result | |---|---|---| | Python only | `test_generate.py` | **7 failures**: U+0085 flow · U+001C flow · U+00A0 · U+0085 block item · U+001C block reviewer · U+0085 before a top-level key · bare CR | | JS only | `assignment.test.cjs` | **6 failures**: U+FEFF flow · U+00A0 · U+FEFF block item · U+FEFF rule reviewers · two leading BOMs · bare CR | | Python, CR hunk only (planned comprehension form) | `test_generate.py` | **1 failure**: bare CR — and nothing else | **Corpus sweep, both ports executed head-to-head across all 33 documents** (pre-change ports read via `git show`, post-change from the worktree): | | port-vs-port disagreements | a port ≠ the corpus answer | |---|---|---| | before | **9 / 33** | **11 / 33** | | after | **0 / 33** | **0 / 33** | The 2-document gap is the pair where both ports agreed *and* both were wrong: `U+00A0` (both trimmed NBSP) and the bare CR (both trimmed CR). **Negative-claim falsification.** The docs added here state that a padded login "will not route" — a capability denial, so it was attempted rather than assumed. Every padding character (U+00A0, U+0085, U+FEFF, U+001C, CR) in every position (leading, trailing, both) was parsed by both post-change ports and the resulting token pushed through the runtime's own gate, `normalizeLogin` + `eligible()` lifted verbatim out of `assign-reviewers.yml`: - all 16 documents produce a **byte-identical token on both ports**; - all 15 padded tokens return `eligible() === false` and never reach `addAssignees`, while the unpadded `alice` control returns `true`; - `normalizeLogin` only strips a leading `@` and lowercases — it does **not** trim, so nothing downstream rescues the padding. The gate is `/^[a-z\d](?:[a-z\d-]{0,38})$/i`, anchored, which no padded token can satisfy. A GitHub login cannot contain any of these characters, so there is no path on which a padded login routes. **Is the new reading *correct*, or only self-consistent?** Checked against PyYAML as the reference (a probe, not a repo dependency — this repo's Python is stdlib-only outside `coderabbit-config/`). For the two padding characters a real YAML parser admits into a document, the new behaviour is what PyYAML produces and the old behaviour was not: `[<NBSP>alice<NBSP>]` → `'\xa0alice\xa0'` and `[<BOM>alice<BOM>]` → `'alice'`, in both flow and block form. PyYAML rejects a U+001C document outright and reads U+0085 as a line break, and errors on a tab in these positions; these minimal ports diverge there by design, and no routing outcome turns on it since none of those tokens can be a GitHub login. ## Residual - **A consumer `reviewers.yml` with a U+00A0-padded login stops routing that person after the caller SHA bump, silently and invisibly.** This is the one behaviour change here that can remove routing that works today, and the character is invisible in every editor. I byte-scanned the only `reviewers.yml` reachable from this repo, `.github/reviewers.yml` — 6076 chars, no CRLF, **zero** occurrences of U+FEFF, U+0085, U+00A0, U+001C-U+001F, U+000B-U+000D, U+2028 or U+2029 — so nothing in *this* repo routes differently. **The consumer repos' own `reviewers.yml` files were not scanned and could not be**: the `ASSIGN_REVIEWERS_CALLERS` roster is a write-only repo secret with no read path from here, and the repos are private. The source ticket asserts three such files were byte-scanned in the preceding investigation; only one of the three is reachable from this repo, so two remain unexercised by *this* PR. Worth a one-off scan of each enrolled caller's `reviewers.yml` for U+00A0 before the SHA bump reaches them — a `grep -P '\xc2\xa0' .github/reviewers.yml` per caller is the whole check. - **The upstream PRs this is stacked on are unmerged.** #311 (on #305) was open at authoring time; #309, which the plan said to rebase over "if it has landed", had not landed and was not incorporated. If #309 lands first it touches `generate.py`'s rewrite path and this will need a rebase — the conflict surface is `parse_reviewer_config`'s line-splitting, which #309 also reads. - **A stray non-s-white whitespace character at column 0 truncates the block above it**, silently — inside `rules:` it drops every later rule. Pinned by three corpus cases and documented in both the README and the caller guide, but the behaviour itself is unchanged and deferred to a follow-up (see "Review round" item 5). Same "used to work, now does not" class as the NBSP item above, and the same consumer-side scan would find it. - **The bare-CR case changes behaviour on both ports, not just Python.** A document whose last line ends in a lone CR with no final newline previously routed (`trim()` and `strip()` both removed the CR); now the CR is part of the login on both sides and it does not route. This follows from trimming s-white only and is pinned by a corpus case, but it is a second, smaller instance of the same "used to route, now does not" class as the NBSP item above. No file in this repo is affected; the same consumer-side scan would cover it. - **Comment/docs prose in the parser regions was rewritten, not just added.** Two existing intent comments (the Python BOM comment, the JS BOM comment) made claims that this change falsifies — "`trim()` below already tolerated it here" is no longer true — so both were corrected in place. A reviewer comparing against the pre-change text should read those two hunks as corrections rather than drive-by edits. ## Provenance - **Authored by:** agent-work loop - **Verified:** `node --test .github/assign-reviewers/tests/*.test.cjs`: 130 passed, 0 failed; `python3 -m unittest discover -s .github/refresh-reviewers/tests -p 'test_*.py'`: 65 passed, 0 failed; `python3 .github/workflow-pins/check_workflow_pins.py`: OK, 11 workflows, 0 defaults; `python3 -m unittest discover -s .github/workflow-pins/tests -p 'test_*.py'`: 341 passed, 0 failed; `python3 .github/agents-md-integrity/check_agents_md.py --root .`: passed (2 pre-existing warnings); `python3 -m unittest discover -s .github/agents-md-integrity/tests -p 'test_*.py'`: 46 passed, 0 failed; `assign-reviewers.yml` parses as YAML and its script region parses as an async function body; `generate.py` compiles. First round: three mutation reverts red exactly the expected corpus cases (7 / 6 / 1 as tabulated above); a both-ports head-to-head sweep of all 33 corpus documents: 9 pre-change disagreements → 0; a falsification probe of all 15 padded-login shapes through the runtime's own `normalizeLogin`+`eligible()` gate: none route. Review round: each of the five findings reproduced before being changed, and four further mutation reverts red exactly the tests named under "Review round". - **Deviations:** the CR-removal form deviates from the planned implementation (`re.split(r"\r?\n", ...)` instead of a trailing-CR strip per `split("\n")` piece) because the planned form introduced a fresh port divergence on a bare-CR final line; evidence and the pinning corpus case are under "Judgment calls". A 12th corpus case was added beyond the 11 the plan tabulated, for that boundary. The plan's predicted JS mutation failures (U+0085, U+001C) do not occur and cannot — ECMAScript `trim()` never removed those characters; the observed JS mutation set is the 6 tabulated above. Both `.github/assign-reviewers/README.md` and `docs/callers/assign-reviewers.md` had their rule *count* updated alongside the added rule, slightly beyond the planned one-sentence caller-guide edit. No acceptance criterion was skipped. **Review round (`2a8c65a`):** the planned "deliberately untouched" treatment of the two `rstrip()` calls in `_rewrite_flow_line` was reversed — review showed the rewriter and the parser then disagree about the value span, which drops routing, so both ends now trim `S_WHITE`. Reading the committed config as bytes, carrying the CR onto rewritten block item lines, opening the output `newline=""`, and the new invalid-configured-login warning are all beyond the original plan; each is justified under "Review round". One finding (the whitespace-only line) was pinned and documented but deliberately NOT fixed, and is filed as a follow-up. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi

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