From 0de14ead71434fd254e328201e42c1fcb46c3c04 Mon Sep 17 00:00:00 2001 From: "claude[bot]" <209825114+claude[bot]@users.noreply.github.com> Date: Thu, 17 Sep 2026 20:59:57 +0000 Subject: [PATCH] docs: review document for #571 Issue: #571 User-Visible: no --- docs/reviews/CODE-REVIEW-571-r1.md | 175 +++++++++++++++++++++++++++++ 1 file changed, 175 insertions(+) create mode 100644 docs/reviews/CODE-REVIEW-571-r1.md diff --git a/docs/reviews/CODE-REVIEW-571-r1.md b/docs/reviews/CODE-REVIEW-571-r1.md new file mode 100644 index 00000000..59777fde --- /dev/null +++ b/docs/reviews/CODE-REVIEW-571-r1.md @@ -0,0 +1,175 @@ +# CODE-REVIEW — issue #571, заход r1 + +Материал: `f77bdaf52eb1f5b8cec205d55eaf58e17e3ba288`, ветка `issue/571-golden-capture-provenance`, +два коммита на `origin/dev` (`3f95dac4`, `f77bdaf5`). Инфраструктурный трек: класс A +(`src/**`, `custom_components/houseplan/**/*.py`, манифесты) не тронут ни строкой — +проверено `git diff origin/dev...HEAD --name-only` с фильтром по этим путям, список пуст. + +## Скоуп + +Проблема из аудита: `demo/golden/accept.mjs` не знал, на какой платформе сняты кадры, +и записывал в `baselines-index.json` **свою** платформу под именем `platform` — +что и дало на `ad4000f9` ложь `"platform": "win32"` у кадров Linux-прогона +34853080375. Задача переносит провенанс съёмки в отчёт и разводит `capturedOn`/ +`acceptedOn` в индексе. Продуктового поведения не меняет, `docs/CHANGELOG*` не +трогает — согласуется с `User-Visible: no` в обоих коммитах. + +## Как проверялось + +1. Прочитан весь diff (`git diff origin/dev...HEAD`) по файлам: + `scripts/capture-environment.mjs`, `demo/golden/policy.mjs`, `demo/golden/accept.mjs`, + `demo/golden/run.mjs`, `scripts/mutation-registry.mjs`, + `test/capture-environment.test.mjs`, `test/golden-capture-provenance.test.mjs`, + `docs/images/screenshots.json`, бандл (`dist/**`, `custom_components/houseplan/frontend/**`). +2. Прослежена логика `accept.mjs` построчно для всех пяти сценариев приёмки из + тела issue (см. таблицу ниже) — доказательство чтением плюс сверка с + автотестами, которые эти же сценарии исполняют. +3. **Тесты не просто прочитаны — прогнаны лично**: + `node --test --test-name-pattern="#571" test/golden-capture-provenance.test.mjs` + → `7 pass / 0 fail`. Тесты запускают настоящий `accept.mjs` в изолированном + каталоге эталонов (`--baselines=`), не подменяют модуль. +4. **Дисциплина «тест умеет падать» проверена сама, не со слов автора.** Вручную + внесены обе мутации из `scripts/mutation-registry.mjs` (сохранял и + восстанавливал файлы, рабочее дерево осталось чистым — `git status --short` + пуст после проверки): + - `demo/golden/accept.mjs`: `capturedOn: capturedOn,` → `capturedOn: acceptance.platform,` — + упали 2 из 7 тестов (AC1 и «чужая среда с причиной»); + - `scripts/capture-environment.mjs`: fail-closed throw в `reportCaptureProvenance` + заменён на `return { provenance: null, legacy: true }` — упал целевой тест + `#571 схема 2 fail-closed…`. + Оба мутанта убиты, ровно теми тестами, что названы в `guard` соответствующей + записи реестра. +5. Сверены неизменные инварианты: `imageSha256` во всех 11 записях + `docs/images/screenshots.json` не менялся (`git diff ... | grep -E '^[+-].*imageSha256'` + пусто) — изменился только `sourceFingerprint`/`sourceSha256`, что подтверждает + заявление «пересъёмка не потребовалась, картинки байт-в-байт совпали». + Аналогично не менялись `demo/golden/baselines/**` (нет ни одного файла в диффе) — + PNG эталоны не трогались, поэтому трейлеры `Release:`/`Baseline-Reviewed:` не + требуются. +6. `node scripts/smoke-select.mjs --base origin/dev --head HEAD` → + «Исполняемого frontend-диффа нет… Browser-smoke этим диффом не выбираются». + Смоки не прогонялись — выбирать нечего, диффа в `src/**` нет. +7. Проверено, что `docs-accept.mjs` (приёмка скриншотов документации) не задета + и не несёт аналогичного дефекта: там нет поля `capturedOn`, есть только + `acceptedOn` (платформа приёмщика, честно так и названная) — предположение + автора о возможном родственном разрыве не подтвердилось, отдельный issue не + нужен. +8. Проверены трейлеры обоих коммитов (`git log`): `Issue: #571`, + `User-Visible: no` — оба присутствуют, changelog не тронут, что для `no` + корректно. + +### Сценарии приёмки из issue — прослежены по коду + +| # | Сценарий | Где в коде | Вердикт | +|---|---|---|---| +| 1 | Linux-отчёт принят на Windows с причиной: `capturedOn=linux`, `acceptedOn=win32`, причина в манифесте | `accept.mjs:45-75`, цикл по `[capturedOn, acceptance.platform]` + `foreignAllowed` | подтверждено тестом `#571 AC1: чужая среда съёмки с причиной` | +| 2 | Тот же сценарий без причины — отказ до записи | тот же цикл, `throw` до `mkdirSync(baselineRoot)`/`writeFileSync` | подтверждено тестом `#571 AC2`, включая проверку «индекс не тронут» | +| 3 | Same-platform без override, обе стороны корректны | `foreignAllowed` вычисляется как `null`, когда обе платформы — канон | подтверждено тестом `#571 AC1: артефакт Linux принимается` | +| 4 | Тампер report/PNG и неполный артефакт — fail-closed | не менялось (`sourceFingerprint`-проверка и `goldenAcceptanceRefusal`) | подтверждено тестом `#571 подмена PNG и неполный артефакт` | +| 5 | `npm test`; адресный тест policy/acceptance | см. п.3 выше плюс общий Validate | зелёный прогон на этом SHA (см. ниже) | + +## Гейты — что прогнано, что нет и почему + +- **Не прогонял заново** `npx tsc --noEmit`, `npm test` (полный), `npm run build` + со сверкой бандла: Validate на `f77bdaf5` уже зелёный + ([run 35270925670](https://github.com/Matysh/houseplan-card/actions/runs/35270925670)) — + сошлись на этом прогоне как источнике для этих трёх гейтов. +- **Прогнал сам, адресно**: `node --test --test-name-pattern="#571" test/golden-capture-provenance.test.mjs` + (7/7 pass) плюс обе ручные мутации (см. выше) — потому что дисциплина + «тест умеет падать» касается именно тех тестов, на которые опирается + вердикт, а не всего набора. +- **`node scripts/check-docs.mjs --strict`** — не перепрогонял: diff не трогает + `src/**` (условие явно снимает обязательность), и та же проверка уже часть + зелёного Validate на `f77bdaf5`. Косвенно подтверждено чтением: `imageSha256` + не изменился ни для одной из 11 сцен, значит `check-docs` не мог покраснеть + из-за визуальных данных. +- **Инварианты модели** (`npm run invariants`) — не запускал: diff не трогает + геометрию, `layout`, `marker.space`, `open_spans` или толщину стен. Неприменимо. +- **Browser-smokes** (`demo/smoke_*.mjs`) — не прогонял: `smoke-select.mjs` + подтвердил отсутствие исполняемого frontend-диффа (`src/**` не тронут). +- **`npm run golden:verify` / golden-джоба** — не прогонял и не требовалось: + PNG-эталоны не менялись, изменения — в контракте приёмки, покрытом + собственными юнит-тестами, реально исполняющими `accept.mjs`. Автор + корректно отметил, что реальная приёмка после мержа сама запишет + честный индекс схемы 2. +- **`python -m pytest tests_backend`** — не прогонял: `custom_components/**/*.py` + не тронут. +- **Perf-профили** — неприменимо, в AC не названы и код не тронут. +- **`mutation-gate --check`** — не перепрогонял целиком (автор указал 0 FAIL + в хендоффе), но сами два новых мутанта проверил вручную (см. выше) — + это и есть содержательная часть гейта для этой задачи. + +## Проверено и корректно + +- Провенанс собирается там, где снимаются кадры (`captureProvenance` в + `scripts/capture-environment.mjs`, вызывается из `demo/golden/run.mjs`), + а не выводится приёмщиком из своего `process.platform` — ровно то, что + требует AC2 issue. +- `reportCaptureProvenance` — честный fail-closed: схема ≥2 без `capture` + или без `capture.platform` бросает исключение с явной причиной; схема <2 — + отдельная явная ветка `legacy: true`, `capturedOn` уезжает `null`, а не + угадывается. Мутационно проверено. +- Отказ по чужой среде происходит **до** любой записи в индекс — `throw` + расположен раньше `mkdirSync(baselineRoot)` и `writeFileSync(manifestPath)` + по тексту файла; тест `#571 AC2` дополнительно сверяет байт-в-байт, что + индекс не изменился. +- Причина осознанного обхода (`HP_ALLOW_FOREIGN_CAPTURE`) едет в + `foreignCapture.reason` индекса, а не только в stdout — закрывает второй + провал из аудита («причина осталась в stdout»). +- `indexCapturedOn()` читает обе схемы одним правилом и не путает + `platform` (схема 1) с `capturedOn` (схема 2) — юнит-тест перебирает все + четыре случая, включая «схема 2 без `capturedOn`» → `null`, а не откат + к чужому полю. +- Отмена запрета «`run.mjs` не трогать» (#455) внесена и оплачена в той же + ветке: бандл пересобран (класс D, `dist/**` и + `custom_components/houseplan/frontend/**` в диффе), индекс скриншотов + документации переснят вторым коммитом с сохранением всех 11 `imageSha256` + — не переложено на будущую задачу. +- Свидетели гоняют настоящий `accept.mjs`, а не мок: `--baselines=` + сделан ровно для того, чтобы «отказ до записи» была исполнимым + утверждением, а не декларацией. +- Трейлеры `Issue:`/`User-Visible: no` на обоих коммитах корректны, + changelog не тронут, что для `no` ожидаемо. + +## Чего не проверял (и почему это не блокирует) + +- Полный `npm test` / `tsc` / `build` лично не гонял — Validate на этом SHA + уже зелёный, дублирование было бы потерей времени без нового сигнала. +- Реальную golden-приёмку в CI (запись индекса схемы 2 «в бою») не + выполнял — она в скоуп задачи и не входит, и юнит-тесты покрывают тот же + код путём, включая изоляцию от рабочего дерева. +- Интеграционный путь `witnessCheck.refusal` → `environmentNote` в + `accept.mjs` (строки с `previousIndex`/`indexCapturedOn(previousIndex)`) + проверен только чтением, не исполнением: специального теста, гоняющего + именно отказ по свидетелям с последующей запиской о среде, в диффе нет. + Логика — прямой перенос имён (`environment.platform` → `capturedOn`, + `manifest.platform` → `indexCapturedOn(previousIndex)`) без изменения + формы; читается корректно, но это не доказательство исполнением. Не + выношу как находку: ветка не новая (существовала и до задачи в похожем + виде), риск регрессии низкий, а полноценный тест этого пути стоил бы + отдельной инфраструктуры (два последовательных прогона приёмки с разными + средами) непропорционально размеру задачи. + +## Находки + +Нет ни High, ни Medium. Задача решает ровно проблему из аудита, не +расширяет и не сужает скоуп, все AC подтверждены автотестами, исполненными +лично, две мутации проверены руками поверх заявления автора. + +## Вердикт + +Зелёный. + +--- + + + +## Материал раунда + +- Ветка: `issue/571-golden-capture-provenance`, коммит `f77bdaf52eb1` — ребейз его осиротит, и это нормально: ниже якоря, которые ребейз не меняет. +- Дерево материала: `8c131f1d70153bc7634264d5f83dab0b656088c4` + ``` + git log --all --format='%H %T' | grep 8c131f1d7015 + ``` +- Тело issue: `3ca7e6df6ede172521144a672fefa1a946c0af10242125f62e6b835a399babe0` +- Вердикт конвейера: `green` · High 0