Files
houseplan-card/docs/reviews/SPEC-REVIEW-276-r3.md
2026-08-24 02:11:52 +00:00

26 KiB
Raw Permalink Blame History

SPEC-REVIEW-276-r3

  • Issue: https://github.com/Matysh/houseplan-card/issues/276
  • Этап: ТЗ на ревью (PROCESS.md §2.4)
  • Заход: r3 · блокирующих циклов израсходовано 1 из 4 (r1 — красный, 1 цикл; r2 — зелёный, цикла не образует, #227)
  • ТЗ: docs/specs/276-coincident-partition-reconciliation.md
  • Предыдущий раунд: docs/reviews/SPEC-REVIEW-276-r2.md, вердикт зелёный, получен на commit ca516643
  • Вердикт: зелёный · High: 0 · Medium: 0 · Low: 3 (все сняты с записью)

Скоуп

Обычный трек, ТЗ — отдельный файл. Формальный диапазон дельты — ca516643..cfc8f211 (единственный тронутый файл — сам спек, 23 добавленные / 13 удалённых строк, подтверждено git show --stat cfc8f211).

Разбор в этом раунде — полный, а не по дельте, и вот почему. r3 не была вызвана находкой ревью: после зелёного r2 автор взял issue в разработку, проверил обе реально воспроизводимые проблемные оси на анонимизируемой fixture и обнаружил, что вторая пара — не «room wall 20 + partition 20», как было зафиксировано в §2 до этого раунда, а «room wall 30 + partition 20» (вложенная, разной толщины). Ревьюированное условие r2 «partition.cm равна эффективной толщине» (единственный допустимый случай — точное совпадение толщин) исправило бы только первую пару и оставило бы вторую реальную пару в «неоднозначном» статусе без изменений — то есть не решило бы заявленный в issue сценарий для половины подтверждённого воспроизведения. Дельта переписывает эту часть контракта: критерий приёмки (§4.4/§4.3), сама трансформация (§5.4), результат для пользователя («увеличение не создаёт ступень», §3.4), не-скоуп (§8) и два AC (AC1 текстуально не тронут, AC2 расширен). Это подпадает под явно перечисленное в PROCESS §2.10 основание для полного разбора — «смена контракта поведения»: раньше разнотолщинные пары были категорически исключены («Не входит: разные толщины room wall и partition»), теперь одна из них (вложенная, центрированная) канонизируется автоматически с итоговой толщиной max. Такое расширение автоматического поведения Optimize нельзя проверить, доверившись формулировкам r1/r2 — нужно заново прочитать весь документ и перепроверить его математику и согласованность с остальными разделами.

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

  • Перечитаны docs/SCOPE.md, AGENTS.md, PROCESS.md §2.4, §2.5, §2.10, §4, §7.1, §7.2 целиком.
  • Прочитано тело issue #276 и все 14 комментариев (аналитика, связанные #277/ #278, оба хендоффа автора ТЗ, оба вердикта r1/r2, техническая находка и техническая правка перед r3, обе SHA-корректировки).
  • Найдены документы предыдущих раундов: docs/reviews/SPEC-REVIEW-276-r1.md (красный, commit a06c94b9) и docs/reviews/SPEC-REVIEW-276-r2.md (зелёный, commit ca516643) — оба уже закоммичены в ветку, SHA в обоих назван явно.
  • Прочитан весь документ ТЗ целиком (294 строки, все 15 разделов), а не только дельта — согласно решению о полном разборе выше.
  • Дельта явно объявлена и прочитана как патч: git diff ca516643..cfc8f211 -- docs/specs/276-coincident-partition-reconciliation.md.
  • Проверена математика центрального продуктового решения дельты (§3.4/§4.4): прочитан src/physical-geometry.ts:192-226 (physicalBodyParts(), halfDepth: wallCmToUnits(partition.cm, ...) / 2) и docs/WALL-THICKNESS.md §2 («every thick wall grows half outward and half inward from the polygon edge») — подтверждено, что и room wall, и independent partition растут симметрично ±cm/2 от одной и той же centreline. Условие §4.2 требует точного endpoint-to-endpoint совпадения оси ⇒ у обоих тел одна и та же centreline. Union двух центрированных прямоугольных полос на одной оси математически точно равен более широкой полосе, т.е. max(roomCm, partitionCm) — не эвристика, а точное тождество, как и заявляет документ.
  • Сверены все места дельты друг с другом на непротиворечивость: §3.4 ↔ §4.4 ↔ §5.4 ↔ §8 ↔ §10.2 (риск) ↔ AC2 ↔ §15.2 (техпредположение) — единая формулировка max(roomCm, partitionCm) для centred exact bodies используется везде, кроме одного расхождения именования (см. L1 ниже). Отдельно проверено, что старая формулировка «Не входит: разные толщины room wall и partition» не осталась где-то ещё в файле: grep -n "разные толщины" docs/specs/276-*.md — 0 совпадений вне заменённой строки §8.
  • Проверено согласование нового правила с AC5 (не тронут дельтой): «после 20→10/30 существует ровно одно тело выбранной толщины» — совместимо с новым правилом, поскольку AC5 описывает поведение после Apply уже реконциолированной стены (Thickness работает штатно), а не сам момент reconciliation.
  • Проверена идемпотентность (AC8) после нового правила: после Apply partition удалена, поэтому повторный optimizePlans() не находит кандидата вне зависимости от того, было ли обновление толщины — логической коллизии нет.
  • Проверено соответствие §10.1 (файлы/модули, не тронут дельтой) дереву повторно: src/plan-optimizer.ts, src/partition-openings.ts, src/plan-geometry-preflight.ts, src/houseplan-card.ts, test/plan-optimizer.test.mjs, test/partition-openings.test.mjs — все существуют.
  • Проверено соответствие названного будущего смока паттерну: ls demo/smoke_optimize_*.mjs — три существующих смока с тем же префиксом (smoke_optimize_coordinate_canonicalization.mjs, smoke_optimize_geometry_preflight.mjs, smoke_optimize_micro_interval.mjs), заявленное имя demo/smoke_optimize_coincident_partition.mjs соответствует конвенции и ещё не существует (корректно заявлено как новый файл).
  • Проверена запись docs/specs/README.md:73 — ссылка на файл спека двухсторонняя и не устарела.
  • Проверена трассируемость коммита дельты: git show cfc8f211 — единственный файл, трейлеры Issue: #276 / User-Visible: no корректны (док-только правка, класс C, без изменения поведения кода). Хендофф-комментарий автора называет Коммит: 2a8b64e7 — такого объекта не существует (git cat-file -t 2a8b64e7 → «Not a valid object name»); фактический SHA дельты — cfc8f211, подтверждён последующим комментарием автора и временем коммита (05:03:58+03:00 = 02:03:58Z, комментарии-исправления в 02:04:02Z/ 02:04:08Z) — контент соответствует, ошибка только в указанном идентификаторе (см. L3 ниже).
  • Гейты typecheck/test/build/check-docs не прогонялись: ни один коммит диапазона (a06c94b9..cfc8f211) не трогает src/**, продуктовый код не менялся ни разу за всю историю этой задачи. Неприменимо к спек-ревью того же основания, что в r1/r2.
  • Browser smoke / golden / инварианты модели не прогонялись: geometry/wall код реализации ещё не написан.

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

r2 вернул зелёный вердикт без единой находки («Находки: Нет», Low о неверном SHA был снят с записью, не возвращался автору) — таблицы «закрытых находок» в классическом виде для r2 не существует, закрывать нечего. Единственное замечание r2 («находка того же типа... автору стоит подставлять реальный SHA») адресовано на будущее, а не как блокирующее — и не было закрыто: то же самое случилось снова в этом раунде (см. L3).

Унаследовано из r2 — переподтверждено полным разбором, а не принято на веру

Раунд разобран полностью (см. «Скоуп» выше), поэтому ничего в этом списке не принято слепо — каждый пункт перепрочитан в текущей редакции документа и подтверждён не противоречащим дельте. Ссылка сохранена по требованию §2.10: docs/reviews/SPEC-REVIEW-276-r2.md, SHA ca516643.

  • Продуктовая рамка (§1 сценарий/персона, часть §2 про происхождение дефекта в коде) и её соответствие docs/SCOPE.md J4/J6 — не изменилась, только фактура второй реальной пары (30/20 вместо 20/20).
  • Контракт one-shot server Undo без Redo (§6, AC7) — текст дельтой не тронут, перепроверен построчно повторно: расхождений с _alignOptimizedConfirm/_undoPlanOptimization и docs/USER-GUIDE.ru.md:1444 не появилось.
  • Разведение с #277 (Resize ambiguity) и #278 (wall-union fallback) — §8 не затронут в этой части.
  • AC3, AC4, AC6, AC7, AC9–AC14 — тексты не менялись, доказательства указаны у каждого, перечитаны повторно на непротиворечивость с новым §4/§5 — не нашёл конфликта (hosted-opening rehost и preflight не зависят от того, какая толщина в итоге записывается).
  • §10 (perf budget), §11 (touch) — не затронуты дельтой, перечитаны, риски и туч-контракт закрывают DoR §2.5 как и раньше.
  • Preflight #199 (src/plan-geometry-preflight.ts, test/plan-geometry-preflight.test.mjs) — существует, перепроверено.

Находки

Блокирующих (High/Medium) находок нет. Три Low, все сняты с записью — не возвращаются автору.

L1 (Low, снято с записью). Разное именование одной переменной в двух соседних формулах

Где: §4.4 (строка 81) пишет max(sharedCm, partition.cm), а §3.4 (строка 59), §5.4 (строка 108) и таблица рисков §10.2 (строка 217) — везде max(roomCm, partitionCm). Оба обозначения означают одну и ту же величину (эффективную толщину room-wall стороны атомарного shared interval до преобразования), но использованы два разных identifier-имени (sharedCm/roomCm) без объявленной синонимии. Не найдено других мест: grep -n "sharedCm" даёт единственное совпадение (строка 81).

Не блокирует: контракт однозначен по смыслу в каждом отдельном разделе, значение величины не расходится, автор реализации сам выбирает имя переменной в коде (§7.1, «наименование» — техническое решение). Снимаю как редакционную неточность, а не содержательное расхождение.

L2 (Low, снято с записью). AC1 сохраняет квалификатор «same-thickness», хотя условие эксплицитно расширено дельтой

Где: §12, AC1 (строка 237): «Exact same-thickness shared-wall partition определяется независимо от направления endpoints и порядка комнат» — текст не тронут дельтой. После правки r3 условие §4.4 больше не требует совпадения толщин (partition.cm теперь просто «конечна и положительна»), а AC2 явно покрывает и same-cm, и narrower-, и wider-partition случаи. Формально AC1 по-прежнему описывает только частный (равнотолщинный) фрагмент детектора и не противоречит остальному документу — это может быть намеренной декомпозицией теста (AC1 проверяет инвариантность направления/порядка на самом простом фикстуре, AC2 — полную матрицу толщин), но неявно, без пояснения такого разделения обязанностей между AC1 и AC2.

Не блокирует: детектор направления/порядка действительно не зависит от толщины, дублирования проверки по существу не создаёт риска для реализации, а AC2 покрывает всю матрицу explicитно. Снимаю с записью — уточнение формулировки AC1 (например, «на equal-cm fixture» вместо голого «same-thickness») можно внести попутно при реализации, без нового цикла ревью.

L3 (Low, снято с записью). Второй подряд неверный SHA в хендофф-комментарии автора — повтор находки r2

Где: комментарий автора перед этим раундом называет «Коммит: 2a8b64e7» — git cat-file -t 2a8b64e7 подтверждает: такого объекта в репозитории нет. Фактический SHA дельты — cfc8f211, что сам автор поправил отдельным комментарием четырьмя минутами позже. Содержание дельты соответствует описанным правкам (проверено выше в «Как проверялось»), расхождение только в идентификаторе.

Это дословно та же находка, что была снята как Low в docs/reviews/SPEC-REVIEW-276-r2.md («находка того же типа, что требование §2.10 "SHA в вердикте не назван — находка", только с обратной стороны — автору стоит подставлять реальный SHA, а не плейсхолдер»), и она повторилась во втором подряд хендоффе. Не блокирует ревью (автор сам публикует исправление до начала разбора, содержание не пострадало), но заслуживает более заметной записи, чем в прошлый раз: если паттерн проявится в третий раз, это стоит поднять до Medium как систематическую проблему трассируемости, а не разовую опечатку.

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

  • Центральное продуктовое решение дельты математически точное, а не эвристика на словах. max(roomCm, partitionCm) для точно совпадающей по оси (endpoint-to-endpoint) центрированной пары действительно равен физическому union двух симметрично растущих ±cm/2 тел — подтверждено чтением кода роста стен (physicalBodyParts(), wallCmToUnits(cm) / 2) и docs/WALL-THICKNESS.md §2, не только текстом спека.
  • Причина возврата к ТЗ обоснована реальными данными, а не догадкой. Вторая реально воспроизводимая пара (room wall 30 + partition 20) не была учтена ревьюированным в r2 условием «толщины обязаны совпадать» — без этой правки issue закрылась бы формально (AC2 матрица и unit-тесты прошли бы), но не решила бы половину заявленного в issue воспроизведения. Правка списка затронутых разделов (§3.4/§4.4/§5.4/§8/§10.2/AC2/§15.2) полная и внутренне согласованная (кроме L1/L2).
  • Явное разделение случаев, для которых max не применим, сохранено и расширено, а не ослаблено. §8 теперь читает «неоднородный effective room profile, несколько coincident partitions либо конфликты, для которых один max не описывает исходный physical envelope» — то есть новое правило не тайно расширяет автоматизацию на составные/множественные конфликты, а остаётся строго в границах одного centred exact совпадения.
  • Расширение автоматического поведения Optimize не требует отдельного продуктового вопроса владельцу. Формально это решение в «пограничном случае» (§7.1, класс продуктовых вопросов), но: (а) видимая пользователю толщина при этом не меняется — она и до, и после реконциляции равна видимому physical envelope union, который уже существовал (просто собранному из двух тел с артефактами боковой границы, а не из одного); (б) применяется только по явной команде «Оптимизировать» с preview/report и одноразовым Undo, уже принятыми продуктом ранее в этой же задаче — то есть ничего не меняется без осознанного действия и возможности отмены. Граница между техническим и продуктовым решением по §7.1 («что человек видит или делает») здесь на стороне «не видит нового» — считаю решение автора корректно техническим, эскалация владельцу не нужна.
  • DoR (§2.5) остаётся полностью закрыт. Ни один обязательный раздел не исчез и не стал неполным дельтой: риски (§10.2) получили новую строку по этому же классу дефекта, затронутые файлы (§10.1) не изменились, AC пронумерованы с доказательством, откат (§14) не тронут и остаётся верным.
  • Трассируемость issue ↔ ТЗ ↔ ревью корректна: оба предыдущих документа ревью существуют в дереве с точными SHA, ссылка docs/specs/README.md:73 актуальна, коммит дельты несёт верные трейлеры класса C.

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

  • Не запускал npx tsc --noEmit / npm test / npm run build / node scripts/check-docs.mjs — ни один коммит всего диапазона задачи (a06c94b9..cfc8f211) не трогает src/**; продуктовый код не менялся ни разу с начала этой задачи. Гейты реализации к спек-ревью неприменимы — то же основание, что в r1 и r2.
  • Не запускал browser smoke / golden / инварианты модели (model-invariants) — geometry/wall-реализация ещё не написана, это код будущего раунда код-ревью. Инварианты (#254) станут обязательны там, когда появится экспорт/конфиг с реальной геометрией.
  • Не проверял реальный анонимизируемый экспорт напрямую (файл не приложен, по тем же причинам, что в r1/r2) — доверяю описанию автора о второй паре «room wall 30 + partition 20», перепроверив только его математические следствия в документе, а не сам экспорт.
  • Не оценивал трудозатраты/сложность реализации расширенной матрицы (7/10 в аналитике не пересматривался) — вне компетенции спек-ревью.

Вывод

Блокирующих находок нет. Дельта r3 меняет контракт поведения (расширяет автоматическую канонизацию с «только равные толщины» до «любая positive partition thickness, итог — max(roomCm, partitionCm)»), что по PROCESS §2.10 требует полного разбора, а не разбора по дельте — этот разбор выполнен: вся математика проверена по коду роста стен, а не принята на слова, все связанные разделы (контракт, AC, риски, не-скоуп, техпредположения) согласованы друг с другом. Три Low-находки (несовпадающее именование переменной, стилистически устаревший квалификатор в AC1, повторный неверный SHA в хендоффе) не влияют на корректность или проверяемость ТЗ и сняты с записью здесь. DoR (§2.5) закрыт полностью, открытых продуктовых вопросов владельцу нет — расширение автоматизации Optimize обосновано как техническое решение, не требующее эскалации. ТЗ готово к S5-ready.