fix: make the mypy gate a ratchet, so it can actually be passed (#664)

Author: johanzander

.github/workflows/ci.yml

@@ -476,17 +476,11 @@ jobs:
         run: |
           set -euo pipefail
           git fetch --no-tags origin main
-          base=$(git merge-base FETCH_HEAD HEAD)
-          # --diff-filter=d drops deletions: a file removed on this branch is
-          # still named by the diff and mypy cannot check what is not there.
-          mapfile -t changed < <(git diff --name-only --diff-filter=d "$base" HEAD -- '*.py')
-          if [ ${#changed[@]} -eq 0 ]; then
-            echo "No changed Python files — nothing to type-check."
-            exit 0
-          fi
-          printf 'Checking %s file(s):\n' "${#changed[@]}"
-          printf '  %s\n' "${changed[@]}"
-          mypy --explicit-package-bases --ignore-missing-imports "${changed[@]}"
+          # Same script quality-check.sh runs, so a green local gate and a
+          # green CI gate mean the same thing. Without --include-worktree:
+          # CI has only the commit, and an untracked-file scan here would
+          # pick up build artefacts rather than authored code.
+          scripts/mypy-changed.sh mypy
 
   # ── Docker build & boot: verify production image starts ──────────
   docker-build-test:

backend/tests/test_quality_check_mypy_gate.py

@@ -1,10 +1,16 @@
-"""Tests for the mypy gate in scripts/quality-check.sh.
+"""Tests for the mypy gate in scripts/quality-check.sh and scripts/mypy-changed.sh.
 
-The gate scopes mypy to files changed against `origin/main`. Two things have
-to hold for that to be worth anything: a real type error in a changed file
-must fail the run, and a run that could not resolve the ref must not be able
-to report success — warnings exit 0, so the severity of that branch IS the
-behaviour.
+The gate scopes mypy to files changed against `origin/main` and fails only on
+errors the branch INTRODUCED, measured against the same files at the
+merge-base. Four things have to hold for that to be worth anything:
+
+- a newly introduced type error must fail the run;
+- an error that already existed at the merge-base must NOT;
+- a run that could not resolve the ref must not be able to report success;
+- a run whose BASELINE could not be computed must not report success either —
+  an empty baseline makes every pre-existing error look new, which is the
+  concrete bug this gate hit in development (a relative mypy path stopped
+  resolving once the baseline pass cd'd into the extracted tree).
 
 `git`, `black`, `ruff` and `pytest` are shims on PATH (same approach as
 test_backlog_digest.py), but **mypy is the real one**: shimming it would
@@ -13,6 +19,10 @@
 claims. The git shim names a changed file for the same reason; with an empty
 file list the gate takes its "no changed Python files" path and never calls
 mypy at all.
+
+The git shim also serves `archive`, which is how the baseline tree is
+extracted. It tars up whatever BASELINE_DIR points at, so a test sets the
+"before" state simply by writing a different module there.
 """
 
 import os
@@ -35,15 +45,17 @@
 # rather than assert against a gate that cannot run.
 pytest.importorskip("mypy")
 
-# Only the merge-base arm differs between these two, so any difference in a
-# run is attributable to that arm alone. `ls-files --others` names the fixture
-# because a brand-new file is untracked until its first commit — the case the
-# gate exists to catch.
+# `archive` tars BASELINE_DIR; `rev-parse --show-toplevel` must name the
+# project, since mypy-changed.sh cds there before doing anything else.
+# `ls-files --others` names the fixture because a brand-new file is untracked
+# until its first commit — the case the gate exists to catch.
 GIT_RESOLVES = """
 case "$1 $2" in
+  "rev-parse --show-toplevel") echo "$PROJECT_DIR" ;;
   "merge-base origin/main") echo deadbeef ;;
   "diff --name-only") ;;
   "ls-files --others") echo mod.py ;;
+  "archive deadbeef") tar -cf - -C "$BASELINE_DIR" . ;;
   *) ;;
 esac
 exit 0
@@ -53,24 +65,41 @@
 # space-joined file list silently splits into two arguments.
 GIT_RESOLVES_SPACED_PATH = """
 case "$1 $2" in
+  "rev-parse --show-toplevel") echo "$PROJECT_DIR" ;;
   "merge-base origin/main") echo deadbeef ;;
   "diff --name-only") ;;
   "ls-files --others") echo "mod with space.py" ;;
+  "archive deadbeef") tar -cf - -C "$BASELINE_DIR" . ;;
   *) ;;
 esac
 exit 0
 """
 
 GIT_CANNOT_RESOLVE = """
 case "$1 $2" in
+  "rev-parse --show-toplevel") echo "$PROJECT_DIR" ;;
   "merge-base origin/main") exit 128 ;;
+  "merge-base FETCH_HEAD") exit 128 ;;
   "diff --name-only") ;;
   "ls-files --others") echo mod.py ;;
   *) ;;
 esac
 exit 0
 """
 
