fix: make the mypy gate a ratchet, so it can actually be passed - #664
Conversation
#614 scoped mypy to changed files with the intent that "the legacy backlog burns down as files get edited". As written it could not be satisfied: mypy reports errors in the modules a checked file imports, not only in the file named on the command line. Handing it one core/bess file pulled in ha_api_controller.py and 30 others, so the gate demanded exactly the repo-wide cleanup its own comment ruled out. Measured on #643's six-line fix: 404 errors across 31 files, none of them introduced by that branch. No PR touching core/bess could pass. It went unnoticed because #614 merged on Aug 19, after the last core/bess PRs (#618, #619) had already landed. Two changes make the gate mean what it intended: - --follow-imports=silent, so errors are attributed to the files under test rather than to whatever they import. - a baseline comparison against the merge-base, so only NEW error signatures fail. Signatures drop line numbers, so inserting a line above an existing error is not a new one. The baseline is computed rather than committed — the merge-base tree is extracted with `git archive` and type-checked on the fly. No generated file to drift, and no `git worktree add`: this repo is shared with other checkouts and live sessions, and a lint gate should not register and remove worktrees as a side effect. CI and quality-check.sh now run the same script, so a green local gate and a green CI gate mean the same thing. Verified against the real #643 diff: "no new errors; 85 pre-existing in touched files", exit 0, where the old gate failed it on 404. The gate's tests are rewritten rather than extended — they pinned the old "changed files must be clean" semantics. Every guard they had is kept (vacuity, unresolvable ref, spaced paths) plus the two halves that matter now: a pre-existing error must pass, and an uncomputable baseline must fail closed. rules.md claimed all code must pass mypy with zero errors, which was never true and is now explicitly not the rule; it documents the ratchet instead. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015EswRHZDV575RZYPBq7L7o
|
@claude-bot review |
![bess-manager-claude-bot[bot]](https://avatars.githubusercontent.com/in/3024553?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: fix/mypy-gate-ratchet
Root cause matches evidence. Verified independently: running the old invocation style (mypy --explicit-package-bases --ignore-missing-imports core/bess/ha_api_controller.py, no --follow-imports=silent) against current main produces 305 errors across 28 files from checking a single file — confirming the PR's claim that #614's gate demanded repo-wide cleanup by construction, not by accident.
Verified locally (fresh venv, not just trusting the PR description):
.venv/bin/pytest backend/tests/test_quality_check_mypy_gate.py -v→ 6/6 pass.venv/bin/pytest -m "not slow"→ 2190 passed, 50 skippedblack --check .andruff check .→ cleanshellcheck scripts/mypy-changed.sh→ zero warnings (the one shellcheck hit inquality-check.shis pre-existing, unrelated line 685, not part of this diff)- Ran
scripts/mypy-changed.sh .venv/bin/mypy --include-worktreeagainst this branch's own diff →✅ mypy OK (changed files), confirming the "this PR proves itself" claim in the description
Traced the script logic line-by-line (scripts/mypy-changed.sh):
MYPYis resolved to an absolute path before the latercd "$repo_root", so the fix for the "relative.venv/bin/mypybreaks inside the extracted baseline tree" bug is real and correctly ordered.signatures()'s sed (s/^([^:]+):[0-9]+:([0-9]+:)? /\1: /) correctly strips bothfile:line:andfile:line:col:mypy output shapes, verified against both formats.comm -13diffing is valid because bothhead_errorsandbase_errorsflow through the samesignatures()function, which sorts — a real precondition forcomm, satisfied.- Baseline exit-status handling (
base_status -gt 1→ fail closed with exit 2) correctly distinguishes mypy's "errors found" (1) from a crash/misconfiguration (>1), matching the documented fail-closed intent. - New-vs-deleted-file handling is consistent:
--diff-filter=dkeeps deletions out ofchanged[](so mypy is never asked to check a file that no longer exists), and a file with no baseline entry (new on this branch) has all its errors counted as new — matches "new files are expected to be clean."
Architecture/rules compliance: no Optional[x], no hasattr/getattr fallbacks, no new classes, no sensor/API-layer touch, no exception-string matching — none of these apply to this diff (shell + test + docs only), and none are violated.
Tests check behavior, not implementation: the test suite drives the gate through shims (git, mypy) and asserts on the script's observable contract (exit code, stdout messages, error-count deltas) rather than internal script structure — consistent with docs/agents/testing.md. The delta-based pairing (test_newly_introduced_type_error_fails_the_gate vs test_a_preexisting_error_does_not_fail_the_gate, identical HEAD source, only the baseline arm differs) is a sound way to rule out "gate never ran" as an explanation for a pass.
Minimal, no scope creep: touches exactly what's needed — one new script that both ci.yml and quality-check.sh delegate to (eliminating the duplicated inline logic that existed before), the test file rewritten to match the new ratchet semantics, and a one-paragraph doc correction in rules.md that was simply false before this PR (claimed "zero errors" when a ~2900-error backlog already existed).
One process nit, not a blocker: docs/agents/rules.md's Debugging Protocol (step 9) asks a fix's scope assessment (local vs. structural vs. needs-second-opinion) to be stated explicitly in the PR description. This PR doesn't use that literal framing, but the "Fix" section effectively documents the same judgment — consolidating logic into one script both callers delegate to, with explicit rationale for rejecting alternatives (git worktree add, a committed baseline file). Given the reasoning is present, just not labeled, this doesn't rise to a blocking finding.
No rule violations, no correctness bugs found. APPROVE.
2 participants
Scope assessment
Local, not structural. The gate already had one owner — the mypy step — invoked from two places with duplicated inline logic. This consolidates that logic into a single script both callers delegate to, rather than giving it a new owner.
The diff adds no parameter, flag, default-fallback, second construction site, or extra trigger whose job is to route around an ordering/timing/dependency problem. Two alternatives were considered and rejected with reasons recorded in the script header: a committed baseline file (drifts, needs regenerating) and
git worktree addfor baseline extraction (side effects on a repo shared with live agent sessions).Every failure path fails closed — unresolvable merge-base, unextractable baseline, and a crashed baseline mypy run all exit 2 rather than reporting a pass.
Summary
--follow-imports=silentso mypy errors are attributed to the files under test, not to everything they importquality-check.shnow share one script, so a green local gate and a green CI gate mean the same thingRoot cause
#614 scoped mypy to changed files, intending that "the legacy backlog burns down as files get edited". As written the gate could not be satisfied at all: mypy reports errors in the modules a checked file imports, not only in the file named on the command line. Handing it one
core/bessfile pulls inha_api_controller.pyand 30 others — so the gate demanded exactly the repo-wide cleanup its own comment ruled out ("Repo-wide is not an option (2914 errors across 191 files)").Measured on #643's six-line fix: 404 errors across 31 files, none introduced by that branch. No PR touching
core/besscould pass. It went unnoticed because #614 merged Aug 19, after the lastcore/bessPRs (#618, #619) had already landed — #643 was simply the first to hit it.Fix
scripts/mypy-changed.showns the logic;ci.ymlandquality-check.shboth call it.The baseline is computed, not committed — the merge-base tree is extracted with
git archiveand type-checked on the fly. No generated file to drift or regenerate, and deliberately notgit worktree add: this repo is shared with other checkouts and live agent sessions, and a lint gate should not register and remove worktrees as a side effect.Error signatures drop line numbers, so inserting a line above an existing error does not read as a new one. A file added on this branch has no baseline entry, so new files are still expected to be clean.
Two bugs found while building it, both now pinned by tests:
.venv/bin/mypyno longer resolves. It silently produced an empty baseline, turning all 61 pre-existing errors in one file into "newly introduced". Fixed by resolving mypy to an absolute path, and by checking the baseline run's exit status rather than discarding it.quality-check.shinvoked the script by a cwd-relative path — correct from the repo root, but it meant the gate silently never ran from anywhere else. Now resolved againstquality-check.sh's own location.Test plan
./scripts/quality-check.sh— fast suite 2190 passed / 50 skipped, frontend 125 passed, mypy gate green on its own diff✅ mypy OK (no new errors; 85 pre-existing in touched files), exit 0 — where the old gate failed it on 404 errorsshellchecknot run — not installed locallyEvidence the test discriminates
git archiveshim at a well-typed source while HEAD keeps the ill-typed onetest_a_preexisting_error_does_not_fail_the_gateandtest_newly_introduced_type_error_fails_the_gateuse the identical HEAD source and differ only in the baseline — so a pass cannot come from mypy never runningtest_an_uncomputable_baseline_fails_the_gatenow pins the fail-closed behaviourOutcome-level coverage
backend/tests/test_quality_check_mypy_gate.py, rewritten rather than extended — its cases pinned the old "changed files must be clean" semantics. Every guard it had is kept (vacuity, unresolvable ref, spaced paths); the two new ones are the halves that matter now: a pre-existing error must pass, and an uncomputable baseline must fail closed. 6/6.Documentation
docs/agents/rules.mdclaimed all code must pass mypy "with zero errors/warnings". That was never true — there is a ~2900-error backlog — and it is now explicitly not the rule. It documents the ratchet instead.No
CHANGELOG.mdentry: internal tooling with no user-visible effect, consistent with #656/#654/#651.Note
This PR proves itself — CI runs the new gate on the diff that changes it. It also unblocks
fix/issue-643-grid-charge-read, pushed and waiting on this to land.🤖 Generated with Claude Code
https://claude.ai/code/session_015EswRHZDV575RZYPBq7L7o