Files
houseplan-card/docs/reviews/SPEC-REVIEW-198-r1.md

19 KiB
Raw Permalink Blame History

Ревью ТЗ — issue #198, цикл r1

  • Этап: S4-spec-review (PROCESS.md §2.4)
  • Артефакт ТЗ: docs/specs/198-optimize-micro-interval.md, коммит 9d6cd8b на ветке issue/198-optimize-micro-interval
  • Issue: #198
  • Ревьюер: Claude (роль «ревьюер ТЗ», отдельная сессия от аналитика/автора)
  • Трек: обычный (не small) — совпадает с метками issue (bug, P3, S4-spec-review, без small) и с явной отметкой аналитики «лёгкий трек: нет»; сложность/риск оценены аналитикой в 5/10 и 7/10, что и оправдывает обычный трек с лимитом ревью 4 цикла (не 2, как на лёгком/коротком).

Скоуп ревью

Оценивалось ТЗ docs/specs/198-optimize-micro-interval.md целиком: наличие обязательных разделов §7.1 PROCESS.md, однозначность и доказуемость AC1–AC10, отсутствие догадок, выданных за решённый факт, соответствие docs/SCOPE.md (job J6 «Keep the plan true as the home evolves»), docs/WALL-THICKNESS.md, docs/USER-GUIDE.ru.md, docs/CONFIG-COMPATIBILITY.md и текущему коду (src/plan-optimizer.ts, src/wall-thickness.ts, custom_components/houseplan/websocket_api.py, custom_components/houseplan/validation.py). Продуктовый код не менялся и не мог быть изменён (задача на этапе ТЗ) — диапазон коммитов origin/dev...origin/issue/198-optimize-micro-interval содержит только docs/specs/198-optimize-micro-interval.md и одну строку в docs/specs/README.md.