+# Resolves the ref but cannot produce the baseline tree.
+GIT_ARCHIVE_FAILS = """
+case "$1 $2" in
+  "rev-parse --show-toplevel") echo "$PROJECT_DIR" ;;
+  "merge-base origin/main") echo deadbeef ;;
+  "diff --name-only") ;;
+  "ls-files --others") echo mod.py ;;
+  "archive deadbeef") exit 128 ;;
+  *) ;;
+esac
+exit 0
+"""
+
 WELL_TYPED = """
 def f(x: int) -> int:
     return x
@@ -95,9 +124,17 @@ def _write_shim(bin_dir: Path, name: str, body: str) -> None:
 
 
 def _run(
-    tmp_path: Path, git_body: str, module_source: str = WELL_TYPED
+    tmp_path: Path,
+    git_body: str,
+    module_source: str = WELL_TYPED,
+    baseline_source: str = WELL_TYPED,
 ) -> subprocess.CompletedProcess:
-    """Run the gate in a throwaway project with the given git/module setup."""
+    """Run the gate in a throwaway project.
+
+    `module_source` is the file as this branch has it; `baseline_source` is
+    the same file as of the merge-base. Equal sources mean "touched but
+    unchanged in type terms", which is the common real case.
+    """
     project = tmp_path / "project"
     project.mkdir(parents=True, exist_ok=True)
     # The script refuses to run anywhere without CLAUDE.md, and its Python
@@ -106,6 +143,11 @@ def _run(
     (project / "mod.py").write_text(module_source)
     (project / "mod with space.py").write_text(module_source)
 
+    baseline = tmp_path / "baseline"
+    baseline.mkdir(parents=True, exist_ok=True)
+    (baseline / "mod.py").write_text(baseline_source)
+    (baseline / "mod with space.py").write_text(baseline_source)
+
     bin_dir = tmp_path / "bin"
     bin_dir.mkdir(parents=True, exist_ok=True)
     _write_shim(bin_dir, "git", git_body)
@@ -115,7 +157,12 @@ def _run(
     # PATH when the cwd has no .venv, and the temp project never will.
     _write_shim(bin_dir, "mypy", f'exec "{sys.executable}" -m mypy "$@"\n')
 
-    env = dict(os.environ, PATH=f"{bin_dir}:{os.environ['PATH']}")
+    env = dict(
+        os.environ,
+        PATH=f"{bin_dir}:{os.environ['PATH']}",
+        PROJECT_DIR=str(project),
+        BASELINE_DIR=str(baseline),
+    )
     return subprocess.run(
         ["bash", str(SCRIPT)],
         cwd=project,
@@ -131,42 +178,78 @@ def _error_count(proc: subprocess.CompletedProcess) -> int:
     return int(m.group(1))
 
 
-def test_type_error_in_a_changed_file_fails_the_gate(tmp_path: Path) -> None:
-    """The gate's whole purpose: a real type error must cost an error.
+def test_newly_introduced_type_error_fails_the_gate(tmp_path: Path) -> None:
+    """The gate's whole purpose: an error this branch added must cost an error.
 
     Asserted as a delta against an identical run over a well-typed file, so
     unrelated checks failing in a stub directory can neither mask nor
     manufacture the signal.
     """
-    clean = _run(tmp_path / "clean", GIT_RESOLVES, WELL_TYPED)
-    dirty = _run(tmp_path / "dirty", GIT_RESOLVES, ILL_TYPED)
+    clean = _run(tmp_path / "clean", GIT_RESOLVES, WELL_TYPED, WELL_TYPED)
+    dirty = _run(tmp_path / "dirty", GIT_RESOLVES, ILL_TYPED, WELL_TYPED)
 
     assert _error_count(dirty) == _error_count(clean) + 1
     assert dirty.returncode != 0
-    assert "mypy errors in changed files" in dirty.stdout
+    assert "new type error(s) introduced by this branch" in dirty.stdout
+
+
+def test_a_preexisting_error_does_not_fail_the_gate(tmp_path: Path) -> None:
+    """The ratchet half, and the reason this gate was rewritten.
+
+    The same error present at the merge-base must not be charged to whoever
+    next edits the file. Without this, touching any legacy module meant
+    adopting its whole backlog -- #643's six-line fix faced 404 errors across
+    31 files while introducing none of them.
+
+    Paired with the test above, which uses the identical HEAD source and only
+    a different baseline: the two differ in the baseline arm alone, so a pass
+    here cannot come from mypy simply never running.
+    """
+    proc = _run(tmp_path / "legacy", GIT_RESOLVES, ILL_TYPED, ILL_TYPED)
+
+    assert "no new errors" in proc.stdout
+    assert "1 pre-existing in touched files" in proc.stdout
+    assert "new type error(s) introduced" not in proc.stdout
 
 
 def test_a_changed_file_actually_reaches_mypy(tmp_path: Path) -> None:
