diff --git a/docs/reviews/CODE-REVIEW-576-r1.md b/docs/reviews/CODE-REVIEW-576-r1.md new file mode 100644 index 00000000..75933142 --- /dev/null +++ b/docs/reviews/CODE-REVIEW-576-r1.md @@ -0,0 +1,85 @@ +# CODE-REVIEW-576-r1 + +**Issue:** [#576 — Owner toolchain: фактически включить pinned Windows/WSL setup и получить зелёный verify](https://github.com/Matysh/houseplan-card/issues/576) +**Материал:** ветка `issue/576-owner-toolchain-adoption`, коммит `810061d5f85082546bfa400fb516bce6ff2f9284` (HEAD, детач-чекаут ровно на нём) +**Заход:** r1 · блокирующих циклов израсходовано 0 из 4 +**Класс:** инфраструктура — ни одного файла класса A в диффе (проверено `git diff origin/dev...HEAD --stat`: `AGENTS.md`, `docs/STATUS.md`, `scripts/gate-small.mjs`, `test/editor-dialog-modules.test.mjs`, `test/gate-small.test.mjs`, `test/golden-capture-provenance.test.mjs`). Ускоренный вход в `S7-code-review` применён корректно, ТЗ и его ревью не требовались. + +## Скоуп + +Задача — не имплементация фичи, а фактическая приёмка pinned Windows/WSL toolchain (#557) на машине владельца: выполнить реальные прогоны на Windows entrypoint и в WSL ext4-клоне и зафиксировать точные команды/версии/exit-коды. По ходу первого прогона на Windows обнаружились и здесь же исправлены три инфраструктурные причины ложного красного результата: + +1. `gate:small` гонял `npm test` параллельно со сборкой, хотя часть unit-тестов (`test/bundle-assets.test.mjs`) читает файлы из `dist/**`, который `build` в этот момент пересоздаёт — гонка чтения/записи. +2. `test/golden-capture-provenance.test.mjs` вызывал POSIX `cp -r`, которого нет на нативном Windows. +3. `test/editor-dialog-modules.test.mjs` строил `file:`-URL из Windows-пути (`C:\...`) через `new URL(file, new URL(ROOT, 'file:'))` — некорректная конструкция URL на бэкслэшах. + +Диф также обновляет `AGENTS.md` и `docs/STATUS.md`, фиксируя фактический результат приёмки — документация класса C в том же коммите, как и предписано. + +## Как проверялось + +Материал уже на пинованном SHA `810061d5` — `git fetch`/`checkout` не выполнялись. + +| Гейт | Статус | Как учтён | +|---|---|---| +| `npx tsc --noEmit`, `npm test`, `npm run build` + сверка бандла | не перегонял полностью | Validate на этом SHA зелёный (run [35461062755](https://github.com/Matysh/houseplan-card/actions/runs/35461062755)), проверено `gh run view 35461062755 --json status,conclusion,headSha` → `completed`/`success`/`810061d5…` — тот же SHA, что HEAD | +| `node --test` по трём затронутым test-файлам | прогнал | `node --test test/gate-small.test.mjs test/editor-dialog-modules.test.mjs test/golden-capture-provenance.test.mjs` → 7 + 14 тестов, все `pass`, 0 `fail` | +| Мутация reorder-фикса | прогнал | временно вернул `scripts/gate-small.mjs` на версию `origin/dev` (старый параллельный `npm test`, без экспорта `postBuildSteps`) и повторно прогнал `test/gate-small.test.mjs` → `SyntaxError: does not provide an export named 'postBuildSteps'`, тест падает; вернул файл в исходное состояние, `git status` после — чисто | +| `node scripts/smoke-select.mjs --base origin/dev --head HEAD` | прогнал | «Исполняемого frontend-диффа нет (`src/**/*.ts` не тронут)» — смоки этим диффом не выбираются, прогон браузерных смоков не нужен | +| `node scripts/check-docs.mjs` | не запускал | diff не трогает `src/**` — условие запуска не выполнено | +| `golden:verify`, `pytest tests_backend`, `model-invariants`, perf-профили | не запускал | diff не меняет рендер/визуал/геометрию/Python/производительность — ни один триггер не сработал | +| Чтение `scripts/windows-toolchain.ps1`, `scripts/wsl-setup.sh` (не изменены этим диффом) | проверено чтением, не исполнением | подтверждает точность формулировок из комментария владельца (AC5, см. ниже) | + +## Находки + +Не найдено High/Medium/Low ни в скоупе, ни вне него. + +## Что проверено и корректно + +- **Причина reorder-фикса реальна.** `test/bundle-assets.test.mjs` действительно читает `dist/houseplan-card.js`, `dist/houseplan-panel.js`, `dist/houseplan-assets.json` и т.д. через `readFileSync(new URL('../dist/...', import.meta.url))` — старый параллельный запуск `npm test` рядом со `npm run build` был гонкой за один и тот же каталог. Новый код: `parallelSteps` не содержит `npm test`, `postBuildSteps()[0]` — юниты, и они идут строго после `buildOk`. +- **Тест умеет падать.** Показано выше мутацией: откат `gate-small.mjs` к версии `dev` ломает `test/gate-small.test.mjs` (пропавший экспорт делает падение ещё нагляднее, чем просто неверный порядок, но сам факт, что тест жёстко привязан к новому контракту модуля, подтверждён). +- **Windows-специфичные фиксы точечные и не расширяют скоуп.** `cpSync(BASELINES, sandbox, { recursive: true })` заменяет `execFileSync('cp', ['-r', BASELINES + '/.', sandbox])` — семантика «скопировать содержимое каталога» сохранена (проверено прогоном всех трёх `#571 AC*` тестов — 14/14 `pass`, включая ветки с `foreignCapture`). `resolve(ROOT, file)` вместо URL-конструктора устраняет платформенно-зависимую сборку пути; сам тест `#592` по-прежнему проходит. +- **`HOST_TEST_ALLOWANCE` в `golden-capture-provenance.test.mjs` не ослабляет проверку.** Прочитан `demo/golden/accept.mjs`: `foreignAllowed` срабатывает и когда `capturedOn` (платформа кадров из отчёта) отличается от канона, и когда `acceptance.platform` (платформа **исполнения** приёмки) отличается — то есть даже фикстура с `platform: 'linux'` требует `HP_ALLOW_FOREIGN_CAPTURE` при запуске самого юнит-теста на Windows/WSL. Новая константа `HOST_TEST_ALLOWANCE` корректно передаёт `#576: unit-тест приёмки из закреплённого Windows toolchain` только на не-Linux хосте теста (`process.platform === 'linux' ? '' : …`), на Linux CI остаётся `''` — поведение теста на каноничном раннере не меняется, что подтверждает и зелёный Validate на этом SHA. +- **Трейлеры и changelog.** Единственный коммит несёt `Issue: #576` и `User-Visible: no` — верно, продуктового поведения нет, changelog не требуется. +- **AC по тексту issue:** + - AC1 (точные команды/версии/exit-коды в комментарии, не «verified» без вывода) — выполнен: комментарий владельца перечисляет каждую команду, время выполнения и exit-код для обоих окружений. + - AC2 (pinned entrypoint больше не берёт системные Node 24/Python 3.12) — подтверждено записанными путями (`houseplan-toolchain\node-v22.23.2…`, `.venv-ci\Scripts\python.exe`, Python 3.14.7). + - AC3 (WSL verify: HA subset без skip + Linux capture) — подтверждено (`test_ha_setup.py`, `7 passed`, без упоминания skip; `panel-wide-view-light-en`, `passed`). + - AC4 (идемпотентность повторного запуска) — подтверждено (`NODE_REUSED=True`/`PYTHON_REUSED=True`/`CHROMIUM_REUSED=True` на Windows; «Checked 6 packages in 27ms» на повторном WSL verify). + - AC5 (fail-closed с понятной инструкцией при отсутствии machine-level компонента) — проверено чтением необязанных этим диффом `scripts/windows-toolchain.ps1:98` (`throw 'uv is required. Install it once with: winget install --id astral-sh.uv --source winget'`) и `scripts/wsl-setup.sh:30` (сообщение про `bash scripts/wsl-setup.sh`) — текст комментария владельца дословно совпадает с кодом. + - AC6 (Linux exact-SHA CI остаётся каноном) — не оспаривается, зафиксировано и в комментарии, и в `AGENTS.md`/`PROCESS.md` без изменений канона. + + Все шесть AC этой задачи по своей природе — доказательство исполнения на реальном железе владельца (Windows/WSL), а не автотестом в CI: это прямо следует из формулировки issue («Комментарий содержит точные команды … "проверено" без вывода недостаточно»). Ревьюер не переисполняет эти прогоны (не имеет доступа к машине владельца) и оценивает их по критерию AC1 — названы точные команды и результат, а не голословное заявление. Это ровно тот случай, где записанная эволюция «выполнено и запротоколировано» — единственный применимый вид доказательства. + +## Чего не проверял + +- Не запускал `.\scripts\windows-toolchain.ps1` и `bash scripts/wsl-setup.sh --verify` сам — ревью идёт в Linux CI-окружении без доступа к Windows/WSL машине владельца; полагаюсь на записанные в комментарии точные команды, версии путей и exit-коды (AC1 это прямо разрешает). +- Не переисполнял `npx tsc --noEmit` / `npm test` (полный) / `npm run build` — заменено зелёным Validate на этом самом SHA (см. таблицу гейтов), точечный `node --test` по изменённым файлам прогнан отдельно. +- `check-docs.mjs`, `golden:verify`, `pytest tests_backend`, `model-invariants`, perf-профили — не запускал: ни один триггер (`src/**`, визуал, Python, геометрия, названное в AC влияние на перф) не задет этим диффом. +- Не проверял поведение `windows-toolchain.ps1`/`wsl-setup.sh` при отсутствующем `uv`/`nvm` исполнением (это не изменено данным диффом) — только чтением, что зафиксировано в разделе AC5 выше. + +## Вердикт + +Зелёный. Три точечных инфраструктурных исправления корректны, подтверждены (для reorder-фикса — мутацией с падением теста), тесты для остальных двух фиксов зелёные и осмысленно проверяют новый контракт. Документация синхронна с кодом. Продуктовый код не затронут, changelog не требуется. AC доказаны в мере, применимой к их природе (протокол реального прогона на машине владельца). + +--- + +## Материал раунда + +- SHA: `810061d5f85082546bfa400fb516bce6ff2f9284` (= `origin/dev...HEAD` голова на момент ревью) +- Дерево: рабочая копия проверена `git status` — чисто, без незакоммиченных изменений +- Диапазон: `git diff origin/dev...HEAD` — 6 файлов, +33/-21 +- Validate: run `35461062755`, `completed`/`success`, `headSha=810061d5f85082546bfa400fb516bce6ff2f9284` + +--- + + + +## Материал раунда + +- Ветка: `issue/576-owner-toolchain-adoption`, коммит `810061d5f850` — ребейз его осиротит, и это нормально: ниже якоря, которые ребейз не меняет. +- Дерево материала: `a192995bf538c4d384a9c18f5c61343bdb46cf50` + ``` + git log --all --format='%H %T' | grep a192995bf538 + ``` +- Тело issue: `3a2f2dde461f156726c441e9c48594bcf8bf30d06809ed86fc59804f48086c5d` +- Вердикт конвейера: `green` · High 0