[PR #309] [MERGED] fix(refresh-reviewers): byte-faithful rewrite — bytes read, CRLF-aware block inserts, refuse multi-line flow sequences #309

New issue

Closed

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

bsakaguchi commented

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

Owner

📋 Pull Request Information

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

Base: main ← Head: matt/be-15304-refresh-reviewers-byte-faithful-rewrite

📝 Commits (2)

b3916b9 fix(refresh-reviewers): byte-faithful rewrite — bytes read, CRLF-aware block inserts, refuse multi-line flow sequences
0bc0348 fix(refresh-reviewers): make the skip decision reach the report, and the endings reach every inserted line

📊 Changes

4 files changed (+1077 additions, -87 deletions)

View changed files

📝 .github/refresh-reviewers/README.md (+34 -2)
📝 .github/refresh-reviewers/generate.py (+332 -53)
📝 .github/refresh-reviewers/tests/test_generate.py (+693 -27)
📝 docs/callers/refresh-reviewers.md (+18 -5)

📄 Description

ELI-5

refresh-reviewers promises to touch only the reviewer lists in your .github/reviewers.yml and leave every other byte alone. Three things quietly broke that promise. If your file used Windows line endings, the whole file came back with Unix ones. If a reviewer list was written as a block list, the replacement items came out with Unix endings even in a Windows file. And if a list was spread over two lines (reviewers: [alice, … bob]), the rewrite edited the first line and left bob] stranded, producing a file that is not valid YAML at all. This PR reads the file as raw bytes, carries each line's own ending through the rewrite, and refuses to touch a list it cannot rewrite on one line — warning about it instead and saying how to fix it.

What changed

generate.py — read the committed config without newline translation. The git show that loads the config ran with subprocess.run(..., text=True); Python's universal-newline translation turns a CRLF config's \r\n into \n before the surgical rewrite ever sees the file, so every line of the proposal differs from the committed bytes. It is now read as bytes and decoded explicitly in a new, unit-testable read_committed_config(branch, config_path) -> str | None. A blob that is not valid UTF-8 now takes the documented clean no-op (with a ::warning:: naming the offending byte offset) rather than a lossy replace-decoded rewrite of somebody's committed file. The proposal is written with newline="" so the runner OS cannot translate the endings a second time on the way out.
rewrite_config — block-form insertions carry the document's line ending. text.split("\n") leaves the \r on the end of every CRLF line, so the flow and scalar rewrites — which edit a span within a line — were already byte-faithful. Only the inserted block item lines had no line to inherit from. Each inserted item now copies the ending of the item line it replaces, detected per line, so a deliberately mixed-ending file comes back exactly as mixed as it went in.
rewrite_config — refuse to rewrite an unterminated (multi-line) flow sequence. Every edit the rewrite makes is confined to the one line a location names, so reviewers: [alice, whose ] lives on a later line has no location shape it can safely take: replacing the opening line's [...] span strands the continuation lines as orphaned YAML. A new _flow_is_unterminated() tags those locations ("unterminated", i) instead of ("flow", i) at the two sites that can produce one (the default_pool line and set_key's flow branch; the scalar branch cannot). rewrite_config skips them, a new public unterminated_locations(locs) names them as ("default_pool" | "rules[<n>].reviewers", line_idx), main emits one ::warning:: per list with file and line, and the drift PR's body gains a line naming each skipped list and the one-line fix.
_parse_flow is untouched, so the config half of parse_reviewer_config's return is unchanged — only the location tag moved. Proven directly rather than assumed: see Verified.
Docs. The surgical-rewrite paragraphs in .github/refresh-reviewers/README.md and docs/callers/refresh-reviewers.md now state the line-ending guarantee and both shapes the rewrite deliberately cannot manage (the pre-existing inline-comment-on-a-block-item caveat, and the new multi-line flow one). The "Location shapes" comment block in generate.py gains the third shape and the reason it exists.

Reachability

The entrypoint is the workflow_call reusable refresh-reviewers.yml; the producer is a human editing a caller repo's .github/reviewers.yml. No config in any current caller triggers either defect, so this is a latent fix, not a live incident. No behaviour changes at all for an LF, single-line config — the existing byte-identical tests are untouched and green.

Verified

