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
Conversation
scripts/check_trailing_newline.py new gate checking changelog.d md files and benchmarks/data/README.md for exactly one trailing 0x0a tests/test_trailing_newline_gate.py six fixture cases plus four variants changelog.d/tsk-7m6ju5-trailing-newline-gate.md added
|
ⓘ Qodo reviews are paused because your trial has ended. Ask your workspace admin to add credits to resume reviews. Manage billing |
|
Warning Review limit reached
Next review available in: 9 minutes Limit details: You’ve used all 1 included review currently available under your plan. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| ) | ||
|
|
||
|
|
||
| def _last_byte_hex(file_path: Path) -> str | None: |
There was a problem hiding this comment.
[WARNING]: _last_byte_hex is defined but never called
This 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 it to have Kilo Code address this issue.
| ``pass`` is ``False`` (``None`` when the file is empty or when pass | ||
| is True). | ||
| """ | ||
| try: |
There was a problem hiding this comment.
[WARNING]: _check_file silently treats unreadable files as passing
Catching OSError and returning (True, None) means any file that cannot be read (permissions, deleted mid-scan, etc.) is silently treated as valid. This could mask real issues. Consider logging a warning or treating unreadable files as violations.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| 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.
[SUGGESTION]: env variable is set but never used
The env dictionary is populated with TRAILING_NEWLINE_ROOT but never passed to check_main. The test relies on monkey-patching tng.REPO_ROOT instead. Remove the unused env variable to avoid confusion.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| 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.
[SUGGESTION]: Docstring inaccurately describes the fixture content
The docstring says "fixture ends 0x2e" but the string "### Release notes" ends with 0x73 ('s'), not 0x2e ('.'). Update the docstring to match the actual content.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| _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.
[SUGGESTION]: test_red_no_newline only asserts one of two failing paths
Both changelog.d/test.md and benchmarks/data/README.md lack trailing newlines, but only changelog.d/test.md is asserted. Add an assertion for benchmarks/data/README.md to fully verify the failure path.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| _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.
[SUGGESTION]: test_red_blank_line_at_eof does not verify both failing paths
Both files end with blank lines at EOF, but only the generic failure message is asserted. Add assertions for both changelog.d/test.md and benchmarks/data/README.md to fully verify the behavior.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
Code Review SummaryStatus: 6 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (3 files)
Fix these issues in Kilo Cloud Reviewed by step-3.7-flash · Input: 86.4K · Output: 20.5K · Cached: 610.4K |
|
BLOCK, but the gate script itself is correct and should be kept almost exactly as written. The byte logic is right, it reports the offending path and the actual hex byte, and I confirmed it goes red on a real offender rather than only on a fixture. The block is about one thing only: nothing in this repo ever runs it. Reviewed as a trial merge into current master ( What I verified worksI dropped a real fragment That is the positive control, and it passes. The 10 new tests also pass, and the empty-file, blank-line-at-EOF and hex-reporting cases are all genuinely covered. Why it is blocked
The consequence, measured end to end rather than reasoned about. With that Baseline is 1617, so all 10 new gate tests ran and passed while the exact defect they exist to catch sat in the tree, untouched and unreported. Every test in the file examines a One more findingThe documented Minor: Not wanted in the revision: no change to the byte-checking logic, and no widening of Revision card |
|
Correction to my review above: the revision card is I closed
One thing I should have said more plainly in the review: |
… ship green (#374) The gate script from #368 was correct and is kept almost as-is, but nothing ever invoked it: no workflow, Makefile, shell script or config referenced it, and every one of its ten tests used a tmp_path fixture, so a fragment whose last byte was not 0x0a sat in changelog.d unreported while the suite passed. Adds .github/workflows/trailing-newline-gate.yml, modelled on the three sibling gates, plus a test that runs the gate against the repository's own tree so a plain pytest run goes red as well as CI. Covers the documented TRAILING_NEWLINE_ROOT override, which had none: it is read at import time, so the in-process tests could not reach it and replacing it with the bare default left all ten green. The two new subprocess tests fail under that mutant. Also drops an em dash from a comment and terminates both new files with 0x0a, which the gate exists to require.
1 participant
CARD TITLE (intent, not commit subject): 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
Autonomous build of board card tsk-7m6ju5.
Files:
changelog.d/tsk-7m6ju5-trailing-newline-gate.md | 2 +
scripts/check_trailing_newline.py | 116 ++++++++++++++++
tests/test_trailing_newline_gate.py | 170 ++++++++++++++++++++++++
3 files changed, 288 insertions(+)