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

.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,101 @@
+"""Tests for the mypy gate in scripts/quality-check.sh.
+
+The gate scopes mypy to files changed against `origin/main`, so it depends on
+that ref resolving. When it cannot, the run type-checks nothing — and a check
+that verified nothing must not be able to report success. Warnings exit 0, so
+the severity of that branch is the whole behaviour under test.
+
+Everything expensive is a shim on PATH (`git`, `black`, `ruff`, `mypy`,
+`pytest`), same approach as test_backlog_digest.py: the script runs against a
+throwaway directory, not this repo.
+"""
+
+import os
+import re
+import stat
+import subprocess
+from pathlib import Path
+
+REPO_ROOT = Path(__file__).resolve().parents[2]
+SCRIPT = REPO_ROOT / "scripts" / "quality-check.sh"
+
+# A git shim that resolves origin/main, and one that cannot. Only the
+# merge-base arm differs — everything else the gate calls is identical, so a
+# difference in the run is attributable to that arm alone.
+GIT_RESOLVES = """
+case "$1 $2" in
+  "merge-base origin/main") echo deadbeef ;;
+  "diff --name-only") ;;
+  "ls-files --others") ;;
+  *) ;;
+esac
+exit 0
+"""
+
+GIT_CANNOT_RESOLVE = """
+case "$1 $2" in
+  "merge-base origin/main") exit 128 ;;
+  "diff --name-only") ;;
+  "ls-files --others") ;;
+  *) ;;
+esac
+exit 0
+"""
+
+
+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) -> subprocess.CompletedProcess:
+    """Run the gate in a throwaway project with the given `git` behaviour."""
+    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("x = 1\n")
+
+    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", "mypy", "pytest"):
+        _write_shim(bin_dir, tool, "exit 0\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_unresolvable_origin_main_fails_the_gate(tmp_path: Path) -> None:
+    """A run that type-checked nothing must not be able to exit 0.
+
+    Asserted as a delta against the resolvable run rather than an absolute
+    count, so unrelated checks failing in a stub directory cannot mask or
+    manufacture the signal.
+    """
+    resolvable = _run(tmp_path / "ok", GIT_RESOLVES)
+    unresolvable = _run(tmp_path / "broken", GIT_CANNOT_RESOLVE)
+
+    assert _error_count(unresolvable) == _error_count(resolvable) + 1
+    assert unresolvable.returncode != 0
+
+
+def test_resolvable_origin_main_runs_mypy(tmp_path: Path) -> None:
+    """The control: with the ref resolvable the gate actually checks types."""
+    proc = _run(tmp_path / "ok", GIT_RESOLVES)
+    assert "Checking mypy on changed files" in proc.stdout
+    assert "Cannot resolve origin/main" not in proc.stdout

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,59 @@ 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.
+            changed=""
+            while IFS= read -r f; do
+                case "$f" in *.py) [ -e "$f" ] && changed="$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 [ -z "$changed" ]; 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:"
+                echo "   $MYPY --explicit-package-bases --ignore-missing-imports$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

@@ -61,6 +61,14 @@
 # Exit codes:
 #   0  a verdict landed; verdict on stdout
 #   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.
 #   1  usage/precondition error
 set -euo pipefail