mirror of
https://github.com/Matysh/houseplan-card
synced 2026-09-29 03:09:36 +00:00
claude[bot]
@@ -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/<id>.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-предложения не блокируют и оставлены на
|
||||
усмотрение автора.
|
||||
|
||||
**Вердикт: зелёный.**
|
||||
|
||||
---
|
||||
|
||||
<!-- material-anchors: сгенерировано конвейером (#414) -->
|
||||
|
||||
## Материал раунда
|
||||
|
||||
- Ветка: `issue/557-local-toolchain`, коммит `c1c3743f5d3e` — ребейз его осиротит, и это нормально: ниже якоря, которые ребейз не меняет.
|
||||
- Дерево материала: `936599df3d798fa91322dbb8cffe52a045684dc3`
|
||||
```
|
||||
git log --all --format='%H %T' | grep 936599df3d79
|
||||
```
|
||||
- Тело issue: `262503aba6ef449b2c9695754329d6261461bb6e2950e662179378c9299879f8`
|
||||
- Вердикт конвейера: `green` · High 0
|
||||
Reference in New Issue
Block a user