Beyond unit tests, both defects were reproduced and the fix confirmed end to end through the real main() entrypoint, against throwaway git repos, with the GitHub API stubbed:

CRLF config (7 committed lines, CRLF throughout). Before: the proposal came out with 0 CRLF and 6 bare LF — every line's bytes differ from what was committed. After: 6 CRLF, 0 bare LF, and only the two intentionally-edited lists differ from the committed text.
Multi-line flow sequence (default_pool: [old-a, / old-b]). Before: the proposal was default_pool: [alice] / old-b] — a file yaml.safe_load refuses (while parsing a block mapping), i.e. a drift PR that, if merged, breaks the consumer's reviewer config outright. After: the list is left exactly as committed, ::warning::.github/reviewers.yml:1: default_pool is a multi-line flow sequence… is emitted, the PR body names it, and the output loads cleanly.

Negative-claim falsification

This diff ships a refusal path ("left unchanged", "the generator only rewrites single-line lists"), so the capability-denial check applies. The denied capability was attempted via every path the product offers before the refusal was written: rewrite_config is the only writer of a reviewers.yml anywhere in this repo — assign-reviewers.yml's JS parseReviewerConfig is read-only, and no script or workflow emits one — and running that one path on a multi-line flow sequence produces the unparseable output quoted above. So the behaviour being removed is corruption, not function. The refusal also ships a working redirect rather than a dead end: putting the list on one line is handled by the flow rewrite, which is covered by green tests, and both the ::warning:: and the PR-body line say exactly that.

Judgment calls

PyYAML is not installed in CI. The plan preferred adding pip install pyyaml to test-refresh-reviewers.yml so the yaml.safe_load leg runs there. AGENTS.md pins this repo to stdlib-only Python with one narrowly-scoped, hash-pinned exception, and widening that would be a change to a gated invariant rather than a test tweak — so the leg is kept skipUnless-guarded (the plan's stated alternative) and it will skip on CI. It is not load-bearing: the stdlib re-parse leg beside it reds on the same defect, confirmed by the mutation check below.
read_committed_config keeps the specified str | None signature, which cannot distinguish "missing" from "not UTF-8" at the call site. Both diagnostics are preserved anyway: the helper prints the specific decode error itself, and the caller's no-op reason names both causes.
newline="" has no test, deliberately. Python only translates line endings on write on Windows, so any assertion about it is vacuous on the ubuntu-latest runner. The inline comment carries the reason instead.
report.json's per-list changed flag can read true for a list the rewrite skipped as unterminated (the refresher still computes what it would propose, which is the more useful thing to show a human next to the "put it on one line" note). The top-level changed, which gates whether a PR opens at all, is computed from the actual text diff and stays correct, and skipped_unterminated in the same report disambiguates.

Residual

Multi-line flow sequences are still mis-PARSED, by both ports. [alice, / bob] parses to ["alice"] — the continuation logins are dropped — in the Python generator and in assign-reviewers.yml's JS parser alike, so a consumer with that shape is already being routed on a truncated reviewer list at assignment time. This PR deliberately does not touch that: the parse half must stay in lockstep with the JS port, so fixing it is a two-port change with its own compatibility surface and is worth its own ticket. What this PR guarantees is only that the rewrite can no longer turn that mis-parse into a corrupted file.
The shared parser corpus is not yet on main. The upstream PR that adds TestSharedParserCorpus to tests/test_generate.py (and the docstrings that go with it) is still open, so this branch is based on origin/main at cf7b8d9 and will need a rebase once that lands; expect a textual conflict in tests/test_generate.py around the class list, not a semantic one. Because that corpus does not exist yet, it could not be exercised as an acceptance artifact. Two substitutes were run instead: the config half of parse_reviewer_config was diffed against the pre-change parser across 24 fixtures (LF and CRLF, including both unterminated shapes, empty flows and a ] inside a trailing comment) and is identical in all 24, and .github/assign-reviewers/tests/assignment.test.cjs — the existing cross-port suite — is unmodified and green at 68/68. A reviewer should still re-run the corpus after the rebase.
Two artifacts named upstream could not be read. The originating read-only investigation's findings comment lives in a private tracker this branch has no access to, and the first consumer's caller PR is in a private repository; neither was opened, so this change rests on the plan text plus the defects independently reproduced here (all three were reproduced from scratch on cf7b8d9 before any code was written). Nothing in either artifact was assumed.
Mutation check, as specified. Reverting the block-insert change alone reds 4 tests (test_block_rewrite_keeps_crlf, test_block_default_pool_keeps_crlf, and test_line_endings_are_never_mixed_in on the block/crlf and block-pool/crlf cases). Reverting the unterminated-flow change alone reds 12 (the round-trip re-parse leg on all four unterminated cases, the yaml.safe_load leg on the same four, and the four targeted tests). Reverting the bytes-read change alone reds the 2 read-helper tests. Full suite green at 76 tests; repo self-checks check_workflow_pins.py and check_agents_md.py --root . both pass (the two check_agents_md warnings — AGENTS.md at 155 lines, no CODEOWNERS — are pre-existing on main and untouched here).

