diff --git a/OPERATIONS.md b/OPERATIONS.md index 4182923e..f0848843 100644 --- a/OPERATIONS.md +++ b/OPERATIONS.md @@ -36,7 +36,9 @@ Scope a run to what changed, which matches the correct-as-next-edited rule: python3 scripts/prose_lint.py . --diff origin/develop ``` -Whole-tree discovery reads only files git tracks, so `python3 scripts/prose_lint.py .` and `--diff` do not see a new file until it is staged, and a clean whole-tree run proves nothing about an unstaged one. An explicit path is always read, tracked or not, so name a new file directly to check it before staging. +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. That holds where git can describe the tree. Where it cannot, the run warns on stderr and falls back to a filesystem walk that skips the generated roots by name and applies no ignore rules. + +Every run states its scope on stderr, as the files read and, for a diff-scoped run, the changed lines inside them. A gate that read nothing otherwise prints what a gate with nothing to report prints, and each false clean found so far exited 0 in silence. ## Runbooks diff --git a/TODO.md b/TODO.md index 7f922638..4017f750 100644 --- a/TODO.md +++ b/TODO.md @@ -406,7 +406,7 @@ One pull request writing down the agent-to-agent messaging this fleet has now us Small work with no research to preserve, selectable one bullet at a time. - **Answer the symmetric reading of [`.editorconfig`][editorconfig], a path-specific section naming files that do not exist**, which is the half of [#633][issue-633] the `eol-coverage` check deliberately left open. The dead-pin reading it does ship is the `.gitattributes` side, and the same question on the other document is not the same shape: this repo's `[.github/workflows/*]` and `[catalog/snippets/workflows/*]` sections are legitimately broad, and the issue's own first attempt at it produced false positives because the matcher did not expand brace syntax, which [`scripts/repo_gate.py`][repo-gate] already implements. Measure the exemption against the live corpus before building the gate rather than after, since a stale exemption hands out a work list that damages correct documents, and decide whether `forward-declared` carries across or whether an editorconfig section needs its own marker. -- **Reconsider whether the pre-commit hook runs the doc gates now that they are diff-scoped.** [`scripts/README.md`][scripts] records the current decision and its reason, that doc linters stay out of the hook so it stays fast, which was sound when the only mode was a whole-tree sweep, and a diff-scoped run finishes in about a second. The failure it would prevent is the most repeated one on record, comment sentences wrapped across lines caught after the commit rather than before it. Weigh it against the standing preference for a fast hook and against a hook that runs the gate from the wrong directory, which is its own false clean. +- **Reconsider whether the pre-commit hook runs the doc gates now that they are diff-scoped.** [`scripts/README.md`][scripts] records the current decision and its reason, that doc linters stay out of the hook so it stays fast, which was sound when the only mode was a whole-tree sweep, and a diff-scoped run finishes in about a second. The failure it would prevent is the most repeated one on record, comment sentences wrapped across lines caught after the commit rather than before it. Weigh it against the standing preference for a fast hook. The other objection, a hook running the gate from the wrong directory and reporting its own false clean, no longer applies: the rule set, the file set, the diff, and the keys joining them are all read from the repository being scanned rather than from wherever the process stands. - **Audit the fleet's shell surface by size and branching, and decide per script whether Python with unit tests is cheaper.** The evidence is the review record rather than a language preference, since a non-trivial shell script earns findings round after round while every gate under [`scripts/`][scripts] carries a test file beside it and converges in one or two. The measure is lines, branch count, and the review rounds each has cost. `repo-config/configure.sh` and the agent-safety installer are the two worth measuring, and a bootstrap script that needs the Python it exists to install is not a rewrite worth having, which protects the installer more than the config script. - **Make a table of contents standard for a long document rather than for the README alone.** [`spec/readme-structure.md`][readme-structure] fixes one at README position 4 and no other hub file carries one, which leaves the three longest documents without it, `CODESTYLE.md` at 516 lines, `GOVERNANCE.md` at 436 and `WORKFLOW.md` at 301, measured on `develop` at `3d1a0b1` on 2026-08-06. Settle the threshold in headings or lines so the audit can check it, and settle how it sits with the reference-link exception, since the four agent-instruction files keep inline links exactly because they are read one section at a time, which is the property that makes a contents list worth having in them. The mechanical constraint is that the list is filled by the Markdown All in One extension on save, so a file nobody opens in the editor grows a stale list, which is worse than absent because it is read as current. - **Converge this repo's Python on the ruff configuration it already declares, then add the formatting half to the pre-commit hook.** `pyproject.toml` carries `[tool.ruff]` and [`spec/project-types.json`][project-types] declares `python.ruff.config`, yet no workflow runs ruff and the tree does not pass it, measured on `develop` at `6d020b1` on 2026-08-09 with ruff 0.16.2: `ruff format --check` reports 13 of 57 files would be reformatted, and `ruff check` reports 106 errors, of which 39 are auto-fixable. The largest groups are 24 `PLW1510` (a `subprocess.run` with no `check`), 17 `FURB167` (`re.M` for `re.MULTILINE`), 11 `EXE001` (a shebang on a non-executable file, which wants reading against the `eol-coverage` shebang set rather than fixed blindly), 9 `BLE001` and 9 `SIM117`. The hook deliberately ships without the ruff step for this reason, since a gate failing on the corpus it guards blocks every commit from the moment it lands, which is the measure-the-corpus-first rule applied to a gate rather than to an exemption. Decide whether CI gains a ruff job in the same pass, since a formatter enforced only by a hook is enforced only on the machines that enabled it. diff --git a/scripts/README.md b/scripts/README.md index 60956e28..30a3c7ff 100644 --- a/scripts/README.md +++ b/scripts/README.md @@ -36,6 +36,8 @@ The default rule set covers comment shape (`comment-wrap` and `comment-case`) al A wide scan skips the trees this repo generates rather than authors, currently `reports/`, which [`spec/audit.py`][audit] writes. A finding there is the audit engine's phrasing rather than an author's, so no edit to that tree can fix it, and leaving them in made the repo's own number mostly generated output. Naming such a path directly still reads it (`prose_lint.py reports`), so nothing becomes uncheckable. +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. That is the file set where git can describe the tree, and where it cannot the run warns and walks the filesystem instead, which applies no ignore rules and skips the generated roots by name. 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. + In Markdown an HTML comment carrying no sentence punctuation is treated as a structural marker rather than commentary, so it takes neither a capital nor a sentence split. The reference-link group headers, the ToC-omit directive, and the `agent-safety` install markers are each matched verbatim by a tool, so rewriting one to satisfy the rule breaks whatever reads it. A Markdown comment that does punctuate a sentence is prose and is judged as prose. The `spelling` rule covers the US English convention where cspell does not reach. That gate reads README and HISTORY only, deliberately, because gating every Markdown file would mean endlessly padding `cspell.json` with technical terms, so a British spelling anywhere else in the tree had nothing checking it. The banned words are generated from stems rather than listed one by one, since an inflected spelling is as wrong as its base and a hand-listed family drifts as soon as one form is added without the others. Two words are deliberately absent: `analyses` is the US plural of `analysis` as much as it is a British verb form, and `cancelled` is a GitHub Actions job status rather than prose. diff --git a/scripts/prose_lint.py b/scripts/prose_lint.py index cbfc99bf..feeedd7c 100644 --- a/scripts/prose_lint.py +++ b/scripts/prose_lint.py @@ -91,11 +91,66 @@ def rel(path: Path) -> str: return path.as_posix().removeprefix('./') -def changed_lines(base: str) -> dict[str, set[int]] | None: - """Map path -> set of line numbers added/changed vs `base`. None if git fails.""" +def repo_key(path: Path, root: Path) -> str: + """The repository-relative posix key that joins a scanned file to a diff entry. + + A diff names every file relative to the repository top level, while a path argument arrives + absolute, relative to a subdirectory, or dotted. The two are compared here and nowhere else, + since comparing them in whatever form each happened to arrive in is what let an absolute path + argument match no diff entry at all and report a clean run over a tree it had read in full. + + A path outside `root` has no repository-relative form and keeps its own, so nothing in a diff + can match it. That is the honest answer rather than a coincidental one. + """ + try: + return path.resolve().relative_to(root.resolve()).as_posix() + except (ValueError, OSError): + return rel(path) + + +def all_lines(path: Path) -> set[int]: + """Every line number in `path`, which is the changed scope of a file that is entirely new.""" try: - d = subprocess.run(['git', 'diff', '--unified=0', '--no-color', base, '--'], - capture_output=True, text=True, check=True).stdout + return set(range(1, len(path.read_bytes().splitlines()) + 1)) + except OSError: + return set() + + +def untracked_paths(root: Path) -> list[str]: + """Paths relative to `root` that git holds no history for and is not ignoring. + + They are repository-relative only where `root` is the repository top level, which is how + `changed_lines` calls it, since a diff key is repository-relative. `discover` passes the + directory it was asked about and joins the names onto it, so both readings hold at once. + + 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` + omits it too, so this is not a diff-mode quirk: a whole-tree sweep passed over it as well. + Ignored paths stay out, since a build output is not authored text. + """ + try: + r = subprocess.run(['git', '-C', str(root), 'ls-files', '-z', '--others', + '--exclude-standard'], capture_output=True, text=True) + except (OSError, ValueError): + return [] + if r.returncode != 0: + return [] + return [name for name in r.stdout.split('\0') if name] + + +def changed_lines(base: str, root: Path) -> dict[str, set[int]] | None: + """Map repository-relative path -> line numbers this working tree adds vs `base`. + + None if git fails. The diff is taken at `root`, the repository being scanned, rather than + wherever the process happens to stand. Keys anchored on the working directory match nothing + once the two differ, and a `diff.relative` setting would re-anchor them the same way. + + An untracked file counts as added in full, since a change whose whole point is adding a file + otherwise scopes to nothing and reports a clean run on exactly the file it added. + """ + try: + d = subprocess.run(['git', '-C', str(root), 'diff', '--unified=0', '--no-color', base, + '--'], capture_output=True, text=True, check=True).stdout except (subprocess.CalledProcessError, FileNotFoundError): return None out: dict[str, set[int]] = {} @@ -110,6 +165,10 @@ def changed_lines(base: str) -> dict[str, set[int]] | None: start = int(m.group(1)) count = int(m.group(2) or 1) out[cur].update(range(start, start + count)) + for name in untracked_paths(root): + target = root / name + if is_text(target): + out[name] = all_lines(target) return out @@ -132,18 +191,18 @@ def asked_about(key: str, paths: list[str]) -> bool: def unread_diff_files(scope: dict[str, set[int]], paths: list[str], - excludes: tuple[str, ...]) -> list[str]: + excludes: tuple[str, ...], root: Path) -> list[str]: """Files the diff names that this run was asked about and could have read, in sorted order. - Keys are resolved against the repository top level rather than the working directory, because - `git diff` reports repository-relative paths while discovery keys off the directory the run - started in. Reading them against the working directory would make this list empty from a - subdirectory, which is the one place it most needs to be full. + Both sides are read against `root`, the repository being scanned, because `git diff` reports + repository-relative paths while a request arrives in whatever form the caller typed. Reading + either against the working directory empties this list from a subdirectory and empties it for + an absolute path argument, which are the two places it most needs to be full. """ - root = Path(repo_root(Path('.')) or '.') + asked = [repo_key(Path(p), root) for p in paths] out: list[str] = [] for key in sorted(scope): - if not asked_about(key, paths): + if not asked_about(key, asked): continue if any(x in key for x in excludes): continue @@ -155,6 +214,24 @@ def unread_diff_files(scope: dict[str, set[int]], paths: list[str], return out +def scope_note(read: int, discovered: int, lines: int | None, base: str | None) -> str: + """What the run actually read, stated on every verdict rather than only on a busy one. + + Five routes to a false clean are on record and every one of them exits 0 in silence: an + unresolvable base widening to a whole-tree scan, a diff taken in one repository while scanning + another, a path under no repository, an absolute path argument whose keys matched no diff + entry, and an untracked file no diff names. Each guard so far closes the route a reviewer + happened to see, and the sixth is found that way or not at all. What every one of them shares + is that a scope of nothing prints exactly what a clean tree prints, which is a property of the + output and not of any single route. Stating the scope is what a reader needs to tell "read + nothing" from "found nothing", so it is printed even when the count is the whole tree. + """ + if base is None: + return f'scope: {read} file(s) read, whole tree' + return (f'scope: {read} of {discovered} file(s) read, {lines} changed line(s), ' + f'diff against {base!r}') + + def home_path_findings(lineno: int, line: str) -> list[tuple[int, str, str]]: """Absolute home paths on this line that name a real account. @@ -184,21 +261,6 @@ def operational_checkout(root: Path) -> bool: and not (root / 'repo-config' / 'develop.json').is_file()) -def repo_prefix(root: Path) -> str: - """Where `root` sits inside its repository, as a posix prefix, or '' when git cannot say. - - The generated-tree decision has to be made against the repository-relative path. Reading the - filesystem path instead lets a directory *above* the checkout decide it, so a repository - cloned under a parent named `reports` had its own `reports/` tree scanned as authored. - """ - try: - r = subprocess.run(['git', '-C', str(root), 'rev-parse', '--show-prefix'], - capture_output=True, text=True) - except (OSError, ValueError): - return '' - return r.stdout.strip() if r.returncode == 0 else '' - - def quoted(paths) -> str: """Paths as a sorted, quoted, comma-joined list for an error message. @@ -254,48 +316,61 @@ def is_text(path: Path) -> bool: return False -def discover(paths: list[str], excludes: tuple[str, ...] = ()) -> list[Path]: - """Every authored text file the rules govern, scoped by what git tracks. +def discover(paths: list[str], excludes: tuple[str, ...] = (), + root: Path | None = None) -> list[Path]: + """Every authored text file the rules govern, scoped by git where git can answer. + + Where it cannot, the fallback walk applies no ignore rules at all and asserts the generated + roots by name instead, so the scoping there is weaker than the paragraph below describes. It + warns on stderr, since a quieter file set that reads the same is how a sweep stops covering + what it claims to. The line-endings rule already requires a repo-wide sweep be scoped to `git ls-files` rather than a directory list, which covers what its author thought of and silently stops covering whatever is added next. An extension allowlist has that same defect, so the filter here is - whether the file is text, not whether its suffix was thought of. + whether the file is text, not whether its suffix was thought of. An untracked file is authored + text the same way, and reading only the tracked list passed over a new file until it was + staged, so a clean sweep proved nothing about the one file a change existed to add. + + `root` is the repository every path is judged against, passed in so that the caller's verdict + and this file set are keyed alike. Judging against the filesystem path instead lets a + directory *above* the checkout decide: a repository cloned under a parent named `reports` had + its own `reports/` tree scanned as authored, and an absolute argument carried its whole parent + chain into every `--exclude` test. An explicit file argument bypasses discovery, so a single file can always be checked directly. """ - found: list[Path] = [] + found: list[tuple[Path, str]] = [] for raw in paths: p = Path(raw) + base = p if p.is_dir() else (p.parent if p.is_file() else Path('.')) + anchor = root if root is not None else Path(repo_root(base) or base) if p.is_file(): - found.append(p) + found.append((p, repo_key(p, anchor))) continue - root = p if p.is_dir() else Path('.') - tracked = tracked_paths(root) - if tracked is None: - print(f'warning: git cannot describe {root}, falling back to a filesystem walk', + candidates = tracked_paths(base) + # `tracked_paths` answers None for a tree git cannot describe and for an empty answer. + # Only the first of those justifies a walk. + # Whether git can describe a tree is settled by asking git, never by its answer's size. + # Read as emptiness, a subtree of new files fell back and scanned the ignored ones under it. + # It also printed that git could not describe a tree git describes fine. + if candidates is None and not repo_root(base): + print(f'warning: git cannot describe {base}, falling back to a filesystem walk', file=sys.stderr) - tracked = walk_paths(root) - # Judge against the repository-relative path, never the filesystem one. - # A directory above the checkout must not decide whether a file is generated. - # An absolute argument otherwise carried its whole parent chain into the test. - prefix = repo_prefix(root) - for q in tracked: - try: - inside = Path(prefix) / q.relative_to(root) - except ValueError: - # Unreachable while both come from the same root, and kept safe rather than tidy. - # With no repository-relative path there is nothing to judge, so scan the file. - # Skipping on doubt is how a gate reports clean over what it never read. - found.append(q) - continue - if GENERATED_TREES.isdisjoint(inside.parts): - found.append(q) - elif not GENERATED_TREES.isdisjoint(Path(prefix).parts): - # The root named is itself inside a generated tree, so it was asked for. - found.append(q) - keep = [p for p in found - if not any(x in rel(p) for x in excludes) and p.is_file() and is_text(p)] + # A walk reports what is on disk, so it carries the untracked files already. + # It applies no ignore rules, which is why it is reserved for having no other answer. + candidates = walk_paths(base) + else: + candidates = (candidates or []) + [base / name for name in untracked_paths(base)] + # The path named is itself inside a generated tree, so that tree was asked for. + asked_inside_generated = not GENERATED_TREES.isdisjoint(Path(repo_key(base, + anchor)).parts) + for q in candidates: + key = repo_key(q, anchor) + if GENERATED_TREES.isdisjoint(Path(key).parts) or asked_inside_generated: + found.append((q, key)) + keep = [q for q, key in found + if not any(x in key for x in excludes) and q.is_file() and is_text(q)] return sorted(set(keep)) @@ -1151,36 +1226,25 @@ def main(argv: list[str] | None = None) -> int: print('note: home-path is not checked in an operational repository, where an absolute ' 'path is the operator instruction rather than observed data.', file=sys.stderr) - # Checked before discovery, which reads every tracked file to classify it as text. - # A run this rejects would otherwise pay that cost and throw the result away. - # `--list-files` is exempt, since it reports the scan scope and never consults the diff. - if a.diff and not a.list_files: - # `git diff` runs in the current directory while the paths may name another checkout. - # Scanning one repository and diffing another intersects to nothing. - # The run then reports clean, which is the false clean this gate exists to prevent. - # It cost a real verification once, where a branch read zero from the wrong directory. - # A path under no repository at all fails the same way, and more quietly. - # Discovery walks the filesystem, then every absolute key misses the repo-relative ones. - # Requiring the same root covers both, where testing for a different one did not. - here = repo_root(Path('.')) - for raw in (a.paths or ['.']): - there = repo_root(Path(raw)) - if here and there != here: - where = there or 'no git repository' - print(f'error: --diff resolves against {here}, but {raw} is in {where}. ' - 'Run the gate from the repository being scanned, since a diff taken ' - 'elsewhere scopes every finding away and reports a false clean.', - file=sys.stderr) - return 2 - - files = discover(a.paths or ['.'], tuple(a.exclude)) + # Every input to a verdict is read from the repository scanned rather than from the process. + # That covers the rule set above, the file set, the diff, and the keys joining the last two. + # An earlier guard refused a scan of one repository while the process stood in another. + # The diff was taken where the process stood, which intersected the scan to nothing. + # Anchoring the diff on the scan root is what that guard was approximating. + # It answers the case correctly rather than refusing it. + # A path under no repository now fails at the diff itself, which is the honest error. + files = discover(a.paths or ['.'], tuple(a.exclude), scan_root) + # Computed once, by the helper and the root discovery used. + # A key derived twice is a key that can disagree with itself. + keys = {f: repo_key(f, scan_root) for f in files} if a.list_files: for f in files: - print(rel(f)) + print(keys[f]) return 0 - scope = changed_lines(a.diff) if a.diff else None + discovered = len(files) + scope = changed_lines(a.diff, scan_root) if a.diff else None if a.diff and scope is None: # Widening to the whole tree answers a different question, and answers it silently. # A caller scoping to a change gets the backlog reported as though the change made it. @@ -1192,7 +1256,7 @@ def main(argv: list[str] | None = None) -> int: 'checkout carries its history.', file=sys.stderr) return 2 if scope is not None: - matched = [f for f in files if rel(f) in scope] + matched = [f for f in files if keys[f] in scope] # The floor every verdict below rests on, asserted rather than guarded. # Each route to a false clean so far was closed after a reviewer saw it. # The next is closed that way or not at all, which is what a floor covers. @@ -1202,7 +1266,7 @@ def main(argv: list[str] | None = None) -> int: # An image or a lock file is that case. # So the comparison is against the diff's own list of files this run could have read. if scope and not matched: - unread = unread_diff_files(scope, a.paths or ['.'], tuple(a.exclude)) + unread = unread_diff_files(scope, a.paths or ['.'], tuple(a.exclude), scan_root) if unread: shown = ', '.join(unread[:5]) + (' and more' if len(unread) > 5 else '') print(f'error: the diff against {a.diff!r} names {len(unread)} readable file(s) ' @@ -1218,16 +1282,18 @@ def main(argv: list[str] | None = None) -> int: bykind: dict[str, int] = {} byfile: dict[str, int] = {} for f in files: - allowed = scope.get(rel(f)) if scope is not None else None + allowed = scope.get(keys[f]) if scope is not None else None for ln, kind, msg in check_file(f, rules): if allowed is not None and ln not in allowed: continue total += 1 bykind[kind] = bykind.get(kind, 0) + 1 - byfile[rel(f)] = byfile.get(rel(f), 0) + 1 + byfile[keys[f]] = byfile.get(keys[f], 0) + 1 if not a.summary: - print(f'{rel(f)}:{ln}: {kind}: {msg}') + print(f'{keys[f]}:{ln}: {kind}: {msg}') + inscope = sum(len(scope[keys[f]]) for f in files) if scope is not None else None + print(scope_note(len(files), discovered, inscope, a.diff), file=sys.stderr) if a.summary or total: print(f'\n{total} violation(s) across {len(byfile)} file(s)', file=sys.stderr) for k, v in sorted(bykind.items(), key=lambda kv: -kv[1]): diff --git a/scripts/test_prose_lint.py b/scripts/test_prose_lint.py index b36bbf84..f59ab50d 100644 --- a/scripts/test_prose_lint.py +++ b/scripts/test_prose_lint.py @@ -1202,7 +1202,6 @@ def test_a_parent_directory_above_the_checkout_does_not_decide_generated(self) - (repo / 'authored.md').write_text('fine\n', encoding='utf-8') (repo / 'reports' / 'audit.md').write_text('fine\n', encoding='utf-8') with mock.patch.object(prose_lint, 'tracked_paths', return_value=None), \ - mock.patch.object(prose_lint, 'repo_prefix', return_value=''), \ contextlib.redirect_stderr(io.StringIO()): found = prose_lint.discover([str(repo)]) self.assertEqual(['authored.md'], [p.name for p in found]) @@ -1436,9 +1435,12 @@ class TestChangedLines(unittest.TestCase): ) def run_diff(self, stdout: str = '', returncode: int = 0): + # Untracked files are a second source for the same map and are asserted separately. + # The parse is read here alone rather than through whatever the tree happens to hold. done = subprocess.CompletedProcess(args=[], returncode=returncode, stdout=stdout, stderr='') - with mock.patch.object(prose_lint.subprocess, 'run', return_value=done): - return prose_lint.changed_lines('origin/develop') + with mock.patch.object(prose_lint.subprocess, 'run', return_value=done), \ + mock.patch.object(prose_lint, 'untracked_paths', return_value=[]): + return prose_lint.changed_lines('origin/develop', Path('.')) def test_each_hunk_maps_to_the_lines_it_adds(self) -> None: """A single-line hunk carries no count, and a deletion-only hunk adds nothing.""" @@ -1458,9 +1460,9 @@ def test_a_git_failure_is_none_rather_than_an_empty_scope(self) -> None: """An empty scope filters every file out and reports a clean run, which is a false pass.""" with mock.patch.object(prose_lint.subprocess, 'run', side_effect=subprocess.CalledProcessError(1, 'git')): - self.assertIsNone(prose_lint.changed_lines('origin/develop')) + self.assertIsNone(prose_lint.changed_lines('origin/develop', Path('.'))) with mock.patch.object(prose_lint.subprocess, 'run', side_effect=FileNotFoundError): - self.assertIsNone(prose_lint.changed_lines('origin/develop')) + self.assertIsNone(prose_lint.changed_lines('origin/develop', Path('.'))) class TestCli(unittest.TestCase): @@ -1508,39 +1510,24 @@ def test_a_bare_run_checks_comment_shape(self) -> None: with mock.patch.object(prose_lint, 'discover', return_value=[bait]): self.assertEqual(1, prose_lint.main([])) - def test_diffing_one_repository_while_scanning_another_is_refused(self) -> None: - """The intersection is empty, so it reports a clean run over an unchecked tree. - - `git diff` runs in the current directory while the paths may name another checkout. - A PhotoCleaner branch reported zero findings when the gate was run from the hub's - directory and three when run from its own, and the zero was believed. - """ - with mock.patch.object(prose_lint, 'repo_root', - side_effect=lambda p: '/hub' if str(p) == '.' else '/other'), \ - mock.patch.object(prose_lint, 'discover') as disc: - self.assertEqual(2, prose_lint.main(['--diff', 'HEAD', '/other/tree'])) - # Refused before discovery, which reads every tracked file to classify it as text. - disc.assert_not_called() - - def test_a_path_under_no_repository_is_refused_too(self) -> None: - """It fails the same way as a different repository, and more quietly. + def test_a_path_under_no_repository_is_refused_rather_than_scoped_to_nothing(self) -> None: + """A diff needs a repository, and having none is an error rather than an empty scope. - Discovery falls back to a filesystem walk, then every absolute key misses the diff's - repository-relative ones, so the scope drops every file and the run exits 0. Testing for - a *different* root missed this, because there is no root to differ from. + Discovery falls back to a filesystem walk, and an empty scope would drop every file it + found and report a clean run. The refusal now comes from the diff itself, which is where + the impossibility actually is: a guard comparing the scan root against the process's own + root could not see this at all, because there is no root to differ from. """ # A real directory, since a path that does not exist is refused earlier for another reason. - # That would pass this assertion without ever exercising the diff guard. + # That would pass this assertion without ever reaching the diff. loose = self.tmp / 'loose' loose.mkdir() - with mock.patch.object(prose_lint, 'repo_root', - side_effect=lambda p: '/hub' if str(p) == '.' else ''), \ - contextlib.redirect_stderr(io.StringIO()) as err: + with contextlib.redirect_stderr(io.StringIO()) as err: self.assertEqual(2, prose_lint.main(['--diff', 'HEAD', str(loose)])) - self.assertIn('no git repository', err.getvalue()) + self.assertIn('cannot diff against', err.getvalue()) def test_list_files_still_reports_scope_across_repositories(self) -> None: - """It reports the scan scope and never consults the diff, so the guard must not stop it.""" + """It reports the scan scope and never consults the diff, so no diff failure reaches it.""" clean = self.tmp / 'clean.md' clean.write_text('fine\n', encoding='utf-8') other = self.tmp / 'other' @@ -1551,7 +1538,7 @@ def test_list_files_still_reports_scope_across_repositories(self) -> None: self.assertEqual(0, prose_lint.main(['--list-files', '--diff', 'HEAD', str(other)])) def test_a_matching_repository_is_not_refused(self) -> None: - """The guard must not reject the ordinary case it sits in front of.""" + """The ordinary case, kept as the floor under every narrowing rule above it.""" clean = self.tmp / 'clean.md' clean.write_text('Nothing here breaks a rule.\n', encoding='utf-8') with mock.patch.object(prose_lint, 'repo_root', return_value='/hub'), \ @@ -2034,8 +2021,7 @@ def test_a_run_that_matched_a_file_asserts_nothing_further(self) -> None: bait.write_text(f'{DUP} thing\n', encoding='utf-8') with mock.patch.object(prose_lint, 'repo_root', return_value=str(self.tmp)), \ mock.patch.object(prose_lint, 'discover', return_value=[bait]), \ - mock.patch.object(prose_lint, 'changed_lines', - return_value={prose_lint.rel(bait): {1}}): + mock.patch.object(prose_lint, 'changed_lines', return_value={'bait.md': {1}}): self.assertEqual(1, prose_lint.main(['--check', 'dupword', '--diff', 'HEAD'])) def test_a_deliberately_narrowed_scan_is_not_told_the_narrowing_is_a_defect(self) -> None: @@ -2083,8 +2069,7 @@ def test_diff_keys_resolve_against_the_repository_rather_than_the_working_direct (self.tmp / 'scripts').mkdir() nested = self.tmp / 'scripts' / 'tool.py' nested.write_text('# A clean comment.\n', encoding='utf-8') - with mock.patch.object(prose_lint, 'repo_root', return_value=str(self.tmp)): - found = prose_lint.unread_diff_files({'scripts/tool.py': {1}}, ['.'], ()) + found = prose_lint.unread_diff_files({'scripts/tool.py': {1}}, ['.'], (), self.tmp) self.assertEqual(['scripts/tool.py'], found) def test_asked_about_reads_a_prefix_as_a_directory_boundary(self) -> None: @@ -2097,6 +2082,244 @@ def test_asked_about_reads_a_prefix_as_a_directory_boundary(self) -> None: self.assertFalse(prose_lint.asked_about('docs/x.md', ['catalog'])) +class TestTheScanScopeIsTheScopeReported(unittest.TestCase): + """What the run read, against real repositories rather than a mocked answer about them. + + Every false clean on record lives in the join between two coordinate systems: a diff names + files relative to the repository top level, and a path argument arrives in whatever form the + caller typed. Mocking `repo_root` or `discover` supplies that join already made, so the cases + below build repositories and let git answer. Both defects were reported clean by the 210-case + suite at `e2a99f1`, which was green in full, and both are here. + + The invariant: the rule set, the file set, the diff, and the keys that join the last two are + all read from the repository being scanned, and none of them from the directory the process + happens to stand in. A verdict states the scope it covered, since reading nothing prints what + finding nothing prints otherwise. + """ + + BAIT = '# New\n\nA line with a repeated the the word in it.\n' + + def setUp(self) -> None: + self.tmp = Path(self.enterContext(tempfile.TemporaryDirectory())) + self.enterContext(contextlib.redirect_stdout(io.StringIO())) + self.err = self.enterContext(contextlib.redirect_stderr(io.StringIO())) + + def git(self, root: Path, *args: str) -> None: + # Signing is disabled explicitly, since a host that signs by default cannot commit here. + subprocess.run(['git', '-C', str(root), '-c', 'user.email=gate@example.invalid', + '-c', 'user.name=gate test', '-c', 'commit.gpgsign=false', *args], + check=True, capture_output=True) + + def repo(self, name: str = 'repo') -> Path: + """A real repository holding one committed clean file, which is the base every case diffs.""" + root = self.tmp / name + root.mkdir() + self.git(root, 'init', '-q') + (root / 'DOC.md').write_text('# Doc\n\nA clean line.\n', encoding='utf-8') + self.git(root, 'add', '-A') + self.git(root, 'commit', '-qm', 'base') + return root + + def run_in(self, cwd: Path, *argv: str) -> int: + with contextlib.chdir(cwd): + return prose_lint.main(['--check', 'dupword', *argv]) + + def test_an_absolute_path_argument_reads_what_the_relative_one_reads(self) -> None: + """The reported defect: same directory, same repository, same ref, and a clean verdict. + + `prose_lint.py . --diff BASE` reported the finding while `prose_lint.py /abs/path --diff + BASE` reported nothing and exited 0. Discovery returned absolute paths and the diff named + repository-relative ones, so the intersection was empty and an empty intersection is what + a clean tree looks like. The same-root guard could not fire, because the run genuinely was + in the right repository. + """ + root = self.repo() + (root / 'DOC.md').write_text(self.BAIT, encoding='utf-8') + self.git(root, 'commit', '-qam', 'change') + self.assertEqual(1, self.run_in(root, '.', '--diff', 'HEAD~1')) + self.assertEqual(1, self.run_in(root, str(root), '--diff', 'HEAD~1')) + + def test_an_untracked_file_is_in_the_scope_of_the_change_that_adds_it(self) -> None: + """The second reported defect, on identical bytes either side of a `git add`. + + A new file read clean while unstaged and reported its findings the moment it was + committed. `git diff` never names an untracked file, so a change whose whole point is + adding one scoped to nothing, and a change is exactly when a new file is most worth + reading. + """ + root = self.repo() + (root / 'NEW.md').write_text(self.BAIT, encoding='utf-8') + self.assertEqual(1, self.run_in(root, '.', '--diff', 'HEAD')) + self.git(root, 'add', 'NEW.md') + self.assertEqual(1, self.run_in(root, '.', '--diff', 'HEAD')) + + def test_an_untracked_file_is_read_by_a_whole_tree_sweep_too(self) -> None: + """`git ls-files` omits it as well, so the hole was never particular to `--diff`. + + A sweep was the documented way to check what a diff-scoped run might have missed, and it + passed over the same file for its own reason. + """ + root = self.repo() + (root / 'NEW.md').write_text(self.BAIT, encoding='utf-8') + self.assertEqual(1, self.run_in(root, '.')) + + def test_an_ignored_file_is_read_by_neither(self) -> None: + """A build output is not authored text, so the widening stops where git's own rule does.""" + root = self.repo() + (root / '.gitignore').write_text('generated/\n', encoding='utf-8') + self.git(root, 'add', '.gitignore') + self.git(root, 'commit', '-qm', 'ignore') + (root / 'generated').mkdir() + (root / 'generated' / 'out.md').write_text(self.BAIT, encoding='utf-8') + self.assertEqual(0, self.run_in(root, '.')) + self.assertEqual(0, self.run_in(root, '.', '--diff', 'HEAD')) + + def test_a_generated_tree_stays_out_when_the_file_in_it_is_untracked(self) -> None: + """The generated-tree rule is about who wrote the prose, so tracking does not change it.""" + root = self.repo() + (root / 'reports').mkdir() + (root / 'reports' / 'audit.md').write_text(self.BAIT, encoding='utf-8') + self.assertEqual(0, self.run_in(root, '.')) + + def test_a_binary_file_is_not_read_because_it_is_untracked(self) -> None: + """The text test decides what the rules can read, and it is applied to both sources.""" + root = self.repo() + (root / 'logo.png').write_bytes(b'\x89PNG\r\n\x1a\n\x00 the the') + self.assertEqual(0, self.run_in(root, '.', '--diff', 'HEAD')) + + def test_a_run_from_a_subdirectory_keys_on_the_repository(self) -> None: + """It used to be refused with advice to run from the top level, which was the guard talking. + + The keys differed rather than the repositories, so anchoring both sides on the scanned + repository answers the case instead of turning it away. + """ + root = self.repo() + (root / 'sub').mkdir() + (root / 'sub' / 'note.md').write_text(self.BAIT, encoding='utf-8') + self.assertEqual(1, self.run_in(root / 'sub', '.', '--diff', 'HEAD')) + + def test_scanning_one_repository_while_standing_in_another_diffs_the_one_scanned(self) -> None: + """A refusal was standing in for this, because the diff was taken where the process stood. + + The rule set already came from the scanned repository. The diff and the keys now come from + there too, which is the whole of what the earlier guard was approximating. + """ + here = self.repo('here') + there = self.repo('there') + (there / 'DOC.md').write_text(self.BAIT, encoding='utf-8') + self.git(there, 'commit', '-qam', 'change') + self.assertEqual(1, self.run_in(here, str(there), '--diff', 'HEAD~1')) + + def test_a_subtree_argument_reads_the_untracked_files_inside_it(self) -> None: + """`git ls-files` prints names relative to its `-C` directory, for `--others` as well. + + Review read it the other way round and proposed joining both lists on the repository top + level instead, which would point every name at the wrong place and drop the subtree + silently. Measured on git 2.51: `git -C sub ls-files --others` prints `untracked.md` and + `deep/untracked2.md`, not the `sub/` forms. The tracked half of this is pinned by + TestDiscovery, and the untracked half is pinned here because the two lists are joined the + same way and a reader has no reason to expect them to differ. + + The keys reported stay repository-relative whichever form the argument took, which is what + makes a finding in a subtree name a path the repository recognizes. + + The discovered count is asserted rather than the findings alone. Listing from the top level + instead reads the whole repository, and every file outside the subtree was clean, so the + findings agreed under both and proved nothing. What the argument narrows is the count. + """ + root = self.repo() + (root / 'sub' / 'deep').mkdir(parents=True) + (root / 'sub' / 'near.md').write_text(self.BAIT, encoding='utf-8') + (root / 'sub' / 'deep' / 'far.md').write_text(self.BAIT, encoding='utf-8') + for arg in ('sub', str(root / 'sub')): + with self.subTest(arg=arg): + err = self.enterContext(contextlib.redirect_stderr(io.StringIO())) + with contextlib.redirect_stdout(io.StringIO()) as out: + self.assertEqual(1, self.run_in(root, arg, '--diff', 'HEAD')) + reported = sorted(line.split(':')[0] for line in out.getvalue().splitlines() + if line.strip()) + self.assertEqual(['sub/deep/far.md', 'sub/near.md'], reported) + # DOC.md sits outside the subtree, so a run that reads three files read too much. + self.assertIn('2 of 2 file(s) read', err.getvalue()) + + def test_a_subtree_holding_no_tracked_file_is_still_described_by_git(self) -> None: + """The walk is for a tree git cannot describe, not for one whose answer is empty. + + `tracked_paths` returns None for both, deliberately, because an initialized but empty + checkout answering with an empty list would scan nothing and read as a pass. Reading that + None as "git cannot describe this" sent a subtree holding only new files down the walk, + which applies no ignore rules, so a build output under it was scanned and reported. The + run also printed that git could not describe a tree git describes perfectly well. + + Whether git can describe a tree is settled by asking git, never by the size of its answer. + """ + root = self.repo() + (root / '.gitignore').write_text('newdir/ignored.md\n', encoding='utf-8') + self.git(root, 'add', '.gitignore') + self.git(root, 'commit', '-qm', 'ignore') + (root / 'newdir').mkdir() + (root / 'newdir' / 'authored.md').write_text(self.BAIT, encoding='utf-8') + (root / 'newdir' / 'ignored.md').write_text(self.BAIT, encoding='utf-8') + with contextlib.redirect_stdout(io.StringIO()) as out: + self.assertEqual(1, self.run_in(root, 'newdir')) + reported = sorted(line.split(':')[0] for line in out.getvalue().splitlines() if line.strip()) + self.assertEqual(['newdir/authored.md'], reported) + self.assertNotIn('git cannot describe', self.err.getvalue()) + + def test_a_relative_diff_setting_does_not_re_anchor_the_keys(self) -> None: + """`diff.relative` anchors a diff's paths on the process's directory, not the repository. + + This case passed before the fix too, because both sides were anchored on that directory + and agreed by accident. It is pinned because only one side moved: the keys must come from + the repository whatever the caller's configuration says, and a setting is an input shape + like any other. + """ + root = self.repo() + self.git(root, 'config', 'diff.relative', 'true') + (root / 'sub').mkdir() + (root / 'sub' / 'note.md').write_text('A clean line.\n', encoding='utf-8') + self.git(root, 'add', '-A') + self.git(root, 'commit', '-qm', 'sub') + (root / 'sub' / 'note.md').write_text(self.BAIT, encoding='utf-8') + self.assertEqual(1, self.run_in(root / 'sub', '.', '--diff', 'HEAD~1')) + + def test_a_clean_run_states_what_it_read(self) -> None: + """The class the two defects belong to, which no per-route guard covers. + + Both exited 0 in silence, and so did the three routes closed before them. A reader cannot + tell a gate that read nothing from a gate with nothing to report unless the run says which + it was, so the count is printed on a clean verdict rather than only on a busy one. + """ + root = self.repo() + self.assertEqual(0, self.run_in(root, '.')) + self.assertIn('1 file(s) read, whole tree', self.err.getvalue()) + + def test_a_scope_of_nothing_is_reported_as_nothing(self) -> None: + """A change the rules cannot read is honestly clean, and says so as a count of zero.""" + root = self.repo() + (root / 'logo.png').write_bytes(b'\x89PNG\r\n\x1a\n\x00binary') + self.git(root, 'add', '-A') + self.git(root, 'commit', '-qm', 'add a logo') + self.assertEqual(0, self.run_in(root, '.', '--diff', 'HEAD~1')) + self.assertIn('0 of 1 file(s) read, 0 changed line(s)', self.err.getvalue()) + + def test_the_reported_scope_counts_only_what_the_verdict_covered(self) -> None: + """A count wider than the read is the same lie in a smaller font. + + The narrowed file set and the lines inside it are what the findings were drawn from, so + those are what the note states, with the discovered total beside them for contrast. + """ + root = self.repo() + (root / 'OTHER.md').write_text('# Other\n\nA clean line.\n', encoding='utf-8') + self.git(root, 'add', '-A') + self.git(root, 'commit', '-qm', 'other') + (root / 'DOC.md').write_text(self.BAIT, encoding='utf-8') + self.git(root, 'commit', '-qam', 'change') + self.assertEqual(1, self.run_in(root, '.', '--diff', 'HEAD~1')) + # Two of the three lines changed, since the blank line between them did not. + self.assertIn('1 of 2 file(s) read, 2 changed line(s)', self.err.getvalue()) + + class TestHarness(unittest.TestCase): def test_this_module_collects_a_plausible_number_of_cases(self) -> None: """A module whose cases fail to load still reports OK, which is a pass proving nothing."""