Как проверялось

  1. Прочитаны docs/SCOPE.md, AGENTS.md, PROCESS.md целиком.
  2. Прочитано тело issue #198 и оба комментария через gh issue view --json, включая полную историю меток (gh api .../events) — аналитика с продуктовыми вопросами Q1–Q3 под blocked+S3-spec (14:46) и последующий комментарий автора «ТЗ готово к ревью» со ссылкой на defaults владельца (17:10), между которыми снята метка blocked (17:08). Проверено, что все три вопроса имеют явный default в тексте аналитики и что итоговое ТЗ (§6) реализует ровно эти default'ы, а не какой-то другой вариант.
  3. Прочитан весь текст ТЗ построчно, сверен с §7.1 (обязательные разделы) и §2.5 (DoR-чеклист).
  4. Прочитан канонический docs/WALL-THICKNESS.md целиком — модель walls (§1: «one physical stretch has one thickness», exact endpoints как продуктовая граница толщины, «Normalisation merges consecutive solid pieces into each maximal run of equal thickness»), чтобы проверить, что ТЗ не переопределяет lossless-контракт, а вводит узкое исключение только для явного Optimize.
  5. Проверены заявленные идентификаторы и функции чтением продуктового кода src/plan-optimizer.ts (optimizePlans() целиком, строки 248–407) и src/wall-thickness.ts (export function degradeWalls :274, rekeyWallsAfterMove :368, wallIntervals :1186, normalizeWallIntervals :1258) и src/space-geometry.ts (NORM_W=1000, GRID_PITCH=NORM_W/GRID_N, GRID_STEP_N=1/GRID_N). Подтверждено: все константы и функции, на которые ссылается ТЗ, существуют и называются именно так; заявленный порядок вызова в optimizePlans() — rekeyWallsAfterMove() → normalizeWallIntervals() → degradeWalls() (строки 353–364) — соответствует месту вставки, которое ТЗ предлагает («после rekey… и до финальной lossless-нормализации»): новый helper встаёт между rekey и normalizeWallIntervals(), чтобы последняя сама собрала уже одинаковые по толщине три записи в один максимальный пролёт — именно так, как описывает §6 ТЗ.
  6. Проверено происхождение canonicalized/wallsMerged (строки 280–373, 401–402 plan-optimizer.ts): canonicalized инкрементируется при изменении JSON-снимка spans/links/walls пространства, wallsMerged считает разницу числа записей до/после — ТЗ корректно заявляет переиспользование существующих полей отчёта без нового счётчика (§4, §8).
  7. Проверено соответствие терминологии docs/USER-GUIDE.ru.md §19 («Общие настройки → Оптимизировать планы», «серверная отмена», строка таблицы «Стены | Одинаковые соседние участки толщины объединяются», gs.optimize_undo/gs.optimize_undone в src/i18n/ru.json/en.json, реальный UI-код _optimizeUndoBusy/_canOptimizeUndo/ houseplan/plan/optimize_undo в src/houseplan-card.ts) — ТЗ не изобретает новый UI-текст и корректно описывает уточнение уже существующей строки таблицы, а не новую функцию.
  8. Проверено, что backend не требует изменений (§12 ТЗ): ws_plan_optimize в custom_components/houseplan/websocket_api.py (строки 1323–1441) принимает уже готовый config/layout от фронтенда как непрозрачный блок и делает только атомарную запись + одноглубинный backup для Undo; validation.py (строки 460–842) уже валидирует записи walls с a/b (точные endpoints) — существующая с #197. Новых полей схемы, миграции model_version или изменений CONFIG_SCHEMA не требуется, что совпадает с заявлением ТЗ.
  9. Прочитан docs/CONFIG-COMPATIBILITY.md целиком — подтверждено отсутствие необходимости новой записи реестра: результат Optimize — один валидный WallEntry{cm,a,b} в уже существующем формате (используется с #197), readable старыми клиентами тем же механизмом key-fallback, что и любая другая exact-endpoint запись; ТЗ (§7, §12) корректно не заявляет отдельный compatibility-артефакт.
  10. Проверено существующее browser-покрытие Optimize UI: demo/smoke_grid_snap.mjs (строка 195, проверяет ровно один вызов houseplan/plan/optimize) и demo/smoke_warm_dialogs.mjs (диалог _alignDialog) — ни один не прогоняет полный цикл Preview→Apply→Undo→Cancel. Заявление ТЗ (§10) «Targeted smoke расширяет существующий Optimize smoke либо получает отдельное имя» корректно не переоценивает существующее покрытие и оставляет выбор конкретного файла реализации (§13 п.3).
  11. Проверена реестровая практика mutation-gate (scripts/mutation-gate.mjs, test/mutation-gate.test.mjs, .github/workflows/mutation-gate.yml, аналогичные ссылки в специфика́циях #200/#201/#203) — AC9 ссылается на реально существующий, а не изобретённый инструмент.
  12. Проверено, что docs/specs/README.md получил ровно одну новую строку в таблице «Issue ↔ ТЗ» (без добавления запрещённой §7.3 колонки «Статус ТЗ»), и что коммит 9d6cd8b несёт корректные трейлеры Issue: #198 / User-Visible: no (документация без изменения поведения на этой стадии, класс C).

Код не менялся, чтения было достаточно: вопрос ревью ТЗ — «выполнимо и проверяемо ли», а не «работает ли реализация». Гейты (typecheck/test/ build/smoke) не запускались — на этапе ТЗ они неприменимы, продуктовый код отсутствует.

Проверено и корректно

  • Все обязательные разделы §7.1 присутствуют: сценарий и персона (§1), что человек увидит до/после без терминов реализации (§2), проблема/подтверждённая причина (§3), scope/non-scope (§4–5), контракт поведения (§6), preview/запись/Undo/данные (§7), UX/i18n/touch (§8), AC1–AC10 с доказательством (§9), план автотестов (§10), риски и производительность (§11), release-артефакты и rollback (§12), явный блок принятых предположений (§13).
  • Продуктовые вопросы Q1–Q3 закрыты по существу, а не молча. Аналитика владельца сформулировала три вопроса с default'ами (изолированный островок короче 0.5×pitch; торец/разные соседи — всегда сохранять; очистка только в explicit Optimize); ТЗ §6 реализует ровно эти default'ы пункт за пунктом (условия 1–6), а §13 п.5 явно фиксирует «продуктовых вопросов больше нет». Открытых догадок, выданных за факт, не найдено — все нетривиальные утверждения (о происхождении дефекта, о поведении rekeyWallsAfterMove(), о том, что Optimize не создаёт островок из равномерного пролёта) дословно совпадают с уже подтверждённым исполнением на fixture #197 из комментария аналитики, а не являются новой гипотезой автора ТЗ.
  • Контракт §6 предметно защищает документированный lossless-инвариант. Все шесть условий (строгий порог длины, оба соседа collinear/solid/равной толщины, отличие центральной толщины, запрет на любой топологический разрыв на границах центра, принадлежность одной физической линии) вместе не дают схлопнуть намеренную границу толщины, торцевой интервал или разнородных соседей — соответствует Q2 defaults и §1 WALL-THICKNESS.md («exact endpoints make a thickness boundary independent…»). Non-cascading правило (кандидаты по исходному, не по уже изменённому snapshot; конфликтующий interval не трогается) закрывает реальный риск «каскад съедает длинную зону», явно перечисленный в §11.
  • Все AC пронумерованы, однозначны и несут явный способ доказательства (unit/matrix/existing regression/browser smoke/mutation gate/gates), выполняется требование DoR §2.5. AC1–AC2 задают точную числовую границу в обоих масштабах, AC3–AC5 — исчерпывающую негативную матрицу и permutation/immutability, AC7 — сквозной UI-цикл Preview/Apply/Undo/Cancel, AC9 явно требует падающего мутанта.
  • Non-scope точен и не оставляет двойного толкования (§5): явно исключены универсальный минимум длины, фоновая очистка при Save/render/редактировании, изменение lossless-хелперов и schema, починка #197/#199/#201, эвристики по большинству/максимуму — каждый пункт реальная граница, подтверждённая чтением кода (backend/validation не требуют изменений — see «Как проверялось» п.8–9).
  • Backend и compatibility корректно исключены из release-артефактов: подтверждено чтением websocket_api.py/validation.py, что config передаётся фронтендом уже готовым и валидируется существующей с #197 схемой WallEntry{cm,a,b}; docs/CONFIG-COMPATIBILITY.md не требует новой записи, потому что результат Optimize — не новый формат данных.
  • UX/i18n/touch раздел корректно пуст (§8): новых controls, строк и touch-путей нет, что подтверждено чтением реального UI-кода Optimize/Undo (_canOptimizeUndo, gs.optimize_undo) — фиче не нужна отдельная поверхность, значит docs/TOUCH-SUPPORT.md не нарушается по построению.
  • Технические решения корректно помечены как предположения (§13): определение «геометрического узла», момент проверки соседства (после rekey), точное расположение helper'а и имя smoke-файла. Ревьюер эти предположения не оспаривает — они разумны, не влияют на AC и явно оставлены свободными для реализации.
  • Release-артефакты названы явно (§12): оба changelog при User-Visible: yes в implementation-коммите, точная строка в docs/USER-GUIDE.ru.md §19, уточнение lossless-контракта в docs/WALL-THICKNESS.md, обновление docs/TESTING.md, unit/smoke/mutation; golden/i18n/backend/security/performance-артефакты корректно исключены с объяснением почему (данные без нового рендер-пути, backend не тронут, профиль малых списков стен).

Находки

Находок High, Medium или Low не выявлено. Спецификация грамотно опирается на уже подтверждённое исполнением (аналитика владельца) и на реально существующий код/документацию; ни одно нетривиальное продуктовое или техническое утверждение не осталось непроверенным при чтении кода.

Чего не проверял

  • Не проверялась реализация — на этапе ТЗ её не существует; код-ревью будет отдельным циклом (S7-code-review) после написания кода.
  • Не запускались npm test/npm run build/смоки/gates: код не менялся, гейты ТЗ не требуют их прогона на этом этапе (документация — класс C, продуктовый код не тронут).
  • Не проверялась конкретная реализация helper'а (структура функции, сигнатура, точное имя файла unit/smoke) — ТЗ корректно оставляет это технической свободой реализации (§13 п.3) при неизменных AC.
  • Не оценивалась производительность на реальном большом плане — риск в §11 обоснованно сведён к «полиномиальная проверка на малых wall lists, вне render/state tick»; это утверждение проверяется код-ревью, а не спецификацией.
  • Не проверялось независимо, кем по факту было принято решение владельца по Q1–Q3 (аккаунт Matysh, тип User, без GitHub App, снял blocked и оставил комментарий) — это происходит за пределами репозитория (см. docs/SCOPE.md: Telegram — основной канал обратной связи владельца), и проверка этого факта вне полномочий и возможностей ревью ТЗ; сам текст defaults и их реализация в ТЗ взаимно согласованы, что и является предметом этого ревью.

Вердикт

Все обязательные разделы на месте, все AC однозначны и снабжены способом доказательства, причина дефекта и предлагаемый контракт подтверждены чтением кода и совпадают с уже исполненной диагностикой аналитики (а не являются новой непроверенной гипотезой), lossless-инвариант WALL-THICKNESS.md явно защищён шестью условиями и негативной матрицей, non-scope точен, backend и config-compatibility корректно исключены с подтверждением по коду. Открытых продуктовых вопросов нет, Q1–Q3 закрыты по существу. Находок нет.

Вердикт: зелёный · цикл r1/4 · High: 0 · Medium: 0 → в задаче