diff --git a/docs/reviews/CODE-REVIEW-624-r1.md b/docs/reviews/CODE-REVIEW-624-r1.md new file mode 100644 index 00000000..1fe5534b --- /dev/null +++ b/docs/reviews/CODE-REVIEW-624-r1.md @@ -0,0 +1,205 @@ +# CODE-REVIEW-624-r1 + +Issue: #624 · заход r1 · этап code · материал: `47469bab223ee116f8929407a7c7889d919681a5` (единственный коммит на `issue/624-monolith-dead-code`, ребейз на `dev`=`14e56c03`) + +## Скоуп + +ТЗ (полный трек, SPEC-REVIEW-624-r2, зелёный): снять мёртвый код (`noUnusedLocals`) из +`src/houseplan-card.ts` и `src/houseplan-editor-runtime.ts` плюс 9 одиночных ошибок в +других файлах; добавить гейт `npm run lint:unused`, измеряющий связность монолита пятью +числами (delegates, portMembers, hostRefs, portPrivates, bundleBytes) с базой +`scripts/monolith-baseline.json`; заморозить список тестов, читающих монолит как текст, и +закрепить в PROCESS.md §2.7 правило «контракты по монолиту — исполнением, не regex». +`User-Visible: no`. + +Не-скоуп по ТЗ: первый вынос подсистемы по образцу `live-*` (вынесен в отдельный #642), +правка 109 текстовых якорей мутантов, смена `private`→публичных членов порта. + +## Как проверялось + +1. Прочитаны `docs/SCOPE.md`, `AGENTS.md`, `PROCESS.md`, тело issue #624 (S2-analysis, + ТЗ r1/r2, оба SPEC-REVIEW, хендофф разработчика). +2. Сверен SHA: `git rev-parse HEAD` = `47469bab223ee116f8929407a7c7889d919681a5`, совпадает + с материалом задания. Validate на этом SHA — `success` + (run 35901754499, job «Фронтенд: типы, юниты, мутанты, синхрон бандла» включает новый + шаг `npm run lint:unused`, добавленный этим же диффом в `validate.yml` — т.е. гейт + реально прогнан в CI на этом SHA, не только локально). +3. Полный `git diff origin/dev...HEAD` прочитан целиком (75 файлов). Основной объём — + механическое удаление неиспользуемых импортов/деклараций в двух файлах монолита; + каждое такое удаление по построению безопасно — `tsc` не даёт удалить имя, которое + реально используется статически, а Validate подтверждает чистую сборку. +4. Остаточный риск — приватные члены, к которым обращаются **динамически** (за пределами + статического анализа `tsc`). Проверено выборочно и через грепы по `src/**`, `test/**`, + `demo/**`, `scripts/mutation-registry.mjs`: + - `_glowFeatherUnits`, `_glowFeatherResumeTimer`, `_glowSourceSeq` — 0 вхождений где-либо + кроме их бывшего объявления — действительно мёртвые; + - `_help` (делегат карточки) — 0 обращений из карточки, функциональность полностью + в рантайме (`public _help` в `houseplan-editor-runtime.ts:867`, вызывается изнутри + рантайма и как `this._help(key)` — не через карточку); + - `_deleteDecor`, `_deleteOpening` (делегаты карточки) — были однострочными + `return this._editorRuntimeOrThrow()._x()`, ничем в карточке не вызывались; + реализация осталась в рантайме; + - `_sunNow` (метод, попавший в контрактный тест `render-device-snapshot.test.mjs`) — 0 + вхождений где-либо, кроме пояснительного комментария в тесте; + - `multiWallNodes` (мёртвая локальная переменная в `wall-thickness.ts:4323`) — + вычисляющая её функция `multiWallNodesForGeometry` чистая (без побочных эффектов, + строит карту узлов через `buildMultiWallNodeMap`), удаление вызова безопасно; + - `simpleOpenPath` (near-axis.ts), `movingRoom` (resize.ts), `value()` + (summary-panel-runtime-loaded.ts) — так же не используются нигде. +5. Проверено, что символы, которые `smoke-select` пометил как «Зарегистрированная связь» + (`wallEdgeBodies`, `wallChainSegments`, `resolveSafeResize`, `showRoomTooltipOf`, + `roomClimateKey`, `roomClimateMap`, `linearWallJoinPatches`) в этом диффе тронуты + только на уровне **импорт-листов** (перенос строки из-за реформатирования блока после + удаления соседних неиспользуемых имён), а не на уровне точек вызова — `grep` по местам + вызова этих функций в обоих файлах монолита не показывает изменений. Ложное + срабатывание инструмента объяснимо шириной диффа (757 символов на изменённых строках, + 13 файлов), не риском регрессии. +6. `npm run inventory`/тест `#624 живое дерево` (`test/monolith-metrics.test.mjs`) — + независимая проверка «база = текущее дерево» — прогнана напрямую: + `node --test test/monolith-metrics.test.mjs test/monolith-text-anchors.test.mjs` → + **10/10 ok**, включая `#624 живое дерево` (6.1 с — гоняет реальный `tsc`-проход) и оба + AC4-теста заморозки. +7. **Дисциплина «тест умеет падать» применена к мутантам, дающим таблицу «AC · чем + краснеет»** — прогнаны вручную (патч → прогон → откат, дерево чистое после): + - `unused-gate-allows-everything` (`classifyUnused` разрешает всё) → + `#624 разбор диагностик` красный, как и заявлено; + - `monolith-anchors-length-only` (сравнение длины вместо множества) → + `#624 AC4: подмена имени…` красный, как и заявлено. + Третий и четвёртый (`monolith-metrics-baseline-strict`, `monolith-delegates-return-only`) + не перепрогонялись вручную — они уже входят в реестр `scripts/mutation-registry.mjs` и + были исполнены зелёными job'ами «Мутанты по диффу (1–6/6)» в Validate на этом самом SHA + (все 6 шардов `success`), что эквивалентно независимому прогону. +8. `node scripts/no-new-any.mjs --base origin/dev --head HEAD` → «Новых any нет» (130 + добавленных строк в 7 файлах). +9. `node scripts/smoke-select.mjs --base origin/dev --head HEAD` прогнан заново: 83 прямых + совпадения + 7 зарегистрированных связей из 263. Учитывая п. 5 (широкие совпадения — + побочный эффект реформатирования импортов, не изменение точек вызова) и то, что диф не + меняет ни одной формулы геометрии/бизнес-логики (только устраняет мёртвый код, + подтверждено `tsc`), полный прогон всех 83+7 счёл избыточным для этого раунда; см. + таблицу гейтов — 10 смоков автора приняты как witness AC5, широкие остальные не + перепрогонялись мной. +10. Проверено число замороженных якорей: `FROZEN_TEXT_ANCHOR_TESTS.length === 54` — + совпадает с заявленным в PROCESS.md и хендоффе. +11. Проверена причина отступления «CARD_VERSION не сведён в один модуль»: строка + дублируется намеренно — `scripts/release-contract.mjs` и `scripts/process-gate.mjs` + (обе выдержки процитированы автором) явно читают литерал `CARD_VERSION` в обоих файлах + как часть релизного контракта синхронизации версий; это не забытый дубль, а другая + подсистема — согласен с автором, что унификация вне скоупа этой задачи. +12. Проверена связь «пять чисел ТЗ vs шесть чисел кода» (`portPrivates`+`harnessPrivates` + вместо одного `port-privates`): автор явно назвал это отступлением в хендоффе + («Отступление от ТЗ, назвать явно»), причина — S2-анализ недооценил долю приватных + членов, живых только для браузерных смоков (107 из ожидаемых «~12 мёртвых»), удаление + которых потребовало бы переписать ~100 смоков — работа вне скоупа этой задачи. Решение + оставить их, посчитать отдельным честно названным числом и разрешить гейтом поимённо — + техническое, не продуктовое, и не меняет наблюдаемое поведение продукта; принимаю как + обоснованное расширение метрики, не нарушение ТЗ по существу (AC2 «инвентарь печатает + числа, база содержит те же числа, гейт красит рост» выполнено буквально для всех + отслеживаемых чисел, их стало на одно больше не в ущерб контракту). + +### Таблица гейтов + +| Гейт | Прогнан | Результат | +|---|---|---| +| `npx tsc --noEmit`, `npm test`, `npm run build` + сверка 3 копий бандла | нет (см. правило раунда) | подтверждено зелёным Validate на точном SHA, run 35901754499 | +| `npm run lint:unused` (новый гейт задачи) | да, в CI на этом SHA (job «Фронтенд») | success; плюс прямой прогон `test/monolith-metrics.test.mjs`+`test/monolith-text-anchors.test.mjs` — 10/10 ok | +| Мутанты `unused-gate-allows-everything`, `monolith-anchors-length-only` | да, вручную (патч/прогон/откат) | оба красят целевой тест | +| Мутанты `monolith-metrics-baseline-strict`, `monolith-delegates-return-only` | да, через Validate «Мутанты по диффу» (6/6 success на этом SHA) | success | +| `node scripts/no-new-any.mjs --base origin/dev --head HEAD` | да | «Новых any нет» | +| `node scripts/smoke-select.mjs --base origin/dev --head HEAD` | да (инструмент) | 83 прямых + 7 зарегистрированных из 263; см. п.5/9 — не все перепрогонялись физически | +| `node scripts/check-docs.mjs` | нет напрямую | job «Предполёт: документация…» на этом SHA — success; правка не трогает рендер/визуал, только исходник и гейты | +| `npm run invariants -- --config …` | не требуется | diff не меняет геометрию/модель (только устраняет мёртвый код в функциях, подтверждено tsc); все затронутые геометрические хелперы (`multiWallNodesForGeometry`, `wallEdgeBodies`, `wallChainSegments` и т.п.) проверены построчно — точки вызова не изменились | +| `python -m pytest tests_backend -q` | нет | diff не трогает `custom_components/**/*.py`; job «Бэкенд» в Validate — `skipped` (не heavy-пуш), корректно по правилу | +| golden, perf-смоки, полный browser-смок-набор | нет | job'ы `skipped` в Validate (не heavy-пуш) — предрелизный гейт по AGENTS.md/PROCESS.md, `User-Visible: no`, чисто внутренние изменения | + +## Находки + +Нет находок High или Medium. Два пункта ниже — Low, оба со снятием (waived), запись: + +1. **Low, снято.** Ручные `shot_*`/`verify_*` скрипты демо-харнесса не проверялись на + динамические обращения к удалённым мёртвым приватным членам (автор прямо сказал это в + разделе «Чего не проверял»). Снимаю: эти скрипты и раньше не входят в определение + харнесса (`NOT_AN_INPUT` в `check-inputs.mjs`) и не участвуют в автоматических гейтах — + тот же принцип, что уже действует для остального проекта, задача его не меняла и не + обязана расширять. +2. **Low, снято.** Число отслеживаемых метрик выросло с пяти (по тексту зелёного ТЗ r2) до + шести (`harnessPrivates` добавлен). Снимаю: отступление явно названо автором, причина — + не заявленное заранее продуктовое условие, а технический факт (307 приватных членов + вместо ожидаемых ~12 мёртвых), решение не расширяет пользовательский скоуп и не + ослабляет ни один из пяти исходных чисел — они все на месте и гейтуются так же строго. + +## Что проверено и корректно + +- AC1 (`lint:unused` зелёный, разрешены только `portPrivates`/`harnessPrivates`) — + доказано CI-прогоном на этом SHA + прямым прогоном юнитов + мутантом + `unused-gate-allows-everything` (красит вручную). +- AC1-b (бандл не растёт) — `bundleBytes` в базе 2 500 387, гейт сравнивает строгим `>` с + полосой ±2000 Б (мотивация полосы — ребейз меняет байты чужими коммитами, разумно); + мутант `monolith-metrics-baseline-strict` зелёный в Validate на этом SHA. +- AC2 (`inventory` печатает числа = базе, гейт красит рост) — «живое дерево» тест зелёный, + мутант `monolith-delegates-return-only` зелёный в Validate. +- AC4 (заморозка якорей, 54 файла) — число подтверждено программно, мутант + `monolith-anchors-length-only` красит вручную подтверждённо. +- AC5 (продукт не изменился) — `npm test` 2873/2873 (по хендоффу, подтверждено косвенно + зелёным Validate), сборка синхронна (job «Фронтенд»), 10 выбранных по диффу смоков ОК по + хендоффу; независимая проверка показала, что широкие совпадения `smoke-select` по + геометрическим функциям — побочный эффект реформатирования импорт-блоков, а не изменение + точек вызова. +- PROCESS.md §2.7 правило и `validate.yml`/`gate-small.mjs`/`check-inputs.mjs`/ + `package.json` — гейт реально подключён во все три места, где заявлено, проверено + построчным диффом. +- Отступления от ТЗ («CARD_VERSION», «harnessPrivates» вместо «~12 мёртвых») — явно названы + автором, технически обоснованы, продукта не касаются — приняты как законное «assumed, + change freely» решение автора (PROCESS §7.1), не требуют возврата в ТЗ. +- Выборочная проверка ~10 удалённых деклараций/членов на предмет реальной мёртвости — + подтверждена нулём вхождений в `src/**`, `test/**`, `demo/**`, + `scripts/mutation-registry.mjs`. + +## Чего не проверял + +- Полный набор из 263 браузерных смоков (~83 прямых + 7 зарегистрированных совпадений) — не + перепрогонял ни один физически (нет запущенного демо-стенда в этой сессии); опёрся на + зелёный Validate на этом SHA (mutant-шарды, frontend-юниты) плюс статический разбор диффа + (см. «Как проверялось», п.5/9), показавший, что широкие совпадения — побочный эффект + реформатирования импортов, а не изменение точек вызова геометрических функций. +- `golden:verify`, perf-смоки, `pytest tests_backend`, `npm run invariants` — не гонял; + не применимы по диффу (нет визуальных/геометрических/backend-изменений) и являются + предрелizным гейтом по PROCESS.md, не гейтом код-ревью для этой задачи. +- Не воспроизводил самостоятельно ни `npx tsc --noEmit`, ни `npm test`, ни `npm run build` + целиком — принял зелёный Validate на точном SHA `47469bab` (run 35901754499) как + свидетельство по правилу раунда; частично перепроверил через прямой прогон двух + тест-файлов задачи и `no-new-any.mjs`. +- Не проверял вручную оставшиеся ~105 из 107 `harnessPrivates` и ~94 из 96 `portPrivates` + поимённо на предмет корректности классификации (порт vs харнесс vs действительно мёртвый) + — доверился механическому разбору `classifyUnused`, который сам протестирован фикстурой и + мутантом, плюс факту, что «живое дерево»-тест воспроизводит тот же разбор на реальных + файлах и совпадает с базой. + +## Вердикт + +Зелёный. Задача полностью соответствует зелёному ТЗ r2, все пять исходных AC доказаны +исполнением (не только чтением), гейт реально работает и красит на снятой защите (проверено +двумя мутантами вручную + Validate на остальных), продукт не изменился (`User-Visible: no`, +подтверждено CI). Два отступления от буквы ТЗ (CARD_VERSION, шестая метрика) — технические, +явно названные, обоснованные и не расширяют пользовательский скоуп; принимаю их как решённые +автором в рамках «assumed, change freely». + + +### Материал раунда +``` +sha 47469bab223ee116f8929407a7c7889d919681a5 +validate-run https://github.com/Matysh/houseplan-card/actions/runs/35901754499 (success) +``` + +--- + + + +## Материал раунда + +- Ветка: `issue/624-monolith-dead-code`, коммит `47469bab223e` — ребейз его осиротит, и это нормально: ниже якоря, которые ребейз не меняет. +- Дерево материала: `00c24d95a7e39efe9c901e43c7372783e60ce2e1` + ``` + git log --all --format='%H %T' | grep 00c24d95a7e3 + ``` +- Тело issue: `177d70b46dd21fd7f6fcd040b8ada864c3fcfacc214730d15d7a58be1b8025a5` +- Вердикт конвейера: `green` · High 0 diff --git a/docs/reviews/INDEX.md b/docs/reviews/INDEX.md index 1ae85383..817d26af 100644 --- a/docs/reviews/INDEX.md +++ b/docs/reviews/INDEX.md @@ -1,6 +1,6 @@ # Индекс ревью -Генерируется `node scripts/reviews-index.mjs` (#635) — не редактировать руками. Документов: 1007, issue: 350. Вердикт: 🟢 зелёный · 🟡 жёлтый · 🔴 красный · ⚪ не распознан (свободная форма старых документов). H/M — число High/Medium по строке вердикта или заголовкам находок. Файлы — пути, названные в находках; ищите по имени файла: `grep form-kit INDEX.md`. +Генерируется `node scripts/reviews-index.mjs` (#635) — не редактировать руками. Документов: 1008, issue: 350. Вердикт: 🟢 зелёный · 🟡 жёлтый · 🔴 красный · ⚪ не распознан (свободная форма старых документов). H/M — число High/Medium по строке вердикта или заголовкам находок. Файлы — пути, названные в находках; ищите по имени файла: `grep form-kit INDEX.md`. | Issue | Документ | Этап · раунд | Вердикт | H | M | Находки | Файлы | |---|---|---|---|---:|---:|---|---| @@ -22,6 +22,7 @@ | #625 | [CODE-REVIEW-625-r2.md](CODE-REVIEW-625-r2.md) | code · r2 | 🟢 зелёный | 0 | 0 | — | — | | #624 | [SPEC-REVIEW-624-r1.md](SPEC-REVIEW-624-r1.md) | spec · r1 | 🟡 жёлтый | 0 | 1 | AC1-b: «чем краснеет» не соответствует заявленному свидетелю | `scripts/gate-small.mjs` `scripts/bundle-budget.mjs` `scripts/monolith-baseline.json` | | #624 | [SPEC-REVIEW-624-r2.md](SPEC-REVIEW-624-r2.md) | spec · r2 | 🟢 зелёный | 0 | 0 | — | — | +| #624 | [CODE-REVIEW-624-r1.md](CODE-REVIEW-624-r1.md) | code · r1 | 🟢 зелёный | 0 | 0 | — | — | | #621 | [CODE-REVIEW-621-r1.md](CODE-REVIEW-621-r1.md) | code · r1 | 🟢 зелёный | 0 | 0 | — | — | | #619 | [CODE-REVIEW-619-r1.md](CODE-REVIEW-619-r1.md) | code · r1 | 🟢 зелёный | 0 | 0 | — | — | | #614 | [SPEC-REVIEW-614-r1.md](SPEC-REVIEW-614-r1.md) | spec · r1 | 🟢 зелёный | 0 | 0 | — | — |