fix: enforce mypy on changed files, and make the PR review bot always submit a verdict

.github/workflows/ci.yml

@@ -443,22 +443,51 @@ jobs:
     name: Code quality
     runs-on: ubuntu-latest
     steps:
+      # Full history: the mypy step below needs a real merge-base against
+      # main, which a shallow clone cannot produce.
       - uses: actions/checkout@v5
+        with:
+          fetch-depth: 0
 
       - uses: actions/setup-python@v5
         with:
           python-version: "3.13"
           cache: pip
 
       - name: Install tools
-        run: pip install black ruff
+        run: pip install black ruff mypy
 
       - name: Black formatting
         run: black --check . --exclude="/(build|\.venv|node_modules)/"
 
       - name: Ruff linting
         run: ruff check . --exclude="build,.venv,node_modules"
 
+      # Scoped exactly like scripts/quality-check.sh: code you touch must
+      # type-check, and the legacy backlog burns down as files get edited.
+      # Repo-wide is not an option (2914 errors across 191 files).
+      #
+      # This job aggregates into the Merge gate, which is the single required
+      # status check on main. Enforcing mypy only in quality-check.sh would
+      # leave it unenforced on exactly the path that gates merges — a
+      # violation could still reach main through any ordinary PR, which is
+      # the gap this whole change exists to close.
+      - name: mypy on changed files
+        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[@]}"
+
   # ── Docker build & boot: verify production image starts ──────────
   docker-build-test:
     name: Docker build & boot

.github/workflows/pr-review.yml

@@ -106,6 +106,16 @@ jobs:
                    - REQUEST_CHANGES — at least one rule violation or correctness bug
                    - COMMENT  — questions/observations only
 
+                 **Submitting this review is mandatory in every case,
+                 including when you find nothing wrong.** Finishing the run
+                 without calling `gh pr review` is a failed run, not a clean
+                 one. Silence is indistinguishable from a crash to everything
+                 downstream, and it has a concrete cost: GitHub keeps showing
+                 the LAST explicit verdict, so a prior REQUEST_CHANGES goes on
+                 blocking the merge no matter how many clean runs follow, and
+                 the review can never be cleared by re-running. If the diff is
+                 clean, say so in one line and APPROVE.
+
               5. The summary must include:
                  - Whether the fix matches the linked issue's root-cause
                  - Whether the test added would catch a regression

backend/tests/test_quality_check_mypy_gate.py

