Coverage for src/keel/tdd.py: 100%
175 statements
« prev ^ index » next coverage.py v7.16.2, created at 2026-10-02 20:26 +0000
1"""``implement_mode: tdd`` — the test-first s4 profile and its commit-order gate (#1020).
3Some implementers skip parts of an issue, and nothing catches it until a reviewer reads
4the diff. A test-first contract catches it at s8 instead: the acceptance criteria are
5committed as *failing tests* before a line of implementation exists, so the gates
6themselves say whether the criteria were met.
8``tdd`` is an **s4 profile**, exactly as ``compound`` is — the backbone step ids do not
9change. In ``tdd`` mode s4 runs in two phases against the same provider, one
10``keel delegate run`` call and one commit each:
12``tests`` the failing tests derived from the issue's acceptance criteria, as a
13 test-only diff (the gates are expected red here);
14``implementation`` the change that turns them green.
16This module is the pure half — the mode resolution, the commit parser, and the
17``tdd-order`` verifier that decides whether the branch really was written that way:
19* :func:`resolve_mode` — ``knobs.implement_mode`` + the per-run ``--tdd`` flag -> a
20 :class:`Mode`, rendered by ``keel plan``/``keel ship --json`` so every host runs the
21 same profile;
22* :func:`test_globs` — where a project says its tests live, read off
23 ``policy_pack.test_groups``;
24* :func:`parse_commits` — ``git log`` output -> :class:`Commit` records;
25* :func:`check_order` — the gate: the first non-merge commit on the branch adds or
26 modifies test paths and touches nothing else, an implementation commit follows it, no
27 later commit deletes a test, and the gate run is green.
29**What this gate does not do.** It reads *commit order and paths*, and nothing else. It
30does not run phase A's tests, does not verify they were red, and cannot tell whether the
31committed tests assert anything at all. Three residuals follow from that, each a
32reviewer's catch rather than a gate's:
34* a first commit adding an empty file under ``tests/`` satisfies the "adds a test" rule,
35 and so does one that merely *renames* an existing test within the test paths;
36* a test deleted inside a **merge from a side branch** is not judged, because merges are
37 skipped so that a deletion made on the base is not blamed on this implementer (see the
38 comment at the merge skip in :func:`check_order`);
39* nothing here says the tests are good, only that the branch has the shape of a
40 test-first run.
42The red-then-green half is the implementer's brief and its PR body, not this gate.
44Pure and deterministic: no wall-clock, no randomness, no I/O, and — at module scope — no
45keel imports at all. The git reads the gate needs — the base ref's exact lookup, then
46:func:`keel.git.commit_log` — happen in the CLI, through the same thin seam every other
47command reads git through; core is handed the log's *output*.
48"""
50from __future__ import annotations
52import fnmatch
53from collections.abc import Iterable, Mapping, Sequence
54from dataclasses import dataclass, field
55from typing import Any
57#: ``knobs.implement_mode`` values. ``default`` is the single-pass s4 keel has always run.
58DEFAULT_MODE = "default"
59TDD_MODE = "tdd"
60MODES = (DEFAULT_MODE, TDD_MODE)
62#: The gate id ``tdd`` mode adds at the s8 test phase.
63GATE_ID = "tdd-order"
65#: The two s4 phases, in the only order that is TDD.
66PHASE_TESTS = "tests"
67PHASE_IMPLEMENTATION = "implementation"
68PHASES = (PHASE_TESTS, PHASE_IMPLEMENTATION)
70#: Record separator git writes before each commit's format line, and the field separator
71#: inside it. Both are control characters that cannot occur in a path or a subject line,
72#: so a commit message containing a newline — or a filename containing one — cannot be
73#: read as another commit.
74RECORD_SEP = "\x1e"
75FIELD_SEP = "\x1f"
77#: The ``--format`` :func:`parse_commits` reads. Kept here, next to the parser, so the
78#: argv in :func:`keel.git.commit_log` and the parser cannot drift apart.
79LOG_FORMAT = f"{RECORD_SEP}%H{FIELD_SEP}%P{FIELD_SEP}%s"
82@dataclass(frozen=True)
83class Mode:
84 """The resolved s4 implement profile and where it came from."""
86 name: str
87 source: str
89 @property
90 def is_tdd(self) -> bool:
91 return self.name == TDD_MODE
93 def as_dict(self) -> dict[str, Any]:
94 """JSON-stable record for ``keel plan`` / ``keel ship --json``."""
95 return {
96 "mode": self.name,
97 "tdd": self.is_tdd,
98 "source": self.source,
99 "phases": list(PHASES) if self.is_tdd else [],
100 "gate": GATE_ID if self.is_tdd else None,
101 }
104def resolve_mode(configured: Any = None, *, flag: bool = False) -> Mode:
105 """The s4 profile for this run: ``--tdd`` > ``knobs.implement_mode`` > ``default``.
107 A per-run flag can only *select* the stricter profile — there is no ``--no-tdd``,
108 because a project that configured a test-first contract has said the contract is the
109 policy, and a flag that switched it off from the command line would make the policy
110 advisory. Unknown values are impossible here (the schema owns the vocabulary) and
111 read as ``default`` rather than raising: this resolver runs on every ship.
112 """
113 if flag:
114 return Mode(TDD_MODE, "flag:--tdd")
115 value = configured.strip() if isinstance(configured, str) else ""
116 if value == TDD_MODE:
117 return Mode(TDD_MODE, "knobs.implement_mode")
118 return Mode(DEFAULT_MODE, "default")
121def test_globs(policy_pack: Mapping[str, Any] | None) -> tuple[str, ...]:
122 """Where this project's tests live, from ``policy_pack.test_groups``.
124 A group's ``paths`` are *selectors* — the paths that make the group relevant — and on
125 a real project they routinely include the implementation surface (keel's own ``unit``
126 group selects ``src/**`` as well as ``tests/**``). Read as test paths they would make
127 this gate vacuous: a first commit touching only ``src/`` would pass as "tests", and
128 the implementation commit that must follow it would then have nowhere to land.
130 So a group may declare ``test_paths`` — the paths its *tests* occupy — and **when any
131 group declares one, only the declared ones count**. Mixing the remaining groups'
132 selectors back in would re-import exactly the implementation surface ``test_paths``
133 exists to exclude. A project that declares none keeps the plain reading of
134 ``paths``, and :func:`check_order` fails closed when that leaves nothing at all.
135 """
136 pack = policy_pack if isinstance(policy_pack, Mapping) else {}
137 groups = pack.get("test_groups")
138 if not isinstance(groups, Mapping):
139 return ()
140 declared: list[str] = []
141 selectors: list[str] = []
142 for _name, group in sorted(groups.items(), key=lambda item: str(item[0])):
143 if not isinstance(group, Mapping):
144 continue
145 declared.extend(_globs(group.get("test_paths")))
146 selectors.extend(_globs(group.get("paths")))
147 return _unique(declared) if declared else _unique(selectors)
150def _globs(raw: Any) -> list[str]:
151 """Non-blank string entries of a path list (anything else contributes nothing)."""
152 if not isinstance(raw, Sequence) or isinstance(raw, (str, bytes, bytearray)):
153 return []
154 return [entry.strip() for entry in raw if isinstance(entry, str) and entry.strip()]
157def _unique(globs: Iterable[str]) -> tuple[str, ...]:
158 """De-duplicated, order-preserving — the gate's message lists these verbatim."""
159 return tuple(dict.fromkeys(globs))
162def is_test_path(path: str, globs: Sequence[str]) -> bool:
163 """Does ``path`` sit under one of the project's test globs?
165 ``fnmatch`` semantics, the same matcher :mod:`keel.classify` uses for
166 ``tier3_globs`` and ``docs_gate_paths``, so one project writes one kind of glob.
167 """
168 return any(fnmatch.fnmatch(path, glob) for glob in globs)
171#: ``--name-status`` letters this module reasons about. Only ``D`` is load-bearing: every
172#: other status leaves the path present in the tree after the commit.
173ADDED = "A"
174MODIFIED = "M"
175DELETED = "D"
176RENAMED = "R"
177COPIED = "C"
180@dataclass(frozen=True)
181class Change:
182 """One path a commit touched, and what it did to it.
184 ``--name-only`` cannot tell an addition from a deletion, which made a first commit
185 that only ran ``git rm`` over the test suite look exactly like one that wrote it. The
186 status is what separates "wrote the failing tests" from "removed the failing tests".
187 """
189 status: str
190 path: str
191 #: Where a rename or copy came *from*; ``None`` for every other status. Recorded
192 #: because a path that left the test tree is missing from ``path`` by construction:
193 #: after ``git mv tests/test_a.py src/legacy.py`` the only mention of the test is
194 #: here. :func:`check_order` needs it to tell a move-out from an ordinary rename.
195 source: str | None = None
197 @property
198 def deleted(self) -> bool:
199 return self.status == DELETED
201 @property
202 def present(self) -> bool:
203 """Does the path exist in the tree after this commit? (Everything but ``D``.)"""
204 return not self.deleted
207@dataclass(frozen=True)
208class Commit:
209 """One commit on the branch: what it is, and what it did to which paths."""
211 sha: str
212 subject: str = ""
213 changes: tuple[Change, ...] = ()
214 merge: bool = False
216 @property
217 def short(self) -> str:
218 """The 7-character sha operators read in a gate message."""
219 return self.sha[:7]
221 @property
222 def files(self) -> tuple[str, ...]:
223 """Every path the commit touched, in commit order, deletions included."""
224 return tuple(change.path for change in self.changes)
226 def as_dict(self) -> dict[str, Any]:
227 return {
228 "sha": self.sha,
229 "subject": self.subject,
230 "changes": [
231 {"status": c.status, "path": c.path, "source": c.source} for c in self.changes
232 ],
233 "files": list(self.files),
234 "merge": self.merge,
235 }
238def _change(line: str) -> Change | None:
239 """One ``--name-status`` line -> a :class:`Change` (``None`` when it is not one).
241 A rename or copy prints ``R100<TAB>old<TAB>new``. The destination is the path that
242 exists afterwards, so that is ``path``; the origin is kept as ``source``, because a
243 rename *out of* the test tree removes a test as surely as ``git rm`` does and the
244 destination alone cannot show it.
246 Deliberately glob-free: this turns git's output into records, and *which* paths are
247 tests is :func:`check_order`'s question against the project's policy.
248 """
249 fields = line.split("\t")
250 if len(fields) < 2 or not fields[0].strip():
251 return None
252 status = fields[0].strip()[0].upper()
253 renamed = status in (RENAMED, COPIED) and len(fields) > 2
254 path = (fields[2] if renamed else fields[1]).strip()
255 source = fields[1].strip() if renamed else None
256 if not path:
257 return None
258 return Change(status, path, source or None)
261def parse_commits(text: str | None) -> tuple[Commit, ...] | None:
262 """Parse :data:`LOG_FORMAT` + ``--name-status`` output, oldest commit first.
264 ``None`` in, ``None`` out — :func:`keel.git.commit_log` reports an unreadable
265 history as ``None``, and that must stay distinct from "the branch has no commits"
266 all the way into :func:`check_order`, which blocks on the first and can say so.
267 """
268 if text is None:
269 return None
270 commits: list[Commit] = []
271 for chunk in text.split(RECORD_SEP):
272 if not chunk.strip():
273 continue
274 header, _, body = chunk.partition("\n")
275 sha, _, rest = header.partition(FIELD_SEP)
276 parents, _, subject = rest.partition(FIELD_SEP)
277 sha = sha.strip()
278 if not sha:
279 continue
280 commits.append(
281 Commit(
282 sha=sha,
283 subject=subject.strip(),
284 changes=tuple(
285 change
286 for change in (_change(line) for line in body.splitlines() if line.strip())
287 if change is not None
288 ),
289 # A merge has more than one parent. Merges are skipped rather than
290 # judged: a merge from the base branch carries every path the base
291 # moved, which is not this implementer's commit order.
292 merge=len(parents.split()) > 1,
293 )
294 )
295 return tuple(commits)
298#: Machine-readable outcomes of :func:`check_order`. Callers branch on the code; the
299#: message is for the operator reading the gate line.
300OK = "ok"
301UNREADABLE_HISTORY = "unreadable-history"
302NO_TEST_PATHS = "no-test-paths"
303NO_COMMITS = "no-commits"
304EMPTY_FIRST_COMMIT = "empty-first-commit"
305IMPLEMENTATION_FIRST = "implementation-first"
306NO_TESTS_ADDED = "no-tests-added"
307TESTS_DELETED = "tests-deleted"
308NO_IMPLEMENTATION_COMMIT = "no-implementation-commit"
309GATES_RED = "gates-red"
312@dataclass(frozen=True)
313class OrderResult:
314 """The ``tdd-order`` verdict: did this branch put its tests first?"""
316 ok: bool
317 code: str
318 message: str
319 tests_commit: str | None = None
320 implementation_commit: str | None = None
321 #: The paths that decided a failure, in commit order: the non-test paths to move out
322 #: of the first commit, or the deleted tests to restore.
323 offending: tuple[str, ...] = ()
324 test_globs: tuple[str, ...] = field(default_factory=tuple)
326 def as_dict(self) -> dict[str, Any]:
327 return {
328 "ok": self.ok,
329 "code": self.code,
330 "message": self.message,
331 "tests_commit": self.tests_commit,
332 "implementation_commit": self.implementation_commit,
333 "offending": list(self.offending),
334 "test_globs": list(self.test_globs),
335 }
338def removed_test(change: Change, globs: Sequence[str]) -> str | None:
339 """The test path this change *removed*, or ``None`` — deletions and moves-out.
341 Two spellings of the same act, and the gate has to see both. ``git rm
342 tests/test_a.py`` is the obvious one. ``git mv tests/test_a.py src/legacy_test_a.py``
343 is the same act wearing a rename: the test stops being collected by the suite that
344 was red in phase A, and the destination path alone cannot show it because the only
345 mention of the test is the rename's *source*.
347 A rename **within** the test tree is an ordinary move and stays fine. A **copy**
348 (``C``) is not a removal at all — its source still exists after the commit — so it is
349 excluded even when the destination lands outside the test tree. That exclusion is
350 **defensive only**: :func:`keel.git.commit_log` never passes ``-C``/``--find-copies``,
351 so git does not emit a ``C`` status for this gate's input and the branch is unreachable
352 from the real argv. It stays because the vocabulary is git's, not keel's.
353 """
354 if change.deleted:
355 return change.path if is_test_path(change.path, globs) else None
356 if change.status != RENAMED or change.source is None:
357 return None
358 if is_test_path(change.source, globs) and not is_test_path(change.path, globs):
359 return change.source
360 return None
363def check_order(
364 commits: Sequence[Commit] | None,
365 *,
366 test_globs: Sequence[str] = (),
367 gates_green: bool | None = None,
368) -> OrderResult:
369 """Was this branch written test-first? A pure function of the commits and the policy.
371 The contract, in the order it is checked:
373 1. the history is readable at all (``None`` is git failing, never an empty branch);
374 2. the project says where its tests live (see :func:`test_globs`) — without that
375 there is nothing to check against, and a gate that cannot look must not pass;
376 3. the first non-merge commit touches at least one path, and **only** test paths;
377 4. that commit *adds or modifies* at least one test — a first commit that only
378 deletes tests is the opposite of writing them;
379 5. no later commit *removes* a test — deleted outright, or renamed out of the test
380 paths, which stops the suite collecting it just as surely. Making the failing
381 tests go away is the cheapest way to make phase B "pass", and it is the move this
382 gate exists to refuse;
383 6. a later commit touches a non-test path — the implementation the tests were
384 written for;
385 7. the gate run is green, when the caller measured one.
387 ``gates_green`` is tri-state. ``None`` means the caller made no gate observation, so
388 only the commit order is judged; ``False`` blocks, because a branch whose tests were
389 committed first and are still red has not finished phase B.
391 **The boundary.** This reads commit order and paths, and nothing else. It never runs
392 phase A's tests, so it cannot report that they were red, and it cannot tell whether
393 the committed tests assert anything — an empty file under ``tests/`` satisfies rule 4,
394 and so does a rename of an existing test within the test paths. Rule 5 has its own
395 residual: a test deleted inside a merge from a side branch is never judged, because
396 merges are skipped (the comment at the skip says why, and what that costs). Those
397 remain a reviewer's questions; the gate makes the *shape* of a test-first run
398 machine-checkable, not the quality of the tests.
399 """
400 globs = tuple(test_globs)
401 if commits is None:
402 return OrderResult(
403 False,
404 UNREADABLE_HISTORY,
405 "could not read the branch history, so the test-first commit order cannot be "
406 "verified; run this where the branch and its base are both present",
407 test_globs=globs,
408 )
409 if not globs:
410 return OrderResult(
411 False,
412 NO_TEST_PATHS,
413 "implement_mode: tdd needs to know which paths are tests, and this project "
414 "declares none; add policy_pack.test_groups.<group>.test_paths (or .paths) "
415 "naming the paths the tests live in",
416 test_globs=globs,
417 )
418 # Merges are skipped throughout, so the deletion scan below never judges what a merge
419 # commit brought in. That is a deliberate trade-off, not a proof of safety, and the
420 # earlier claim here — "the only paths a merge carries are the base's" — was simply
421 # wrong. It holds for a merge *from the base*, which is the common case and the reason
422 # to skip: a test legitimately deleted on the base arrives through every branch that
423 # integrates it, and judging merges would block this implementer for someone else's
424 # work, on a change they did not make.
425 #
426 # The residual, named because it is real: a merge from a *side* branch carries that
427 # branch's paths, not the base's. `git rm tests/test_a.py` on a side branch merged
428 # with `--no-ff` removes the test with no non-merge commit recording the deletion, so
429 # rule 5 does not see it. A reviewer reading the diff does. Closing it would mean
430 # telling a base merge from a side merge, which the commit list alone cannot do —
431 # both are a second parent — so the gate declares the gap instead of guessing.
432 work = [commit for commit in commits if not commit.merge]
433 if not work:
434 return OrderResult(
435 False,
436 NO_COMMITS,
437 "the branch carries no non-merge commit, so there is no test-first commit to "
438 "verify; commit the failing tests from the issue's acceptance criteria first",
439 test_globs=globs,
440 )
441 first = work[0]
442 if not first.changes:
443 return OrderResult(
444 False,
445 EMPTY_FIRST_COMMIT,
446 f"the first commit {first.short} touches no file, so it cannot be the "
447 "test-first commit; commit the failing tests first",
448 tests_commit=first.sha,
449 test_globs=globs,
450 )
451 offending = tuple(
452 change.path for change in first.changes if not is_test_path(change.path, globs)
453 )
454 if offending:
455 return OrderResult(
456 False,
457 IMPLEMENTATION_FIRST,
458 f"the first commit {first.short} touches implementation paths, so this "
459 f"branch was not written test-first: {', '.join(offending)} "
460 f"(test paths: {', '.join(globs)})",
461 tests_commit=first.sha,
462 offending=offending,
463 test_globs=globs,
464 )
465 if not any(change.present for change in first.changes):
466 # Every path in the first commit is a test path *and* every one of them is a
467 # deletion: `git rm` over the suite, which reads as "tests first" to anything
468 # that only looks at names. Writing the failing tests is the phase; removing
469 # them is its inverse.
470 removed = tuple(change.path for change in first.changes)
471 return OrderResult(
472 False,
473 NO_TESTS_ADDED,
474 f"the first commit {first.short} only deletes tests and adds none, so there "
475 f"are no failing tests for phase B to satisfy: {', '.join(removed)}",
476 tests_commit=first.sha,
477 offending=removed,
478 test_globs=globs,
479 )
480 deleted = tuple(
481 removed
482 for commit in work[1:]
483 for change in commit.changes
484 if (removed := removed_test(change, globs)) is not None
485 )
486 if deleted:
487 return OrderResult(
488 False,
489 TESTS_DELETED,
490 f"a commit after the tests commit {first.short} removes tests (deleted, or "
491 "renamed out of the test paths), which is the cheapest way to make phase B "
492 f"pass without implementing anything: {', '.join(deleted)}",
493 tests_commit=first.sha,
494 offending=deleted,
495 test_globs=globs,
496 )
497 implementation = next(
498 (
499 commit
500 for commit in work[1:]
501 if any(not is_test_path(change.path, globs) for change in commit.changes)
502 ),
503 None,
504 )
505 if implementation is None:
506 return OrderResult(
507 False,
508 NO_IMPLEMENTATION_COMMIT,
509 f"the tests commit {first.short} is the whole branch: no later commit touches "
510 "an implementation path, so phase B never ran",
511 tests_commit=first.sha,
512 test_globs=globs,
513 )
514 if gates_green is False:
515 return OrderResult(
516 False,
517 GATES_RED,
518 f"the tests commit {first.short} came first, but the gates are red — phase B "
519 "runs until the tests it was written against pass",
520 tests_commit=first.sha,
521 implementation_commit=implementation.sha,
522 test_globs=globs,
523 )
524 return OrderResult(
525 True,
526 OK,
527 f"tests committed first in {first.short}, implementation in {implementation.short}",
528 tests_commit=first.sha,
529 implementation_commit=implementation.sha,
530 test_globs=globs,
531 )
534def phase_implementers(
535 pairs: Iterable[tuple[str, str]] = (),
536 *,
537 default: str | None = None,
538) -> dict[str, str | None]:
539 """``--phase-implementer <phase>=<label>`` pairs -> one label per phase.
541 ``default`` is the run's ``--implementer``, used for any phase not named explicitly —
542 which is the ordinary case, because the profile requires both phases to run on the
543 same provider. The point of allowing them to differ is that a run where they *did*
544 differ records the fact instead of quietly presenting a single label for both.
545 Unknown phase names are dropped here; the CLI rejects them at parse time.
546 """
547 resolved: dict[str, str | None] = {phase: default for phase in PHASES}
548 for phase, label in pairs:
549 if phase in resolved:
550 resolved[phase] = label
551 return resolved
554def phase_records(
555 result: OrderResult | None,
556 *,
557 implementers: Mapping[str, str | None] | None = None,
558) -> list[dict[str, Any]] | None:
559 """The two s4 phases as ledger records, or ``None`` when the run had no TDD phases.
561 A phase whose commit could not be identified records ``commit: null`` rather than
562 being dropped: the ledger says a TDD run happened and which half of it is missing,
563 which is the question a closure comment is read for.
565 Each record also carries the ``implementer`` that ran that phase, so *"the same
566 provider wrote the tests and the implementation"* — the rule the profile rests on and
567 which nothing else in the record could show — is auditable after the fact rather than
568 assumed. It is emit-only, like the gate seat in ``knobs.team``: keel records what the
569 orchestrator reports, and two different labels are a finding for a reader, not
570 something core can independently prove.
571 """
572 if result is None:
573 return None
574 seats = implementers or {}
575 return [
576 {
577 "phase": PHASE_TESTS,
578 "commit": result.tests_commit,
579 "implementer": seats.get(PHASE_TESTS),
580 },
581 {
582 "phase": PHASE_IMPLEMENTATION,
583 "commit": result.implementation_commit,
584 "implementer": seats.get(PHASE_IMPLEMENTATION),
585 },
586 ]