-
-
Notifications
You must be signed in to change notification settings - Fork 3
Add a trailing-newline gate over changelog.d and the pinned data READMEs: the defect has now shipped in three PRs and no gate anywhere in the repo checks for it #368
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,2 @@ | ||
| ### Added | ||
| - Trailing-newline gate added to prevent silent corruption of release notes from missing trailing newlines in changelog fragments and the benchmarks data README. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,116 @@ | ||
| #!/usr/bin/env python3 | ||
| """Trailing-newline gate. | ||
|
|
||
| Verifies that every file in scope ends with exactly one ``0x0a`` (newline). | ||
| A file missing its trailing newline silently concatenates the next appended | ||
| entry onto the last line, corrupting release notes in ``changelog.d/`` and | ||
| the pinned-data ``README.md``. | ||
|
|
||
| Scope (deliberately narrow): | ||
| - ``changelog.d/*.md`` | ||
| - ``benchmarks/data/README.md`` | ||
|
|
||
| An optional ``TRAILING_NEWLINE_ROOT`` environment variable overrides the repo | ||
| root the gate scans (defaulting to the tree containing this script). This lets | ||
| the gate target an arbitrary checkout or a fixture directory -- useful for CI | ||
| on staged changes and for reproducible local demos. | ||
|
|
||
| Usage: | ||
| python scripts/check_trailing_newline.py | ||
| TRAILING_NEWLINE_ROOT=/tmp/fixture python scripts/check_trailing_newline.py | ||
| (exits 0 when every file in scope ends in exactly one newline, 1 otherwise) | ||
| """ | ||
| from __future__ import annotations | ||
|
|
||
| import os | ||
| import sys | ||
| from pathlib import Path | ||
|
|
||
| _REPO_ROOT = Path(__file__).resolve().parent.parent | ||
| REPO_ROOT = Path(os.environ.get("TRAILING_NEWLINE_ROOT") or _REPO_ROOT) | ||
|
|
||
| SCOPE_GLOBS = ( | ||
| "changelog.d/*.md", | ||
| "benchmarks/data/README.md", | ||
| ) | ||
|
|
||
|
|
||
| def _last_byte_hex(file_path: Path) -> str | None: | ||
| """Return the last byte of *file_path* as a two-digit hex string, or None | ||
| if the file has fewer than 1 byte.""" | ||
| try: | ||
| data = file_path.read_bytes() | ||
| except (OSError,): | ||
| return None | ||
| if len(data) == 0: | ||
| return None | ||
| return f"{(data[-1]):02x}" | ||
|
|
||
|
|
||
| def _check_file(file_path: Path) -> tuple[bool, str | None]: | ||
| """Check whether *file_path* ends with exactly one newline. | ||
|
|
||
| Returns ``(pass, last_byte_hex)``: | ||
| - ``pass`` is ``True`` when the file ends with exactly one ``0x0a``. | ||
| - ``last_byte_hex`` is the hex value of the actual last byte when | ||
| ``pass`` is ``False`` (``None`` when the file is empty or when pass | ||
| is True). | ||
| """ | ||
| try: | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [WARNING]: Catching Reply with |
||
| data = file_path.read_bytes() | ||
| except (OSError,): | ||
| return True, None | ||
|
|
||
| # Empty file: not an offender (nothing to concatenate onto). | ||
| if len(data) == 0: | ||
| return True, None | ||
|
|
||
| # Last byte must be 0x0a. | ||
| if data[-1] != 0x0a: | ||
| last_hex = f"{(data[-1]):02x}" | ||
| return False, last_hex | ||
|
|
||
| # Last byte is 0x0a. Check that the second-to-last byte is NOT 0x0a | ||
| # (which would be a blank line at EOF, i.e. more than one trailing newline). | ||
| if len(data) >= 2 and data[-2] == 0x0a: | ||
| # Blank line at EOF: the file ends with 0x0a0a or more. | ||
| # The last byte is still 0x0a, but it's not "exactly one". | ||
| last_hex = "0a" | ||
| return False, last_hex | ||
|
|
||
| # Exactly one trailing newline (0x0a) -> clean. | ||
| return True, None | ||
|
|
||
|
|
||
| def _scan_scope() -> list[tuple[Path, bool, str | None]]: | ||
| """Scan all files in the scope and return (path, passes, last_byte_hex).""" | ||
| results: list[tuple[Path, bool, str | None]] = [] | ||
| for glob in SCOPE_GLOBS: | ||
| for path in sorted(REPO_ROOT.glob(glob)): | ||
| passed, last_byte = _check_file(path) | ||
| results.append((path, passed, last_byte)) | ||
| return results | ||
|
|
||
|
|
||
| def main(argv: list[str] | None = None) -> int: | ||
| del argv # unused, kept for interface consistency | ||
| violations: list[str] = [] | ||
|
|
||
| for path, passed, last_byte in _scan_scope(): | ||
| if not passed: | ||
| violations.append(f" {path}: last byte 0x{last_byte}") | ||
|
|
||
| if violations: | ||
| print("trailing-newline-gate FAIL:") | ||
| for v in violations: | ||
| print(v) | ||
| return 1 | ||
|
|
||
| # Still need to report empty-file cases. | ||
| # Print nothing extra for empty files — they are not offenders. | ||
| print("trailing-newline-gate: clean") | ||
| return 0 | ||
|
|
||
|
|
||
| if __name__ == "__main__": | ||
| sys.exit(main()) | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,170 @@ | ||
| """Tests for scripts/check_trailing_newline.py. | ||
|
|
||
| Proves the trailing-newline gate in both directions: | ||
|
|
||
| * GREEN, passing case: all files in scope end with exactly one ``0x0a``. | ||
| * RED, failing case: a file missing its trailing newline exits non-zero and | ||
| names the offending path and its last byte. | ||
| * RED, vacuous-fixture case: an empty file is not an offender. | ||
| * RED, blank-line-at-EOF case: a file ending in two newlines (``0x0a0a``) | ||
| exits non-zero. | ||
| * RED, two offenders in one run: both paths are reported. | ||
| * SCOPE, file outside scope: a file with no newline outside the globs is | ||
| ignored (exit 0). | ||
| """ | ||
| from __future__ import annotations | ||
|
|
||
| import io | ||
| import os | ||
| import sys | ||
| from contextlib import redirect_stdout | ||
| from pathlib import Path | ||
|
|
||
| import scripts.check_trailing_newline as tng | ||
| from scripts.check_trailing_newline import main as check_main | ||
|
|
||
| REPO_ROOT = Path(__file__).resolve().parent.parent | ||
| GATE_SCRIPT = REPO_ROOT / "scripts" / "check_trailing_newline.py" | ||
|
|
||
|
|
||
| def _write(repo: Path, rel: str, text: str) -> Path: | ||
| path = repo / rel | ||
| path.parent.mkdir(parents=True, exist_ok=True) | ||
| path.write_bytes(text.encode("utf-8")) | ||
| return path | ||
|
|
||
|
|
||
| # ---------------------------------------------------------------------- | ||
| # Integration tests: the six required scenarios | ||
| # ---------------------------------------------------------------------- | ||
|
|
||
|
|
||
| def _repo(tmp_path: Path) -> Path: | ||
| """Create a minimal repo checkout under *tmp_path*.""" | ||
| repo = tmp_path / "repo" | ||
| (repo / "changelog.d").mkdir(parents=True) | ||
| (repo / "benchmarks" / "data").mkdir(parents=True) | ||
| return repo | ||
|
|
||
|
|
||
| def _run_check(repo: Path) -> tuple[int, str]: | ||
| """Run the gate against *repo* and return (exit_code, stdout).""" | ||
| original = tng.REPO_ROOT | ||
| env = os.environ.copy() | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [SUGGESTION]: The Reply with |
||
| env["TRAILING_NEWLINE_ROOT"] = str(repo) | ||
| buf = io.StringIO() | ||
| try: | ||
| tng.REPO_ROOT = repo | ||
| with redirect_stdout(buf): | ||
| rc = check_main([]) | ||
| finally: | ||
| tng.REPO_ROOT = original | ||
| return rc, buf.getvalue() | ||
|
|
||
|
|
||
| class TestTrailingNewlineGate: | ||
| def test_green_all_end_0x0a(self, tmp_path): | ||
| """fixture ends 0x0a -> exit 0""" | ||
| repo = _repo(tmp_path) | ||
| _write(repo, "changelog.d/test.md", "### Release notes\n") | ||
| _write(repo, "benchmarks/data/README.md", "Pinned data\n") | ||
| rc, out = _run_check(repo) | ||
| assert rc == 0, f"expected exit 0, got {rc}: {out!r}" | ||
| assert "trailing-newline-gate: clean" in out | ||
|
|
||
| def test_red_no_newline(self, tmp_path): | ||
| """fixture ends 0x2e (no newline) -> exit 1, offending path in output""" | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [SUGGESTION]: Docstring inaccurately describes the fixture content The docstring says "fixture ends 0x2e" but the string Reply with |
||
| repo = _repo(tmp_path) | ||
| _write(repo, "changelog.d/test.md", "### Release notes") | ||
| _write(repo, "benchmarks/data/README.md", "Pinned data") | ||
| rc, out = _run_check(repo) | ||
| assert rc == 1, f"expected exit 1, got {rc}: {out!r}" | ||
| assert "changelog.d/test.md" in out | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [SUGGESTION]: Both Reply with |
||
|
|
||
| def test_red_blank_line_at_eof(self, tmp_path): | ||
| """fixture ends 0x0a0a (blank line at EOF) -> exit 1 (exactly one newline)""" | ||
| repo = _repo(tmp_path) | ||
| _write(repo, "changelog.d/test.md", "### Release notes\n\n") | ||
| _write(repo, "benchmarks/data/README.md", "Pinned data\n\n") | ||
| rc, out = _run_check(repo) | ||
| assert rc == 1, f"expected exit 1, got {rc}: {out!r}" | ||
| assert "trailing-newline-gate FAIL" in out | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [SUGGESTION]: Both files end with blank lines at EOF, but only the generic failure message is asserted. Add assertions for both Reply with |
||
|
|
||
| def test_green_empty_fixture(self, tmp_path): | ||
| """empty fixture -> exit 0""" | ||
| repo = _repo(tmp_path) | ||
| changelog = repo / "changelog.d" / "empty.md" | ||
| changelog.write_text("") | ||
| readme = repo / "benchmarks" / "data" / "README.md" | ||
| readme.write_text("") | ||
| rc, out = _run_check(repo) | ||
| assert rc == 0, f"expected exit 0 for empty fixtures, got {rc}: {out!r}" | ||
| assert "trailing-newline-gate: clean" in out | ||
|
|
||
| def test_two_offenders_one_run(self, tmp_path): | ||
| """two offenders in one run -> exit 1 and BOTH paths reported""" | ||
| repo = _repo(tmp_path) | ||
| _write(repo, "changelog.d/test1.md", "### Release notes") | ||
| _write(repo, "changelog.d/test2.md", "### More notes") | ||
| _write(repo, "benchmarks/data/README.md", "Pinned data\n") | ||
| rc, out = _run_check(repo) | ||
| assert rc == 1, f"expected exit 1, got {rc}: {out!r}" | ||
| assert "changelog.d/test1.md" in out | ||
| assert "changelog.d/test2.md" in out | ||
|
|
||
| def test_scope_respected_outside_globs(self, tmp_path): | ||
| """file outside the scope globs, no newline -> exit 0 (scope is respected)""" | ||
| repo = _repo(tmp_path) | ||
| outside = repo / "src" / "orphan.md" | ||
| outside.parent.mkdir(parents=True, exist_ok=True) | ||
| outside.write_text("orphan content") | ||
| rc, out = _run_check(repo) | ||
| assert rc == 0, f"expected exit 0 when outside scope, got {rc}: {out!r}" | ||
| assert "trailing-newline-gate: clean" in out | ||
|
|
||
| def test_last_byte_hex_report(self, tmp_path): | ||
| """Verify the exact hex byte is reported for a no-newline file.""" | ||
| repo = _repo(tmp_path) | ||
| # Content ending with 0x2e (period) and no trailing newline. | ||
| _write(repo, "changelog.d/test.md", "### Release notes.") | ||
| rc, out = _run_check(repo) | ||
| assert rc == 1 | ||
| # The output should contain the path and the hex byte | ||
| lines = out.strip().splitlines() | ||
| # Find lines containing the path | ||
| path_lines = [l for l in lines if "changelog.d/test.md" in l] | ||
| assert len(path_lines) >= 1, f"Expected path in output, got: {out!r}" | ||
| # The hex byte should be 0x2e (period, the last byte). | ||
| assert "0x2e" in out | ||
|
|
||
| def test_passing_with_exact_one_newline(self, tmp_path): | ||
| """Verify files with exactly one trailing newline pass.""" | ||
| repo = _repo(tmp_path) | ||
| _write(repo, "changelog.d/test.md", "### Release notes\n") | ||
| readme = repo / "benchmarks" / "data" / "README.md" | ||
| readme.write_text("Pinned data\n") | ||
| rc, out = _run_check(repo) | ||
| assert rc == 0, f"expected exit 0 for files with exactly one newline, got {rc}: {out!r}" | ||
| assert "trailing-newline-gate: clean" in out | ||
|
|
||
| def test_benchmarks_readme_no_newline(self, tmp_path): | ||
| """RED: benchmarks/data/README.md missing trailing newline.""" | ||
| repo = _repo(tmp_path) | ||
| _write(repo, "changelog.d/test.md", "### Release notes\n") | ||
| _write(repo, "benchmarks/data/README.md", "Pinned data") | ||
| rc, out = _run_check(repo) | ||
| assert rc == 1, f"expected exit 1 for missing README trailing newline, got {rc}: {out!r}" | ||
| assert "benchmarks/data/README.md" in out | ||
|
|
||
| def test_changelog_multiple_offenders(self, tmp_path): | ||
| """RED: multiple changelog fragments all missing newlines.""" | ||
| repo = _repo(tmp_path) | ||
| _write(repo, "changelog.d/fix1.md", "Fix one") | ||
| _write(repo, "changelog.d/fix2.md", "Fix two") | ||
| _write(repo, "changelog.d/fix3.md", "Fix three") | ||
| _write(repo, "benchmarks/data/README.md", "Pinned data\n") | ||
| rc, out = _run_check(repo) | ||
| assert rc == 1, f"expected exit 1 for multiple changelog offenders, got {rc}: {out!r}" | ||
| assert "changelog.d/fix1.md" in out | ||
| assert "changelog.d/fix2.md" in out | ||
| assert "changelog.d/fix3.md" in out | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[WARNING]:
_last_byte_hexis defined but never calledThis function is dead code. It computes a hex representation of the last byte but is never invoked anywhere in the module. Remove it to avoid confusion for future maintainers.
Reply with
@kilocode-bot fix itto have Kilo Code address this issue.