Report What the Gate Read, and Key Both Sides on the Repository Scanned - #668
Conversation
Two reported false cleans, both reproduced before the fix and both members of one class: a run that reads nothing prints what a run with nothing to report prints. An absolute path argument scoped to nothing. Discovery returned absolute paths while the diff named repository-relative ones, so the intersection was empty from the same directory, the same repository and the same ref that reported findings for `.`. The same-root guard could not fire, because the run genuinely was in the right repository. An untracked file was invisible. `git diff` never names one, and `git ls-files` omits it too, so a whole-tree sweep passed over it for its own reason. A file read clean while unstaged and reported its findings once committed, on identical bytes. The invariant: every input to a verdict is read from the repository being scanned, never from the directory the process stands in. #666 established that for the rule set. The file set, the diff and the keys joining them still read the working directory. - One `repo_key` helper puts both sides in repository coordinates, replacing three hand-rolled idioms of which two disagreed. That disagreement was the defect. - Discovery reads tracked plus untracked-and-not-ignored, and the diff counts an untracked file as added in full. Ignored paths, generated trees and binaries stay out. - The diff is taken at the scan root, so a `diff.relative` setting cannot re-anchor it and a subdirectory run works rather than exiting 2. - Every run states its scope on stderr, including a clean one, which is the class fix. All five known false cleans exit 0 in silence and no per-route guard catches the sixth. The #520 guard refusing a scan of one repository from another is removed: anchoring the diff on the scan root is what it was approximating, so the case is answered rather than turned away. `repo_prefix` goes with it as dead code. Twelve new cases build real repositories rather than mocking `repo_root` and `discover`, since a mock supplies the join that was broken. Eight of the twelve fail against the pre-fix source. `--list-files` is byte-identical over the hub and timings are unchanged. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR hardens scripts/prose_lint.py against “false clean” outcomes by making diff scoping and path keying consistently anchored to the repository being scanned, adding untracked-file coverage, and emitting an explicit scope note on every run so “read nothing” is no longer indistinguishable from “found nothing”.
Changes:
- Anchor diff parsing and diff/file key joins on the scanned repository via
repo_key(...), and always print a scope summary to stderr. - Include untracked (non-ignored) files in both discovery and diff-scoped runs so new files are checked before staging.
- Expand test coverage with real git repositories and update operator documentation to match the new behavior.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| TODO.md | Updates an entry to reflect the removal of the wrong-directory false-clean objection. |
| scripts/test_prose_lint.py | Adds integration-style tests that reproduce prior false-clean scenarios and assert scope reporting. |
| scripts/README.md | Documents the new invariants: repo-anchored inputs, untracked inclusion, and always-on scope reporting. |
| scripts/prose_lint.py | Implements repo-anchored keying/diffing, untracked handling, and scope notes; removes the prior guard. |
| OPERATIONS.md | Updates operational guidance to reflect untracked inclusion and scope reporting. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Uh oh!
There was an error while loading. Please reload this page.
Review read `git ls-files` as printing repository-root-relative names under `-C`, and proposed listing both the tracked and the untracked sets from the repository top level. Measured on git 2.51, `git -C sub ls-files --others` prints `untracked.md` and `deep/untracked2.md` rather than the `sub/` forms, so the premise does not hold. TestDiscovery already pins the tracked half against the same misreading; this pins the untracked half, since the two lists are joined the same way and nothing tells a reader they might differ. Adopting the proposal would not break the paths. It would discard the narrowing the path argument asks for and read the whole repository instead. The first version of this test passed under the proposed change as well, since every file outside the subtree was clean and the findings agreed either way. It now asserts the discovered count, which is the quantity the argument narrows: 2 of 2 when the subtree scopes the run, 2 of 3 when it does not. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.
Suppressed comments (4)
scripts/prose_lint.py:123
untracked_paths()describes its return values as repository-relative, but the function is called with subdirectory roots (e.g., indiscover()), so git returns paths relative to the-Cdirectory. Adjust the docstring to say paths are relative to the passedroot(and become repository-relative only whenrootis the repo root).
def untracked_paths(root: Path) -> list[str]:
"""Repository-relative paths git holds no history for and is not ignoring.
An untracked file is the whole of what a change adds and `git diff` never names one, so a
scope built from the diff alone reads a new file as absent rather than as new. `git ls-files`
scripts/README.md:39
- This paragraph states the file set is "what git tracks plus what it is not ignoring", but
discover()intentionally falls back to a filesystem walk when git cannot describe the tree, and that fallback does not apply ignore rules. Consider a short qualifier here so the README doesn't overstate the guarantee.
The file set is what git tracks plus what it is not ignoring, and the diff scope counts an untracked file as added in full, so a new file is read before it is staged rather than after. Everything a verdict rests on is read from the repository named on the command line: the rule set, the file set, the diff, and the repository-relative keys that join the last two. Reading any of them from the directory the process happens to stand in is what produced every false clean on record, including an absolute path argument that reported clean over a tree it had read in full. Each run states its scope on stderr, because a gate that read nothing prints what a gate with nothing to report prints.
OPERATIONS.md:39
- This sentence says discovery reads "what git tracks plus what it is not ignoring", but
prose_lint.pyfalls back to a filesystem walk when git cannot describe the tree and does not apply ignore rules in that mode. Add a small qualifier to avoid implying.gitignoreis always honored.
Discovery reads what git tracks plus what it is not ignoring, so a new file is read before it is staged and a `--diff` run counts every line of one as added. Reading the tracked list alone meant a new file was invisible to both modes until it was staged, which reported clean on exactly the file a change existed to add. An ignored path stays out, since a build output is not authored text, and an explicit path is always read whatever git says about it.
scripts/prose_lint.py:318
discover()'s opening docstring line says discovery is scoped to what git "is not ignoring", but whentracked_paths()returns None it falls back towalk_paths(), which does not apply.gitignorerules. Clarify the docstring opening to mention that ignore handling only applies when git can describe the tree.
"""Every authored text file the rules govern, scoped by what git holds and is not ignoring.
Four suppressed review findings, all correct and all one overstatement. The docs and two docstrings described the file set as what git tracks plus what it is not ignoring, with no qualifier. When git cannot describe the tree, `discover` falls back to `walk_paths`, which consults no ignore rules and asserts the generated roots by name instead. So the claim covered a mode that never consults git at all. `untracked_paths` had the same shape in miniature: its docstring said the names are repository-relative, while `discover` passes the directory it was asked about and git returns names relative to that. Both callers are correct and the docstring described only one of them. Nothing here changes behaviour. Each of the four is a claim wider than what the code does, which is the kind that survives review by being read as a summary. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
ptr727
commented
Aug 11, 2026
Round 2 generated no inline comments and four suppressed ones. All four are correct and all four are fixed in 9752542. They are one overstatement in four places. The fourth is the same shape in miniature. No behaviour changed. Recording that these were suppressed rather than raised inline: a claim wider than the code is exactly the kind that reads as a summary and survives review, so the suppressed list is worth reading in full rather than treating as noise. 222 self-tests, the prose gate, |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.
Suppressed comments (1)
scripts/prose_lint.py:355
- discover() treats
tracked_paths(base) == Noneas "git cannot describe" and falls back to a filesystem walk that ignores.gitignore. Because tracked_paths() also returns None on empty output, scanning a subtree with no tracked files (but still inside a repo) can incorrectly fall back and start scanning ignored build outputs under that subtree.
If base is inside a git repo (via repo_root), an empty tracked set should be treated as an empty list (and still combined with untracked_paths), reserving the filesystem-walk fallback for the true no-git / not-a-repo case.
candidates = tracked_paths(base)
if candidates is None:
print(f'warning: git cannot describe {base}, falling back to a filesystem walk',
file=sys.stderr)
# A walk reports what is on disk, so it carries the untracked files already.
One suppressed review finding, reproduced and real. `tracked_paths` answers None both for a tree git cannot describe and for one git describes as holding no tracked files. `discover` read that None as the first case only, so a subtree holding nothing but new files took the filesystem walk, which applies no ignore rules. Measured before the fix, on a repository whose `.gitignore` names `newdir/ignored.md`: $ prose_lint.py newdir --check dupword warning: git cannot describe newdir, falling back to a filesystem walk newdir/authored.md:3: dupword: duplicated word 'the' newdir/ignored.md:3: dupword: duplicated word 'the' Git describes that tree perfectly well, the warning was false, and the ignored build output was scanned and reported. The conflation predates this branch, and what made it reachable is untracked files now being part of the file set, since a subtree of new files is exactly the shape that lists no tracked ones. The fallback is now gated on `repo_root(base)`, which asks git the question directly rather than inferring it from the size of an answer. A directory under no repository still warns and still walks. `tracked_paths` keeps its contract, because the empty-checkout reason it exists for is unchanged. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
ptr727
commented
Aug 11, 2026
Round 3 generated no inline comments and one suppressed one. It is correct, and it is fixed in 6c06b5a. Reproduced first, on a repository whose Git describes that tree perfectly well. The conflation predates this branch. What made it reachable is untracked files now being part of the file set, since a subtree of new files is exactly the shape that lists no tracked ones. The fallback is now gated on The new case fails against the previous head with Second round running where every finding arrived suppressed rather than inline, and both were real. 223 self-tests, the prose gate, |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.
Suppressed comments (2)
scripts/test_prose_lint.py:2092
- The docstring hard-codes an exact pre-fix test-count ("209 passing tests"), which will inevitably drift as cases are added/removed and can become misleading. Consider wording this without a specific number so the comment stays accurate over time.
below build repositories and let git answer. Two defects were reported clean by a suite of 209
passing tests, and both are here.
scripts/test_prose_lint.py:2125
contextlib.chdiris only available on Python 3.11+, which raises the minimum Python version for running this test module. If that is not an intentional requirement for the scripts test suite, use an explicitos.chdirtry/finally so the tests stay compatible with older Python 3.x runtimes.
def run_in(self, cwd: Path, *argv: str) -> int:
with contextlib.chdir(cwd):
return prose_lint.main(['--check', 'dupword', *argv])
Review flagged a hard-coded test count as drifting as cases are added. It was also inaccurate: the suite that reported both defects clean held 210 cases, not 209. The number now carries the commit it was measured at, which is how this repository writes every other measured claim, so a reader can check it rather than compare it against a suite that has since grown. The second suppressed finding is declined and measured. `contextlib.chdir` needs Python 3.11, and the module already called `unittest.TestCase.enterContext` in fourteen places at `e2a99f1`, which needs 3.11 too. The floor is unchanged and predates this branch. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
ptr727
commented
Aug 11, 2026
Round 4 generated no inline comments and two suppressed ones. One accepted and one declined, both measured, in 634b05f. Accepted, and it was worse than reported. The docstring cited "a suite of 209 passing tests". The finding was that a hard-coded count drifts. It was also simply wrong: the suite at Declined on measurement. Third round running where every finding arrived suppressed rather than inline, and four of the five have been real. Reading the suppressed list in full is not optional on this repository. |
ptr727
commented
Aug 11, 2026
Answering the Suppressed comments (4) raised in review 4902605796 on
Fixed in 9752542. The docstring now reads "Paths relative to
Fixed in 9752542. The paragraph now says that is the file set where git can describe the tree, and that where it cannot the run warns and walks the filesystem, which applies no ignore rules and skips the generated roots by name.
Fixed in 9752542. Same correction, in the operator wording: the claim now carries "That holds where git can describe the tree", and the fallback and its warning are stated.
Fixed in 9752542. The opening line now reads "scoped by git where git can answer", and the paragraph beneath it states that the fallback applies no ignore rules and warns on stderr so the weaker scoping is visible in the run. All four are one overstatement in four places, and none changed behaviour. Worth recording that all four arrived suppressed rather than inline, on a round whose overview says it "generated no new comments". |
ptr727
commented
Aug 11, 2026
Answering the Suppressed comments (1) raised in review 4902643179 on
Fixed in 6c06b5a. Reproduced before fixing, on a repository whose Git describes that tree perfectly well, so the warning was false and the run reported a file the repository had told it to ignore. The fallback is now gated on The conflation predates this branch. What made it reachable is untracked files now being part of the file set, since a subtree of new files is exactly the shape that lists no tracked ones. The new case fails against the previous head with |
ptr727
commented
Aug 11, 2026
Answering the Suppressed comments (2) raised in review 4902668593 on
Fixed in 634b05f, and the finding understated it. The count was also wrong: running the test module at
Disproven. It raises no floor. This module already called Round 5 on |
…ng (#670) Five commits, `1927e9a..56f4d7d`. Nineteen files, +4241/-136. ## What lands - **#664** `b0d0d13` — a shellcheck gate in `validate-task.yml`, with the file list from `git ls-files '*.sh'` so a new script is gated without editing the step. Also wired `scripts/test_host_gate.py` into the self-test step, which was running in no workflow at all. - **#666** `8c6fd27` — `prose_lint.py` chose its rule set from the working directory rather than the scanned repository, so standing in an operational repo and scanning a release repo silently discarded `home-path`, the rule that exists because real paths reached a public comment. - **#665** `e2a99f1` — a host stamp at `~/.claude/agent-safety-stamp.json` plus `--report`, so "is this machine current" has an answer that is not a tick in an issue. Also fixed `install.py` taking no arguments while both wrappers passed `"$@"`, which made `--help` perform a full install. - **#668** `6864a9b` — the two remaining `prose_lint.py` false cleans, fixed as a class. An absolute path argument scoped a `--diff` run to nothing and exited 0, and an untracked file was invisible to both a diff-scoped run and a whole-tree sweep. Every input to a verdict now derives from the repository being scanned, and every run states the scope it read. - **#667** `56f4d7d` — the host bootstrap tooling under `host-setup/linux/`, its `bootstrap.sh` loader, `scripts/test_bootstrap.py`, and the rules the scripts run under. ## Review record Every one of the five closed its Copilot loop on its own pull request. #668 ran five rounds and #667 seven, and between them eighteen findings arrived as suppressed comments rather than as inline threads, thirteen of which were real. Two of those were defects that would otherwise have shipped in the gate this promotion carries: a subtree of new files taking a filesystem walk that applies no ignore rules, and a docstring count that was wrong as well as brittle. ## Consequence worth stating The `GOVERNANCE.md` "Hub-Hosted Tooling" paragraph #667 added makes every carrying repository's copy a past revision once this reaches `main`. That is the ordinary consequence of a canonical moving rather than a defect, but a repository meeting it first as a red audit line will read it as a surprise. HomeAutomation-Config has already re-vendored it by content rather than by bytes, since a byte copy from a CRLF hub into an LF repository rewrites every line to change one paragraph. ## Verified on this head `develop` at `56f4d7d`, in sync with `origin/develop`. Local run of the CI invocations: 223 prose self-tests, the prose gate over 117 files, `repo_gate` (eol, eol-coverage, sha-pin), `spec/validate.py` with 22 cataloged, markdownlint over 45 files, and editorconfig-checker, all clean. Merge as a **merge commit**, never a squash, and without `--delete-branch`: this pull request's head is `develop` itself.
Two false cleans reported by the ESPHome-Config agent, both reproduced against
developbefore any fix, and both members of one class: a run that reads nothing prints what a run with nothing to report prints.The two reports, reproduced
Constructed repositories, one seeded finding each. Verdicts are pre-fix.
prose_lint.py . --diff BASEprose_lint.py /abs/path --diff BASE, same directory, same repository, same refThe absolute-path run returned absolute paths from discovery while the diff named repository-relative ones, so the intersection was empty. The same-root guard could not fire, because the run genuinely was in the right repository.
The untracked hole is not a
--diffquirk.git ls-filesomits an untracked file exactly asgit diffdoes, so a whole-tree sweep passed over it for its own reason, which retires the workaroundOPERATIONS.mddocumented.The invariant
Every input to a verdict is read from the repository being scanned, and none of them from the directory the process happens to stand in. That covers the rule set, the file set, the diff, and the keys joining the last two. #666 established it for the rule set alone; the other three still read the working directory.
repo_keyhelper puts both sides in repository coordinates. It replaces three hand-rolled idioms, one already correct and two not, and that disagreement was the defect.diff.relativecannot re-anchor it and a subdirectory run works rather than exiting 2 with advice to move.All five known false cleans exit 0 in silence. Per-route guards only ever close the route somebody thought of, and the sixth is found by a reviewer or not at all.
One deliberate removal
The #520 guard refusing a scan of one repository while standing in another is gone. It existed because the diff was taken where the process stood; anchoring the diff on the scan root is what it was approximating, so the case is answered rather than turned away.
repo_prefixgoes with it as dead code. Its two cases are replaced: a path under no repository is still refused, now by the diff itself, and scanning one repository from another is asserted to diff the one scanned.Verification
Twelve new cases build real git repositories rather than mocking
repo_rootanddiscover, because a mock supplies the join that was broken. Eight of the twelve fail against the pre-fix source; the four that pass are exclusion cases, and each was checked rather than assumed redundant. One, adiff.relativecase, passed because both sides were anchored on the process's directory and agreed by accident, and its docstring now claims only that.repo_gate(eol, eol-coverage, sha-pin),spec/validate.py, markdownlint over 45 files, editorconfig-checker: all clean locally.--list-filesbyte-identical over this repository, 117 files.TODO.mdline 409 named the wrong-directory false clean as an open objection to running doc gates in the pre-commit hook; that objection no longer applies and the entry says so.🤖 Generated with Claude Code