[PR #313] [MERGED] fix(reviewers): warn when an unrecognised column-0 line ends a default_pool/rules block, on both ports #315

New issue

Closed

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

bsakaguchi commented

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

Owner

📋 Pull Request Information

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

Base: main ← Head: matt/be-15377-reviewers-parser-unrecognised-key-warning

📝 Commits (10+)

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
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
21ce8bf fix(reviewers): warn when an unrecognised column-0 line ends a default_pool/rules block, on both ports
ba664a5 fix(reviewers): warn only where a column-0 line breaks something, and escape the annotation
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)

📊 Changes

8 files changed (+793 additions, -28 deletions)

View changed files

📝 .github/assign-reviewers/README.md (+15 -1)
📝 .github/assign-reviewers/parser-corpus.json (+41 -3)
📝 .github/assign-reviewers/tests/assignment.test.cjs (+196 -0)
📝 .github/refresh-reviewers/generate.py (+170 -5)
📝 .github/refresh-reviewers/tests/test_generate.py (+234 -6)
📝 .github/workflows/assign-reviewers.yml (+94 -9)
📝 docs/callers/assign-reviewers.md (+31 -4)
📝 docs/callers/refresh-reviewers.md (+12 -0)

📄 Description

ELI-5

Both copies of the reviewers.yml parser stop reading a list the moment they hit a line that starts at column 0 and isn't default_pool: or rules:. A line containing nothing but one non-breaking space is such a line — it looks empty, isn't, and silently throws away every owner or rule below it. Now each parser says so: one warning naming the line number and showing the character as \u00a0.

It warns only where something actually broke, which is the part the review cycle corrected. A ---/... document marker, a version: key before the first block, and a stray key between two complete blocks all leave every later default_pool:/rules: parsing, so they are silent — the first version annotated them anyway, asserting the block above was ended and the rules below were dropped, on documents whose parse is complete. Narrowing the trigger also removed a flood: a tab-indented or zero-indented config used to warn once per line and spend GitHub's ~10-annotation budget on symptoms.

Why not just treat those lines as blank

Because real YAML doesn't. PyYAML raises ScannerError on an NBSP-at-column-0 document and ReaderError on a form feed, so widening the blank-line test would make these parsers accept documents YAML rejects — a new divergence in place of the old one. Making the truncation visible is the fix that leaves parse results alone, which is why the corpus expected values in this diff are untouched.

What changed

