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 #311] [MERGED] fix(reviewers): converge both reviewers.yml parsers on last-wins duplicate keys, s-white comments and a leading BOM #312

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

Base: main ← Head: matt/be-15312-reviewers-parser-convergence

📝 Commits (8)

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
62980a5 fix(reviewers): converge both reviewers.yml parsers on last-wins duplicate keys, s-white comments and a leading BOM
3559301 fix(reviewers): repoint locs on a shadowed default_pool, anchor the flow span, scope the last-wins claim
bc66d13 Merge remote-tracking branch 'origin/main' into matt/be-15312-reviewers-parser-convergence
3140808 fix(reviewers): trim tokens with one explicit space-and-tab set in both reviewers.yml parsers (#312)
4f5209f Merge remote-tracking branch 'origin/main' into matt/be-15312-reviewers-parser-convergence
61f923a fix(reviewers): warn for every configured login, not only the ones a run routes through

📊 Changes

7 files changed (+766 additions, -41 deletions)

View changed files

📝 .github/assign-reviewers/README.md (+39 -0)
📝 .github/assign-reviewers/parser-corpus.json (+248 -0)
📝 .github/assign-reviewers/tests/assignment.test.cjs (+101 -1)
📝 .github/refresh-reviewers/generate.py (+98 -19)
📝 .github/refresh-reviewers/tests/test_generate.py (+150 -1)
📝 .github/workflows/assign-reviewers.yml (+98 -20)
📝 docs/callers/assign-reviewers.md (+32 -0)

📄 Description

No longer stacked. #305 has merged and GitHub retargeted this PR to main.
#312 (the deferred token-trim follow-up below) was stacked on this branch and has
merged into it, so this PR now carries that work too — see "Second round" below.

ELI-5

Two different programs read the same reviewers.yml file: the workflow that assigns owners to a PR (JavaScript, inline in assign-reviewers.yml) and the generator that rewrites that file when expertise drifts (Python, in generate.py). Neither uses a real YAML library, so each has its own small hand-written parser — and in three places those two parsers disagreed about what the same bytes mean. This makes them agree, and adds cases to the shared fixture file so they cannot silently drift apart again.

What changed

Three convergence points, each decided by the preceding spike and each now pinned in the shared corpus:

1. Duplicate top-level default_pool: is last-wins, with a warning. YAML requires unique keys and PyYAML's de-facto last-wins is the reference. The JS block arm used to append to the flow list it should have replaced (default_pool: [alice] then default_pool: / - bob parsed as ["alice", "bob"] on the JS side and ["bob"] on the Python side); it now resets first. Both ports warn and neither rejects — a malformed map must not hard-fail routing or the drift generator.

2. # opens a comment only at column 0 or after a space or a tab (YAML s-white), spelled out on both sides. /\s/ and isspace() are both wider than s-white and wider in different directions, so the ports genuinely disagreed: isspace() treats U+0085 and U+001C as whitespace and /\s/ does not; /\s/ treats U+FEFF as whitespace and isspace() does not. Each of those three characters was a live divergence.

3. A single leading U+FEFF is stripped from the document before splitting into lines, on both sides. JS trim() already tolerated it; Python strip() does not treat it as whitespace, so a BOM-prefixed config had its first default_pool: key skipped entirely and routed on an empty pool.

rewrite_config deliberately keeps its own text.split("\n") and the BOM with it — its contract is byte-faithfulness. Dropping one character off the head of line 0 changes no line index, and _rewrite_flow_line locates the bracket span within the line, so the BOM survives a rewrite in place. A new test pins exactly that.

The duplicate-key warning is the one part the shared corpus cannot carry, because the channels differ (core.warning vs a printed ::warning:: line), so each suite asserts its own. Both are keyed on "the key was seen" rather than "the list is non-empty" — an emptiness test would make the JS port warn on default_pool: [] / default_pool: [bob] where the Python port stayed silent, reintroducing a divergence inside the fix for one.

The JS parity harness now injects a core stub into the new Function scope it extracts the parser into. That is load-bearing, not cosmetic: core is ambient inside a github-script step but not inside the harness, and the shipped call site sits inside a catch that logs "nothing to do" and returns — so a core that were not in scope would silently disable routing rather than fail loudly. The stub also captures the warnings, which is what lets a test assert one fired.

Non-vacuity by mutation

Each edit reverted one at a time, everything else in place; the table records what actually went red.

Reverted edit	Suite	Corpus case / test that reds
JS block arm appends instead of replacing	node	`corpus config — duplicate default_pool, flow then block`, `— second one empty`, plus the JS warning test
JS comment rule back to `/\s/`	node	`corpus config — U+FEFF before # is not a comment`
JS leading-BOM strip removed	node	nothing — see below
JS duplicate-key warning removed	node	`a duplicate top-level default_pool: warns once…`, `an empty first default_pool: still warns…`
Python comment rule back to `isspace()`	unittest	`test_config_cases [U+0085 before # is not a comment]`, `[U+001C before # is not a comment]`
Python `removeprefix` removed	unittest	`test_config_cases [leading BOM before the first key]`, `test_bom_flow_default_pool_rewrite_keeps_the_bom`
Python duplicate-key warning removed	unittest	`test_duplicate_warns_once_and_last_wins`, `test_empty_first_list_still_warns`
review round: `locs` falls back to the shadowed key	unittest	`test_locs_drop_a_shadowed_key_when_the_winner_has_no_items`
review round: flow span searches any later bracket	unittest	`test_flow_span_is_anchored_to_the_value_not_to_any_later_bracket`
review round: warning re-hardcodes `reviewers.yml`	node	`the duplicate warning names the configured path…`

Note the two comment-rule rows red on disjoint characters, which is the divergence itself showing through: U+FEFF only reds the JS side, U+0085 and U+001C only the Python side.

The one honestly vacuous edit: the JS text.replace(/^\uFEFF/, ''). Reverting it reds nothing, and it cannot — indentOf sees a BOM as zero indent and the very next statement is raw.trim(), whose JS definition already strips U+FEFF. It is kept because the ticket asks for the strip to be explicit in both ports rather than left to a whitespace definition the two languages disagree about, which is the whole point of the change; it is a parity-by-construction edit, not a behavioural one. Three of the ten new corpus cases are likewise pure regression pins that no mutation in the table reds: leading BOM before a comment line (both rules reach the same answer either way), tab before # starts a comment and space before # starts a comment (old and new rules agree on s-white itself — they exist so the narrowing cannot be narrowed further).

Live exposure

Nil, and measured rather than assumed. Every committed reviewers.yml I can read — this repo's own plus the two consumer configs the ticket names — was parsed through the before and after build of both ports:

no BOM on any of them; each opens with a # header line; each has exactly one top-level default_pool:
parsed config byte-identical before and after on both ports, for all three
Python locations identical before and after for all three, so the rewrite path is untouched
zero duplicate-key warnings emitted

So no routing outcome changes for any current caller. The third config the reusable could in principle serve was not readable with this token; see Residual.

Review fixes (cursor-review panel, 6 threads)

The panel raised six findings; five are fixed in 3559301, one is deferred with a
follow-up. Each fix is mutation-checked the same way as the original work.

locs did not follow last-wins (6/6 reviewers). Reproduced before fixing:
default_pool: [alice] / default_pool: / rules: … parsed to [] while
locs still pointed at ("flow", 0), so rewrite_config wrote the refreshed
pool onto the shadowed key — an emitted file that re-parses to an empty pool
and a scheduled run that regenerates the same no-op diff forever. The block arm
now repoints locs["default_pool"] on every occurrence, taking None when the
winner carries no item lines. None is not a new dead end: main() already had
a locs["default_pool"] is None or not dp_after branch that reports the
committed pool unchanged, so this lands on an existing path. Pointing locs at
the item-less winner instead was the alternative, but there is no span to
rewrite there and rewrite_config never inserts lines.

A regression this PR introduced in the rewrite path (1/6). Narrowing
_strip_comment to s-white means that after a non-s-white character the # no
longer opens a comment, so the whole tail is the plain scalar value — and
_rewrite_flow_line, which searched for any [ after the key, rewrote the
bracket inside that comment-shaped tail and left the real value routing:
reviewers: alice<U+00A0># see [bob] became
reviewers: alice<U+00A0># see [new1, new2]. Fixed by anchoring rather than by
reverting the comment rule: the flow span now requires the value itself to
open with [, exactly as _parse_flow already requires before returning a flow
list. That removes an asymmetry between the two instead of papering over it.

The last-wins claim overclaimed (4/6). A second top-level rules: appends on
both ports, with no seen-flag and no warning — verified. Took the reviewers'
second option and scoped the documented guarantee to default_pool: in all three
doc sites, rather than extending the reset to rules:: that would be a fourth
behavioural change here, and silently dropping a previously-honoured rules block
is too big to make unannounced.

Two nits, both real. The duplicate-key warning hardcoded reviewers.yml
though reviewer_config_path can rename it; parseReviewerConfig now takes the
path as an argument and the shipped call site passes configPath. It could not
simply close over the outer binding — the parity harness extracts the parser
region into its own new Function scope injecting only core, so a free
configPath would throw ReferenceError there. And the BOM test's literal U+FEFF
characters are now \ufeff escapes: a literal goes silently vacuous if an
editor normalises it away, since startswith("") is always true. The NBSP in
the new regression test is escaped for the same reason.

Deferred, not fixed: token-level trim/strip still diverges (4/6). Confirmed in
both directions — [<U+FEFF>alice] yields the routable login alice in JS and
\ufeffalice in Python; [x<U+0085>, y] yields x in Python and x\^E in
JS, because strip() takes U+0085/U+001C but not U+FEFF and trim() is the
exact reverse. It is the same class of divergence this PR exists to close, but
closing it means defining one explicit trim set and applying it at every
strip()/trim() call site in both ports plus its own boundary corpus cases —
token parsing rather than the comment/BOM surfaces edited here. Filed as a
follow-up with the reproduction and call sites. Nothing routes differently today:
no readable committed config carries these characters.

Verification

node --test .github/assign-reviewers/tests/*.test.cjs — 114 pass, 0 fail
python3 -m unittest discover -s .github/refresh-reviewers/tests -p 'test_*.py' — 63 pass, 0 fail
python3 .github/workflow-pins/check_workflow_pins.py — OK, 11 workflows, 0 defaults, every ref checkout guarded
python3 .github/agents-md-integrity/check_agents_md.py --root . — passed (2 pre-existing warnings, unrelated)
python3 -m unittest discover -s .github/workflow-pins/tests … — 341 pass; …/agents-md-integrity/tests — 46 pass
yaml.safe_load on assign-reviewers.yml — parses; python3 -W error::SyntaxWarning import of generate.py — clean
the mutation matrix above, run as a scripted revert-one-restore-one sweep (including the three review-round rows)

Both CI path filters already watch parser-corpus.json, so this runs both suites.

Judgment calls

Deviation from the ticket's literal prescription, one line, deliberate. The ticket specifies the JS warning fire on if (out.default_pool.length) and the Python one on locs["default_pool"] is not None. Those two conditions disagree with each other: on default_pool: [] / default_pool: [bob] the Python one warns and the JS one does not, which would ship a fresh JS/Python divergence inside the PR whose purpose is removing them. Both sides use an explicit seen-flag instead. Parsed behaviour is exactly what the ticket specifies — last-wins — and is corpus-pinned; only the warning's trigger differs from the prose, and the two tests named in the table pin the empty-first-list case on each side so the divergence cannot come back.

Chesterton's fence on out.default_pool = [];. This is the one line that changes existing behaviour (append → replace), so: on a first default_pool: it is a no-op because the field is initialised to [], so the only reachable change is a second occurrence — which is the ticket's decision, matches what the JS flow arm and both Python arms already did, and matches PyYAML. git log/git blame on the block arm shows no intent comment or commit message defending accumulation; it reads as an oversight in the original port, which is what the - bob / ["alice","bob"] divergence above demonstrates.

Scope. rewrite_config and the locations bookkeeping are untouched, as the ticket requires — that surface belongs to the separate rewrite-path work. The one new rewrite_config test asserts the existing behaviour still holds under a BOM; it changes no rewrite code.

Negative-claim falsification: trigger did not fire. The diff adds no throw, no deny/dead-end path, no "not supported"/"unavailable" string, and flips no test to assert a dead-end. The two strings it adds are warnings on a path that continues and still routes. The one capability genuinely narrowed — # after U+0085/U+001C/U+FEFF no longer starting a comment — is a deliberate convergence on the YAML spec, is symmetric across both ports, and is pinned in the corpus in the positive direction (those documents now parse to a login, where one port previously truncated them).

Residual

.github/reviewers.yml in a third potential consumer repo was not readable with this token (404 Not Found from the contents API — either the file does not exist there or the token lacks access; I cannot distinguish the two from a 404, and did not try to). The live-exposure measurement above therefore covers three configs, not necessarily every config this reusable serves. If a fourth config exists that carries a BOM, a duplicate default_pool:, or a # preceded by U+0085/U+001C/U+FEFF, its routing would change — the three conditions are cheap to grep for, and a maintainer with broader read access should do so before this lands. Every config actually checked has none of them.
~~The Python locations bookkeeping is left inconsistent for one duplicate-key shape.~~ Now fixed — the panel raised it 6/6 and it is addressed in 3559301 (see Review fixes above). It was pre-existing rather than introduced here, but leaving it while adding a corpus case for exactly that document shape was not defensible: the rewrite wrote the refreshed pool onto the shadowed key and the file re-parsed empty.
Token-level trim()/strip() still diverges between the ports, and is deferred. [<U+FEFF>alice] routes as alice in JS and \ufeffalice in Python; [x<U+0085>, y] is x in Python and x\^E in JS. Reachable but not live — no readable committed config carries these characters — and closing it is a fourth convergence point needing an explicit shared trim set at every call site in both ports plus boundary corpus cases. Filed as a follow-up rather than folded in.
Unexercised artifacts. The ticket's provenance links (a Linear spike and a PR review-thread URL) were not opened — no access from this environment — so the reconstruction of why each of the three rules was decided rests on the ticket body alone. No deployed run of either workflow was exercised: verification is the two test suites, the mutation sweep, and the before/after parse of the three real configs, all local.

Second round (CodeRabbit, 1 thread)

The invalid-configured-login warning did not match its own documented contract.
docs/callers/assign-reviewers.md promises the run warns "whenever a login configured
in reviewers.yml cannot be a GitHub login at all". It did not: the warning fired only
from the candidate helper, reached on two paths — a rule's reviewers once that rule's
paths matched a changed file, and default_pool only on a run that fell through to the
fallback. A rotted owner in an area this PR did not touch stayed silent, and a padded
default_pool entry could go unreported for as long as any rule kept matching, right up
until the fallback fired and needed it. Backwards, given the padding is invisible
precisely so the warning is what finds it.

Fixed by sweeping every config.rules[].reviewers entry and the whole default_pool
once up front through a warning-only warnConfiguredLogin. Warn broadly, route
narrowly: the sweep enqueues nothing, so an unmatched rule's owners are reported without
entering candidates, and warnedLogins still holds it to one warning per bad token.
configuredCandidate was removed rather than kept as a second guard — with the sweep in
place, dropping its warn call reds no test, so it was dead redundancy.

That warning shipped in #312 with no test whatsoever. Six now cover it, each pinned
to a distinct mutation:

Reverted edit	Test that reds
the up-front sweep removed	all four padded-login tests (matched rule, unmatched rule, non-fallback `default_pool`, dedupe)
the warning removed entirely	the same four
`warnedLogins` dedupe removed	`each bad token warns once…`, `a padded login in a MATCHED rule…`
keyed on `eligible()` instead of the shape test	`a well-formed login is never warned about, excluded or not`
warn call dropped from `configuredCandidate` (pre-collapse)	nothing — which is why it was removed

\u00a0 is written escaped in the new tests for the reason the earlier round established:
a literal goes silently vacuous if an editor normalises it away.

Both doc sites now state the scope explicitly — the warning reports the state of the
file, not of the diff, and being warned about is not being routed to.

No Python-side change: this is the routing script, outside the shared
PARITY-HARNESS:config-parser region, and generate.py has no equivalent warning.

Negative-claim falsification: trigger did not fire. The change adds core.warning
calls on a path that continues and still routes; it adds no throw, no deny, no
"unavailable"/"not supported" string, and flips no test to assert a dead end. No login
that routed before stops routing — candidate insertion is byte-for-byte the prior logic.

Provenance

Authored by: agent-work loop
Verified: node --test assign-reviewers: 135 passed, 0 failed; unittest refresh-reviewers: 108 passed, 0 failed; workflow-pins tests: 499 passed (32 skipped); agents-md-integrity tests: 46 passed; check_workflow_pins.py: OK (11 workflows, 0 defaults, every ref checkout guarded, 156 uses: SHA-pinned); check_agents_md.py: passed (2 pre-existing warnings); check-org-repo-literals.sh: OK; yaml.safe_load on assign-reviewers.yml: parses; mutation sweep re-run including the 5 new rows for this round; all 7 review threads replied to and resolved (6 fixed, 1 deferred with a follow-up)
Deviations: the duplicate-key warning uses an explicit seen-flag on both ports instead of the ticket's out.default_pool.length / locs["default_pool"] is not None pair, because those two conditions disagree with each other on an empty first list; parsed last-wins behaviour is exactly as specified. A one-line dialect note was also added to the workflow's own header comment, beyond the two doc files the ticket names, since that header is where the schema is documented for callers. The review round then touched two surfaces the ticket fences off (locs bookkeeping and _rewrite_flow_line): the first because the panel raised it 6/6 and this PR adds a corpus case for exactly that shape, the second because the narrowed comment rule introduced the fault there. Both are confined to correctness fixes; no rewrite capability was added. parseReviewerConfig gained an optional second argument (the config path, for the warning text) — additive, and the parity harness still calls it with one. Second round: the configured-login warning was broadened from the two routing paths that reached it to an up-front sweep of the whole config, which is a behaviour change beyond the thread's literal "scan and warn" ask only in that configuredCandidate was deleted rather than left in place — justified by mutation (its warn call reds nothing once the sweep exists). Both doc sites were edited to state the scope, though neither was wrong before: the contract they already described is the one the code now meets.

_{🔄 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/311 **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-15312-reviewers-parser-convergence` --- ### 📝 Commits (8) - [`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 - [`62980a5`](https://github.com/Comfy-Org/github-workflows/commit/62980a5928c62aedf705631d064f3a2cd55994eb) fix(reviewers): converge both reviewers.yml parsers on last-wins duplicate keys, s-white comments and a leading BOM - [`3559301`](https://github.com/Comfy-Org/github-workflows/commit/3559301ae7ac3bd118e8d3b35fd990118384fdfe) fix(reviewers): repoint locs on a shadowed default_pool, anchor the flow span, scope the last-wins claim - [`bc66d13`](https://github.com/Comfy-Org/github-workflows/commit/bc66d13f25f25672c4bd7b711a7c269f0f569871) Merge remote-tracking branch 'origin/main' into matt/be-15312-reviewers-parser-convergence - [`3140808`](https://github.com/Comfy-Org/github-workflows/commit/31408085989180d7804a37931f8c2f5383d05164) fix(reviewers): trim tokens with one explicit space-and-tab set in both reviewers.yml parsers (#312) - [`4f5209f`](https://github.com/Comfy-Org/github-workflows/commit/4f5209f87fb0334e4e34ec2507fbfd43dd3bc3c7) Merge remote-tracking branch 'origin/main' into matt/be-15312-reviewers-parser-convergence - [`61f923a`](https://github.com/Comfy-Org/github-workflows/commit/61f923a7c8b3d8f04429e118e5acd8180bbf4b29) fix(reviewers): warn for every configured login, not only the ones a run routes through ### 📊 Changes **7 files changed** (+766 additions, -41 deletions) <details> <summary>View changed files</summary> 📝 `.github/assign-reviewers/README.md` (+39 -0) 📝 `.github/assign-reviewers/parser-corpus.json` (+248 -0) 📝 `.github/assign-reviewers/tests/assignment.test.cjs` (+101 -1) 📝 `.github/refresh-reviewers/generate.py` (+98 -19) 📝 `.github/refresh-reviewers/tests/test_generate.py` (+150 -1) 📝 `.github/workflows/assign-reviewers.yml` (+98 -20) 📝 `docs/callers/assign-reviewers.md` (+32 -0) </details> ### 📄 Description > **No longer stacked.** #305 has merged and GitHub retargeted this PR to `main`. > #312 (the deferred token-trim follow-up below) was stacked on *this* branch and has > merged into it, so this PR now carries that work too — see "Second round" below. ## ELI-5 Two different programs read the same `reviewers.yml` file: the workflow that assigns owners to a PR (JavaScript, inline in `assign-reviewers.yml`) and the generator that rewrites that file when expertise drifts (Python, in `generate.py`). Neither uses a real YAML library, so each has its own small hand-written parser — and in three places those two parsers disagreed about what the same bytes mean. This makes them agree, and adds cases to the shared fixture file so they cannot silently drift apart again. ## What changed Three convergence points, each decided by the preceding spike and each now pinned in the shared corpus: **1. Duplicate top-level `default_pool:` is last-wins, with a warning.** YAML requires unique keys and PyYAML's de-facto last-wins is the reference. The JS block arm used to *append* to the flow list it should have replaced (`default_pool: [alice]` then `default_pool:` / `- bob` parsed as `["alice", "bob"]` on the JS side and `["bob"]` on the Python side); it now resets first. Both ports warn and neither rejects — a malformed map must not hard-fail routing or the drift generator. **2. `#` opens a comment only at column 0 or after a space or a tab** (YAML s-white), spelled out on both sides. `/\s/` and `isspace()` are both wider than s-white and wider in *different* directions, so the ports genuinely disagreed: `isspace()` treats U+0085 and U+001C as whitespace and `/\s/` does not; `/\s/` treats U+FEFF as whitespace and `isspace()` does not. Each of those three characters was a live divergence. **3. A single leading U+FEFF is stripped from the document** before splitting into lines, on both sides. JS `trim()` already tolerated it; Python `strip()` does not treat it as whitespace, so a BOM-prefixed config had its first `default_pool:` key skipped entirely and routed on an empty pool. `rewrite_config` deliberately keeps its own `text.split("\n")` and the BOM with it — its contract is byte-faithfulness. Dropping one character off the head of line 0 changes no line *index*, and `_rewrite_flow_line` locates the bracket span *within* the line, so the BOM survives a rewrite in place. A new test pins exactly that. The duplicate-key **warning** is the one part the shared corpus cannot carry, because the channels differ (`core.warning` vs a printed `::warning::` line), so each suite asserts its own. Both are keyed on *"the key was seen"* rather than *"the list is non-empty"* — an emptiness test would make the JS port warn on `default_pool: []` / `default_pool: [bob]` where the Python port stayed silent, reintroducing a divergence inside the fix for one. The JS parity harness now injects a `core` stub into the `new Function` scope it extracts the parser into. That is load-bearing, not cosmetic: `core` is ambient inside a `github-script` step but not inside the harness, and the shipped call site sits inside a `catch` that logs "nothing to do" and returns — so a `core` that were *not* in scope would silently disable routing rather than fail loudly. The stub also captures the warnings, which is what lets a test assert one fired. ## Non-vacuity by mutation Each edit reverted one at a time, everything else in place; the table records what actually went red. | Reverted edit | Suite | Corpus case / test that reds | | --- | --- | --- | | JS block arm appends instead of replacing | node | `corpus config — duplicate default_pool, flow then block`, `— second one empty`, plus the JS warning test | | JS comment rule back to `/\s/` | node | `corpus config — U+FEFF before # is not a comment` | | JS leading-BOM strip removed | node | **nothing — see below** | | JS duplicate-key warning removed | node | `a duplicate top-level default_pool: warns once…`, `an empty first default_pool: still warns…` | | Python comment rule back to `isspace()` | unittest | `test_config_cases [U+0085 before # is not a comment]`, `[U+001C before # is not a comment]` | | Python `removeprefix` removed | unittest | `test_config_cases [leading BOM before the first key]`, `test_bom_flow_default_pool_rewrite_keeps_the_bom` | | Python duplicate-key warning removed | unittest | `test_duplicate_warns_once_and_last_wins`, `test_empty_first_list_still_warns` | | **review round:** `locs` falls back to the shadowed key | unittest | `test_locs_drop_a_shadowed_key_when_the_winner_has_no_items` | | **review round:** flow span searches any later bracket | unittest | `test_flow_span_is_anchored_to_the_value_not_to_any_later_bracket` | | **review round:** warning re-hardcodes `reviewers.yml` | node | `the duplicate warning names the configured path…` | Note the two comment-rule rows red on *disjoint* characters, which is the divergence itself showing through: U+FEFF only reds the JS side, U+0085 and U+001C only the Python side. **The one honestly vacuous edit: the JS `text.replace(/^\uFEFF/, '')`.** Reverting it reds nothing, and it cannot — `indentOf` sees a BOM as zero indent and the very next statement is `raw.trim()`, whose JS definition already strips U+FEFF. It is kept because the ticket asks for the strip to be explicit in *both* ports rather than left to a whitespace definition the two languages disagree about, which is the whole point of the change; it is a parity-by-construction edit, not a behavioural one. Three of the ten new corpus cases are likewise pure regression pins that no mutation in the table reds: `leading BOM before a comment line` (both rules reach the same answer either way), `tab before # starts a comment` and `space before # starts a comment` (old and new rules agree on s-white itself — they exist so the narrowing cannot be narrowed further). ## Live exposure Nil, and measured rather than assumed. Every committed `reviewers.yml` I can read — this repo's own plus the two consumer configs the ticket names — was parsed through the **before** and **after** build of *both* ports: - no BOM on any of them; each opens with a `#` header line; each has exactly one top-level `default_pool:` - parsed config **byte-identical before and after** on both ports, for all three - Python `locations` identical before and after for all three, so the rewrite path is untouched - zero duplicate-key warnings emitted So no routing outcome changes for any current caller. The third config the reusable could in principle serve was not readable with this token; see Residual. ## Review fixes (cursor-review panel, 6 threads) The panel raised six findings; five are fixed in `3559301`, one is deferred with a follow-up. Each fix is mutation-checked the same way as the original work. **`locs` did not follow last-wins (6/6 reviewers).** Reproduced before fixing: `default_pool: [alice]` / `default_pool:` / `rules: …` parsed to `[]` while `locs` still pointed at `("flow", 0)`, so `rewrite_config` wrote the refreshed pool onto the *shadowed* key — an emitted file that re-parses to an empty pool and a scheduled run that regenerates the same no-op diff forever. The block arm now repoints `locs["default_pool"]` on every occurrence, taking `None` when the winner carries no item lines. `None` is not a new dead end: `main()` already had a `locs["default_pool"] is None or not dp_after` branch that reports the committed pool unchanged, so this lands on an existing path. Pointing `locs` at the item-less winner instead was the alternative, but there is no span to rewrite there and `rewrite_config` never inserts lines. **A regression this PR introduced in the rewrite path (1/6).** Narrowing `_strip_comment` to s-white means that after a non-s-white character the `#` no longer opens a comment, so the whole tail is the plain scalar *value* — and `_rewrite_flow_line`, which searched for any `[` after the key, rewrote the bracket inside that comment-shaped tail and left the real value routing: `reviewers: alice<U+00A0># see [bob]` became `reviewers: alice<U+00A0># see [new1, new2]`. Fixed by anchoring rather than by reverting the comment rule: the flow span now requires the **value itself** to open with `[`, exactly as `_parse_flow` already requires before returning a flow list. That removes an asymmetry between the two instead of papering over it. **The last-wins claim overclaimed (4/6).** A second top-level `rules:` appends on both ports, with no seen-flag and no warning — verified. Took the reviewers' second option and scoped the documented guarantee to `default_pool:` in all three doc sites, rather than extending the reset to `rules:`: that would be a fourth behavioural change here, and silently dropping a previously-honoured rules block is too big to make unannounced. **Two nits, both real.** The duplicate-key warning hardcoded `reviewers.yml` though `reviewer_config_path` can rename it; `parseReviewerConfig` now takes the path as an argument and the shipped call site passes `configPath`. It could not simply close over the outer binding — the parity harness extracts the parser region into its own `new Function` scope injecting only `core`, so a free `configPath` would throw ReferenceError there. And the BOM test's literal U+FEFF characters are now `\ufeff` escapes: a literal goes silently *vacuous* if an editor normalises it away, since `startswith("")` is always true. The NBSP in the new regression test is escaped for the same reason. **Deferred, not fixed: token-level trim/strip still diverges (4/6).** Confirmed in both directions — `[<U+FEFF>alice]` yields the routable login `alice` in JS and `\ufeffalice` in Python; `[x<U+0085>, y]` yields `x` in Python and `x\^E` in JS, because `strip()` takes U+0085/U+001C but not U+FEFF and `trim()` is the exact reverse. It is the same class of divergence this PR exists to close, but closing it means defining one explicit trim set and applying it at every `strip()`/`trim()` call site in both ports plus its own boundary corpus cases — token parsing rather than the comment/BOM surfaces edited here. Filed as a follow-up with the reproduction and call sites. Nothing routes differently today: no readable committed config carries these characters. ## Verification - `node --test .github/assign-reviewers/tests/*.test.cjs` — 114 pass, 0 fail - `python3 -m unittest discover -s .github/refresh-reviewers/tests -p 'test_*.py'` — 63 pass, 0 fail - `python3 .github/workflow-pins/check_workflow_pins.py` — OK, 11 workflows, 0 defaults, every ref checkout guarded - `python3 .github/agents-md-integrity/check_agents_md.py --root .` — passed (2 pre-existing warnings, unrelated) - `python3 -m unittest discover -s .github/workflow-pins/tests …` — 341 pass; `…/agents-md-integrity/tests` — 46 pass - `yaml.safe_load` on `assign-reviewers.yml` — parses; `python3 -W error::SyntaxWarning` import of `generate.py` — clean - the mutation matrix above, run as a scripted revert-one-restore-one sweep (including the three review-round rows) Both CI path filters already watch `parser-corpus.json`, so this runs both suites. ## Judgment calls **Deviation from the ticket's literal prescription, one line, deliberate.** The ticket specifies the JS warning fire on `if (out.default_pool.length)` and the Python one on `locs["default_pool"] is not None`. Those two conditions disagree with each other: on `default_pool: []` / `default_pool: [bob]` the Python one warns and the JS one does not, which would ship a fresh JS/Python divergence inside the PR whose purpose is removing them. Both sides use an explicit seen-flag instead. Parsed behaviour is exactly what the ticket specifies — last-wins — and is corpus-pinned; only the warning's trigger differs from the prose, and the two tests named in the table pin the empty-first-list case on each side so the divergence cannot come back. **Chesterton's fence on `out.default_pool = [];`.** This is the one line that changes existing behaviour (append → replace), so: on a *first* `default_pool:` it is a no-op because the field is initialised to `[]`, so the only reachable change is a second occurrence — which is the ticket's decision, matches what the JS flow arm and both Python arms already did, and matches PyYAML. `git log`/`git blame` on the block arm shows no intent comment or commit message defending accumulation; it reads as an oversight in the original port, which is what the `- bob` / `["alice","bob"]` divergence above demonstrates. **Scope.** `rewrite_config` and the `locations` bookkeeping are untouched, as the ticket requires — that surface belongs to the separate rewrite-path work. The one new `rewrite_config` *test* asserts the existing behaviour still holds under a BOM; it changes no rewrite code. **Negative-claim falsification: trigger did not fire.** The diff adds no `throw`, no deny/dead-end path, no "not supported"/"unavailable" string, and flips no test to assert a dead-end. The two strings it adds are warnings on a path that continues and still routes. The one capability genuinely narrowed — `#` after U+0085/U+001C/U+FEFF no longer starting a comment — is a deliberate convergence on the YAML spec, is symmetric across both ports, and is pinned in the corpus in the *positive* direction (those documents now parse to a login, where one port previously truncated them). ## Residual - **`.github/reviewers.yml` in a third potential consumer repo was not readable with this token** (`404 Not Found` from the contents API — either the file does not exist there or the token lacks access; I cannot distinguish the two from a 404, and did not try to). The live-exposure measurement above therefore covers three configs, not necessarily every config this reusable serves. If a fourth config exists that carries a BOM, a duplicate `default_pool:`, or a `#` preceded by U+0085/U+001C/U+FEFF, its routing *would* change — the three conditions are cheap to grep for, and a maintainer with broader read access should do so before this lands. Every config actually checked has none of them. - ~~**The Python `locations` bookkeeping is left inconsistent for one duplicate-key shape.**~~ **Now fixed** — the panel raised it 6/6 and it is addressed in `3559301` (see Review fixes above). It was pre-existing rather than introduced here, but leaving it while *adding a corpus case for exactly that document shape* was not defensible: the rewrite wrote the refreshed pool onto the shadowed key and the file re-parsed empty. - **Token-level `trim()`/`strip()` still diverges between the ports, and is deferred.** `[<U+FEFF>alice]` routes as `alice` in JS and `\ufeffalice` in Python; `[x<U+0085>, y]` is `x` in Python and `x\^E` in JS. Reachable but not live — no readable committed config carries these characters — and closing it is a fourth convergence point needing an explicit shared trim set at every call site in both ports plus boundary corpus cases. Filed as a follow-up rather than folded in. - **Unexercised artifacts.** The ticket's provenance links (a Linear spike and a PR review-thread URL) were not opened — no access from this environment — so the reconstruction of *why* each of the three rules was decided rests on the ticket body alone. No deployed run of either workflow was exercised: verification is the two test suites, the mutation sweep, and the before/after parse of the three real configs, all local. ## Second round (CodeRabbit, 1 thread) **The invalid-configured-login warning did not match its own documented contract.** `docs/callers/assign-reviewers.md` promises the run warns *"whenever a login configured in `reviewers.yml` cannot be a GitHub login at all"*. It did not: the warning fired only from the candidate helper, reached on two paths — a rule's reviewers once that rule's paths matched a changed file, and `default_pool` only on a run that fell through to the fallback. A rotted owner in an area this PR did not touch stayed silent, and a padded `default_pool` entry could go unreported for as long as any rule kept matching, right up until the fallback fired and needed it. Backwards, given the padding is invisible precisely so the warning is what finds it. Fixed by sweeping every `config.rules[].reviewers` entry and the whole `default_pool` once up front through a warning-only `warnConfiguredLogin`. Warn broadly, route narrowly: the sweep enqueues nothing, so an unmatched rule's owners are reported without entering `candidates`, and `warnedLogins` still holds it to one warning per bad token. `configuredCandidate` was removed rather than kept as a second guard — with the sweep in place, dropping its warn call reds no test, so it was dead redundancy. **That warning shipped in #312 with no test whatsoever.** Six now cover it, each pinned to a distinct mutation: | Reverted edit | Test that reds | | --- | --- | | the up-front sweep removed | all four padded-login tests (matched rule, unmatched rule, non-fallback `default_pool`, dedupe) | | the warning removed entirely | the same four | | `warnedLogins` dedupe removed | `each bad token warns once…`, `a padded login in a MATCHED rule…` | | keyed on `eligible()` instead of the shape test | `a well-formed login is never warned about, excluded or not` | | warn call dropped from `configuredCandidate` (pre-collapse) | **nothing — which is why it was removed** | `\u00a0` is written escaped in the new tests for the reason the earlier round established: a literal goes silently vacuous if an editor normalises it away. Both doc sites now state the scope explicitly — the warning reports the state of the *file*, not of the diff, and being warned about is not being routed to. No Python-side change: this is the routing script, outside the shared `PARITY-HARNESS:config-parser` region, and `generate.py` has no equivalent warning. **Negative-claim falsification: trigger did not fire.** The change adds `core.warning` calls on a path that continues and still routes; it adds no throw, no deny, no "unavailable"/"not supported" string, and flips no test to assert a dead end. No login that routed before stops routing — candidate insertion is byte-for-byte the prior logic. ## Provenance - **Authored by:** agent-work loop - **Verified:** node --test assign-reviewers: 135 passed, 0 failed; unittest refresh-reviewers: 108 passed, 0 failed; workflow-pins tests: 499 passed (32 skipped); agents-md-integrity tests: 46 passed; check_workflow_pins.py: OK (11 workflows, 0 defaults, every ref checkout guarded, 156 `uses:` SHA-pinned); check_agents_md.py: passed (2 pre-existing warnings); check-org-repo-literals.sh: OK; yaml.safe_load on assign-reviewers.yml: parses; mutation sweep re-run including the 5 new rows for this round; all 7 review threads replied to and resolved (6 fixed, 1 deferred with a follow-up) - **Deviations:** the duplicate-key warning uses an explicit seen-flag on both ports instead of the ticket's `out.default_pool.length` / `locs["default_pool"] is not None` pair, because those two conditions disagree with each other on an empty first list; parsed last-wins behaviour is exactly as specified. A one-line dialect note was also added to the workflow's own header comment, beyond the two doc files the ticket names, since that header is where the schema is documented for callers. The review round then touched two surfaces the ticket fences off (`locs` bookkeeping and `_rewrite_flow_line`): the first because the panel raised it 6/6 and this PR adds a corpus case for exactly that shape, the second because the narrowed comment rule *introduced* the fault there. Both are confined to correctness fixes; no rewrite capability was added. `parseReviewerConfig` gained an optional second argument (the config path, for the warning text) — additive, and the parity harness still calls it with one. Second round: the configured-login warning was broadened from the two routing paths that reached it to an up-front sweep of the whole config, which is a behaviour change beyond the thread's literal "scan and warn" ask only in that `configuredCandidate` was deleted rather than left in place — justified by mutation (its warn call reds nothing once the sweep exists). Both doc sites were edited to state the scope, though neither was wrong before: the contract they already described is the one the code now meets. --- <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