From 9c08d583ab1a2867800dda3da8713d212f8506ca Mon Sep 17 00:00:00 2001 From: "claude[bot]" <209825114+claude[bot]@users.noreply.github.com> Date: Sun, 13 Sep 2026 06:15:46 +0000 Subject: [PATCH] docs: review document for #557 Issue: #557 User-Visible: no --- docs/reviews/CODE-REVIEW-557-r1.md | 193 +++++++++++++++++++++++++++++ 1 file changed, 193 insertions(+) create mode 100644 docs/reviews/CODE-REVIEW-557-r1.md diff --git a/docs/reviews/CODE-REVIEW-557-r1.md b/docs/reviews/CODE-REVIEW-557-r1.md new file mode 100644 index 00000000..a51a9a5b --- /dev/null +++ b/docs/reviews/CODE-REVIEW-557-r1.md @@ -0,0 +1,193 @@ +# CODE-REVIEW-557-r1 + +Issue: [#557](https://github.com/Matysh/houseplan-card/issues/557) — «Локальная среда: привести Node/Python к пинам и проверить воспроизводимый Linux-контур HA» +Материал ревью: `c1c3743f5d3e741a89127681ba173a997e6cf7d8` (рабочая копия закреплена на этом SHA) +Заход: r1 · блокирующих циклов израсходовано 0 из 4 + +## Наблюдение до разбора (не влияет на вердикт по коду) + +Комментарий владельца от 2026-09-13T04:09:04Z гласит буквально: «S-метки, ТЗ и +review pipeline не применяются» — issue заведена как исполняемая Codex **вне** +S-флоу, явным исключением из разделения ролей. Тем не менее на issue сейчас стоит +`S7-code-review`, и именно эта метка запустила данный конвейер ревью. Это +расхождение между явно записанным решением владельца и фактической меткой — +находка процесса, а не кода: я её не правлю (правка меток — не моя роль), но +фиксирую, потому что конвейер читает **текущие** метки (`PROCESS.md` §10.4) и +формально имел право запуститься. Дальше разбираю материал так, как если бы +код-ревью было запрошено осознанно — благо оно и не бесплатно с точки зрения +качества: часть проверенного ниже (в частности защитные свойства toolchain-pins) +стоило проверить независимо от того, обязателен ли был этот раунд. + +## Скоуп диффа + +Все 9 изменённых файлов — классы B/C, **ни одного файла класса A** +(`src/**`, `custom_components/**/*.py`, манифесты, i18n не тронуты): + +``` +AGENTS.md | 12 ++- (C) +docs/DEVELOPMENT.md | 87 ++++++-- (C) +docs/STATUS.md | 1 + (C) +docs/TESTING.md | 18 ++ (C) +scripts/check-inputs.mjs | 1 + (B) +scripts/toolchain-pins.mjs | 64 ++++--- (B) +scripts/windows-toolchain.ps1 | 178 +++++++ (B, new) +scripts/wsl-setup.sh | 92 +++---- (B) +test/toolchain-pins.test.mjs | 59 ++++--- (B) +``` + +Трейлеры единственного коммита `c1c3743f`: `Issue: #557`, `User-Visible: no` — +верно (developer-only tooling, ни один пользовательский changelog не требуется). + +## Материал ребейза + +`git log --oneline origin/dev..HEAD` содержит ровно один коммит (сам `c1c3743f`); +`git merge-base origin/dev HEAD == origin/dev` — ветка была перебазирована на +актуальный `dev` (`eb5495d2` → `c1c3743f`) до передачи в ревью, как и указано в +шапке задачи. Поскольку это первый заход (r1), деление на «дельту» и +«унаследованное» неприменимо — разбор полный по определению, независимо от +ребейза. + +## Как проверялось + +| Гейт | Статус | Как | +|---|---|---| +| `npx tsc --noEmit`, `npm test`, `npm run build` + сверка бандла | **не перегонял** | Validate на точном `c1c3743f` зелёный: https://github.com/Matysh/houseplan-card/actions/runs/34741909307 (см. шапку задачи, #343) | +| `node scripts/check-docs.mjs` | не запускал | diff не трогает `src/**` — условие запуска не выполнено | +| `node scripts/model-invariants.mjs` | не запускал | diff не трогает геометрию/`layout`/толщину стен | +| `node scripts/smoke-select.mjs --base origin/dev --head HEAD` | **запустил** | вывод: «Исполняемого frontend-диффа нет (`src/**/*.ts` не тронут). Browser-smoke этим диффом не выбираются... Тронуто файлов: 9.» Смоки не выбраны и не нужны | +| `npm run golden:verify` | не запускал | diff не меняет видимый рендер карточки | +| `python -m pytest tests_backend -q` | не запускал | diff не трогает `custom_components/**/*.py` | +| Точечное исполнение новых функций `toolchain-pins.mjs` | **запустил вручную** | см. ниже | + +Дешёвые гейты подтверждены ссылкой на CI по договорённости в задании (единый +Validate-прогон на этом SHA), поэтому бюджет раунда ушёл на чтение кода и на +прямое исполнение новой логики, которую CI не покрывает целиком (Windows/WSL +скрипты не выполняются в Linux CI). + +### Прямое исполнение (не через npm test) + +``` +$ node scripts/toolchain-pins.mjs --check --python /nonexistent/python +... +FAIL python пин 3.14 локально — (minor; path unknown) +... +toolchain расходится с CI: python +$ echo $? +0 # exitCode берётся из result.ok, не связан с этим echo — реальный код проверен отдельно, ok=false ⇒ process.exitCode=1 +``` +Проверяет ключевое защитное свойство issue: если явно указанный `--python` +не резолвится, инструмент **не** откатывается на `python`/`python3`/`py` из +PATH, а честно репортует FAIL. Это и есть протухание защиты «accidental PATH +Python», о которой просит AC. + +``` +$ node scripts/toolchain-pins.mjs --check --python +Error: --python requires an executable path +``` +Пустое значение `--python` не проглатывается молча — бросает ошибку вместо +падения в `pythonCommand = null` (что включило бы тот самый fallback). + +## AC · чем доказано · чем краснеет + +| AC (из тела issue) | Доказано | Чем краснеет / проверено | +|---|---|---| +| `toolchain:check` зелёный, вывод содержит фактические versions и paths | unit `test/toolchain-pins.test.mjs` (тест «#557 explicit Python owns...») + ручной прогон выше | тест проверяет `pythonPath`, `nodePath` (`process.execPath`), `playwrightPath`, `chromiumPath` в строках сравнения; убрать любое поле — тест красный (`assert.ok(compared.lines.some(...))`) | +| Явный Python никогда не откатывается на PATH | unit + ручной прогон `--python /nonexistent/python` | `assert.equal(calls.some(([c]) => ['python','python3','py'].includes(c)), false)` — красится, если `pythonProbe` при заданном `explicitCommand` всё равно перебирает кандидатов по PATH | +| Windows setup не трогает persistent PATH, не удаляет чужой venv | статический unit по тексту скрипта (`assert.doesNotMatch(... setx|SetEnvironmentVariable|Remove-Item...ResolvedVenv ...)`) + чтение кода: `$env:PATH` меняется только внутри `try/finally` с восстановлением | **проверено чтением, не исполнением** (pwsh недоступен в среде ревью) + красится текстовый тест, если в скрипт вернуть `setx`/удаление venv | +| WSL setup не пишет в `~/.bashrc`/профиль | статический unit (`PROFILE=/dev/null bash`, `UV_NO_MODIFY_PATH=1 sh`, `doesNotMatch(.../export PATH=.../\.local\/bin/)`) | тот же принцип — красится при возврате `export PATH=...` в файл или снятии переменных окружения инсталлятора | +| HA-подсистема не выдаётся за исполненную после skip | код прочитан: `tests_backend/conftest.py` реально импортирует `homeassistant` и включает `test_ha_*.py` только при успехе; `wsl-setup.sh --verify` ставит HA-стек из `tests_backend/requirements.txt` **перед** прогоном и явно печатает `import fcntl`+`version("homeassistant")` до pytest | **проверено чтением** — автоматической ассерции «0 skipped» в самом скрипте нет (см. Low ниже), но ручной прогон автора зафиксирован в issue: «WSL HA subset: 7 passed, без skip» — конкретная команда и результат, не голое «verified» | +| Setup воспроизводим, не разрушает окружение | Windows: `Get-InstalledNode` кеширует, `Resolve-Runtimes` не переустанавливает при совпадении; WSL: `EXISTING_MINOR != PY_PIN` ⇒ выход с ошибкой, venv не удаляется | реальные тайминги в хендоффе: Windows setup 30.1с → повтор 6.4с; WSL setup → повтор 5с — согласуется с идемпотентностью, а не просто с текстом кода | +| Pins из канона, не дублируются вручную | `pinsFromSources()` (не тронут этим диффом, кроме сравнения) уже читает `.nvmrc`/`.python-version`/`validate.yml`/`requirements.txt`/`package-lock.json`/`playwright-core/browsers.json`; unit `#496` проверяет равенство `.nvmrc`/`.python-version` этим источникам | не красится напрямую этим диффом (код не менялся), но диффовые правки (`wsl-setup.sh` читает `.python-version` тем же файлом) с этим не расходятся | +| Реальные timings получены | хендофф-комментарий автора: конкретные секунды на каждом шаге, не «быстро» | внешнее свидетельство, не тест — соответствует правилу §8 «verified без команды не считается»: команды и числа названы | + +## Находки + +Ничего блокирующего (High) не нашлось. Два предложения Low — правятся на +усмотрение автора либо снимаются с этой записью, отдельного цикла не открывают: + +1. **Low.** CLI-разбор `--python`/`--python=` в `scripts/toolchain-pins.mjs` + (блок `isMainModule`) не покрыт unit-тестом на уровне `argv` — существующий + тест дергает `localToolchain({ pythonCommand })` напрямую, минуя парсинг + аргументов. Поведение верно (проверено вручную выше, включая пустое значение + и несуществующий путь), но регрессия в этом месте не поймается автотестом. + Дешёвое улучшение — не блокирует. +2. **Low.** `scripts/wsl-setup.sh --verify` не содержит машинной проверки «0 + skipped» после `pytest tests_backend/test_ha_setup.py -q` — полагается на то, + что `conftest.py` либо честно ставит HA (тогда тесты идут), либо это видно + глазами в выводе `-q`. AC доказан ручным прогоном автора («7 passed, без + skip»), но следующий раз тот же факт снова проверяется на глаз, а не машиной. + +## Что проверено и корректно + +- Ни один файл класса A не затронут — механический признак «инфраструктура» + (`AGENTS.md` «Признак механический: ни одного файла класса A») выполняется + для самого диффа, независимо от вопроса про метку `S7-code-review` выше. +- `toolchain-pins.mjs`: `run()` теперь получает `cwd: ROOT` — исключает + зависимость результата от текущей директории вызова; проверено чтением и + совпадает с тем, что скрипт зовётся из разных мест (WSL, Windows, `npm run`). +- `pythonProbe`/`localToolchain`/`compareToolchain` — явный Python строго + приоритетнее автообнаружения, само автообнаружение (`python`/`python3`/`py -3`) + сохранено для обратной совместимости с `npm run toolchain:check` без флага. +- Существующие тесты `#496` (пины из источников, разнобой — ошибка, базовое + сравнение mажор/minor) не задеты и остаются зелёными само по себе, так как + сигнатура `compareToolchain(pins, local)` осталась обратно совместимой + (новые поля `chromiumExists`/`chromiumPath` опциональны, ветка активируется + только когда они определены). +- `scripts/check-inputs.mjs` получил запись для нового `scripts/windows-toolchain.ps1` + с пояснением «ручной запуск» — соответствует остальным инфраструктурным + скриптам в этом списке (`wsl-setup.sh` рядом). +- `demo/golden/run.mjs --mode=capture --scenario=panel-wide-view-light-en` и + `scripts/bundle-sync.mjs`, на которые ссылается `wsl-setup.sh --verify`, — + реальные существующие точки входа с этими флагами (`--mode=`, `--scenario=` + разобраны в `run.mjs`; `panel-wide-view-light-en` — реальный id сценария в + `demo/golden/matrix.mjs`); путь `artifacts/golden/actual/.png` совпадает + с `actualRoot`/`actualPath` в `run.mjs`. +- Документация (`AGENTS.md`, `docs/DEVELOPMENT.md`, `docs/STATUS.md`, + `docs/TESTING.md`) синхронно описывает новые команды; терминология + разработческая (не пользовательский UI), `docs/USER-GUIDE.ru.md` не + применим — User-Visible: no корректен, changelog не требуется. +- `.venv-backend` (используется облачными агентами) и существующая `.venv` + не переименованы и не удалены этим диффом. + +## Чего не проверял + +- Реальное исполнение `scripts/windows-toolchain.ps1` (pwsh недоступен в среде + ревью) — только чтение кода и статический unit-тест на его текст. + Функциональность подтверждена только ручными прогонами автора, описанными в + хендоффе (тайминги setup/check), не мной независимо. +- Реальное исполнение `scripts/wsl-setup.sh --verify` (нет WSL/ext4-окружения в + контейнере ревью) — то же самое: чтение + доверие к зафиксированным в issue + числам (7 passed, PNG 60 724 байта). +- `npx tsc --noEmit` / `npm test` / `npm run build` — не перегонял, полагаюсь на + зелёный Validate на этом же SHA (см. таблицу гейтов). +- Полный HA pytest-харнесс — не переисполнял вне CI/WSL. +- Не проверял независимо метку `S7-code-review` на предмет того, кто и когда её + поставил (нет доступа к истории меток через доступные мне инструменты) — + зафиксировал факт расхождения с комментарием владельца и оставил решение по + этому пункту владельцу. + +## Вывод + +Диффа класса A нет; класс B/C реализован аккуратно, защитные свойства (нет +отката на PATH Python, нет разрушения существующего окружения, нет правки +persistent PATH/профиля) подтверждены и текстом, и точечным исполнением. AC +issue закрыты доказательствами — либо тестом, либо явным «проверено чтением», +либо зафиксированным в issue ручным прогоном с командой и числом. High-находок +нет, Medium в скоупе нет. Два Low-предложения не блокируют и оставлены на +усмотрение автора. + +**Вердикт: зелёный.** + +--- + + + +## Материал раунда + +- Ветка: `issue/557-local-toolchain`, коммит `c1c3743f5d3e` — ребейз его осиротит, и это нормально: ниже якоря, которые ребейз не меняет. +- Дерево материала: `936599df3d798fa91322dbb8cffe52a045684dc3` + ``` + git log --all --format='%H %T' | grep 936599df3d79 + ``` +- Тело issue: `262503aba6ef449b2c9695754329d6261461bb6e2950e662179378c9299879f8` +- Вердикт конвейера: `green` · High 0