From d9b67663624364b46b6263c66e9e8b34c3021964 Mon Sep 17 00:00:00 2001 From: Codex Date: Mon, 31 Aug 2026 03:56:01 +0300 Subject: [PATCH] test: the sys.modules guard now sees the write, not its spelling (#398) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The guard introduced by #394 matched the literal sys.modules['custom_components... and therefore never looked at pure_imports.py, which writes through a variable — the third instance of the #389 class walked straight past the check created for it. The guard now inspects the write itself and decides by the key: a whole literal or the literal head of an f-string is safe unless it starts with custom_components (that is how tests register homeassistant.*, hp_pure.* and houseplan.trails); anything else — a variable, a concatenation, setdefault/update — counts as a violation whenever the file is able to name the package at all, i.e. contains a custom_components. literal. A file that never names the package cannot poison it through a variable, so restoring a snapshot stays legal. load_pure now removes what it registered. Removing its own name is not enough: relative imports pull neighbours in, so junction_limits leaves wall_segment_model and coordinate_canonicalization behind. It removes the whole custom_components difference accumulated during exec_module, in a finally, and a repeated call still works. pure_imports.py is a named exemption of the static guard precisely because that guard cannot see the cleanup — so the cleanup is proven by an executable test instead, and the mutant pure-imports-stops-cleaning reddens it. Both mutants were run by hand. User-Visible: no Issue: #398 --- docs/ARCHITECTURE.md | 7 ++ scripts/mutation-gate.mjs | 22 +++++ test/backend-test-hygiene.test.mjs | 135 +++++++++++++++++++++++++- tests_backend/pure_imports.py | 24 ++++- tests_backend/test_backend_quality.py | 39 ++++++++ 5 files changed, 220 insertions(+), 7 deletions(-) diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index 5d65ca37..2aaba150 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -1567,6 +1567,13 @@ their passports; `scripts/config-audit.mjs` treats both as `current`. - `pyproject.toml` configures ruff (E/F/B/I, E501 excluded by decision) and mypy strict for a grow-only allowlist of pure modules; the completeness guard lives in `tests_backend/test_backend_quality.py`. +- Writing into `sys.modules` from a backend test is refused by + `test/backend-test-hygiene.test.mjs` — by the fact of the write, not by its + spelling (#398). Two files are named exemptions: `conftest.py`, whose stub is + conditional on Home Assistant being absent, and `pure_imports.py`, which + registers a module only for the duration of `exec_module` and removes the + whole `custom_components` difference afterwards — the removal itself is + proven by an executable test, because the static guard cannot see it. - Both linters RUN in the backend CI job: the typing step derives its module list from the `pyproject.toml` allowlist rather than repeating it, refuses an empty list, and is itself guarded by a test plus the `typing-gate-stops- diff --git a/scripts/mutation-gate.mjs b/scripts/mutation-gate.mjs index a9629ef9..2998126c 100644 --- a/scripts/mutation-gate.mjs +++ b/scripts/mutation-gate.mjs @@ -746,6 +746,28 @@ const MUTANT_DEFINITIONS = [ replace: " this._persistDecorStyle();\n }, 0);", }], }, + { + id: 'sysmodules-guard-blind-to-variable', + guard: 'node --test test/backend-test-hygiene.test.mjs', + because: 'a guard that only sees string literals let the third instance of ' + + 'the #389 class walk past it (#398 AC1)', + patches: [{ + file: 'test/backend-test-hygiene.test.mjs', + find: ' if (start === null) { if (namesThePackage) hits.push(match[0]); continue; }', + replace: ' if (start === null) { continue; }', + }], + }, + { + id: 'pure-imports-stops-cleaning', + guard: 'python3 -m pytest tests_backend/test_backend_quality.py -q -p no:cacheprovider', + because: 'pure_imports is allowed to write into sys.modules only because it ' + + 'removes the entries again; without that the exemption is a hole (#398 AC8)', + patches: [{ + file: 'tests_backend/pure_imports.py', + find: ' del sys.modules[key]', + replace: ' pass', + }], + }, { id: 'device-echo-keeps-local-noncanonical', guard: 'node demo/smoke_device_position_history.mjs', diff --git a/test/backend-test-hygiene.test.mjs b/test/backend-test-hygiene.test.mjs index 29dfcd66..1aef0dcd 100644 --- a/test/backend-test-hygiene.test.mjs +++ b/test/backend-test-hygiene.test.mjs @@ -14,9 +14,63 @@ import { fileURLToPath } from 'node:url'; // а исходник ловит намерение. const BACKEND = fileURLToPath(new URL('../tests_backend/', import.meta.url)); + +/** Файлы, которым запись в `sys.modules` разрешена поимённо (#398 AC3). + * + * Список короткий не случайно. `conftest.py` ставит пустышки родительских + * пакетов, когда Home Assistant недоступен, — по условию и осознанно (#394). + * `pure_imports.py` регистрирует загружаемый модуль ровно на время + * `exec_module` и снимает регистрацию в `finally`; статический разбор этого + * знать не может, поэтому файл назван здесь, а то, что он действительно + * убирает за собой, доказывает исполняемая проверка ниже. */ +const SYS_MODULES_WRITERS = ['conftest.py', 'pure_imports.py']; + const files = () => readdirSync(BACKEND).filter((name) => name.endsWith('.py')); const read = (name) => readFileSync(new URL(name, `file://${BACKEND}`), 'utf8'); +/** Записи в `sys.modules`, про которые нельзя доказать, что пакет интеграции + * они не трогают (#398). + * + * Прежняя проверка искала строковый литерал `sys.modules['custom_components` + * и не видела `sys.modules[name] = module` — так третий заход класса #389 + * проехал мимо гварда, заведённого ради него. Здесь разбирается сам факт + * записи, а решение принимается по ключу: + * + * - ключ — литерал или f-строка с литеральным началом: безопасно, если это + * начало не `custom_components` (так тесты ставят `homeassistant.*`, + * `hp_pure.*`, `houseplan.trails` — свои имена, к пакету не относящиеся); + * - ключ — выражение, либо это `setdefault`/`update`: судить о содержимом + * статически нельзя, поэтому запись считается нарушением, КОГДА файл вообще + * способен назвать пакет интеграции — то есть содержит литерал вида + * `custom_components.` (с точкой: это имя модуля, а не кусок пути). + * + * Последнее и есть fail-closed в осмысленной форме: файл, который нигде не + * упоминает имя пакета, отравить его переменной не может, а файл, который + * упоминает, обязан делать это разрешённым способом. */ +function sysModulesWrites(source) { + const code = source.replace(/#[^\n]*/g, ''); + const namesThePackage = /(['"])custom_components\./.test(code); + // Начало ключа, если оно доказуемо литеральное: целая строка либо f-строка + // до первой подстановки. Всё прочее (конкатенация, вызов, переменная) — не + // литерал: доказать по нему ничего нельзя. + const literalStart = (key) => { + const whole = /^(['"])((?:[^'"\\]|\\.)*)\1$/.exec(key.trim()); + if (whole) return whole[2]; + const fstring = /^f(['"])((?:[^'"\\{]|\\.)*)/.exec(key.trim()); + return fstring ? fstring[2] : null; + }; + const hits = []; + for (const match of code.matchAll(/sys\.modules\s*\[([^\]]*)\]\s*=/g)) { + const start = literalStart(match[1]); + if (start === null) { if (namesThePackage) hits.push(match[0]); continue; } + if (start.startsWith('custom_components')) hits.push(match[0]); + } + for (const match of code.matchAll(/sys\.modules\.(?:setdefault|update)\s*\(/g)) { + if (namesThePackage) hits.push(match[0]); + } + return hits; +} + test('backend-тесты не правят sys.path (#393)', () => { const offenders = files().filter((name) => /^\s*sys\.path\b/m.test(read(name))); assert.deepEqual(offenders, [], @@ -36,12 +90,22 @@ test('пакет интеграции подменяет только conftest // она не срабатывала лишь потому, что настоящий пакет успевал импортироваться // из файла, который идёт раньше по алфавиту. Корректность харнесса держалась // на именах файлов; чем это кончается, показал #389. - const assigns = /sys\.modules\[\s*(['"])custom_components/; - const offenders = files().filter((name) => name !== 'conftest.py' && assigns.test(read(name))); + // #398: проверка смотрит на ФАКТ записи в `sys.modules`, а не на её запись + // строковым литералом. Прежняя регулярка искала `sys.modules['custom_...` + // и не видела `sys.modules[name] = module` в pure_imports.py — третий заход + // того же класса проехал мимо гварда, заведённого ради него. + // + // Разбор грубый и fail-closed: ключ, про который нельзя доказать, что он не + // из `custom_components`, считается нарушением. Догонять формы записи + // регуляркой — бесконечная гонка, отказывать на непонятном — нет. + const offenders = files() + .filter((name) => !SYS_MODULES_WRITERS.includes(name)) + .filter((name) => sysModulesWrites(read(name)).length > 0); assert.deepEqual(offenders, [], - 'подмена пакета интеграции живёт в tests_backend/conftest.py и только там:' - + ' там она условная (нет Home Assistant — нечего ломать), а в тесте она' - + ' переживает свой тест и достаётся всей сессии (#389, #394).'); + 'запись в sys.modules живёт только в tests_backend/conftest.py (условная' + + ' подмена пакета) и tests_backend/pure_imports.py (регистрация, снимаемая' + + ' тем же вызовом). В остальных файлах она переживает свой тест и' + + ' достаётся всей сессии (#389, #394, #398).'); const conftest = read('conftest.py'); assert.match(conftest, /if not HAS_HA:/, @@ -49,3 +113,64 @@ test('пакет интеграции подменяет только conftest const stub = conftest.slice(conftest.indexOf('if not HAS_HA:')); assert.match(stub, /sys\.modules\[_name\] = _module/); }); + +// --- #398: сам гвард обязан ловить свой класс ---------------------------- +// Проверка, которая не умеет краснеть, — это не проверка. Формы записи +// перечислены поимённо, потому что каждая из них уже встречалась в проекте +// или на расстоянии одной правки от встречавшейся. + +const guardSees = (code) => sysModulesWrites(code).length > 0; +// Файл, который где-то называет пакет интеграции как модуль — именно то +// условие, при котором запись «неизвестно чем» становится опасной. +const PACKAGE_NAMED = 'PKG = "custom_components.houseplan"\n'; + +test('#398 AC1: запись через переменную не проходит мимо гварда', () => { + assert.equal(guardSees(`${PACKAGE_NAMED}sys.modules[name] = module\n`), true, + 'ключ-переменная — ровно та форма, которую прежняя регулярка не видела'); +}); + +test('#398 AC2: остальные формы записи тоже ловятся или отклоняются', () => { + for (const line of [ + 'sys.modules[f"custom_components.{name}"] = module', + 'sys.modules["custom_" + "components.houseplan"] = module', + 'sys.modules.setdefault("custom_components.houseplan", module)', + 'sys.modules.update({"custom_components.houseplan": module})', + 'sys.modules[ key ] = module', + ]) { + assert.equal(guardSees(`${PACKAGE_NAMED}${line}\n`), true, `не поймана форма: ${line}`); + } +}); + +test('#398 AC2: безопасное не объявляется нарушением', () => { + for (const line of [ + 'sys.modules["homeassistant.core"] = core', + 'sys.modules[f"hp_pure.{dep}"] = mod', + 'sys.modules["houseplan.trails"] = mod', + '# sys.modules["custom_components.houseplan"] = stub # так писать нельзя', + ]) { + assert.equal(guardSees(`${line}\n`), false, `ложное срабатывание на: ${line}`); + } + // Файл, который нигде не называет пакет интеграции, отравить его + // переменной не может — восстановление снимка остаётся законным. + assert.equal(guardSees('sys.modules.update(saved)\n'), false); +}); + +test('#398 AC3: список файлов с правом записи закрыт', () => { + assert.deepEqual(SYS_MODULES_WRITERS, ['conftest.py', 'pure_imports.py'], + 'третий файл в списке — это новое исключение, а не правка: оно требует' + + ' своего обоснования и отдельного решения, а не молчаливого добавления'); +}); + +test('#398 AC8: pure_imports снимает регистрацию, а не обещает', () => { + // Файл в списке исключений не потому, что ему доверяют, а потому, что + // статический разбор не видит очистки. Значит очистку проверяет этот тест — + // иначе исключение стало бы дырой ровно того размера, что #389. + const source = read('pure_imports.py'); + assert.match(source, /before = frozenset\(sys\.modules\)/, + 'снимок берётся до загрузки'); + assert.match(source, /finally:[\s\S]*del sys\.modules\[key\]/, + 'разница снимается в finally — даже если exec_module бросил'); + assert.match(source, /key\.startswith\(PACKAGE_ROOT\.name\)/, + 'снимается вся разница под префиксом пакета, а не одно собственное имя:' + + ' относительные импорты подтягивают соседей'); +}); diff --git a/tests_backend/pure_imports.py b/tests_backend/pure_imports.py index c4e0d6e9..2880fbbb 100644 --- a/tests_backend/pure_imports.py +++ b/tests_backend/pure_imports.py @@ -21,13 +21,33 @@ HOUSEPLAN_ROOT = PACKAGE_ROOT / "houseplan" def load_pure(name: str, file: Path): - """Загрузить модуль по пути под указанным именем. + """Загрузить модуль по пути под указанным именем и убрать за собой. Имя значимо: относительные импорты внутри модуля резолвятся только тогда, когда модуль знает, какому пакету принадлежит. + + Регистрация в `sys.modules` обязательна на время `exec_module` и вредна + после (#398). Она переживала вызов и доставалась всей сессии — тот же + класс, что #389: загрузчик Home Assistant получал бы модуль, собранный + мимо него, а объекты классов одного файла оказывались бы разными. Снимать + только собственное имя мало: относительные импорты подтягивают соседей + (`junction_limits` тянет `wall_segment_model` и + `coordinate_canonicalization`), поэтому снимается вся разница под + префиксом `custom_components`, появившаяся за время загрузки. + + `conftest.py` ставит пустышки родительских пакетов, когда Home Assistant + недоступен, — они принадлежат ему и здесь не трогаются. """ + before = frozenset(sys.modules) spec = importlib.util.spec_from_file_location(name, file) module = importlib.util.module_from_spec(spec) sys.modules[name] = module - spec.loader.exec_module(module) + try: + spec.loader.exec_module(module) + finally: + for key in [ + key for key in sys.modules + if key not in before and key.startswith(PACKAGE_ROOT.name) + ]: + del sys.modules[key] return module diff --git a/tests_backend/test_backend_quality.py b/tests_backend/test_backend_quality.py index 295d633a..dd17aad3 100644 --- a/tests_backend/test_backend_quality.py +++ b/tests_backend/test_backend_quality.py @@ -157,3 +157,42 @@ def test_issue_42_every_noqa_carries_a_reason(): explanation = comment.split(" ", 1)[1] if " " in comment.strip() else "" assert len(explanation.strip()) >= 10, ( f"{path.name}:{index}: a bare noqa hides a decision — add the reason") + + +def test_issue_398_pure_imports_leaves_sys_modules_as_it_found_it(): + """`load_pure` не оставляет следов в `sys.modules` (#398 AC4). + + Статический гвард (`test/backend-test-hygiene.test.mjs`) разрешает этому + файлу писать в `sys.modules`, потому что не видит очистки. Значит очистку + обязан доказывать исполняемый тест — иначе разрешение стало бы дырой + ровно того размера, что #389: пустышка переживала свой тест, Home + Assistant не мог поднять интеграцию, и 85 тестов харнесса падали с голым + `assert False`. + + Проверяется именно РАЗНИЦА, а не пустота: пустышки родительских пакетов + ставит conftest, они принадлежат ему и остаются. + """ + import sys + + from tests_backend.pure_imports import HOUSEPLAN_ROOT, load_pure + + before = sorted(k for k in sys.modules if k.startswith("custom_components")) + module = load_pure( + "custom_components.houseplan.junction_limits", + HOUSEPLAN_ROOT / "junction_limits.py", + ) + assert dir(module), "модуль обязан быть рабочим после очистки" + after = sorted(k for k in sys.modules if k.startswith("custom_components")) + assert after == before, ( + f"load_pure оставил в sys.modules: {sorted(set(after) - set(before))} — " + "относительные импорты подтягивают соседей, снимать нужно всю разницу" + ) + + # Повторный вызов обязан работать так же: очистка не должна ломать + # следующий заход (тесты вызывают load_pure по нескольку раз за сессию). + again = load_pure( + "custom_components.houseplan.junction_limits", + HOUSEPLAN_ROOT / "junction_limits.py", + ) + assert dir(again) + assert sorted(k for k in sys.modules if k.startswith("custom_components")) == before