@@ -0,0 +1,184 @@
+"""Tests for the mypy gate in scripts/quality-check.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.
+
+`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
+leave the gate's actual invocation — its flags, its quoting, the file list it
+builds — unexercised, which is how a test passes while proving less than it
+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.
+"""
+
+import os
+import re
+import stat
+import subprocess
+import sys
+from pathlib import Path
+
+import pytest
+
+REPO_ROOT = Path(__file__).resolve().parents[2]
+SCRIPT = REPO_ROOT / "scripts" / "quality-check.sh"
+
+# Reach mypy through the interpreter running the tests, not through a fixed
+# `.venv/bin/mypy` path. CI installs dependencies without that venv layout, so
+# the fixed path exec'd a binary that did not exist -- mypy then "failed" in
+# every run alike, the delta assertions collapsed to zero, and the suite went
+# red on CI while passing locally. If mypy is not importable at all, skip
+# 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.
+GIT_RESOLVES = """
+case "$1 $2" in
+  "merge-base origin/main") echo deadbeef ;;
+  "diff --name-only") ;;
+  "ls-files --others") echo mod.py ;;
+  *) ;;
+esac
+exit 0
+"""
+
+# Same as GIT_RESOLVES but names a path with a space in it — the case a
+# space-joined file list silently splits into two arguments.
+GIT_RESOLVES_SPACED_PATH = """
+case "$1 $2" in
+  "merge-base origin/main") echo deadbeef ;;
+  "diff --name-only") ;;
+  "ls-files --others") echo "mod with space.py" ;;
+  *) ;;
+esac
+exit 0
+"""
+
+GIT_CANNOT_RESOLVE = """
+case "$1 $2" in
+  "merge-base origin/main") exit 128 ;;
+  "diff --name-only") ;;
+  "ls-files --others") echo mod.py ;;
+  *) ;;
+esac
+exit 0
+"""
+
+WELL_TYPED = """
+def f(x: int) -> int:
+    return x
+
+
+f(1)
+"""
+
+ILL_TYPED = """
+def f(x: int) -> int:
+    return x
+
+
+f("not an int")
+"""
+
+
+def _write_shim(bin_dir: Path, name: str, body: str) -> None:
+    p = bin_dir / name
+    p.write_text("#!/bin/sh\n" + body)
+    p.chmod(p.stat().st_mode | stat.S_IEXEC)
+
+
+def _run(
+    tmp_path: Path, git_body: str, module_source: str = WELL_TYPED
+) -> subprocess.CompletedProcess:
+    """Run the gate in a throwaway project with the given git/module setup."""
+    project = tmp_path / "project"
+    project.mkdir(parents=True, exist_ok=True)
+    # The script refuses to run anywhere without CLAUDE.md, and its Python
+    # block is skipped unless a .py file exists.
+    (project / "CLAUDE.md").write_text("# stub\n")
+    (project / "mod.py").write_text(module_source)
+    (project / "mod with space.py").write_text(module_source)
+
+    bin_dir = tmp_path / "bin"
+    bin_dir.mkdir(parents=True, exist_ok=True)
+    _write_shim(bin_dir, "git", git_body)
+    for tool in ("black", "ruff", "pytest"):
+        _write_shim(bin_dir, tool, "exit 0\n")
+    # Real mypy, reached through a passthrough: the gate resolves tools from
+    # 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']}")
+    return subprocess.run(
+        ["bash", str(SCRIPT)],
+        cwd=project,
+        capture_output=True,
+        text=True,
+        env=env,
+    )
+
+
+def _error_count(proc: subprocess.CompletedProcess) -> int:
+    m = re.search(r"^Errors: (\d+)$", proc.stdout, re.MULTILINE)
+    assert m, f"no summary in output:\n{proc.stdout}"
+    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.
+
+    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)
+
+    assert _error_count(dirty) == _error_count(clean) + 1
+    assert dirty.returncode != 0
+    assert "mypy errors in changed files" in dirty.stdout
+
+
+def test_a_changed_file_actually_reaches_mypy(tmp_path: Path) -> None:
+    """Guards the vacuity the delta test 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.
+    """
+    proc = _run(tmp_path / "clean", GIT_RESOLVES, 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)
+    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
+
+
+def test_a_changed_path_with_a_space_is_still_checked(tmp_path: Path) -> None:
+    """The file list is an array, so a spaced path stays one argument.
+
+    Space-joined, this path splits into `mod`, `with` and `space.py` — mypy
+    is handed three names that do not exist, which is a mypy failure and so
+    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)
+
+    assert "✅ mypy OK (changed files)" in clean.stdout
+    assert _error_count(dirty) == _error_count(clean) + 1

docs/agents/local-agent-environment.md

@@ -389,6 +389,17 @@ redundant:
   token from the macOS keychain, which is XPC, not network. The same block is
   why `git push` emitted `failed to store: 100001` — the credential helper
   could not cache the credential, though the push itself still landed.
+
+  **When a keychain read does fail, `gh` does not error — it silently falls
+  back to a token with fewer scopes**, so the symptom is a plain `Forbidden`
+  on an API call that should have been permitted, and the obvious conclusion
+  ("I lack permission for this operation") is wrong. Run `gh auth status`
+  first: `✓ Logged in ... (keyring)` plus the expected scope line means the
+  token is fine and the failure is something else. A `Failed to log in ...
+  (keyring)` line means you are on the fallback token, and the fix is to
+  restore keychain access or re-run `gh auth refresh`, NOT to widen anything.
+  Diagnosed the expensive way: a `PUT .../dismissals` returned `Forbidden`
+  in-sandbox and `DISMISSED` outside it, same account, same command.
 - **`network.allowLocalBinding`** — `podman info` returned *"dial tcp
   127.0.0.1:64752: connect: operation not permitted"*. The podman VM is reached
   over a local TCP port, so `filesystem.allowRead` and

requirements-dev.txt

@@ -3,6 +3,7 @@ pytest-cov
 httpx
 black
 ruff
+mypy
 pyyaml
 podman-compose
 websockets

scripts/quality-check.sh

@@ -96,6 +96,64 @@ if find . -name "*.py" -not -path "./build/*" -not -path "./.venv/*" -not -path
         echo "   Install with: python3 -m venv .venv && .venv/bin/pip install -r requirements-dev.txt"
         ERRORS=$((ERRORS + 1))
     fi
+    # mypy, scoped to files this branch actually changed.
+    #
+    # `docs/agents/rules.md` requires mypy to pass, but nothing ran it: not
+    # this gate, not CI, and it was not even in requirements-dev.txt. Turning
+    # it on repo-wide is not an option — a measured 2914 errors across 191
+    # files, 417 of them outside tests. So the rule is enforced going forward
+    # instead of retroactively: code you touch must type-check, and the legacy
+    # backlog burns down as files get edited.
+    #
+    # Three sources, because each misses something the others catch:
+    # committed-on-this-branch, uncommitted-but-tracked, and untracked. The
+    # last one matters most — a brand-new file is untracked until its first
+    # commit, and this gate is meant to run BEFORE that commit. Leaving it out
+    # let a probe file with a known error pass the gate silently.
+    #
+    # A file deleted on this branch still appears in the diff, hence the -e
+    # test. No changed Python files is a pass, not a skip-with-warning.
+    #
+    # An unresolvable origin/main is an ERROR, not a warning: warnings exit
+    # 0, so a run that type-checked nothing would print "Errors: 0" and
+    # report success for a check that never happened.
+    if MYPY=$(py_tool mypy); then
+        echo "🔸 Checking mypy on changed files..."
+        base=$(git merge-base origin/main HEAD 2>/dev/null || echo "")
+        if [ -z "$base" ]; then
+            echo "❌ Cannot resolve origin/main — mypy checked nothing."
+            echo "   Run: git fetch origin main"
+            ERRORS=$((ERRORS + 1))
+        else
+            # sort -u is load-bearing: a file changed on the branch AND dirty
+            # in the working tree appears in both diffs, and mypy fails with
+            # "Duplicate module named ..." when handed the same path twice.
+            #
+            # An array, not a space-joined string: a path containing a space
+            # or a glob character would otherwise be split into two arguments
+            # or expanded against the working tree.
+            changed=()
+            while IFS= read -r f; do
+                case "$f" in *.py) [ -e "$f" ] && changed+=("$f") ;; esac
+            done <<EOF
+$( { git diff --name-only "$base" HEAD; git diff --name-only HEAD; git ls-files --others --exclude-standard; } | sort -u)
+EOF
+            if [ ${#changed[@]} -eq 0 ]; then
+                echo "✅ mypy OK (no changed Python files)"
+            elif ! "$MYPY" --explicit-package-bases --ignore-missing-imports "${changed[@]}" >/dev/null 2>&1; then
+                echo "❌ mypy errors in changed files. Run:"
+                printf '   %s --explicit-package-bases --ignore-missing-imports %s\n' \
+                    "$MYPY" "${changed[*]}"
+                ERRORS=$((ERRORS + 1))
+            else
+                echo "✅ mypy OK (changed files)"
+            fi
+        fi
+    else
+        echo "❌ mypy not found in .venv/bin or on PATH — cannot verify types."
+        echo "   Install with: .venv/bin/pip install -r requirements-dev.txt"
+        ERRORS=$((ERRORS + 1))
+    fi
 else
     echo "ℹ️  No Python files found to check"
 fi

scripts/request-pr-review.sh

@@ -60,8 +60,16 @@
 #
 # Exit codes:
 #   0  a verdict landed; verdict on stdout
-#   2  timed out waiting (recent PR Review runs dumped for diagnosis)
 #   1  usage/precondition error
+#   2  timed out waiting (recent PR Review runs dumped for diagnosis)
+#
+# A timeout used to mean "the bot found nothing and said nothing" as often as
+# it meant a real failure, because the reviewer only spoke up when it had
+# findings. pr-review.yml now requires a summary review on every run, including
+# a clean one, so a timeout is once again a genuine signal worth investigating.
+# That matters beyond diagnosis: GitHub blocks a merge on the LAST explicit
+# verdict, so a silent clean run could never clear a prior REQUEST_CHANGES —
+# the review had to be dismissed by hand.
 set -euo pipefail
 
 pr="${1:?usage: request-pr-review.sh <pr-number> [timeout-seconds]}"