Files
houseplan-card/docs/reviews/SPEC-REVIEW-669-r2.md
T
2026-09-27 06:41:04 +00:00

13 KiB
Raw Blame History

SPEC-REVIEW-669-r2

  • Issue: https://github.com/Matysh/houseplan-card/issues/669
  • Этап: S4-spec-review (ревью ТЗ, PROCESS.md §2.4)
  • Трек: полный (метки bug, P1, S4-spec-review; ни small, ни trivial не выставлены — не изменилось с r1).
  • Материал: тело issue #669, раздел ## ТЗ, редакция 2 (комментарий автора «ТЗ, редакция 2 — по ревью r1»). Полный текст получен gh issue view 669 --json body,comments.
  • Заход: r2 · блокирующих циклов израсходовано 1 из 4
  • Роль: ревьюер ТЗ (не автор)

Скоуп ревью (без изменений с r1)

Регрессия производительности в cleanFloorForRoom (src/clean-floor.ts:47): каждый промах кэша чистого пола сразу вычитает все лестницы пространства (geometryAreaMinusStairs → geometryMinusStairs, src/stairs.ts:234-250) — хотя площадь читают только два потребителя (подсказка комнаты, PDF), а путь пола (нужный четырём местам рендера) вычитания не требует. Задача переносит вычисление площади на момент первого чтения (лениво, мемоизировано в том же кэше) и добавляет отбор лестниц по пересечению bbox перед polyclip. SCOPE-связь с J1 не менялась и не пересматривается в r2.

Закрытие раунда r1

Находка r1 Чем закрыта Где это видно
Medium — AC3/преамбула «Ожидаемое поведение» обещают полностью зелёный performance_smoke, но spaceSwitchMs (обязательная hardMaxMs-метрика того же job) уже проваливается на точном SHA независимо от этой задачи и вынесена в отдельный #675; разрыв между «AC3 выполнен» и «issue решает свою же преамбулу» не был явно назван. Автор выбрал вариант (a) из ревью: явно ограничил обещание двумя метриками и назвал #675 сопутствующим условием. Новый раздел «Связь с „Ожидаемым поведением“» в теле issue: «Эта задача обещает бюджеты modelReadyMs и firstStableRenderMs… Бюджет spaceSwitchMs того же job тоже обязателен, но от лестниц не зависит… он закрывается #675. Полностью зелёный perf-smoke кандидата и выпуск v1.78.0-beta.5… — результат этой задачи вместе с #675. Ни одна из двух задач поодиночке этого не обещает.» Дополнительно AC3 дописан: «spaceSwitchMs этим AC не утверждается — это условие #675; если job красный только по spaceSwitchMs, AC3 считается выполненным, а выпуск ждёт #675.»
Low — «Проблема» утверждала «пять мест рендера запрашивают только путь пола», чтением кода подтверждено четыре; пятое (:11873) читает .area. Число и классификация исправлены в тексте. «Проблема»: «…хотя четыре места рендера запрашивают только путь пола (.path: src/houseplan-card.ts:9059, 9229, 10459, 11011), а площадь читают лишь подсказка комнаты при наведении (_roomArea, :11864, чтение .area на :11873) и PDF (resolveRoomArea, :10182)».

Обе находки закрыты правкой текста ТЗ (комментарий автора помечен «ТЗ, редакция 2»), без привлечения владельца — как и предписывал вердикт r1 (технические, не продуктовые вопросы).

Унаследовано из r1 (без повторной проверки)