Provenance

Authored by: agent-work loop
Verified: python3 -m unittest discover -s .github/refresh-reviewers/tests -p 'test_*.py' -v: 76 passed, 0 failed; node .github/assign-reviewers/tests/assignment.test.cjs: 68 passed, 0 failed (file unmodified); python3 .github/workflow-pins/check_workflow_pins.py: OK, 11 workflows, 0 violations; python3 .github/agents-md-integrity/check_agents_md.py --root .: passed with 2 pre-existing warnings; three mutation reverts each reding only their intended tests (4 / 12 / 2); two end-to-end main() runs against throwaway git repos with the API stubbed, results quoted under Verified.
Deviations: pip install pyyaml was not added to .github/workflows/test-refresh-reviewers.yml — see Judgment calls; the yaml.safe_load leg skips on CI and the stdlib re-parse leg covers the same regression. Not rebased onto the still-open shared-parser-corpus PR — see Residual.

_{🔄 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/309 **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-15304-refresh-reviewers-byte-faithful-rewrite` --- ### 📝 Commits (2) - [`b3916b9`](https://github.com/Comfy-Org/github-workflows/commit/b3916b9dd0b75a5aeb3f20f66d494f447dce2b16) fix(refresh-reviewers): byte-faithful rewrite — bytes read, CRLF-aware block inserts, refuse multi-line flow sequences - [`0bc0348`](https://github.com/Comfy-Org/github-workflows/commit/0bc03489e45e56024f5ecb7421856642653f259d) fix(refresh-reviewers): make the skip decision reach the report, and the endings reach every inserted line ### 📊 Changes **4 files changed** (+1077 additions, -87 deletions) <details> <summary>View changed files</summary> 📝 `.github/refresh-reviewers/README.md` (+34 -2) 📝 `.github/refresh-reviewers/generate.py` (+332 -53) 📝 `.github/refresh-reviewers/tests/test_generate.py` (+693 -27) 📝 `docs/callers/refresh-reviewers.md` (+18 -5) </details> ### 📄 Description ## ELI-5 `refresh-reviewers` promises to touch only the reviewer lists in your `.github/reviewers.yml` and leave every other byte alone. Three things quietly broke that promise. If your file used Windows line endings, the whole file came back with Unix ones. If a reviewer list was written as a block list, the replacement items came out with Unix endings even in a Windows file. And if a list was spread over two lines (`reviewers: [alice,` … `bob]`), the rewrite edited the first line and left `bob]` stranded, producing a file that is not valid YAML at all. This PR reads the file as raw bytes, carries each line's own ending through the rewrite, and refuses to touch a list it cannot rewrite on one line — warning about it instead and saying how to fix it. ## What changed - **`generate.py` — read the committed config without newline translation.** The `git show` that loads the config ran with `subprocess.run(..., text=True)`; Python's universal-newline translation turns a CRLF config's `\r\n` into `\n` before the surgical rewrite ever sees the file, so every line of the proposal differs from the committed bytes. It is now read as bytes and decoded explicitly in a new, unit-testable `read_committed_config(branch, config_path) -> str | None`. A blob that is not valid UTF-8 now takes the documented clean no-op (with a `::warning::` naming the offending byte offset) rather than a lossy `replace`-decoded rewrite of somebody's committed file. The proposal is written with `newline=""` so the runner OS cannot translate the endings a second time on the way out. - **`rewrite_config` — block-form insertions carry the document's line ending.** `text.split("\n")` leaves the `\r` on the end of every CRLF line, so the flow and scalar rewrites — which edit a span *within* a line — were already byte-faithful. Only the *inserted* block item lines had no line to inherit from. Each inserted item now copies the ending of the item line it replaces, detected per line, so a deliberately mixed-ending file comes back exactly as mixed as it went in. - **`rewrite_config` — refuse to rewrite an unterminated (multi-line) flow sequence.** Every edit the rewrite makes is confined to the one line a location names, so `reviewers: [alice,` whose `]` lives on a later line has no location shape it can safely take: replacing the opening line's `[...]` span strands the continuation lines as orphaned YAML. A new `_flow_is_unterminated()` tags those locations `("unterminated", i)` instead of `("flow", i)` at the two sites that can produce one (the `default_pool` line and `set_key`'s flow branch; the scalar branch cannot). `rewrite_config` skips them, a new public `unterminated_locations(locs)` names them as `("default_pool" | "rules[<n>].reviewers", line_idx)`, `main` emits one `::warning::` per list with file and line, and the drift PR's body gains a line naming each skipped list and the one-line fix. - **`_parse_flow` is untouched**, so the `config` half of `parse_reviewer_config`'s return is unchanged — only the location tag moved. Proven directly rather than assumed: see *Verified*. - **Docs.** The surgical-rewrite paragraphs in `.github/refresh-reviewers/README.md` and `docs/callers/refresh-reviewers.md` now state the line-ending guarantee and both shapes the rewrite deliberately cannot manage (the pre-existing inline-comment-on-a-block-item caveat, and the new multi-line flow one). The "Location shapes" comment block in `generate.py` gains the third shape and the reason it exists. ## Reachability The entrypoint is the `workflow_call` reusable `refresh-reviewers.yml`; the producer is a human editing a caller repo's `.github/reviewers.yml`. No config in any current caller triggers either defect, so this is a **latent** fix, not a live incident. No behaviour changes at all for an LF, single-line config — the existing byte-identical tests are untouched and green. ## Verified Beyond unit tests, both defects were reproduced and the fix confirmed end to end through the real `main()` entrypoint, against throwaway git repos, with the GitHub API stubbed: - **CRLF config** (7 committed lines, CRLF throughout). Before: the proposal came out with `0` CRLF and `6` bare LF — every line's bytes differ from what was committed. After: `6` CRLF, `0` bare LF, and only the two intentionally-edited lists differ from the committed text. - **Multi-line flow sequence** (`default_pool: [old-a,` / ` old-b]`). Before: the proposal was `default_pool: [alice]` / ` old-b]` — a file `yaml.safe_load` refuses (`while parsing a block mapping`), i.e. a drift PR that, if merged, breaks the consumer's reviewer config outright. After: the list is left exactly as committed, `::warning::.github/reviewers.yml:1: default_pool is a multi-line flow sequence…` is emitted, the PR body names it, and the output loads cleanly. ## Negative-claim falsification This diff ships a refusal path ("left unchanged", "the generator only rewrites single-line lists"), so the capability-denial check applies. The denied capability was attempted via every path the product offers before the refusal was written: `rewrite_config` is the **only** writer of a `reviewers.yml` anywhere in this repo — `assign-reviewers.yml`'s JS `parseReviewerConfig` is read-only, and no script or workflow emits one — and running that one path on a multi-line flow sequence produces the unparseable output quoted above. So the behaviour being removed is corruption, not function. The refusal also ships a working redirect rather than a dead end: putting the list on one line is handled by the flow rewrite, which is covered by green tests, and both the `::warning::` and the PR-body line say exactly that. ## Judgment calls - **PyYAML is not installed in CI.** The plan preferred adding `pip install pyyaml` to `test-refresh-reviewers.yml` so the `yaml.safe_load` leg runs there. `AGENTS.md` pins this repo to stdlib-only Python with one narrowly-scoped, hash-pinned exception, and widening that would be a change to a gated invariant rather than a test tweak — so the leg is kept `skipUnless`-guarded (the plan's stated alternative) and it will **skip** on CI. It is not load-bearing: the stdlib re-parse leg beside it reds on the same defect, confirmed by the mutation check below. - **`read_committed_config` keeps the specified `str | None` signature**, which cannot distinguish "missing" from "not UTF-8" at the call site. Both diagnostics are preserved anyway: the helper prints the specific decode error itself, and the caller's no-op reason names both causes. - **`newline=""` has no test**, deliberately. Python only translates line endings on write on Windows, so any assertion about it is vacuous on the `ubuntu-latest` runner. The inline comment carries the reason instead. - **`report.json`'s per-list `changed` flag** can read `true` for a list the rewrite skipped as unterminated (the refresher still computes what it *would* propose, which is the more useful thing to show a human next to the "put it on one line" note). The top-level `changed`, which gates whether a PR opens at all, is computed from the actual text diff and stays correct, and `skipped_unterminated` in the same report disambiguates. ## Residual - **Multi-line flow sequences are still mis-PARSED, by both ports.** `[alice,` / ` bob]` parses to `["alice"]` — the continuation logins are dropped — in the Python generator and in `assign-reviewers.yml`'s JS parser alike, so a consumer with that shape is already being routed on a truncated reviewer list at assignment time. This PR deliberately does not touch that: the parse half must stay in lockstep with the JS port, so fixing it is a two-port change with its own compatibility surface and is worth its own ticket. What this PR guarantees is only that the *rewrite* can no longer turn that mis-parse into a corrupted file. - **The shared parser corpus is not yet on `main`.** The upstream PR that adds `TestSharedParserCorpus` to `tests/test_generate.py` (and the docstrings that go with it) is still **open**, so this branch is based on `origin/main` at `cf7b8d9` and **will need a rebase** once that lands; expect a textual conflict in `tests/test_generate.py` around the class list, not a semantic one. Because that corpus does not exist yet, it could not be exercised as an acceptance artifact. Two substitutes were run instead: the `config` half of `parse_reviewer_config` was diffed against the pre-change parser across 24 fixtures (LF and CRLF, including both unterminated shapes, empty flows and a `]` inside a trailing comment) and is **identical in all 24**, and `.github/assign-reviewers/tests/assignment.test.cjs` — the existing cross-port suite — is unmodified and green at 68/68. A reviewer should still re-run the corpus after the rebase. - **Two artifacts named upstream could not be read.** The originating read-only investigation's findings comment lives in a private tracker this branch has no access to, and the first consumer's caller PR is in a private repository; neither was opened, so this change rests on the plan text plus the defects independently reproduced here (all three were reproduced from scratch on `cf7b8d9` before any code was written). Nothing in either artifact was assumed. - **Mutation check, as specified.** Reverting the block-insert change alone reds 4 tests (`test_block_rewrite_keeps_crlf`, `test_block_default_pool_keeps_crlf`, and `test_line_endings_are_never_mixed_in` on the `block/crlf` and `block-pool/crlf` cases). Reverting the unterminated-flow change alone reds 12 (the round-trip re-parse leg on all four unterminated cases, the `yaml.safe_load` leg on the same four, and the four targeted tests). Reverting the bytes-read change alone reds the 2 read-helper tests. Full suite green at 76 tests; repo self-checks `check_workflow_pins.py` and `check_agents_md.py --root .` both pass (the two `check_agents_md` warnings — `AGENTS.md` at 155 lines, no `CODEOWNERS` — are pre-existing on `main` and untouched here). ## Provenance - **Authored by:** agent-work loop - **Verified:** `python3 -m unittest discover -s .github/refresh-reviewers/tests -p 'test_*.py' -v`: 76 passed, 0 failed; `node .github/assign-reviewers/tests/assignment.test.cjs`: 68 passed, 0 failed (file unmodified); `python3 .github/workflow-pins/check_workflow_pins.py`: OK, 11 workflows, 0 violations; `python3 .github/agents-md-integrity/check_agents_md.py --root .`: passed with 2 pre-existing warnings; three mutation reverts each reding only their intended tests (4 / 12 / 2); two end-to-end `main()` runs against throwaway git repos with the API stubbed, results quoted under *Verified*. - **Deviations:** `pip install pyyaml` was not added to `.github/workflows/test-refresh-reviewers.yml` — see *Judgment calls*; the `yaml.safe_load` leg skips on CI and the stdlib re-parse leg covers the same regression. Not rebased onto the still-open shared-parser-corpus PR — see *Residual*. --- <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:19 +00:00