.github/refresh-reviewers/generate.py (parse_reviewer_config) — at the outer-loop fallthrough, a non-empty line with _indent_of(raw) == 0 that matched neither key prints one ::warning::, on two arms:
- terminator — the line ended an open default_pool:/rules: block. Tracked by an ended_block flag the scans set at their break and the next outer iteration consumes; sound because both scans break on a non-blank column-0 line without advancing the cursor, so the next iteration is guaranteed to be the terminating line.
- near miss — the line names a supported key without opening one. Covers <BOM><BOM>default_pool: and <NEL>rules:, where the stray character is not indentation, and (new here) rules:v2: / default_pool:[alice].
Everything else at column 0 is silent, including ---/.... Indented fallthrough lines stay silent as before: they're the orphaned tail of an already-reported truncation, and one warning per orphan would bury the one that explains them.
_is_key replaces startswith on both ports. A bare prefix test claims a different key that shares the prefix: yaml.safe_load("rules:v2:\n - paths: [a]") is {"rules:v2": [...]}, a key named rules:v2, yet the prefix test parsed its children as live routing rules — silently honouring an unsupported key. Likewise yaml.safe_load("default_pool:[alice]") is the plain string default_pool:[alice], because YAML reads key: as a mapping only with s-white or a line end after the colon. Both ports now require that, so both shapes fall through to the near-miss warning instead. This is the one parse change in the PR (default_pool:[alice] no longer yields a pool) — deliberate, YAML-correct, loud, and corpus-pinned; see Residual.
_wc_escape in generate.py. ::warning:: was built by hand with no workflow-command escaping, so a % in a config token (printable ASCII, so _show_invisible left it) decoded into a mangled multi-line annotation, and a CR/LF in the caller-configured reviewer_config_path — interpolated raw — could start a second runner command. %→%25, CR→%0D, LF→%0A over the whole message, path prefix included, applied to the duplicate-key warning too. Same three characters and same order as @actions/core's escapeData, which is what core.warning already does for the JS port.
.github/workflows/assign-reviewers.yml (parseReviewerConfig, inside PARITY-HARNESS:config-parser) — the same conditions, same message text, via core.warning (which escapes for itself).
showInvisible moved up into the parity region. This is the one non-obvious edit and the riskiest line in the diff. The plan said to reuse the existing helper; it was declared at line 639 while parseReviewerConfig is called at line 598. A const sits in the temporal dead zone until its own line executes, so a parser reaching for it would have thrown ReferenceError at run time on any config with a stray column-0 key — i.e. it would have converted a silent truncation into a hard routing failure. It now lives above the parser, inside the region, which also makes it reachable from the extracted span the corpus harness executes. Its other caller (the invalid-configured-login warning) is unchanged. Note the pre-existing comment at that site already flagged exactly this trap for the escape helper declared further down — the hazard was documented, just not upstream of this change.
_show_invisible in generate.py — the Python port of it, so both ports render the escape \u00a0 identically rather than ascii()'s '\xa0'. See Deviations. It now emits a surrogate pair above the BMP: "\\u%04x" % ord(c) treats 4 as a minimum width, so U+1F600 rendered as the five-digit \u1f600 — not a valid \u escape, and ambiguous — while JS (no u flag on its regex) walks code units and emits \ud83d\ude00. The helper escapes the whole token, so any emoji in a stray line or a configured login diverged.
Both parsers take the config path (source_path / the existing sourcePath), so the annotation names the file a caller actually configured via reviewer_config_path instead of hardcoding reviewers.yml.
Docs — .github/assign-reviewers/README.md, docs/callers/assign-reviewers.md (now quoting the message a consumer will see) and docs/callers/refresh-reviewers.md (the drift run emits it too, and matters more there: the generator rewrites the truncated list it parsed and leaves the orphaned items below the stray line where they are).

Tests

.github/refresh-reviewers/tests/test_generate.py — NBSP-ends-default_pool and NBSP-ends-rules each warn exactly once and parse unchanged; the indented NBSP document is silent; an unknown owners: key at column 0 warns once; both warnings carry the configured path when given and no None: when not; a comments-and-blanks document of the shape every live consumer config has is silent.
.github/assign-reviewers/tests/assignment.test.cjs — the same cases through helperWarnings, asserting both the \u00a0 escape and the literal U+00A0 in the message's own prose, plus two full-script tests that run the whole shipped script: rather than the extracted region. Those exist specifically to catch the TDZ hazard above: they're the only thing that proves showInvisible resolves at both call sites, and the invalid-login path had no test at all before this PR, so the move would otherwise have been unverified where it was riskiest.
Both suites now carry an exhaustive table of which corpus cases warn and how many times, read in both directions. Per-case assertions can't catch the interesting regression here — one that made the warning fire on ordinary blank, comment or indented lines would warn on nearly every case and nothing would notice. The Python side compares assertEqual(counts, CORPUS_WARNINGS); the JS side gained the matching every CORPUS_WARNINGS key names a corpus case that still exists, so a renamed or deleted case can no longer silently retire its own assertion.
The negative half is now pinned directly rather than left to the acceptance-shape test: test_document_markers_are_silent, test_metadata_key_before_any_block_is_silent, test_non_yaml_file_is_silent, test_zero_indented_block_sequence_warns_once_not_per_item, and the near-miss trio (test_invisible_prefix_before_a_key_warns_as_a_near_miss, which also asserts a single leading BOM stays silent because the parser strips it; test_prefix_sharing_key_is_not_honoured_and_warns; test_no_space_after_the_colon_is_not_a_key). JS mirrors for each.
test_workflow_command_characters_are_escaped parses a config whose stray line is 100%0Adone with a source_path containing a newline, then asserts the output is a single physical line containing a%0Ab.yml: and (100%250Adone) — i.e. the runner decodes back to literal text rather than splitting the line or starting a second command.
New TestShowInvisible covers all three bands (BMP, printable ASCII, astral), and the JS suite asserts the surrogate pair from its side so a later u flag there cannot silently reopen the divergence.