Дельта r1→r2 — точечная правка двух фрагментов текста (один новый раздел, одна цифра с перечислением строк). Она не меняет ни один AC по существу, ни контракт поведения, ни границы скоупа/не-скоупа, ни план тестов, ни затронутые файлы, ни риски, ни откат, ни release-артефакты — весь остальной текст ТЗ идентичен редакции 1. Поэтому без повторной проверки принимается (документ docs/reviews/SPEC-REVIEW-669-r1.md, материал: тело issue, дерево 2c7a4c124868cf7499d7b5c725ee028d3810eb62):

  • Обязательные разделы §7.1 присутствуют и в правильном порядке; первые два — продуктовые, без терминов реализации.
  • AC1, AC2, AC4 однозначны, с явной парой «доказательство / чем краснеет»; тестовый план реализуем без изменения контракта пути/геометрии.
  • Контракт поведения («в пределах относительной погрешности 1e-9») и «Не-скоуп» (geometryMinusStairsSteps не заденется, 2.5D-рендер — только замер с явным условием возврата в ТЗ) корректны.
  • Риск «устаревшая площадь при копировании объекта» описан реалистично и не имеет текущего нарушителя (grep на spread/Object.assign результата _cleanFloor/cleanFloorForRoom пуст на момент r1).
  • Release-артефакты («changelog — нет») согласованы с docs/STATUS.md: v1.78.0 ещё не выходил стабильным релизом.
  • Продуктовых открытых вопросов к владельцу нет — видимое поведение не меняется, оставшийся разрыв в AC3 был техническим и закрыт в этом раунде.

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

  1. Получено тело issue #669 текущей редакции и все 5 комментариев (включая комментарий «ТЗ, редакция 2», которого не было в материале r1) — gh issue view 669 --json body,comments.
  2. Сверено дословно, что оба пункта, заявленные автором как правка («Medium: добавлен раздел…», «Low: в „Проблеме“ — четыре места…»), присутствуют в теле issue и закрывают ровно то, что просил r1 (таблица выше) — не переформулировка в духе находки, а её содержательное закрытие.
  3. Перечитан текст AC3 целиком в новой редакции: проверено, что критерий остаётся однозначным и проверяемым — судья (код-ревьюер/разработчик) получает явное правило «job красный только по spaceSwitchMs → AC3 выполнен», а не молчаливое допущение.
  4. Проверена согласованность нового раздела с «Не-скоуп» (bullet «spaceSwitchMs — #675», не менялся с r1) и с «Влиянием» — противоречий нет.
  5. Перечитаны строки кода, на которые ссылается исправленная «Проблема», заново (не полагаясь на вывод r1):
    • grep -n "_cleanFloor(" src/houseplan-card.ts → :9059, :9229, :10459, :11011 читают только .path (подтверждено чтением каждой строки: во всех четырёх результат употребляется как .path без чтения .area); :11873 (_roomArea) присваивает весь объект в clean и ниже читает clean.area — корректно отнесён к читателям площади, как теперь и написано в ТЗ.
    • Существование и формулировка #675 (gh issue view 675) подтверждают контекст нового раздела: заголовок «spaceSwitchMs изометрического профиля стоит на границе бюджета без регрессии кода» — согласуется с утверждением ТЗ «от лестниц не зависит» и «не устраняется этой задачей». Существо #675 не проверялось (отдельная задача, вне скоупа этого ревью).
  6. Проверено, что остальной текст ТЗ (AC1/AC2/AC4, Скоуп, Контракт поведения, План автотестов, Риски, Откат, Release-артефакты, Затронутые файлы, «Принято предположительно») идентичен цитатам, зафиксированным в SPEC-REVIEW-669-r1.md, — расхождений не найдено, отдельная повторная критика не требуется (раздел «Унаследовано» выше).

Находки

Нет. Обе находки r1 закрыты текстом; новых неоднозначностей или новых незакрытых обещаний правка не вносит.

Что проверено и корректно

  • Оба замечания r1 (Medium и Low) закрыты именно так, как просил вердикт r1: Medium — вариантом (a) («ограничить обещание, назвать #675 сопутствующим условием»), без обращения к владельцу (техническая правка, решена автором самостоятельно, как и предписывало «ты его снимаешь и решаешь по существу»); Low — точным числом и корректной классификацией :11873.
  • AC3 в новой редакции остаётся однозначным и проверяемым: явно исключает spaceSwitchMs из своей области и явно называет условие полного релиза.
  • Новый раздел «Связь с „Ожидаемым поведением“» не расширяет и не сужает скоуп задачи (AC1/AC2/Скоуп/Не-скоуп не тронуты) — это уточнение границы обещания, а не новая работа.
  • Тело issue снова содержит все обязательные разделы §7.1 в порядке, продукт и код в описании проблемы согласованы построчно.

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

  • Не проверял #675 по существу — использовал только факт его существования, открытого статуса и заголовка для согласованности ссылки (как и в r1).
  • Не запускал npm test/typecheck/build/perf-смок — реализация не начата (git log --all --oneline | grep -i 669 не показывает ветки реализации), AC1–AC4 по-прежнему доказываются на этапе код-ревью, не здесь.
  • Не проверял заново части ТЗ, не задетые дельтой r1→r2 (см. «Унаследовано»).

Вердикт

Вердикт: зелёный · заход r2 · блокирующих циклов 1/4 · High: 0 · Medium: 0



Материал раунда

  • Ветка: dev, коммит 8b775247a705 — ребейз его осиротит, и это нормально: ниже якоря, которые ребейз не меняет.
  • Дерево материала: c776611697d24a44fcd357c1aa77058ce08520ee
    git log --all --format='%H %T' | grep c776611697d2
    
  • Тело issue: 1325d6906ba9cf19798f52282b54bf435c4603185b0a323e8a7a5ef01204ae9c
  • Вердикт конвейера: green · High 0