diff --git a/docs/reviews/INDEX.md b/docs/reviews/INDEX.md index 9a800244..d8494950 100644 --- a/docs/reviews/INDEX.md +++ b/docs/reviews/INDEX.md @@ -1,6 +1,6 @@ # Индекс ревью -Генерируется `node scripts/reviews-index.mjs` (#635) — не редактировать руками. Документов: 208, issue: 101. Вердикт: 🟢 зелёный · 🟡 жёлтый · 🔴 красный · ⚪ не распознан (свободная форма старых документов). H/M — число High/Medium по строке вердикта или заголовкам находок. Файлы — пути, названные в находках; ищите по имени файла: `grep form-kit INDEX.md`. +Генерируется `node scripts/reviews-index.mjs` (#635) — не редактировать руками. Документов: 209, issue: 102. Вердикт: 🟢 зелёный · 🟡 жёлтый · 🔴 красный · ⚪ не распознан (свободная форма старых документов). H/M — число High/Medium по строке вердикта или заголовкам находок. Файлы — пути, названные в находках; ищите по имени файла: `grep form-kit INDEX.md`. | Issue | Документ | Этап · раунд | Вердикт | H | M | Находки | Файлы | |---|---|---|---|---:|---:|---|---| @@ -15,6 +15,7 @@ | #711 | [CODE-REVIEW-711-r1.md](CODE-REVIEW-711-r1.md) | code · r1 | 🟢 зелёный | 0 | 0 | — | — | | #709 | [CODE-REVIEW-709-r1.md](CODE-REVIEW-709-r1.md) | code · r1 | 🟡 жёлтый | 0 | 1 | canon TESTING.md противоречит себе | `docs/TESTING.md` `scripts/smoke-select.mjs` `scripts/pre-push-gate.mjs` `TESTING.md` `process-digests.test.mjs` | | #709 | [CODE-REVIEW-709-r2.md](CODE-REVIEW-709-r2.md) | code · r2 | 🟢 зелёный | 0 | 0 | — | — | +| #707 | [SPEC-REVIEW-707-r1.md](SPEC-REVIEW-707-r1.md) | spec · r1 | 🟡 жёлтый | 0 | 1 | К1/AC1: strings.json заявлен источником риска ux, но класс A его не видит | `strings.json` `change-classes.mjs` `custom_components/houseplan/strings.json` `scripts/change-classes.mjs` `manifest.json` | | #706 | [CODE-REVIEW-706-r1.md](CODE-REVIEW-706-r1.md) | code · r1 | 🟢 зелёный | 0 | 0 | — | — | | #705 | [CODE-REVIEW-705-r1.md](CODE-REVIEW-705-r1.md) | code · r1 | 🟢 зелёный | 0 | 0 | — | — | | #704 | [CODE-REVIEW-704-r1.md](CODE-REVIEW-704-r1.md) | code · r1 | 🟢 зелёный | 0 | 0 | — | — | diff --git a/docs/reviews/SPEC-REVIEW-707-r1.md b/docs/reviews/SPEC-REVIEW-707-r1.md new file mode 100644 index 00000000..dbd237cf --- /dev/null +++ b/docs/reviews/SPEC-REVIEW-707-r1.md @@ -0,0 +1,222 @@ +# SPEC-REVIEW-707-r1 + +Issue: #707 · этап: spec · трек: ask · заход: r1 · блокирующих циклов израсходовано (после этого раунда): 1/4 + +## Скоуп + +#707 — часть разбиения семипунктовой аналитики 29.09 (#707/#726/#727/#728/#729). В этот +issue входят п.1, п.2, п.4 исходного объёма: единое правило риска по изменённым +участкам диффа (К1), единая функция трека/основания/лимита (К2), решение конвейера +на `S7` и заметка ревьюеру show/ask (К3), новые разделы `task-packet.mjs` (К4), +правка канона и конспектов (К5). Продуктового кода задача не трогает (класс A не +затрагивается), `User-Visible: no`. ТЗ живёт в теле issue под `## ТЗ`, комментарии +проверены (аналитика 29.09 и передача на ревью 30.09). + +## Как проверялось + +Ревью текстовое (этап spec, ветки продуктового кода нет — п.12 ТЗ «этап spec, +ветки нет, инфраструктурный дифф → риск пуст» сам это фиксирует). Проверка шла в +три слоя: + +1. Обязательные разделы §7.1 и однозначность каждого AC — чтением тела issue. +2. **Проверка каждого технического утверждения по текущему коду `dev`**, а не на + слово: все имена файлов, функций, регэкспов, строк конфигурации, упомянутых в + ТЗ, сверены с `grep`/`node -e` на рабочей копии. Список того, что именно + проверено — ниже, в «Что проверено и корректно». Это сделано, потому что ТЗ + содержит десятки конкретных технических утверждений о существующем поведении + («в файле X на строке Y», «список Z даёт…», «сейчас такой проверки нет») — + каждое из них либо доказуемо чтением кода, либо является непроверенной + догадкой, которая по §7.1 — находка. +3. Прогон гейтов не требовался: задача не создаёт и не меняет ни одного файла — + только текст ТЗ в issue. `npx tsc --noEmit` / `npm test` / `npm run build` + нечего было бы проверять на этом этапе (см. «Чего не проверял»). + +## Находки + +### Medium — К1/AC1: `strings.json` заявлен источником риска `ux`, но класс A его не видит + +**Файл:** тело issue #707, раздел «6. Контракт поведения» → К1, строка таблицы +`ux` (столбец «Токен в изменённой строке»). + +**Воспроизведение.** К1 открывается условием: «Судятся только файлы класса A +(`classify` из `change-classes.mjs`); `test/**`, `scripts/**`, `demo/**`, `docs/**` +риска не дают.» Строка `ux` того же К1 называет три источника нового ключа: +`src/i18n/*.json`, `custom_components/**/translations/*.json` **и +`strings.json`**. Проверка самой функции `classify` на рабочей копии: + +``` +$ node -e "import('./scripts/change-classes.mjs').then(({classify}) => + console.log(classify('custom_components/houseplan/strings.json')))" +? +``` + +`custom_components/houseplan/strings.json` не подпадает ни под один паттерн +`CLASS_A` в `scripts/change-classes.mjs` (там только `.py`, `manifest.json` и +`translations/`-каталог) — `classify()` возвращает `'?'`, не `'A'`. Значит файл, +который К1 сам называет входом правила `ux`, будет отфильтрован ещё на первом +шаге («судятся только файлы класса A») и никогда не дойдёт до токен-правила. +Часть контракта AC1 описывает поведение, которое реализовать как написано +невозможно: либо `strings.json` тихо выпадает из правила `ux` (реализация +разойдётся с текстом ТЗ, и это не будет видно в тестах — ни один сценарий +AC1(а)–(и) не проверяет именно этот путь), либо реализатору придётся по своей +инициативе расширить `CLASS_A` в `change-classes.mjs` — файл, которого нет в +разделе «13. Затронутые файлы», и который используется не только этой задачей: +он же определяет `shipLimitViolations` (рамки `ship`) и признак «инфраструктура» +в `resolveTrack`/guard. Расширение класса A задним числом — это побочный эффект +за пределами скоупа, никак не описанный в контракте и не покрытый AC. + +`strings.json` — не гипотетический путь: это реальный, вручную редактируемый файл +конфигурации HA-интеграции (`custom_components/houseplan/strings.json`, история +правок есть), т.е. случай практический, а не краевой теоретический. + +**Почему это находка, а не мелочь.** AC1 — защитный/классифицирующий критерий: он +обязан однозначно определять классы для указанных путей (DoR, §7.1 — «однозначность +каждого AC»). Здесь для одного из трёх явно названных путей поведение не определено +— оно противоречит собственному первому условию правила. + +**Что нужно от автора.** Явно решить одно из двух и записать в ТЗ: (а) убрать +`strings.json` из строки `ux` К1 (ux-риск для HA config-flow строк не отслеживается +этим правилом), либо (б) явно включить путь `custom_components/*/strings.json` в +проверяемый список К1 отдельной оговоркой, не трогая `change-classes.mjs` (например, +нишевым исключением в самой риск-функции, а не в общем `classify`), и добавить +сценарий в план AC1. Любой из двух вариантов дёшев; выбор — продуктовое решение +о том, что RU/EN-строки конфиг-флоу интеграции достойны такого же сигнала риска, +что и `src/i18n/*.json`, — не то, что ревьюер решает за автора. + +Без High в задаче это жёлтый вердикт, правка ТЗ и повторный цикл (§2.4, §4). + +## Что проверено и корректно + +ТЗ необычно тщательно сверено с текущим кодом: я перепроверил около 30 отдельных +технических утверждений, и, кроме находки выше, все подтвердились дословно: + +- Существование и сигнатуры `resolveTrack`, `trackFromLabels`, `hasTrackLabel`, + `shipLimitViolations`, `parseNumstat`, `parseNameStatus`, `SHIP_SRC_LINE_LIMIT` + в `scripts/process-track.mjs`. +- **Реальное расхождение трека при двух метках**, которое К2/AC2 называет + дефектом и чинит: `trackFromLabels` при `track:ship`+`track:ask` возвращает + `'ship'` (проверяет `track:ship` первым), тогда как bash guard в + `.github/workflows/_process.yml` (строки 92–97) при `track:ask` всегда сбрасывает + `SMALL=false` — т.е. сегодня guard даёт `ask`/4, а `process-track.mjs` — `ship`. + Утверждение ТЗ подтвердилось построчно. +- `task-packet.mjs:223` — ровно та строка про «перед S7 ребейз» безусловно для + любого `behind > 0`, которую К4/AC8 переписывает; устарелость с #696 + подтверждается (строка не различает track). +- `classify`/`CLASS_A`/`CLASS_B`/`CLASS_C`/`CLASS_D` в `scripts/change-classes.mjs` + и их точное содержимое (кроме отсутствия `strings.json` — см. находку). +- Порядок шагов в `.github/workflows/_process.yml`: шаг «Трек задачи и рамки ship + (#696)» (строка 424) действительно идёт до шага «Привести ветку к dev» (строка + 481), как того требует К3. +- Существование и сигнатуры `SHIP_MERGE_MARKER_RE`, `renderShipBrief` в + `scripts/ship-review.mjs`; маркер `hp:ship-merge material=...` в конвейере + (строка 1706). +- Все четыре якоря реестра мутаций, которые ТЗ требует перенести, а не удалить: + `guard-infra-keeps-ask-limit`, `packet-infra-track-ignores-show-default`, + `pipeline-ship-ignores-limits`, `ship-review-ignores-merge-marker` — все + существуют в `scripts/mutation-registry.mjs`. +- Отсутствие сегодня `bash -n`-проверки шагов `_process.yml` (AC4) — подтверждено + пустым результатом `grep -rn "bash -n" test/ scripts/`. +- Порог «300 файлов» в guard (`.github/workflows/_process.yml:239`, + `files.length < 300`) — дословное совпадение с описанием К2 «300 файлов и + больше → инфраструктура не доказана». +- Полный список файлов геометрии/touch/devices/perf/visual из таблицы К1 (src/*.ts + и custom_components/houseplan/*.py) — каждое имя, которое я сверил построчно + (physical-geometry, wall-*, junction-limits, coincident-partitions, + coordinate-canonicalization, opening-*, partition-openings, open-spans, + near-axis, align-grid, grid-scale, room-fit, resize*, stairs*, radar-geometry, + zigbee-topology-geometry, device-marker-geometry, plan-geometry-preflight, + plan-optimizer, zero-walls, iso-projection; pointer-modality, + pointer-move-queue, touch-gesture-click-guard, live-interaction-runtime, + live-viewport, viewport-transition, room-gear-drag; device-toggle, + marker-toggle-entity, integration-provider, virtual-light-state, vacuum*, + device-hit-owner; auth.py, http_api.py, websocket_api.py, virtual_lights.py, + vacuum_routes.py, store.py, geometry_migration.py, import_export.py, + validation.py, coordinate_canonicalization.py, junction_limits.py, + wall_segment_model.py, radar_geometry.py, projection.py; + houseplan-render-lifecycle, iso-scene-render, glow-*, day-cycle-render, + initial-load, boot-soft-layout; paper-scene.ts; space-render, stairs-view, + device-visual, device-face; styles.ts/styles/) — существует ровно как названо. +- Наличие `test/process-track.test.mjs` с существующим паттерном извлечения текста + шага `_process.yml` регэкспом (строка 120, 167) — подтверждает реалистичность + плана автотестов АС4/АС6 «как существующие тесты #696». +- Обязательные разделы §7.1 (сценарий, что увидит человек, проблема, скоуп и + не-скоуп, контракт поведения, UX/данные/i18n/perf/touch, AC1–AC14 со способом + доказательства, план автотестов, риски, откат, release-артефакты) — все + присутствуют и в правильном порядке. +- «Принято предположительно» (§10) — 8 пунктов, каждый — реальная техническая + развилка (где живёт код, эвристика К1, длина заметки и т.д.), не маскирует + продуктовый вопрос под техническое решение. +- **§10 п.8** (сужение «метка владельца окончательна» до «метка, подтверждённая + строкой владельца») — автор прямо просит проверить его на ревью. Обоснование в + тексте ТЗ внутренне непротиворечиво: текущий §5 PROCESS.md не различает метку, + поставленную человеком-владельцем, от метки, поставленной аналитиком/агентом + от того же аккаунта (о чём и сам §5 говорит: «Аналитик предлагает трек… и ставит + метку»), и per §10 п.1 задачи по actor события их сегодня действительно не + отличить. Уточнение закрывает реальный пробел, не меняет ничьих прав, которых + не было прежде, и не требует отдельного продуктового вопроса владельцу — трек + решается вердиктом по §7.1 («технический спор автора и ревьюера решается + вердиктом»). Принимаю формулировку как есть. +- Лимиты циклов, таблица `ask`=4/`show`,`ship`=2, «зелёный вердикт цикла не + образует» — согласуются с §4 буквально. +- Соответствие «Не входит» реальным границам: `smoke-select.mjs`, `process-reconcile`, + `process-resume`, `merge-candidate`, тонкий `process.yml` в `main` — задача их не + трогает; список «затронутые файлы» (раздел 13) их действительно не включает. +- Метки на самом issue: `track:ask`, `S4-spec-review`, `P2`, `infra`, `process`, + `tech-debt` — соответствуют заявленному в аналитике треку и статусу. +- П.7 (черновая реализация до вердикта, меняющая правило №1) явно вынесен в #729 и + не входит в контракт #707; раздел «11. Вопрос владельцу» корректно помечен + «не блокирует #707, `blocked` не ставится» — в #707 остаётся только справочная + ссылка, лишнего продуктового вопроса в этой задаче нет. + +## Чего не проверял + +- **Гейты (`npx tsc --noEmit`, `npm test`, `npm run build`) не прогонял** — задача + не меняет ни одного файла репозитория на этом этапе, проверять нечего; на + `S7-code-review` они станут обязательны и будут применяться к реальному диффу. +- Не проверял golden/смоки/бэкенд-pytest/invariants — задача класса A не + содержит, `User-Visible: no`, визуальных и геометрических изменений продукта + нет (сама ТЗ фиксирует это в разделе 7 и 16). +- Не оценивал реальную эффективность эвристики К1 (долю ложных срабатываний/пропусков + на исторических диффах) — это явно измеряемый, но не гейтируемый на этом этапе + вопрос; ТЗ сама называет это «принято предположительно» (§10 п.2) и переносит + измерение эффекта в #728. +- Не проверял, действительно ли `git diff --unified=0` с настройками git по + умолчанию в GitHub Actions runner надёжно распознаёт чистые переименования + (сценарий AC1(и)) — это деталь реализации команды диффа, не зафиксированная в + контракте явным флагом (`-M`/`--find-renames`); при стандартном `diff.renames=true` + (git ≥2.9) поведение ожидаемо корректно, а при сбое эффект безопасен (ложный + риск, а не пропуск) — не поднимаю отдельной находкой, но это стоит держать в + уме при написании AC1(и) как юнит-теста на встроенном диффе, а не на реальном + git. + +## Вердикт + +Один Medium **в скоупе задачи** (AC1/К1: `strings.json` заявлен источником риска +`ux`, но исключён условием «только класс A» того же правила) — без High это +жёлтый вердикт с возвратом автору по §2.4/§2.7. Всё остальное, вплоть до +мелких деталей вроде номеров строк и имён функций, подтвердилось построкой +сверкой с кодом `dev`; спор на ревью — только вокруг одной несостыковки К1. + +**Вердикт: жёлтый · заход r1 · блокирующих циклов 1/4 · High: 0 · Medium: 1 → в задаче** + +## Материал раунда + +- Этап: spec. Материал — тело issue #707 на момент комментария Matysh + 2026-09-30T20:59:00Z («ТЗ написано… Передаю на ревью ТЗ»). +- Кода/ветки продукта не существует (инфраструктурная задача до реализации). +- Сверка кода `dev`: `git rev-parse HEAD` рабочей копии ревью = + `a5a73d1511ae4c09277e16a41c076bfd5eb1fd8e`. + +--- + + + +## Материал раунда + +- Ветка: `dev`, коммит `a5a73d1511ae` — ребейз его осиротит, и это нормально: ниже якоря, которые ребейз не меняет. +- Дерево материала: `5a1bea4a8cb02a6fc04a3a8742cb8b57e57b7c51` + ``` + git log --all --format='%H %T' | grep 5a1bea4a8cb0 + ``` +- Тело issue: `965899906f2230d8473b72e6304be7bb1f58c2045425912b69a9efa788025ff4` +- Вердикт конвейера: `yellow` · High 0