Coverage for src/keel/revertcheck.py: 100%
531 statements
« prev ^ index » next coverage.py v7.16.2, created at 2026-10-02 20:26 +0000
1"""The opt-in ``revert-check`` gate (#1289): does any test notice when a change is undone?
3Coverage proves that a line *ran*, not that an assertion *depends* on it. With
4``fail_under = 100`` enforced, "maintained 100 % coverage" is true of every merged pull
5request before anyone writes it, and an audit of 14 closed fixes found three whose tests
6passed with the fix removed. This gate asks the question coverage cannot: for each
7production change on the branch, revert **that change alone** in a scratch worktree, run
8the project's tests, and require a test to **fail as an assertion**.
10It is mutation testing scoped to one mutation per change — the change itself.
12This module is the pure half, and deterministic:
14* :func:`resolve` — ``knobs.revert_check`` (+ ``build_gate_cmd``, ``gate_timeout_s``) ->
15 :class:`Settings`;
16* :func:`test_paths` — where the tests live, from ``policy_pack.test_groups.*.test_paths``;
17* :func:`parse_diff` / :func:`plan_changes` — the branch's unified diff -> the production
18 changes to revert, each with the patch that undoes it;
19* :func:`read_output` / :func:`classify` — a test run's output -> *caught* (a test failed as
20 an assertion), *errored*, *unnoticed*, *timed out* or *unreadable*;
21* :func:`execute` — the bounded loop (``max_changes``, ``budget_s``), driven through an
22 injected ``run`` callable and ``clock``, so the loop is tested offline;
23* :func:`judge` — the gate's verdict and findings.
25The git and subprocess work — the scratch worktree, ``git apply -R``, the test command —
26lives in :mod:`keel.cli` through :mod:`keel.git` and :mod:`keel.runner`; nothing here
27touches the file system.
29**What it does not do.** The unit is a git hunk (or a file), not a *behaviour*: #871's
30guarded and unguarded arms shared one hunk, so a per-hunk check passes while half that fix
31is unpinned. And a passing check is necessary, not sufficient: #873's test fails without
32its fix and still could not see the regression the fix shipped (#1268), because its fixture
33made the fix and the bug agree. Both remain a reviewer's questions.
34"""
36from __future__ import annotations
38import ast
39import fnmatch
40import posixpath
41import re
42from collections.abc import Callable, Mapping, Sequence
43from dataclasses import dataclass
44from typing import Any
46from .findings import Finding
48#: The gate id, as ``gates:`` lists it.
49GATE_ID = "revert-check"
51#: ``knobs.revert_check.unit``: revert each hunk alone (the default), or each file.
52UNIT_HUNK = "hunk"
53UNIT_FILE = "file"
54UNITS = (UNIT_HUNK, UNIT_FILE)
56#: At most this many changes are reverted per run unless ``max_changes`` says otherwise.
57DEFAULT_MAX_CHANGES = 10
58#: Wall-clock seconds the whole check — the baseline run and every revert — may spend.
59DEFAULT_BUDGET_S = 1800
61#: What counts as production when ``knobs.revert_check.paths`` is not set: a changed file
62#: with a source-code suffix that is not under the project's test paths. Docs, config,
63#: lockfiles and data are left out — reverting a README cannot make a test fail, and
64#: reporting it as unnoticed would be noise, not evidence.
65SOURCE_SUFFIXES: tuple[str, ...] = (
66 ".py",
67 ".pyi",
68 ".js",
69 ".jsx",
70 ".mjs",
71 ".cjs",
72 ".ts",
73 ".tsx",
74 ".go",
75 ".rs",
76 ".java",
77 ".kt",
78 ".kts",
79 ".scala",
80 ".rb",
81 ".php",
82 ".cs",
83 ".fs",
84 ".c",
85 ".h",
86 ".cc",
87 ".cpp",
88 ".cxx",
89 ".hpp",
90 ".m",
91 ".mm",
92 ".swift",
93 ".dart",
94 ".ex",
95 ".exs",
96 ".erl",
97 ".clj",
98 ".lua",
99 ".pl",
100 ".pm",
101 ".r",
102 ".sh",
103 ".bash",
104)
106# Per-change results.
107CAUGHT = "caught" # a test failed as an assertion
108ERRORED = "errored" # tests failed, none as an assertion (an exception, an import error)
109UNNOTICED = "unnoticed" # the test command passed with the change reverted
110TIMED_OUT = "timed-out"
111UNREADABLE = "unreadable" # the command failed and its output says no test failed
112NOT_APPLIED = "not-applied" # the reverse patch did not apply on HEAD
114_BLOCK = "major"
117@dataclass(frozen=True)
118class Settings:
119 """The resolved ``knobs.revert_check`` for one run."""
121 #: The test command each revert runs; ``None`` when neither knob sets one.
122 cmd: str | None
123 #: The knob :attr:`cmd` came from (``knobs.build_gate_cmd`` when neither sets one).
124 cmd_source: str
125 #: Globs that select production files; empty means :data:`SOURCE_SUFFIXES`.
126 paths: tuple[str, ...]
127 unit: str
128 max_changes: int
129 budget_s: int
130 #: The wall-clock limit of one test run (``knobs.gate_timeout_s``).
131 run_timeout_s: int
134def resolve(
135 knob: Mapping[str, Any] | None, *, build_cmd: str | None, gate_timeout_s: int
136) -> Settings:
137 """``knobs.revert_check`` -> :class:`Settings`, defaults filled in.
139 The test command is ``knobs.revert_check.cmd`` when set, else ``knobs.build_gate_cmd``:
140 a project whose suite is slow can point the check at the tests that matter without
141 changing what its ``build`` gate runs. The schema owns the value shapes; anything it
142 would refuse reads as the default here, because this resolver runs on every gate run.
143 """
144 raw = knob if isinstance(knob, Mapping) else {}
145 own = raw.get("cmd")
146 if isinstance(own, str) and own.strip():
147 cmd: str | None = own
148 source = "knobs.revert_check.cmd"
149 else:
150 cmd = build_cmd if isinstance(build_cmd, str) and build_cmd.strip() else None
151 source = "knobs.build_gate_cmd"
152 paths = raw.get("paths")
153 unit = raw.get("unit")
154 return Settings(
155 cmd=cmd,
156 cmd_source=source,
157 paths=tuple(_strings(paths)),
158 unit=unit if unit in UNITS else UNIT_HUNK,
159 max_changes=_positive(raw.get("max_changes"), DEFAULT_MAX_CHANGES),
160 budget_s=_positive(raw.get("budget_s"), DEFAULT_BUDGET_S),
161 run_timeout_s=max(1, int(gate_timeout_s)),
162 )
165def _positive(value: Any, default: int) -> int:
166 """A positive integer knob, or ``default`` (``bool`` is not a number here)."""
167 if isinstance(value, int) and not isinstance(value, bool) and value >= 1:
168 return value
169 return default
172def _strings(raw: Any) -> list[str]:
173 """Non-blank string entries of a list (anything else contributes nothing)."""
174 if not isinstance(raw, Sequence) or isinstance(raw, (str, bytes, bytearray)):
175 return []
176 return [entry.strip() for entry in raw if isinstance(entry, str) and entry.strip()]
179def test_paths(policy_pack: Mapping[str, Any] | None) -> tuple[str, ...]:
180 """Where the project's tests live: every ``policy_pack.test_groups.*.test_paths`` glob.
182 Only the **declared** ``test_paths`` count. A group's ``paths`` are selectors and
183 routinely include the implementation surface (keel's own ``unit`` group selects
184 ``src/**``); read as test paths they would classify every production file as a test,
185 and the gate would skip every change it exists to check.
186 """
187 pack = policy_pack if isinstance(policy_pack, Mapping) else {}
188 groups = pack.get("test_groups")
189 if not isinstance(groups, Mapping):
190 return ()
191 globs: list[str] = []
192 for _name, group in sorted(groups.items(), key=lambda item: str(item[0])):
193 if isinstance(group, Mapping):
194 globs.extend(_strings(group.get("test_paths")))
195 return tuple(dict.fromkeys(globs))
198def is_production(path: str, *, tests: Sequence[str], paths: Sequence[str]) -> bool:
199 """Is ``path`` production code this gate reverts?
201 Never a test path. Then ``paths`` (``fnmatch`` globs, the matcher ``tier3_globs`` and
202 ``test_paths`` use) when the project set it, else a source-code suffix.
203 """
204 if any(fnmatch.fnmatch(path, glob) for glob in tests):
205 return False
206 if paths:
207 return any(fnmatch.fnmatch(path, glob) for glob in paths)
208 return path.lower().endswith(SOURCE_SUFFIXES)
211# --- the diff ------------------------------------------------------------------------
213_HUNK_RE = re.compile(r"^@@ -(\d+)(?:,(\d+))? \+(\d+)(?:,(\d+))? @@")
216@dataclass(frozen=True)
217class Hunk:
218 """One ``@@`` hunk: its header line and body, verbatim."""
220 header: str
221 lines: tuple[str, ...]
222 added: int
223 removed: int
224 #: Context lines. :func:`keel.git.revert_diff` asks for none, so any here means the
225 #: diff is not the shape the plan needs (:func:`context_problem`).
226 context: int = 0
228 @property
229 def pure_addition(self) -> bool:
230 """Only adds lines: reverting it removes code rather than restoring old code."""
231 return self.removed == 0
234@dataclass(frozen=True)
235class FileDiff:
236 """One file's part of a unified diff."""
238 path: str
239 #: The lines from ``diff --git`` up to the first hunk — what ``git apply`` needs to
240 #: know which file, and whether it is new or deleted.
241 header: tuple[str, ...]
242 hunks: tuple[Hunk, ...]
243 status: str # added | deleted | modified
245 @property
246 def mode(self) -> tuple[str, str] | None:
247 """``(old, new)`` when the diff changes the file's mode (``old mode`` / ``new mode``)."""
248 old = next((line[9:] for line in self.header if line.startswith("old mode ")), None)
249 new = next((line[9:] for line in self.header if line.startswith("new mode ")), None)
250 return (old, new) if old and new else None
252 @property
253 def binary(self) -> bool:
254 """git printed no text for it (``Binary files … differ``)."""
255 return any(line.startswith(("Binary files ", "GIT binary patch")) for line in self.header)
257 @property
258 def content_header(self) -> tuple[str, ...]:
259 """The header without its mode lines: a content hunk must not carry the mode along."""
260 return tuple(
261 line for line in self.header if not line.startswith(("old mode ", "new mode "))
262 )
265#: git's C-style escapes in a quoted path (``quote_c_style``), beside ``\\ooo`` octal bytes.
266_C_ESCAPES = {"a": 7, "b": 8, "t": 9, "n": 10, "v": 11, "f": 12, "r": 13, '"': 34, "\\": 92}
269def _unquote(name: str) -> str:
270 """A path as git wrote it -> the path itself.
272 git wraps a name in double quotes when it holds a control character, a quote or a
273 backslash, and escapes those inside (``"a/q\\"x.py"``); a byte is ``\\ooo`` octal. An
274 unquoted name is returned as it is.
275 """
276 if len(name) < 2 or not (name.startswith('"') and name.endswith('"')):
277 return name
278 body, out, i = name[1:-1], bytearray(), 0
279 while i < len(body):
280 char = body[i]
281 octal = body[i + 1 : i + 4]
282 if char == "\\" and body[i + 1 : i + 2] in _C_ESCAPES:
283 out.append(_C_ESCAPES[body[i + 1]])
284 i += 2
285 elif char == "\\" and len(octal) == 3 and all(c in "01234567" for c in octal):
286 out.append(int(octal, 8) & 0xFF)
287 i += 4
288 else:
289 out += char.encode("utf-8", "surrogateescape")
290 i += 1
291 return out.decode("utf-8", "surrogateescape")
294def _closing_quote(text: str) -> int:
295 """The index of the quote that closes the quoted name opening ``text``; ``-1`` if none."""
296 i = 1
297 while i < len(text):
298 if text[i] == "\\":
299 i += 2
300 continue
301 if text[i] == '"':
302 return i
303 i += 1
304 return -1
307def _strip(name: str, prefix: str) -> str:
308 return name[len(prefix) :] if name.startswith(prefix) else name
311def _path_from(line: str, prefix: str) -> str | None:
312 """The path on a ``--- a/x`` / ``+++ b/x`` line; ``None`` for ``/dev/null``.
314 git ends the line with a tab when the name contains a space, and quotes a name that
315 holds a control character, a quote or a backslash (:func:`_unquote`); the path is read
316 back so it matches the project's globs.
317 """
318 name = line[4:].rstrip("\t")
319 if name == "/dev/null":
320 return None
321 return _strip(_unquote(name), prefix)
324def _header_path(line: str) -> str:
325 """The path on a ``diff --git a/x b/x`` line — the only name a file with no ``---`` /
326 ``+++`` lines (a mode change, a binary file) carries.
328 With renames off both names are the same file, so an unquoted pair splits in the middle
329 (a name may hold ``" b/"``), and a quoted one ends at its closing quote. A header that
330 reads neither way falls back to the text after the last ``" b/"``.
331 """
332 rest = line[len("diff --git ") :]
333 if rest.startswith('"'):
334 end = _closing_quote(rest)
335 if end > 0:
336 return _strip(_unquote(rest[: end + 1]), "a/")
337 half = (len(rest) - 5) // 2
338 name = rest[2 : 2 + half]
339 if rest.startswith("a/") and rest[2 + half :] == f" b/{name}":
340 return name
341 return rest.rsplit(" b/", 1)[-1]
344def parse_diff(text: str) -> tuple[FileDiff, ...]:
345 """Split ``git diff --no-renames --src-prefix=a/ --dst-prefix=b/`` output into files.
347 Hunk bodies are consumed by the counts in their ``@@`` header, not by their first
348 character, so a blank context line (``diff.suppressBlankEmpty``) cannot end a hunk
349 early. A file with no hunk — binary, or a mode change — is kept with ``hunks=()``, so
350 the plan can say it was not checked rather than drop it.
351 """
352 files: list[FileDiff] = []
353 lines = text.split("\n")
354 i = 0
355 while i < len(lines):
356 if not lines[i].startswith("diff --git "):
357 i += 1
358 continue
359 header = [lines[i]]
360 fallback = _header_path(lines[i])
361 old = new = None
362 i += 1
363 while i < len(lines) and not lines[i].startswith(("@@", "diff --git ")):
364 line = lines[i]
365 if line.startswith("--- "):
366 old = _path_from(line, "a/")
367 elif line.startswith("+++ "):
368 new = _path_from(line, "b/")
369 header.append(line)
370 i += 1
371 hunks: list[Hunk] = []
372 while i < len(lines) and lines[i].startswith("@@"):
373 hunk, i = _read_hunk(lines, i)
374 hunks.append(hunk)
375 text_header = "\n".join(header)
376 if "\nnew file mode " in text_header:
377 status = "added"
378 elif "\ndeleted file mode " in text_header:
379 status = "deleted"
380 else:
381 status = "modified"
382 files.append(FileDiff(new or old or fallback, tuple(header), tuple(hunks), status))
383 return tuple(files)
386def _read_hunk(lines: list[str], i: int) -> tuple[Hunk, int]:
387 """Read the hunk whose header is ``lines[i]``; return it and the index after it."""
388 header = lines[i]
389 match = _HUNK_RE.match(header)
390 old_left = int(match.group(2) or "1") if match else 0
391 new_left = int(match.group(4) or "1") if match else 0
392 body: list[str] = []
393 added = removed = context = 0
394 i += 1
395 while i < len(lines) and (old_left > 0 or new_left > 0):
396 line = lines[i]
397 tag = line[:1]
398 if tag == "+":
399 added += 1
400 new_left -= 1
401 elif tag == "-":
402 removed += 1
403 old_left -= 1
404 elif tag == "\\":
405 pass # "\ No newline at end of file" belongs to the line before it
406 else: # context, including an empty line git wrote for a blank one
407 old_left -= 1
408 new_left -= 1
409 context += 1
410 body.append(line)
411 i += 1
412 # A trailing "\ No newline at end of file" marker after the last counted line.
413 while i < len(lines) and lines[i].startswith("\\"):
414 body.append(lines[i])
415 i += 1
416 return Hunk(header, tuple(body), added, removed, context), i
419def context_problem(files: Sequence[FileDiff]) -> str | None:
420 """Why the diff cannot be split into independent changes, or ``None`` when it can.
422 Every hunk must carry **no context line**. :func:`keel.git.revert_diff` pins
423 ``--unified=0`` and ``--inter-hunk-context=0``, but ``GIT_DIFF_OPTS`` or a setting keel
424 does not know could still widen a hunk — and a hunk that carries context is either
425 two edits merged into one change, where one caught edit passes the other, or a patch
426 whose context ``--unidiff-zero`` would misplace. Checked here, on what git printed.
427 """
428 widened = [f.path for f in files if any(h.context for h in f.hunks)]
429 if not widened:
430 return None
431 return (
432 f"the diff carries context lines (in {', '.join(widened[:3])}), so its hunks may merge "
433 "independent edits; check GIT_DIFF_OPTS and the diff settings in your git config"
434 )
437@dataclass(frozen=True)
438class Change:
439 """One production change to revert: a label for the report and its patch."""
441 label: str
442 path: str
443 #: A unified diff that ``git apply -R`` undoes on HEAD.
444 patch: str
445 #: Every hunk in it only adds lines — see :func:`judge` for why that matters.
446 pure_addition: bool
449@dataclass(frozen=True)
450class Plan:
451 """What the gate will revert, and what it had to leave out."""
453 changes: tuple[Change, ...]
454 #: Production files with no textual hunk (a binary file, or an empty file added or
455 #: deleted): not checked. A mode-only change is a change of its own, and is checked.
456 unrevertable: tuple[str, ...]
457 #: Every path the diff touched, for the finding that says none was production.
458 touched: tuple[str, ...]
461def plan_changes(
462 files: Sequence[FileDiff], *, tests: Sequence[str], paths: Sequence[str], unit: str
463) -> Plan:
464 """The production changes to revert, ordered by path.
466 Ordered here, not by git: ``diff.orderFile`` reorders git's output, and the order
467 decides which changes ``max_changes`` reaches. Each change is reverted alone, so each
468 must carry only itself:
470 * **A mode change is its own change** (``old mode``/``new mode``). Copied into every
471 content hunk's patch, it was undone with each of them, and one test of the
472 executable bit "caught" every hunk of the file.
473 * **Added Python code is split at its top-level definitions** (:func:`_blocks`) — a
474 hunk that only adds lines, and a new file's content, whatever ``unit`` says for a
475 new file. Undone whole, three added functions where a test calls one read as
476 "noticed" for all three; undone one at a time, each must be noticed on its own. A
477 new file that is one block stays one change, which deletes it. Other languages, and
478 added lines that do not parse on their own, are not split.
479 * Otherwise one change per hunk (``unit: hunk``) or per file (``unit: file``); a
480 deleted file is one change, which restores it.
481 """
482 changes: list[Change] = []
483 unrevertable: list[str] = []
484 for f in sorted(files, key=lambda f: f.path):
485 if not is_production(f.path, tests=tests, paths=paths):
486 continue
487 if f.mode is not None and f.status == "modified":
488 old, new = f.mode
489 mode_patch = "\n".join((f.header[0], f"old mode {old}", f"new mode {new}")) + "\n"
490 changes.append(Change(f"{f.path} (mode {old} -> {new})", f.path, mode_patch, False))
491 if f.binary or not f.hunks:
492 if f.binary or f.mode is None:
493 unrevertable.append(f.path)
494 continue
495 header = f.content_header
496 if f.status == "added":
497 blocks = _blocks(f.hunks[0], f.path) if len(f.hunks) == 1 else []
498 if len(blocks) > 1:
499 plain = (header[0], _old_side(header), *(h for h in header if h[:4] == "+++ "))
500 changes.extend(_block_change(f.path, plain, block) for block in blocks)
501 else:
502 changes.append(
503 Change(f"{f.path} (new file)", f.path, _patch(header, f.hunks), True)
504 )
505 continue
506 if unit == UNIT_FILE or f.status == "deleted":
507 suffix = " (deleted file)" if f.status == "deleted" else ""
508 changes.append(
509 Change(
510 f"{f.path}{suffix}",
511 f.path,
512 _patch(header, f.hunks),
513 all(h.pure_addition for h in f.hunks),
514 )
515 )
516 continue
517 for hunk in f.hunks:
518 blocks = _blocks(hunk, f.path) if hunk.pure_addition else []
519 if len(blocks) > 1:
520 changes.extend(_block_change(f.path, header, block) for block in blocks)
521 continue
522 label = f"{f.path} {hunk.header.split(' @@', 1)[0]} @@"
523 changes.append(Change(label, f.path, _patch(header, (hunk,)), hunk.pure_addition))
524 return Plan(tuple(changes), tuple(unrevertable), tuple(f.path for f in files))
527#: Files whose added code :func:`_blocks` can split: it parses them.
528_PYTHON_SUFFIXES = (".py", ".pyi")
529_DEFINITIONS = (ast.FunctionDef, ast.AsyncFunctionDef, ast.ClassDef)
532def _old_side(header: Sequence[str]) -> str:
533 """A new file's ``--- a/x`` line, from its ``+++ b/x`` line as git wrote it (quoting and
534 all), so a patch that removes part of the file names the file that stays."""
535 new = next(h for h in header if h[:4] == "+++ ")
536 name = new[4:]
537 return "--- " + ('"a/' + name[3:] if name.startswith('"b/') else "a/" + name[2:])
540def _blocks(hunk: Hunk, path: str) -> list[tuple[int, tuple[str, ...]]]:
541 """A pure-addition hunk's lines split at its top-level definitions, as ``(first line, lines)``.
543 Only Python is split, and only when the added lines parse on their own
544 (:func:`_block_starts`): a boundary read from the text alone was wrong both ways — two
545 functions with no blank line between them stayed one change, so a test of one vouched
546 for the other, and a blank line inside a string literal split it, so each half's revert
547 was a syntax error. ``[]`` (keep the hunk whole) otherwise, and for a header that cannot
548 be read. Lines between blocks ride with the block before them (leading ones with the
549 first); a ``\\ No newline`` marker stays with its line.
550 """
551 match = _HUNK_RE.match(hunk.header)
552 if not match or not path.endswith(_PYTHON_SUFFIXES):
553 return []
554 starts = _block_starts([line[1:] for line in hunk.lines if line.startswith("+")])
555 if not starts:
556 return []
557 first = int(match.group(3))
558 blocks: list[tuple[int, list[str]]] = []
559 offset = 0
560 for line in hunk.lines:
561 if line.startswith("+"):
562 if not blocks or offset in starts:
563 blocks.append((first + offset, []))
564 offset += 1
565 blocks[-1][1].append(line)
566 return [(start, tuple(lines)) for start, lines in blocks]
569def _block_starts(added: Sequence[str]) -> set[int]:
570 """The 0-based added lines where a new block starts; empty when there is one block.
572 The lines are dedented by their common leading whitespace and parsed. Each ``def`` and
573 ``class`` (from its first decorator) is a block, and so is each run of other statements
574 between them. Empty when the lines do not parse on their own — part of an expression,
575 a string whose continuation sits left of the rest — or do not share one indent.
576 """
577 texts = [text for text in added if text.strip()]
578 if not texts:
579 return set()
580 width = min(len(text) - len(text.lstrip()) for text in texts)
581 indent = texts[0][:width]
582 if any(not text.startswith(indent) for text in texts):
583 return set()
584 source = "\n".join(text[width:] if text.strip() else "" for text in added) + "\n"
585 try:
586 tree = ast.parse(source)
587 except (SyntaxError, ValueError, RecursionError):
588 return set()
589 starts: set[int] = set()
590 previous: bool | None = None
591 for node in tree.body:
592 definition = isinstance(node, _DEFINITIONS)
593 if previous is not None and (definition or previous):
594 decorators = getattr(node, "decorator_list", [])
595 starts.add(min([node.lineno, *(d.lineno for d in decorators)]) - 1)
596 previous = definition
597 return starts
600def _block_change(path: str, header: Sequence[str], block: tuple[int, tuple[str, ...]]) -> Change:
601 """One block of added lines as its own change: a patch that removes just those lines."""
602 first, lines = block
603 count = sum(1 for line in lines if line.startswith("+"))
604 hunk_header = f"@@ -{first - 1},0 +{first},{count} @@"
605 patch = "\n".join((*header, hunk_header, *lines)) + "\n"
606 return Change(f"{path} {hunk_header}", path, patch, True)
609def _patch(header: Sequence[str], hunks: Sequence[Hunk]) -> str:
610 lines = list(header)
611 for hunk in hunks:
612 lines.append(hunk.header)
613 lines.extend(hunk.lines)
614 return "\n".join(lines) + "\n"
617# --- what a change looks like ------------------------------------------------------
618#
619# Nothing here skips a run. Every production change in scope is reverted and tested: an
620# earlier version skipped changes it judged "inert" (comments, docstrings, formatting),
621# and each proof of inertness had a hole — a Go ``//go:embed`` directive, a Python
622# ``# coding:`` cookie, a comment that moves ``__LINE__`` or ``f_lineno`` — because "no
623# test can observe this change" is not provable in general (#1289 review rounds).
625#: Leading text that makes a line **look** like a comment in some language. A wording
626#: heuristic only (:func:`looks_comment_only`): it never decides whether a change is run.
627_COMMENT_LOOKS: tuple[str, ...] = ("#", "//", "/*", "*/", "* ", "--", ";", "%")
630def _changed_lines(patch: str) -> list[str]:
631 """The ``+``/``-`` lines of a change's hunks, without their marker."""
632 _header, _sep, body = patch.partition("\n@@")
633 return [line[1:] for line in body.split("\n") if line[:1] in ("+", "-")]
636def looks_comment_only(change: Change) -> bool:
637 """Does every changed line look blank or like a comment? **Wording only.**
639 Used to explain a blocking "no test notices this change" finding, never to skip a
640 run: a comment can still change behaviour.
641 """
642 lines = _changed_lines(change.patch)
643 return bool(lines) and all(
644 not line.strip() or line.strip() == "*" or line.strip().startswith(_COMMENT_LOOKS)
645 for line in lines
646 )
649_NAMES = r"[A-Za-z_]\w*(?:\s+as\s+[A-Za-z_]\w*)?(?:\s*,\s*[A-Za-z_]\w*(?:\s+as\s+[A-Za-z_]\w*)?)*"
650_DOTTED = r"[A-Za-z_][\w.]*(?:\s+as\s+[A-Za-z_]\w*)?"
651#: One complete import statement on one line, and nothing else on it — no trailing
652#: comment, no ``;``, no open parenthesis left for a continuation line.
653_IMPORT_LINE = re.compile(
654 rf"^\s*(?:import\s+{_DOTTED}(?:\s*,\s*{_DOTTED})*"
655 rf"|from\s+(?:\.+[\w.]*|[A-Za-z_][\w.]*)\s+import\s+"
656 rf"(?:\*|{_NAMES}|\(\s*{_NAMES}\s*,?\s*\)))\s*$"
657)
660def imports_only(change: Change) -> bool:
661 """Is ``change`` a Python change whose every changed line is a whole import statement?
663 Read from the change's **own** lines, never from a comparison of the two files: an
664 AST cannot see an encoding cookie, and ``# coding: latin-1`` beside an import once
665 passed as "imports only" while it changed what a string literal decodes to. Blank
666 lines are ignored; a comment, a cookie, a continuation line of a multi-line import or
667 anything else makes the answer ``False`` — that change's errors then block, which is
668 the safe side. It classifies a result *after* its run; it never skips one.
669 """
670 if posixpath.splitext(change.path)[1].lower() not in (".py", ".pyi"):
671 return False
672 lines = [line for line in _changed_lines(change.patch) if line.strip()]
673 return bool(lines) and all(_IMPORT_LINE.match(line) for line in lines)
676#: The exceptions that say "a name the tests reach is gone" — all a revert of pure
677#: added code (or of an imported name) can produce. ``AttributeError`` counts only for a
678#: *module* or *class* attribute: ``'NoneType' object has no attribute`` is behaviour.
679_MISSING_NAME = re.compile(
680 r"^(?:NameError|UnboundLocalError|ImportError|ModuleNotFoundError)\b"
681 r"|^AttributeError: (?:(?:partially initialized )?module '[^']*'|type object '[^']*') "
682 r"has no attribute\b"
683)
684#: The start of a Python traceback; its exception line is the first unindented line after it.
685_TRACEBACK = "Traceback (most recent call last):"
686#: A pytest short-summary line's reason (`` - <Exception>: message``), any exception name.
687_PYTEST_REASON = re.compile(r"^(?:FAILED|ERROR) .*? - ([A-Za-z_][\w.]*(?::.*)?)$", re.MULTILINE)
690def _raised(output: str) -> list[str]:
691 """Every exception the output reports, as ``Name: message`` with the module dropped.
693 Read from each traceback's final line — whatever the exception is called, so a
694 custom ``Boom`` or a ``StopIteration`` is seen too — and from pytest's short summary.
695 """
696 found: list[str] = []
697 lines = output.split("\n")
698 for i, line in enumerate(lines):
699 if line.strip() != _TRACEBACK:
700 continue
701 rest = (text for text in lines[i + 1 :] if text.strip() and not text[:1].isspace())
702 found.append(next(rest, ""))
703 found.extend(match.group(1) for match in _PYTEST_REASON.finditer(output))
704 return [
705 name.rsplit(".", 1)[-1] + sep + message
706 for name, sep, message in (text.partition(":") for text in found)
707 ]
710def missing_names_only(output: str) -> bool:
711 """Does every exception the run reports say only that a name is missing?
713 The evidence a lenient reading needs: the error-only result of undoing an addition is
714 a ``nit`` only when the errors are the ones removing code can cause. A ``KeyError``, a
715 ``TypeError``, an attribute missing from an *instance*, any exception keel cannot
716 name, and output with no traceback at all are behaviour, and block.
718 **Every** failure the run counts must be described: a summary of ``2 failed`` whose
719 short summary names one ``NameError`` says nothing about the other, which could be an
720 assertion or a crash (#1289 review round 9). So pytest's undescribed failures
721 (:attr:`Tally.unclassified`) refuse it, and so does a count of failures and errors
722 larger than the exceptions keel read.
723 """
724 found = _raised(output)
725 tally = read_output(output)
726 return (
727 bool(found)
728 and not tally.unclassified
729 and len(found) >= tally.errors + tally.assertions
730 and all(_MISSING_NAME.match(text) for text in found)
731 )
734# --- reading a test run --------------------------------------------------------------
736_UNITTEST_RAN = re.compile(r"^Ran (\d+) tests? in ", re.MULTILINE)
737_UNITTEST_FAILED = re.compile(r"^FAILED \(([^)]*)\)\s*$", re.MULTILINE)
738_PYTEST_SUMMARY = re.compile(
739 r"^=*\s*((?:\d+ [a-z]+(?:, )?)+) in [\d.]+s\b.*$|^=*\s*no tests ran in [\d.]+s\b.*$",
740 re.MULTILINE,
741)
742_PYTEST_COUNT = re.compile(r"(\d+) ([a-z]+)")
743#: A pytest short-summary line. The node id never starts with ``(``, which is what keeps
744#: unittest's own ``FAILED (failures=1)`` from being read as a failed pytest test.
745#:
746#: The node id may contain spaces — a parametrized id (``test_v[hello world]``) or a file
747#: name — so it is read as: a bracketed id up to its ``]``, else one word, else the
748#: shortest text before `` - ``; the reason is what follows `` - `` and may be absent.
749_PYTEST_LINE = re.compile(r"^(FAILED|ERROR) (?!\()(\S*?\[.*?\]|\S+|.+?)(?: - (.*))?$", re.MULTILINE)
750#: A unittest failure section's header: ``FAIL: test_x (pkg.T.test_x)`` is an assertion,
751#: ``ERROR: …`` anything else. What the differential reading compares.
752_UNITTEST_HEADER = re.compile(r"^(FAIL|ERROR): (.+?)\s*$", re.MULTILINE)
753#: A pytest short-summary reason that is an assertion: a rewritten ``assert``, an
754#: ``AssertionError`` (unittest-style assertions under pytest), or ``pytest.fail``.
755#: ``Failed: Timeout`` is excluded: pytest-timeout reports a timed-out test that way, and
756#: a hang is not an assertion.
757_ASSERTION_REASON = re.compile(r"^(?:assert\b|AssertionError\b|Failed:(?! Timeout\b))")
760@dataclass(frozen=True)
761class Tally:
762 """What a test run's output says happened."""
764 #: The output carries a unittest or pytest summary at all.
765 recognized: bool = False
766 #: Tests that ran.
767 ran: int = 0
768 #: Tests that failed as an assertion.
769 assertions: int = 0
770 #: Tests that failed any other way: an exception, an import or collection error, a
771 #: strict unexpected success.
772 errors: int = 0
773 #: pytest counted a failure its short summary does not describe (``-rN``, truncation).
774 unclassified: int = 0
775 #: The ids of the tests that failed as an assertion, where the runner names them.
776 assertion_ids: frozenset[str] = frozenset()
777 #: The ids of every failing test — assertion or not — where the runner names them.
778 failure_ids: frozenset[str] = frozenset()
780 @property
781 def failed(self) -> bool:
782 """Does the output report any failing or erroring test at all?"""
783 return bool(self.assertions or self.errors or self.unclassified or self.failure_ids)
786def read_output(output: str) -> Tally:
787 """Read unittest's and pytest's summaries out of a test command's combined output.
789 **unittest** separates the two cleanly: ``failures`` are the test's
790 ``failureException`` (an ``AssertionError``), ``errors`` are anything else, a test
791 module that failed to import included.
793 **pytest** does not: its ``failed`` count includes a ``NameError`` raised in the test
794 body. The short test summary (``-rfE``, pytest's default since 6.0) names each
795 failure's exception, and only an assertion reason counts; a failure the summary does
796 not describe is *unclassified*, never an assertion.
798 Every summary in the output is summed, so a command running two suites is read whole.
799 """
800 ran = assertions = errors = unclassified = 0
801 recognized = False
802 for match in _UNITTEST_RAN.finditer(output):
803 recognized = True
804 ran += int(match.group(1))
805 for match in _UNITTEST_FAILED.finditer(output):
806 for part in match.group(1).split(","):
807 key, _, value = part.strip().partition("=")
808 count = int(value) if value.isdigit() else 0
809 if key == "failures":
810 assertions += count
811 elif key in ("errors", "unexpected successes"):
812 errors += count
813 pytest_failed = 0
814 for match in _PYTEST_SUMMARY.finditer(output):
815 recognized = True
816 for count, word in _PYTEST_COUNT.findall(match.group(1) or ""):
817 if word in ("passed", "failed", "xfailed", "xpassed"):
818 ran += int(count)
819 if word == "failed":
820 pytest_failed += int(count)
821 elif word in ("error", "errors"):
822 errors += int(count)
823 described = 0
824 asserted: set[str] = set()
825 failing: set[str] = set()
826 for kind, test_id in _UNITTEST_HEADER.findall(output):
827 failing.add(test_id)
828 if kind == "FAIL":
829 asserted.add(test_id)
830 for match in _PYTEST_LINE.finditer(output):
831 failing.add(match.group(2))
832 if match.group(1) != "FAILED":
833 continue # ERROR lines are already in the summary's error count
834 described += 1
835 if _ASSERTION_REASON.match(match.group(3) or ""):
836 assertions += 1
837 asserted.add(match.group(2))
838 else:
839 errors += 1
840 unclassified = max(0, pytest_failed - described)
841 return Tally(
842 recognized,
843 ran,
844 assertions,
845 errors,
846 unclassified,
847 frozenset(asserted),
848 frozenset(failing),
849 )
852def classify(
853 *, exit_ok: bool, timed_out: bool, output: str, baseline_failed: frozenset[str] = frozenset()
854) -> tuple[str, str]:
855 """One reverted run -> ``(result, why)``. Only :data:`CAUGHT` is a pass.
857 The exit code decides pass/fail; the output decides *how* it failed. A test that
858 errored is not a test that asserted — reverting half a fix can raise ``NameError``
859 in every test that imports it, and that proves the import, not the behaviour.
861 **Differential.** ``baseline_failed`` names the tests that failed without the revert.
862 Where the runner names the assertion failures, the change is caught only by one
863 *not* among them, so a failure that was already there cannot certify it.
864 (:func:`baseline_problem` already refuses a baseline with any failure; this holds the
865 rule on its own too.)
866 """
867 if timed_out:
868 return TIMED_OUT, "the test command timed out with the change reverted"
869 if exit_ok:
870 return UNNOTICED, "the test command passed with the change reverted"
871 tally = read_output(output)
872 if tally.assertion_ids and tally.assertion_ids <= baseline_failed:
873 return UNNOTICED, (
874 "the only tests that failed as an assertion also fail without the revert"
875 )
876 if tally.assertions:
877 return CAUGHT, f"{tally.assertions} test(s) failed as an assertion"
878 if not tally.recognized:
879 return UNREADABLE, (
880 "the test command failed, and its output carries no unittest or pytest summary "
881 "to say whether a test failed"
882 )
883 if tally.errors:
884 return ERRORED, f"{tally.errors} test(s) errored and none failed as an assertion"
885 if tally.unclassified:
886 return UNREADABLE, (
887 f"pytest reported {tally.unclassified} failure(s) its short test summary does not "
888 "describe, so an assertion cannot be told from an error (keep pytest's -rf)"
889 )
890 return UNREADABLE, "the test command failed, but no test did"
893def baseline_problem(*, exit_ok: bool, timed_out: bool, output: str) -> str | None:
894 """Why the unreverted run cannot anchor the check, or ``None`` when it can.
896 A failure after a revert means something only if the same command passes — and is
897 readable — on HEAD itself, in the same kind of clean checkout.
898 """
899 if timed_out:
900 return "the test command timed out on a clean checkout of HEAD"
901 if not exit_ok:
902 return (
903 "the test command fails on a clean checkout of HEAD, so a failure with a change "
904 "reverted would prove nothing"
905 )
906 tally = read_output(output)
907 if not tally.recognized:
908 return (
909 "the test command's output carries no unittest or pytest summary; revert-check "
910 "reads those two to tell an assertion from an error"
911 )
912 if tally.ran == 0:
913 return "the test command ran no tests on a clean checkout of HEAD"
914 if tally.failed:
915 # `suite1; suite2` exits with suite2's status: a suite already failing an
916 # assertion would otherwise lend that assertion to every revert.
917 return (
918 "the test command exits 0 on a clean checkout of HEAD but its output reports "
919 "failing tests, so a failure with a change reverted would prove nothing"
920 )
921 return None
924# --- the bounded loop ----------------------------------------------------------------
927@dataclass(frozen=True)
928class RunResult:
929 """One execution of the test command, as the I/O layer observed it."""
931 exit_ok: bool = False
932 timed_out: bool = False
933 output: str = ""
936@dataclass(frozen=True)
937class Reverted:
938 """One change undone in the scratch tree, as the I/O layer observed it."""
940 #: The tree was reset and the reverse patch applied.
941 applied: bool
944@dataclass(frozen=True)
945class ChangeResult:
946 change: Change
947 result: str
948 why: str
949 #: The change only adds code or import lines (:attr:`Change.pure_addition`, or
950 #: :func:`imports_only`) **and** every error its run reports is a missing name
951 #: (:func:`missing_names_only`) — the one error-only result :func:`judge` reads as a
952 #: ``nit``.
953 adds_only: bool = False
956@dataclass(frozen=True)
957class Report:
958 """What :func:`execute` observed."""
960 #: Set when the baseline run cannot anchor the check; nothing was reverted then.
961 baseline: str | None
962 results: tuple[ChangeResult, ...] = ()
963 #: Changes left unchecked by ``max_changes`` or ``budget_s``, with the reason.
964 not_checked: tuple[tuple[Change, str], ...] = ()
967#: ``revert(change)``: reset the scratch tree to ``HEAD`` and undo ``change`` in it.
968Reverter = Callable[[Change], Reverted]
969#: ``test(timeout_s)``: run the test command in the scratch tree as it stands.
970Tester = Callable[[int], RunResult]
973def execute(
974 changes: Sequence[Change],
975 settings: Settings,
976 *,
977 revert: Reverter,
978 test: Tester,
979 clock: Callable[[], float],
980) -> Report:
981 """Run the baseline, then revert each change alone, within the cost bounds.
983 ``max_changes`` caps how many changes are reverted; ``budget_s`` caps the wall clock
984 of the baseline and every revert together, and each run's limit is the smaller of
985 ``knobs.gate_timeout_s`` and what is left of it. A change either bound leaves out is
986 reported **not checked** — never as caught. Every change that is reverted is tested:
987 nothing is skipped for looking harmless.
988 """
989 start = clock()
991 def left() -> float:
992 return settings.budget_s - (clock() - start)
994 def limit(remaining: float) -> int:
995 return max(1, min(settings.run_timeout_s, int(remaining)))
997 base = test(limit(left()))
998 problem = baseline_problem(exit_ok=base.exit_ok, timed_out=base.timed_out, output=base.output)
999 if problem is not None:
1000 return Report(problem)
1001 baseline_failed = read_output(base.output).failure_ids
1002 results: list[ChangeResult] = []
1003 skipped: list[tuple[Change, str]] = []
1004 for index, change in enumerate(changes):
1005 if index >= settings.max_changes:
1006 skipped.append(
1007 (change, f"over knobs.revert_check.max_changes ({settings.max_changes})")
1008 )
1009 continue
1010 remaining = left()
1011 if remaining < 1:
1012 skipped.append(
1013 (change, f"the knobs.revert_check.budget_s budget ({settings.budget_s}s) ran out")
1014 )
1015 continue
1016 undone = revert(change)
1017 if not undone.applied:
1018 results.append(
1019 ChangeResult(change, NOT_APPLIED, "git apply -R could not undo it on HEAD")
1020 )
1021 continue
1022 # Re-read the clock: resetting, cleaning, applying and reading took time too, and a
1023 # run must never start with a limit the budget no longer has.
1024 remaining = left()
1025 if remaining < 1:
1026 skipped.append(
1027 (change, f"the knobs.revert_check.budget_s budget ({settings.budget_s}s) ran out")
1028 )
1029 continue
1030 outcome = test(limit(remaining))
1031 result, why = classify(
1032 exit_ok=outcome.exit_ok,
1033 timed_out=outcome.timed_out,
1034 output=outcome.output,
1035 baseline_failed=baseline_failed,
1036 )
1037 adds_only = (change.pure_addition or imports_only(change)) and missing_names_only(
1038 outcome.output
1039 )
1040 results.append(ChangeResult(change, result, why, adds_only))
1041 return Report(None, tuple(results), tuple(skipped))
1044# --- the verdict ---------------------------------------------------------------------
1047@dataclass(frozen=True)
1048class Verdict:
1049 """The gate's outcome, in :class:`keel.gates.GateOutcome` terms."""
1051 ok: bool
1052 findings: tuple[Finding, ...]
1053 #: The gate cannot judge — only the project's config or environment can fix it.
1054 unconfigured: bool = False
1055 #: Nothing to check: the diff has no production change. Reported ``SKIPPED``.
1056 skipped: bool = False
1059def cannot_judge(why: str) -> Verdict:
1060 """A verdict for a check that could not run: failed, never a pass (#1364)."""
1061 return Verdict(False, (Finding(_BLOCK, f"cannot judge: {why}", GATE_ID),), unconfigured=True)
1064#: Why the gate cannot judge a plan with no guard or test gate beside it (#1289 review).
1065PLANNED_ALONE = (
1066 "revert-check is not a test gate: it reverts each change against a suite the guard and "
1067 "test gates proved green, and none is planned — list build (with knobs.build_gate_cmd) "
1068 "beside it in gates:"
1069)
1072def precheck(settings: Settings, *, tests: Sequence[str], gates_green: bool) -> Verdict | None:
1073 """The verdict when the check must not start, else ``None``.
1075 Order matters: a missing command or test layout is a config problem no run can fix,
1076 so it is reported before the red-gates case an implementer can.
1077 """
1078 if settings.cmd is None:
1079 return cannot_judge(
1080 "no test command: set knobs.revert_check.cmd or knobs.build_gate_cmd "
1081 "in .keel/project.yaml"
1082 )
1083 if not tests:
1084 return cannot_judge(
1085 "revert-check cannot tell tests from production: declare where the tests live "
1086 "in policy_pack.test_groups.<group>.test_paths"
1087 )
1088 if not gates_green:
1089 return Verdict(
1090 False,
1091 (
1092 Finding(
1093 _BLOCK,
1094 "not run: the guard and test gates are red, and a revert check against a "
1095 "red suite cannot tell which failures the revert caused",
1096 GATE_ID,
1097 ),
1098 ),
1099 )
1100 return None
1103def judge(plan: Plan, report: Report | None) -> Verdict:
1104 """The gate's verdict: every production change must make a test fail as an assertion.
1106 * **unnoticed**, **timed out**, **unreadable**, **not applied** and **not checked**
1107 each block, naming the change: a change the gate did not see fail is not a pass.
1108 * **errored** — and **unreadable**, where the suite crashed before its summary — block,
1109 except for a change that only *adds* lines or only changes import lines **and**
1110 whose run reported nothing but missing names (``NameError``, ``ImportError``, a
1111 module or class attribute). Reverting such an addition removes a name, and a test
1112 that calls it can then only error: no assertion can fail against code that is not
1113 there. The error does prove a test depends on it, so it is a ``nit``. Any other
1114 error — a ``KeyError``, a ``TypeError``, an instance attribute — is behaviour a test
1115 should assert on, and blocks, as does any error after reverting a modification.
1116 * A production file with no textual hunk (binary, or an empty file added or deleted)
1117 blocks as **not checked**: nothing could be reverted, and the gate never certifies
1118 what it did not check.
1119 * A diff with no production change is ``SKIPPED`` — judged, with nothing to check.
1121 ``report`` is ``None`` only when there was nothing to execute.
1122 """
1123 findings: list[Finding] = []
1124 for path in plan.unrevertable:
1125 findings.append(
1126 Finding(
1127 _BLOCK,
1128 f"{path}: not checked — no textual hunk to revert (a binary file, or an empty file "
1129 "added or deleted)",
1130 GATE_ID,
1131 )
1132 )
1133 if report is None:
1134 if findings:
1135 return Verdict(False, tuple(findings))
1136 touched = len(plan.touched)
1137 return Verdict(
1138 True,
1139 (
1140 Finding(
1141 "nit",
1142 f"nothing to revert: none of the {touched} changed file(s) is production "
1143 "code (knobs.revert_check.paths, or a source suffix outside the test paths)",
1144 GATE_ID,
1145 ),
1146 ),
1147 skipped=True,
1148 )
1149 if report.baseline is not None:
1150 return cannot_judge(report.baseline)
1151 caught = 0
1152 for item in report.results:
1153 label = item.change.label
1154 if item.result == CAUGHT:
1155 caught += 1
1156 elif item.result in (ERRORED, UNREADABLE) and item.adds_only:
1157 findings.append(
1158 Finding(
1159 "nit",
1160 f"{label}: noticed only as a missing name — {item.why}; the change only "
1161 "adds code or import lines, so no assertion can fail without it",
1162 GATE_ID,
1163 )
1164 )
1165 elif item.result == UNNOTICED:
1166 hint = (
1167 "; it looks comment-only, and keel tests every change, because a comment can "
1168 "still change behaviour (encoding cookies, compiler directives, line "
1169 "numbers) — add a test that notices it, or scope such files out with "
1170 "knobs.revert_check.paths if you do not want them checked"
1171 if looks_comment_only(item.change)
1172 else ""
1173 )
1174 findings.append(
1175 Finding(_BLOCK, f"{label}: no test notices this change — {item.why}{hint}", GATE_ID)
1176 )
1177 else:
1178 findings.append(
1179 Finding(_BLOCK, f"{label}: no test failed as an assertion — {item.why}", GATE_ID)
1180 )
1181 for change, why in report.not_checked:
1182 findings.append(Finding(_BLOCK, f"{change.label}: not checked — {why}", GATE_ID))
1183 total = len(report.results) + len(report.not_checked)
1184 summary = (
1185 f"{caught} of {total} production change(s) made a test fail as an assertion when "
1186 "reverted alone"
1187 )
1188 findings.append(Finding("nit", summary, GATE_ID))
1189 blocked = any(f.severity == _BLOCK for f in findings)
1190 return Verdict(not blocked, tuple(findings))