Conversation
Fixes #1537, which registers on the `develop -> main` promotion rather than here. ## The defect `prose_lint.py` decided whether a run was diff-scoped by the truthiness of `--diff` rather than by its presence, so an empty value read as no `--diff` at all. The run then widened to the whole tree and reported the existing backlog as though the change had introduced it, which is the outcome the refusal on the very next line exists to prevent. An unresolvable ref took that refusal while an empty string walked past it. The exposed path is a local run whose caller substitutes a command's output, `--diff "$(git merge-base origin/develop HEAD)"`. Where `origin/develop` does not exist, as in a single-branch clone of `main`, `git merge-base` writes only to stderr and the script receives `--diff ""`. It was found while documenting that invocation for `ptr727/aiopurpleair`. ## The fix Both the scope computation and the refusal below it test `a.diff is not None`, so an empty value reaches `changed_lines`, where `git diff ''` fails with `fatal: bad revision ''`, and the existing refusal fires naming the value it could not resolve. The issue's own reproduction, against this branch: ``` $ python3 prose_lint.py --diff "" -- . error: cannot diff against '', so the run cannot be scoped to changed lines. Refusing to scan the whole tree instead, since that reports the existing backlog as though this change introduced it. Check the ref exists and that the checkout carries its history. exit=2 ``` It previously exited 1 on `a.md:1: semicolon`, a line the change never touched. ## Tests Two, because one of them alone would not hold the fix: - A unit test asserts the empty value reaches the resolver and takes the refusal, rather than asserting the exit code alone, which the whole-tree path could also produce for its own reasons. - A second test runs the same case against a real repository, since the first mocks the resolver and would stay green if git ever resolved an empty revision. Resolving one to an empty scope reads as a clean tree, which is the other half of what the routing closes. Each was proved by reverting the two production lines, where both fail with `2 != 1`, the whole-tree widening itself. ## Reach No caller in the tree can pass an empty-but-present value today, so nothing that works now starts refusing. `.github/actions/prose-gate/action.yml` rejects an unresolvable `$BASE` before invoking the script, `.github/workflows/validate-task.yml` gates that step on a pull request event, and the Husky, pre-commit and `hub-fetch-run.py` callers all pass a literal `HEAD`. `scope_note` already tested `base is None`, so no truthiness site was left disagreeing with the new one, and `gate_provenance` keeps truthiness deliberately, where an empty value falling through to the next source is what it wants. ## Verification Full local gate set green on this head: 270 cases in `scripts/tests/test_prose_lint.py` and 1405 across the suite, `ruff format --check`, `ruff check`, `mypy`, `prose_lint.py` over the gating rule set, `repo_gate.py`, `spec/validate.py`, `build_dist.py --check`, `canonical_review.py check`, and `docker_lint.py`'s seven linters. No canonical unit changed, so no carried-content pass was owed. Three local strict-review passes ran at the strongest available tier. The first raised three findings and the second six, all answered, which is the two-round edit budget spent. The third found no behavioral defect and four prose findings, open and listed here rather than fixed, per that budget: a missing comma after a fronted participle at `test_prose_lint.py:1906`, a relative pronoun binding to the nearer noun at `:2697`, an unexplained positive control at `:2705`, and both new tests asserting the exit code where the sibling refusal test also asserts the message text. One earlier finding was declined with evidence: that the refusal's closing sentence, "Check the ref exists and that the checkout carries its history", names a remedy that does not fit an empty value. It is pre-existing text this change did not write, the message prints the value with `!r` so an empty base displays as `''`, and growing a per-input remedy enumeration has repeatedly cost this repository more than it returned. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: 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 |
Contributor
There was a problem hiding this comment.
🟢 Approval recommended
The change is narrowly scoped, matches the stated defect, and is covered by targeted tests that exercise both mocked and real-git behavior.
Pull request overview
Promotes the develop fix for .github/actions/prose-gate/prose_lint.py into main, ensuring that an explicitly provided-but-empty --diff value is treated as an invalid base (refusal) rather than being misread as “no diff” (whole-tree scan).
Changes:
- Treat
--diff ""as “diff requested” (presence) rather than “no diff” (truthiness), so the run refuses instead of silently widening. - Add unit + integration-style tests to pin the empty-diff refusal behavior (mocked resolver + real git).
File summaries
| File | Description |
|---|---|
| scripts/tests/test_prose_lint.py | Adds regression tests covering empty --diff handling (mocked and real-git paths). |
| .github/actions/prose-gate/prose_lint.py | Switches diff-scope routing from truthiness to presence checks for --diff. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Promotes one commit from
developtomain.e4aa5d8Read an Empty prose_lint --diff as a Failed Base, Not as No Diff (Read an Empty prose_lint --diff as a Failed Base, Not as No Diff #1566)prose_lint.pydecided whether a run was diff-scoped by the truthiness of--diffrather than by its presence, so--diff ""read as no--diffat all, the run widened to the whole tree, and it reported the existing backlog as though the change had introduced it. That is what the refusal on the next line exists to prevent, and an unresolvable ref took that refusal while an empty string walked past it. The exposed path is a local run whose caller substitutes a command's output,--diff "$(git merge-base origin/develop HEAD)", which yields an empty string wherever the substituted command fails.Both the scope computation and the refusal now test
a.diff is not None, so an empty value reaches the resolver,git diff ''fails, and the refusal fires naming the value it could not resolve. Two tests cover it, one mocking the resolver and one against a real repository, since the mocked one alone would stay green if git ever resolved an empty revision. No caller in the tree can pass an empty-but-present value today, so nothing that works now starts refusing.Copilot reviewed the feature pull request at its merged head, full coverage, no findings and none suppressed.
Closes #1537