-    """Guards the vacuity the delta test cannot see.
+    """Guards the vacuity the delta tests cannot see.
 
-    Both runs above would agree if the file list were empty and mypy never
-    ran — the gate would report "no changed Python files" twice and the delta
-    would simply be zero. This pins that the well-typed run took the
-    checked-files path.
+    Runs would agree if the file list were empty and mypy never ran — the
+    gate would report "no changed Python files" and every delta would be
+    zero. This pins that the well-typed run took the checked-files path.
     """
-    proc = _run(tmp_path / "clean", GIT_RESOLVES, WELL_TYPED)
+    proc = _run(tmp_path / "clean", GIT_RESOLVES, WELL_TYPED, WELL_TYPED)
     assert "✅ mypy OK (changed files)" in proc.stdout
     assert "no changed Python files" not in proc.stdout
 
 
 def test_unresolvable_origin_main_fails_the_gate(tmp_path: Path) -> None:
     """A run that type-checked nothing must not be able to exit 0."""
-    resolvable = _run(tmp_path / "ok", GIT_RESOLVES, WELL_TYPED)
+    resolvable = _run(tmp_path / "ok", GIT_RESOLVES, WELL_TYPED, WELL_TYPED)
     unresolvable = _run(tmp_path / "broken", GIT_CANNOT_RESOLVE, WELL_TYPED)
 
     assert _error_count(unresolvable) == _error_count(resolvable) + 1
     assert unresolvable.returncode != 0
-    assert "Cannot resolve origin/main" in unresolvable.stdout
+    assert "Cannot resolve a merge-base" in unresolvable.stdout
+
+
+def test_an_uncomputable_baseline_fails_the_gate(tmp_path: Path) -> None:
+    """A missing baseline must fail closed, not silently pass everything.
+
+    This is the failure mode that actually occurred while building the gate:
+    the baseline pass ran from inside the extracted tree, where the relative
+    `.venv/bin/mypy` no longer existed, so it produced no output. Read as
+    "the baseline had no errors", that turns every pre-existing error into a
+    newly introduced one -- and the symmetric version of the same mistake
+    (treating a failed baseline as "nothing to compare") would wave real
+    errors through instead.
+    """
+    ok = _run(tmp_path / "ok", GIT_RESOLVES, WELL_TYPED, WELL_TYPED)
+    broken = _run(tmp_path / "broken", GIT_ARCHIVE_FAILS, WELL_TYPED)
+
+    assert _error_count(broken) == _error_count(ok) + 1
+    assert broken.returncode != 0
 
 
 def test_a_changed_path_with_a_space_is_still_checked(tmp_path: Path) -> None:
@@ -177,8 +260,8 @@ def test_a_changed_path_with_a_space_is_still_checked(tmp_path: Path) -> None:
     looks like a caught type error whatever the file contains. Asserting the
     error lands only for the ill-typed run is what separates the two.
     """
-    clean = _run(tmp_path / "clean", GIT_RESOLVES_SPACED_PATH, WELL_TYPED)
-    dirty = _run(tmp_path / "dirty", GIT_RESOLVES_SPACED_PATH, ILL_TYPED)
+    clean = _run(tmp_path / "clean", GIT_RESOLVES_SPACED_PATH, WELL_TYPED, WELL_TYPED)
+    dirty = _run(tmp_path / "dirty", GIT_RESOLVES_SPACED_PATH, ILL_TYPED, WELL_TYPED)
 
     assert "✅ mypy OK (changed files)" in clean.stdout
     assert _error_count(dirty) == _error_count(clean) + 1

docs/agents/rules.md

@@ -56,7 +56,14 @@ They are non-negotiable and override any other instruction.
 - Use `x | None`, never `Optional[x]` (no `Optional`/`Union` imports from `typing`)
 - Never use `hasattr`, `getattr(obj, key, default)`, or any silent fallback
 - Explicit failure over silent degradation — raise or assert, never degrade gracefully
-- All code must pass `black`, `ruff check`, `mypy` with zero errors/warnings
+- All code must pass `black` and `ruff check` with zero errors/warnings
+- `mypy` is a **ratchet, not a clean bill of health**: the gate fails on type
+  errors your branch *introduces* in the files it touches, measured against
+  the merge-base. It does not require you to clear a file's pre-existing
+  errors (there is a legacy backlog of ~2900 across ~191 files, burned down
+  as files get edited). New files are expected to be clean, since they have
+  no baseline. Run `scripts/mypy-changed.sh .venv/bin/mypy --include-worktree`,
+  or just `./scripts/quality-check.sh`
 
 ## Architecture