Corpus sweep — including the part not being changed

The corpus is now 41 documents (37 before this PR), and the sweep is asserted exhaustively in both suites. 10 warn; the remaining 31 are silent, which is the number that matters: it's what says this doesn't fire on well-formed documents.

The three duplicate-default_pool cases keep their one warning each. Of the seven unrecognised-key warnings:

case	warnings	arm	why it's right
`two leading BOMs: only one is stripped`	1	near miss	line 1 is `<BOM>default_pool: [alice]` — unrecognised precisely because the second BOM is not indentation, which is the failure that case exists to pin. No block is open on line 1, so a pure block-termination test would have dropped this
`U+0085 before a top-level key is not indentation`	1	near miss	same, for a leading NEL
`a key sharing a prefix with \`rules:` is not the `rules:` key`	1	near miss	new; `rules:v2:` used to be silently honoured
`no space after the colon is a plain scalar, not a key`	1	near miss	new; `default_pool:[alice]` used to be silently accepted as a flow pool
the two `a U+00A0-only line at column 0 …` cases	1 each	terminator	the original target
`an unknown top-level key at column 0 ends the block`	1	terminator	a misspelling truncates exactly like invisible padding

Two cases that warned in the first version and are now silent, both correctly:

case	was	why silent now
`indented decoys and unknown top-level keys are ignored`	2	`version: 1` and `notes:` sit before the first block, so they end nothing and drop nothing — yet the annotation asserted both, on a document whose expected parse is complete
`YAML document markers are not unrecognised keys` (new)	—	`---`/`...` are syntax, not keys; yamllint's default `document-start` rule requires the leading `---`, so a conformant config has one

Acceptance

Both suites green; the corpus harness still extracts both regions by sentinel (the six sentinel tests and the whole-scalar bracket check all pass).
The document default_pool:\n - alice\n<U+00A0>\n - bob\n parses to ['alice'] on both ports, with exactly one warning naming line 3 and rendering the character. Verified against the shipped JS by executing the region out of the workflow file, and against the Python port directly. Message text is byte-identical across ports:

line 3 is not a recognised top-level key (\u00a0) — it ends the `default_pool:` block above it, and every list item or rule indented below it is dropped; only `default_pool:` and `rules:` are read, and at column 0 one invisible character (e.g. U+00A0) ends a block exactly like a misspelled key does

and the near-miss arm:

line 1 is not a recognised top-level key (rules:v2:) — it is not the supported `rules:` key, so the whole block it opens is ignored; that key is read only at column 0 with nothing but spaces before it and a space, a tab or the end of the line after its colon

Measured volume on the flood shapes, before → after: zero-indented block sequence 5 → 1, tab-indented config 6 → 2, reviewer_config_path aimed at a non-YAML file N → 0.

Zero warnings on all three live consumer .github/reviewers.yml files, on both ports — the two org consumers plus this repo's own. Exercised read-only against each clone's origin/main blob (git show, no checkout, no mutation); parsed pool/rule counts came back non-zero in every case, so the runs were not vacuous. The two org consumers are private repos, so they are not named here and no content from them appears in this PR. That check predates the review-cycle commit and was not re-run against it — see Residual.

Falsification of the capability claim

The warning asserts "only default_pool: and rules: are read", which denies a capability, so it was checked rather than assumed: both parsers match exactly those two strings at column 0 and nothing else; the corpus case indented decoys and unknown top-level keys are ignored already pinned that a version: key is ignored; and every other knob (num_reviewers, exclusions, the load cap) comes from workflow inputs and repo variables, not from this file. No third top-level key is read by either port, so the claim holds.

Residual

The warning is advisory, not enforced. Nothing fails a run or blocks a PR on a truncated reviewers.yml; a consumer who ignores the annotation still gets silently-dropped owners. That is the deliberate never-fail posture this shares with the duplicate-key and invalid-login warnings, not an oversight — but it does mean the fix depends on someone reading run annotations.
The live-consumer check was not re-run against the parse change, and cannot be from here. The _is_key tightening means a consumer whose reviewers.yml writes default_pool:[alice] with no space after the colon gets an empty pool where they previously got a pool — loudly warned about, but a behaviour change. The earlier zero-warning sweep over the three live configs predates this commit and covered the old parser; the two org consumers are private and unreadable from this sandbox, so it could not be repeated. It is a narrow shape (YAML itself reads it as a scalar, so no YAML-aware tool would have produced it) and this repo's own config does not use it, but worth confirming against the two org configs before merge — it is the only input in the PR whose parse result moves.
Unexercised artifacts. The upstream spike whose findings this implementation is derived from was not readable from this sandbox — no access — so the measurement it reports (which characters truncate on which port, on main versus post-#312) was re-derived here from the code and the corpus rather than taken on trust; it agreed. Its worker transcript is a local file on another host and likewise unread. The two private org consumer configs were read (see Acceptance) but cannot be quoted here.

Provenance

Authored by: agent-work loop
Verified: After merging origin/main (PR #311's parser convergence: last-wins duplicate keys, s-white comments, leading-BOM stripping) to resolve a real merge conflict across all 7 parser/test/doc files, re-ran both suites on the merged tree: python3 -m unittest discover -s .github/refresh-reviewers/tests -p 'test_*.py': 126 tests, 0 failures. node --test .github/assign-reviewers/tests/*.test.cjs: 159 tests, 0 fail. check_workflow_pins.py: OK, 156 pinned refs, 0 defaults, every ref checkout guarded. check_agents_md.py --root .: passed (2 pre-existing warnings, unrelated). check-org-repo-literals.sh: OK. The merge combined this PR's unrecognised-key warning logic with main's independently-landed warnConfiguredLogin broadening (JS port) without dropping either side's tests. Not green: .github/coderabbit-config fails one test (test_the_two_halves_partition_the_objects_that_declare_properties, 105 != 104) — reproduced identically on origin/main, pre-existing, untouched by this PR, and path-filtered out of its CI. Not re-run: the live-consumer config sweep (private repos, unreadable from here) — see Residual.
Deviations: Five, all disclosed above rather than silent. (1) The plan permitted ascii(line) on the Python side; it renders NBSP as '\xa0', which does not contain 00a0 and so misses the acceptance criterion that the character be findable as U+00A0 — a _show_invisible port of the JS helper is used instead, which also gives the two ports identical message text. (2) The plan said to reuse the existing JS showInvisible in place; done, but its declaration had to move up into the parity region, because as written it would have thrown a TDZ ReferenceError (details above). (3) The plan described appending to the why of "the three U+00A0-only line cases" the fact that both ports now warn; one of those three is the INDENTED case, which correctly does not warn, so its note records it as the negative control for the warning instead. (4) The plan specified the warning with no carve-outs; the review cycle established that firing on every column-0 fallthrough both overclaimed (the message asserted a block was ended and rules dropped on documents that parse completely) and flooded GitHub's ~10-annotation budget, so the trigger narrowed to two arms with ---/... carved out. That is a deliberate departure from the plan, not an omission. (5) Wider than asked, in three places the review cycle justified: the pre-existing Python duplicate-key warning picks up the new path prefix and the new workflow-command escaping, so the function's two warnings are consistent and match the JS port; and _is_key changes what parses (default_pool:[alice], rules:v2:), which the original scope explicitly excluded — taken because the PR's own warning was the thing failing to catch it, and flagged in Residual as the one input whose parse result moves.

_{🔄 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/313 **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-15377-reviewers-parser-unrecognised-key-warning` --- ### 📝 Commits (10+) - [`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 - [`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 - [`21ce8bf`](https://github.com/Comfy-Org/github-workflows/commit/21ce8bf81a03e8117220f1b3e24896d188d1a587) fix(reviewers): warn when an unrecognised column-0 line ends a default_pool/rules block, on both ports - [`ba664a5`](https://github.com/Comfy-Org/github-workflows/commit/ba664a5e2c78c74d0531ebfe1131556a88244701) fix(reviewers): warn only where a column-0 line breaks something, and escape the annotation - [`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) ### 📊 Changes **8 files changed** (+793 additions, -28 deletions) <details> <summary>View changed files</summary> 📝 `.github/assign-reviewers/README.md` (+15 -1) 📝 `.github/assign-reviewers/parser-corpus.json` (+41 -3) 📝 `.github/assign-reviewers/tests/assignment.test.cjs` (+196 -0) 📝 `.github/refresh-reviewers/generate.py` (+170 -5) 📝 `.github/refresh-reviewers/tests/test_generate.py` (+234 -6) 📝 `.github/workflows/assign-reviewers.yml` (+94 -9) 📝 `docs/callers/assign-reviewers.md` (+31 -4) 📝 `docs/callers/refresh-reviewers.md` (+12 -0) </details> ### 📄 Description ## ELI-5 Both copies of the `reviewers.yml` parser stop reading a list the moment they hit a line that starts at column 0 and isn't `default_pool:` or `rules:`. A line containing nothing but one non-breaking space *is* such a line — it looks empty, isn't, and silently throws away every owner or rule below it. Now each parser says so: one warning naming the line number and showing the character as `\u00a0`. It warns **only where something actually broke**, which is the part the review cycle corrected. A `---`/`...` document marker, a `version:` key before the first block, and a stray key between two complete blocks all leave every later `default_pool:`/`rules:` parsing, so they are silent — the first version annotated them anyway, asserting the block above was ended and the rules below were dropped, on documents whose parse is complete. Narrowing the trigger also removed a flood: a tab-indented or zero-indented config used to warn once per line and spend GitHub's ~10-annotation budget on symptoms. ## Why not just treat those lines as blank Because real YAML doesn't. PyYAML raises `ScannerError` on an NBSP-at-column-0 document and `ReaderError` on a form feed, so widening the blank-line test would make these parsers accept documents YAML rejects — a new divergence in place of the old one. Making the truncation *visible* is the fix that leaves parse results alone, which is why the corpus `expected` values in this diff are untouched. ## What changed - **`.github/refresh-reviewers/generate.py`** (`parse_reviewer_config`) — at the outer-loop fallthrough, a non-empty line with `_indent_of(raw) == 0` that matched neither key prints one `::warning::`, on **two arms**: - **terminator** — the line ended an open `default_pool:`/`rules:` block. Tracked by an `ended_block` flag the scans set at their `break` and the next outer iteration consumes; sound because both scans `break` on a non-blank column-0 line *without* advancing the cursor, so the next iteration is guaranteed to be the terminating line. - **near miss** — the line names a supported key without opening one. Covers `<BOM><BOM>default_pool:` and `<NEL>rules:`, where the stray character is not indentation, and (new here) `rules:v2:` / `default_pool:[alice]`. Everything else at column 0 is silent, including `---`/`...`. Indented fallthrough lines stay silent as before: they're the orphaned tail of an already-reported truncation, and one warning per orphan would bury the one that explains them. - **`_is_key` replaces `startswith` on both ports.** A bare prefix test claims a *different* key that shares the prefix: `yaml.safe_load("rules:v2:\n - paths: [a]")` is `{"rules:v2": [...]}`, a key **named** `rules:v2`, yet the prefix test parsed its children as live routing rules — silently honouring an unsupported key. Likewise `yaml.safe_load("default_pool:[alice]")` is the plain **string** `default_pool:[alice]`, because YAML reads `key:` as a mapping only with s-white or a line end after the colon. Both ports now require that, so both shapes fall through to the near-miss warning instead. This is the one **parse** change in the PR (`default_pool:[alice]` no longer yields a pool) — deliberate, YAML-correct, loud, and corpus-pinned; see Residual. - **`_wc_escape` in `generate.py`.** `::warning::` was built by hand with no workflow-command escaping, so a `%` in a config token (printable ASCII, so `_show_invisible` left it) decoded into a mangled multi-line annotation, and a CR/LF in the caller-configured `reviewer_config_path` — interpolated raw — could start a second runner command. `%`→`%25`, CR→`%0D`, LF→`%0A` over the whole message, path prefix included, applied to the duplicate-key warning too. Same three characters and same order as `@actions/core`'s `escapeData`, which is what `core.warning` already does for the JS port. - **`.github/workflows/assign-reviewers.yml`** (`parseReviewerConfig`, inside `PARITY-HARNESS:config-parser`) — the same conditions, same message text, via `core.warning` (which escapes for itself). - **`showInvisible` moved up into the parity region.** This is the one non-obvious edit and the riskiest line in the diff. The plan said to reuse the existing helper; it was declared at line 639 while `parseReviewerConfig` is *called* at line 598. A `const` sits in the temporal dead zone until its own line executes, so a parser reaching for it would have thrown `ReferenceError` at run time on any config with a stray column-0 key — i.e. it would have converted a silent truncation into a hard routing failure. It now lives above the parser, inside the region, which also makes it reachable from the extracted span the corpus harness executes. Its other caller (the invalid-configured-login warning) is unchanged. Note the pre-existing comment at that site already flagged exactly this trap for the `escape` helper declared further down — the hazard was documented, just not upstream of this change. - **`_show_invisible` in `generate.py`** — the Python port of it, so both ports render the escape `\u00a0` identically rather than `ascii()`'s `'\xa0'`. See Deviations. It now emits a **surrogate pair** above the BMP: `"\\u%04x" % ord(c)` treats 4 as a *minimum* width, so U+1F600 rendered as the five-digit `\u1f600` — not a valid `\u` escape, and ambiguous — while JS (no `u` flag on its regex) walks code units and emits `\ud83d\ude00`. The helper escapes the whole token, so any emoji in a stray line or a configured login diverged. - **Both parsers take the config path** (`source_path` / the existing `sourcePath`), so the annotation names the file a caller actually configured via `reviewer_config_path` instead of hardcoding `reviewers.yml`. - **Docs** — `.github/assign-reviewers/README.md`, `docs/callers/assign-reviewers.md` (now quoting the message a consumer will see) and `docs/callers/refresh-reviewers.md` (the drift run emits it too, and matters more there: the generator rewrites the *truncated* list it parsed and leaves the orphaned items below the stray line where they are). ## Tests - `.github/refresh-reviewers/tests/test_generate.py` — NBSP-ends-`default_pool` and NBSP-ends-`rules` each warn exactly once and parse unchanged; the indented NBSP document is silent; an unknown `owners:` key at column 0 warns once; both warnings carry the configured path when given and no `None:` when not; a comments-and-blanks document of the shape every live consumer config has is silent. - `.github/assign-reviewers/tests/assignment.test.cjs` — the same cases through `helperWarnings`, asserting both the `\u00a0` escape and the literal `U+00A0` in the message's own prose, plus **two full-script tests** that run the whole shipped `script:` rather than the extracted region. Those exist specifically to catch the TDZ hazard above: they're the only thing that proves `showInvisible` resolves at *both* call sites, and the invalid-login path had **no test at all** before this PR, so the move would otherwise have been unverified where it was riskiest. - **Both suites now carry an exhaustive table of which corpus cases warn and how many times**, read in **both directions**. Per-case assertions can't catch the interesting regression here — one that made the warning fire on ordinary blank, comment or indented lines would warn on nearly every case and nothing would notice. The Python side compares `assertEqual(counts, CORPUS_WARNINGS)`; the JS side gained the matching `every CORPUS_WARNINGS key names a corpus case that still exists`, so a renamed or deleted case can no longer silently retire its own assertion. - **The negative half is now pinned directly** rather than left to the acceptance-shape test: `test_document_markers_are_silent`, `test_metadata_key_before_any_block_is_silent`, `test_non_yaml_file_is_silent`, `test_zero_indented_block_sequence_warns_once_not_per_item`, and the near-miss trio (`test_invisible_prefix_before_a_key_warns_as_a_near_miss`, which also asserts a **single** leading BOM stays silent because the parser strips it; `test_prefix_sharing_key_is_not_honoured_and_warns`; `test_no_space_after_the_colon_is_not_a_key`). JS mirrors for each. - `test_workflow_command_characters_are_escaped` parses a config whose stray line is `100%0Adone` with a `source_path` containing a newline, then asserts the output is a **single** physical line containing `a%0Ab.yml: ` and `(100%250Adone)` — i.e. the runner decodes back to literal text rather than splitting the line or starting a second command. - New `TestShowInvisible` covers all three bands (BMP, printable ASCII, astral), and the JS suite asserts the surrogate pair from its side so a later `u` flag there cannot silently reopen the divergence. ## Corpus sweep — including the part not being changed The corpus is now **41** documents (37 before this PR), and the sweep is asserted exhaustively in both suites. **10** warn; the remaining **31** are silent, which is the number that matters: it's what says this doesn't fire on well-formed documents. The three duplicate-`default_pool` cases keep their one warning each. Of the seven unrecognised-key warnings: | case | warnings | arm | why it's right | |---|---|---|---| | `two leading BOMs: only one is stripped` | 1 | near miss | line 1 is `<BOM>default_pool: [alice]` — unrecognised *precisely because* the second BOM is not indentation, which is the failure that case exists to pin. No block is open on line 1, so a pure block-termination test would have dropped this | | `U+0085 before a top-level key is not indentation` | 1 | near miss | same, for a leading NEL | | `a key sharing a prefix with \`rules:\` is not the \`rules:\` key` | 1 | near miss | new; `rules:v2:` used to be silently honoured | | `no space after the colon is a plain scalar, not a key` | 1 | near miss | new; `default_pool:[alice]` used to be silently accepted as a flow pool | | the two `a U+00A0-only line at column 0 …` cases | 1 each | terminator | the original target | | `an unknown top-level key at column 0 ends the block` | 1 | terminator | a misspelling truncates exactly like invisible padding | Two cases that warned in the first version and are now **silent**, both correctly: | case | was | why silent now | |---|---|---| | `indented decoys and unknown top-level keys are ignored` | 2 | `version: 1` and `notes:` sit before the first block, so they end nothing and drop nothing — yet the annotation asserted both, on a document whose expected parse is complete | | `YAML document markers are not unrecognised keys` (new) | — | `---`/`...` are syntax, not keys; yamllint's default `document-start` rule *requires* the leading `---`, so a conformant config has one | ## Acceptance - Both suites green; the corpus harness still extracts both regions by sentinel (the six sentinel tests and the whole-scalar bracket check all pass). - The document `default_pool:\n - alice\n<U+00A0>\n - bob\n` parses to `['alice']` on both ports, with exactly one warning naming **line 3** and rendering the character. Verified against the shipped JS by executing the region out of the workflow file, and against the Python port directly. Message text is byte-identical across ports: ``` line 3 is not a recognised top-level key (\u00a0) — it ends the `default_pool:` block above it, and every list item or rule indented below it is dropped; only `default_pool:` and `rules:` are read, and at column 0 one invisible character (e.g. U+00A0) ends a block exactly like a misspelled key does ``` and the near-miss arm: ``` line 1 is not a recognised top-level key (rules:v2:) — it is not the supported `rules:` key, so the whole block it opens is ignored; that key is read only at column 0 with nothing but spaces before it and a space, a tab or the end of the line after its colon ``` Measured volume on the flood shapes, before → after: zero-indented block sequence **5 → 1**, tab-indented config **6 → 2**, `reviewer_config_path` aimed at a non-YAML file **N → 0**. - **Zero warnings on all three live consumer `.github/reviewers.yml` files**, on **both** ports — the two org consumers plus this repo's own. Exercised read-only against each clone's `origin/main` blob (`git show`, no checkout, no mutation); parsed pool/rule counts came back non-zero in every case, so the runs were not vacuous. The two org consumers are private repos, so they are not named here and no content from them appears in this PR. **That check predates the review-cycle commit and was not re-run against it** — see Residual. ## Falsification of the capability claim The warning asserts "only `default_pool:` and `rules:` are read", which denies a capability, so it was checked rather than assumed: both parsers match exactly those two strings at column 0 and nothing else; the corpus case `indented decoys and unknown top-level keys are ignored` already pinned that a `version:` key is ignored; and every other knob (`num_reviewers`, exclusions, the load cap) comes from workflow inputs and repo variables, not from this file. No third top-level key is read by either port, so the claim holds. ## Residual - **The warning is advisory, not enforced.** Nothing fails a run or blocks a PR on a truncated `reviewers.yml`; a consumer who ignores the annotation still gets silently-dropped owners. That is the deliberate never-fail posture this shares with the duplicate-key and invalid-login warnings, not an oversight — but it does mean the fix depends on someone reading run annotations. - **The live-consumer check was not re-run against the parse change, and cannot be from here.** The `_is_key` tightening means a consumer whose `reviewers.yml` writes `default_pool:[alice]` with no space after the colon gets an **empty pool** where they previously got a pool — loudly warned about, but a behaviour change. The earlier zero-warning sweep over the three live configs predates this commit and covered the old parser; the two org consumers are private and unreadable from this sandbox, so it could not be repeated. It is a narrow shape (YAML itself reads it as a scalar, so no YAML-aware tool would have produced it) and this repo's own config does not use it, but **worth confirming against the two org configs before merge** — it is the only input in the PR whose parse result moves. - **Unexercised artifacts.** The upstream spike whose findings this implementation is derived from was not readable from this sandbox — no access — so the measurement it reports (which characters truncate on which port, on `main` versus post-#312) was re-derived here from the code and the corpus rather than taken on trust; it agreed. Its worker transcript is a local file on another host and likewise unread. The two private org consumer configs were read (see Acceptance) but cannot be quoted here. ## Provenance - **Authored by:** agent-work loop - **Verified:** After merging `origin/main` (PR #311's parser convergence: last-wins duplicate keys, s-white comments, leading-BOM stripping) to resolve a real merge conflict across all 7 parser/test/doc files, re-ran both suites on the merged tree: `python3 -m unittest discover -s .github/refresh-reviewers/tests -p 'test_*.py'`: 126 tests, 0 failures. `node --test .github/assign-reviewers/tests/*.test.cjs`: 159 tests, 0 fail. `check_workflow_pins.py`: OK, 156 pinned refs, 0 defaults, every ref checkout guarded. `check_agents_md.py --root .`: passed (2 pre-existing warnings, unrelated). `check-org-repo-literals.sh`: OK. The merge combined this PR's unrecognised-key warning logic with main's independently-landed `warnConfiguredLogin` broadening (JS port) without dropping either side's tests. **Not green:** `.github/coderabbit-config` fails one test (`test_the_two_halves_partition_the_objects_that_declare_properties`, `105 != 104`) — reproduced identically on `origin/main`, pre-existing, untouched by this PR, and path-filtered out of its CI. **Not re-run:** the live-consumer config sweep (private repos, unreadable from here) — see Residual. - **Deviations:** Five, all disclosed above rather than silent. (1) The plan permitted `ascii(line)` on the Python side; it renders NBSP as `'\xa0'`, which does not contain `00a0` and so misses the acceptance criterion that the character be findable as `U+00A0` — a `_show_invisible` port of the JS helper is used instead, which also gives the two ports identical message text. (2) The plan said to reuse the existing JS `showInvisible` in place; done, but its *declaration* had to move up into the parity region, because as written it would have thrown a TDZ `ReferenceError` (details above). (3) The plan described appending to the `why` of "the three `U+00A0-only line` cases" the fact that both ports now warn; one of those three is the INDENTED case, which correctly does **not** warn, so its note records it as the negative control for the warning instead. (4) The plan specified the warning **with no carve-outs**; the review cycle established that firing on every column-0 fallthrough both overclaimed (the message asserted a block was ended and rules dropped on documents that parse completely) and flooded GitHub's ~10-annotation budget, so the trigger narrowed to two arms with `---`/`...` carved out. That is a deliberate departure from the plan, not an omission. (5) Wider than asked, in three places the review cycle justified: the pre-existing Python duplicate-key warning picks up the new path prefix *and* the new workflow-command escaping, so the function's two warnings are consistent and match the JS port; and `_is_key` changes what **parses** (`default_pool:[alice]`, `rules:v2:`), which the original scope explicitly excluded — taken because the PR's own warning was the thing failing to catch it, and flagged in Residual as the one input whose parse result moves. --- <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:23 +00:00