From bf40e26ed083987f8a19242bea070805f52b6d0a Mon Sep 17 00:00:00 2001 From: "claude[bot]" <209825114+claude[bot]@users.noreply.github.com> Date: Tue, 1 Sep 2026 14:15:51 +0000 Subject: [PATCH] docs: review document for #405 Issue: #405 User-Visible: no --- docs/reviews/SPEC-REVIEW-405-r1.md | 250 +++++++++++++++++++++++++++++ 1 file changed, 250 insertions(+) create mode 100644 docs/reviews/SPEC-REVIEW-405-r1.md diff --git a/docs/reviews/SPEC-REVIEW-405-r1.md b/docs/reviews/SPEC-REVIEW-405-r1.md new file mode 100644 index 00000000..baaf62e5 --- /dev/null +++ b/docs/reviews/SPEC-REVIEW-405-r1.md @@ -0,0 +1,250 @@ +# SPEC-REVIEW-405-r1 + +- Issue: https://github.com/Matysh/houseplan-card/issues/405 +- ТЗ: `docs/specs/405-dropped-promise-and-witness-floor.md` +- SHA ТЗ: `cd0c85b9ad5096d4772f99edc0354dea6a62f241` +- Заход: r1 · блокирующих циклов израсходовано 0 из 4 (до этого раунда) +- Трек: полный (заявлен автором — две несвязанные поверхности, `src/**` и + `scripts/**`; критерий `small` «одна поверхность» не выполняется — согласен) +- Вердикт: **жёлтый** + +## Скоуп ревью + +Первый заход, дельты нет — разбор ТЗ целиком по §7.1: обязательные разделы, +однозначность и доказуемость AC1…AC8, соответствие описанных технических фактов +реальному коду (`src/houseplan-editor-runtime.ts`, `src/houseplan-card.ts`, +`src/editor-secondary.ts`, `demo/smoke_free_walls.mjs`, +`scripts/docs-acceptance.mjs`, `test/docs-acceptance.test.mjs`), а также +продуктовая рамка по `docs/SCOPE.md` (задача инфраструктурная по духу — не про +J1–J7, а про доказательность гейтов, что для класса B/тестовой инфраструктуры +уместно и не требует отдельного продуктового обоснования; правка `src/**` +делает задачу полным треком по §1, но не продуктовой в смысле пользовательского +сценария — ТЗ прямо пишет «видимого поведения не меняет», что не противоречит +скоупу, поскольку задача чинит доказательство существующего инварианта, а не +добавляет функциональность). + +## Как проверялось + +Читкой кода, без исполнения (spec review, гейты не прогонялись — это код-ревью +этап, здесь ему нечего проверять по §2.4). Для каждого технического утверждения +ТЗ — сверка со строками файлов на HEAD (`cd0c85b9`): + +- `src/houseplan-editor-runtime.ts:3050-3150, 5282-5300` — таблица «Место» для + M2 (`_deletePhysicalSelection`, `_runEditorContext`, `_deleteDraftWhole`); +- `src/editor-secondary.ts:200-224` — `runContext`; +- `src/houseplan-card.ts:3015-3022, 7845-7874` — вызывающие и параллельные + обёртки `_deletePhysicalSelection`/`_deleteDraftWhole`/`_deleteDraftSegment`; +- `demo/smoke_free_walls.mjs:1-30, 175-208` — стаб `_confirmDanger` и вызовы + `_deletePhysicalSelection`; +- `scripts/docs-acceptance.mjs:1-128` — `docsWitnessFloor`, + `docsAcceptancePlan`, вычисление `floor`; +- `test/docs-acceptance.test.mjs` — существующие тесты AC7/AC8; +- `scripts/golden-acceptance.mjs:1-200`, `demo/golden/accept.mjs:1-121`, + `demo/golden/run.mjs:800-850` — сверка риска «изменение порога затрагивает + golden»; +- `scripts/mutation-gate.mjs` — конвенция именования мутантов (`id: 'kebab-case'`), + сверена с предложенными `draft-delete-drops-the-promise` и + `witness-floor-counts-survivors` — согласуется. + +Один факт проверен эмпирически (не просто чтением): предположение, что +TypeScript пропустит функцию, аннотированную `(): void`, но фактически +возвращающую `Promise` изнутри, — **проверено `tsc`** на изолированном +файле (см. находку M2-a ниже), потому что от этого зависела оценка серьёзности +находки, а не только её наличие. + +Issue #406 (куда ТЗ отправляет один Low-пункт) прочитан целиком через +`gh issue view 406` — сверка ссылки «#406 «д»». + +## Находки + +### M2-a (Medium, в скоупе) — таблица «Место» неполна: пропущена обёртка `houseplan-card.ts:7856`, и без неё правка не пройдёт `tsc --noEmit` + +Файл: `docs/specs/405-dropped-promise-and-witness-floor.md`, раздел «(1) M2», +таблица «Место» и абзац «Практически». + +**Что не так.** Смок вызывает не `houseplan-editor-runtime.ts`, а +`window.__card` — экземпляр класса `houseplan-card.ts` (`demo/srv/demo.html:194` +создаёт ``, `demo/serve.mjs` его же отдаёт в `window.__card`, +`smoke_free_walls.mjs:194,199` зовёт `c._deletePhysicalSelection()`). Реальная +точка входа — обёртка `src/houseplan-card.ts:7856`: + +```ts +private _deletePhysicalSelection = (): void => { + return this._editorRuntimeOrThrow()._deletePhysicalSelection(); +} +``` + +Таблица «Место» в ТЗ называет только `editor-secondary.ts:221`, +`houseplan-editor-runtime.ts:5290` и `:3073` — эту строку не называет вовсе. +«Практически» перечисляет три функции для правки (`_deletePhysicalSelection` +в runtime, `_runEditorContext`, `runContext`) — тоже без неё. + +**Воспроизведено**: изолированный `tsc --strict --noEmit` на паре функций, +повторяющей форму `houseplan-card.ts:7856` после того, как внутренний вызов +меняет тип на `Promise`, а внешняя обёртка остаётся `(): void`: + +```ts +async function inner(): Promise {} +const wrapper = (): void => { + return inner(); +}; +``` + +``` +error TS2322: Type 'Promise' is not assignable to type 'void'. +``` + +Это не эстетика типов — это реальный отказ гейта `npx tsc --noEmit`, +единственного среди трёх дешёвых гейтов, который правка данной задачи +действительно затрагивает. Если исполнитель проверит только смок (JS, +типов не видит) или отправит правку без прогона `tsc`, регресс поймает CI, +но раунд код-ревью на это тратиться не должен. + +**Это не догадка о недостающем шаге, а видимый паттерн этого же файла.** +Строками ниже, `houseplan-card.ts:7864` и `:7868`, у соседних методов +(`_deleteDraftWhole`, `_deleteDraftSegment`) обёртки **уже** типизированы +`Promise`, зеркаля асинхронные методы runtime. Это подтверждает: в файле +есть устоявшаяся конвенция «тип обёртки повторяет тип метода runtime», и +`_deletePhysicalSelection`-обёртка на `:7856` — единственная, которую эта +задача обязана привести к тому же виду, а ТЗ её не называет. + +**Требуется**: добавить `src/houseplan-card.ts:7856` в таблицу «Место» и +в перечень правок «Практически» — тип обёртки меняется на `Promise` с +`return`, как у `_deleteDraftWhole`/`_deleteDraftSegment` рядом. + +Серьёзность — Medium, не High: контракт AC1–AC8 в остальном верен и +достижим, дефект — не в контракте, а в полноте технической карты внутри уже +названного в скоупе файла (`src/houseplan-card.ts` фигурирует в файлах задачи +неявно — это тот же модуль, где объявлена обёртка `_deleteDraftWhole`, +`_deleteDraftSegment`, уже упомянутых как соседний прецедент); дешёвый гейт +`tsc` его поймает при обычной проверке, но ТЗ, которое ведёт прямиком к +провалу первого же гейта, не «выполнимо без доработки» по мерке §2.4. + +### M4-b (Low) — ссылка «#406 «д»» на несуществующий пункт + +Файл: `docs/specs/405-dropped-promise-and-witness-floor.md`, раздел «Скоуп / +не-скоуп»: + +> затирание `acceptance.declared` при повторной приёмке (#406 «д») — соседняя +> строка того же файла, но другой контракт + +Прочитан issue #406 целиком (`gh issue view 406`): в нём ровно четыре пункта, +(а)–(г) — мёртвые ключи `confirm.*`, роль диалога, непокрытая HA-ветка +подтверждения, снапшоты area. Пункта «(д)» нет. Оригинальный аудит-комментарий +в теле #405 упоминает это как отдельный Low-пункт («Отдельно, из того же +разбора (Low, чинить попутно): повторная приёмка неизменённого набора +затирает `acceptance.declared`… `scripts/docs-accept.mjs:142-150`»), но нигде +не заводит его как пункт «(д)» и не кладёт в #406. + +Итог: Low-находка из исходного аудита сейчас **не отслеживается ни одним +issue** — ссылка ТЗ на несуществующий пункт создаёт впечатление, что она +где-то учтена, хотя это не так. + +**Решение ревьюера**: не правится сейчас (сам дефект `docs-accept.mjs:142-150` +за пределами скоупа #405 — другая строка того же файла, другой контракт, как +верно замечает автор) — но ссылка ложная и должна быть исправлена: либо +пункт действительно добавляется в #406, либо ТЗ отправляет его в новый issue, +либо ТЗ формулирует «Low, пока нигде не заведён» вместо несуществующей ссылки. +Это правится в рамках текущего цикла возврата (Low, не отдельный лимит). + +## Медиум вне скоупа — заведён отдельным issue + +### Golden-эталоны подвержены тому же классу дефекта, что и M4 — #408 + +`scripts/golden-acceptance.mjs:182-183` считает порог свидетелей от +`withBaseline.length` — числа сцен со статусом **не** `missing-baseline`, то +есть от того, что уцелело на диске, а не от размера известной матрицы +(`GOLDEN_SCENARIOS.length`). Проверено чтением всей цепочки +(`demo/golden/run.mjs:836-839` ставит `status: 'missing-baseline'`, когда файла +эталона нет; `goldenAcceptanceRefusal` в `scripts/golden-acceptance.mjs:59-93` +**требует**, чтобы каждая такая сцена была объявлена через `--expect-new` — +и не более; `demo/golden/accept.mjs:64-72` вызывает `goldenWitnessRefusal` уже +после этой проверки). Модельный разбор: `git rm demo/golden/baselines/*.png` ++ объявление всех сцен `--expect-new` → каждая сцена легально проходит +`goldenAcceptanceRefusal` → `withBaseline.length === 0` → +`goldenWitnessFloor(0) === 0` → `witnesses.length (0) < floor (0)` ложно → +отказа `goldenWitnessRefusal` нет. `--reviewed` не является независимой +проверкой (булев CLI-флаг), так что рассуждение ТЗ #405 о том, что M4 +недостаточно прикрыт `--reviewed` для `docs-accept.mjs`, симметрично относится +и сюда. + +ТЗ #405 обсуждает golden только в разделе «Риски» и только применительно к +формуле (`goldenWitnessFloor` та же формула, что `docsWitnessFloor`, и должна +остаться той же) — не к тому, что вызывающий код (`withBaseline.length`) +подвержен той же уязвимости на входе, что чинит M4. Файл другой +(`scripts/golden-acceptance.mjs`, не `scripts/docs-acceptance.mjs`), скоуп +ТЗ его не называет — правка не входит в #405. + +Issue заведён: **#408**, метки `bug`, `P2`, `S1-new`, `infra`, ссылка на #405. + +## Проверено и корректно + +- Обязательные разделы §7.1 присутствуют все: сценарий, что человек увидит, + проблема, скоуп/не-скоуп, контракт, UX, модель данных/миграция, i18n, AC1–AC8 + с доказательством, план автотестов, риски, откат, release-артефакты. +- Продуктовых вопросов владельцу нет, догадок, выданных за факт, в контракте + M2/M4 не найдено (кроме описанного выше пробела в таблице «Место» — это не + догадка, а неполнота карты). +- Таблица «Место» для M2 (кроме отсутствующей строки `houseplan-card.ts:7856`) + точно соответствует коду: `runContext` (`editor-secondary.ts:221`), + `_runEditorContext` (`houseplan-editor-runtime.ts:5290`), + `_deletePhysicalSelection` (`:3073`, ветка `sel.kind === 'draft'` действительно + роняет промис через `return`), `_deleteDraftWhole` (`:3119`, действительно + `async`, `await _confirmDanger`). +- Смок `demo/smoke_free_walls.mjs:16` — стаб `_confirmDanger = async () => true` + резолвится в том же микротаске, и `:194,199` действительно не проверяет + ожидание по-настоящему; воспроизведение из тела issue (макрозадача → два + `FAILED`) правдоподобно и согласуется с кодом. +- `scripts/docs-acceptance.mjs:44-46,107-108` — формула и место вычисления + `floor` от `withCommitted.length` подтверждены; контраст «PNG на месте» vs + «PNG удалены» из тела issue воспроизводим по чтению кода без исполнения. +- Предложенный контракт AC6 (`floor` от размера набора сценариев, не от + наличия файлов) корректно сохраняет существующее поведение AC7 — проверено + арифметически: `docsWitnessFloor(ids.length)` и текущее + `docsWitnessFloor(withCommitted.length)` дают одно и то же значение во всех + ветках существующих тестов `test/docs-acceptance.test.mjs`, включая + «кадр без закоммиченной пары не может быть свидетелем» (`floor` остаётся 1 + в обоих случаях — 3 или 1 округляются к одному порогу на этом наборе). +- «Не-скоуп» по `_deleteDraftSegment` — проверено: у него отдельный, + корректно типизированный путь (`houseplan-card.ts:7868` уже `Promise`, + зовётся напрямую из своей кнопки, не через `_deletePhysicalSelection`) — + исключение обосновано верно, не прячет тот же дефект. +- Риск «изменение порога затрагивает golden» правильно определяет, что *формулу* + трогать нельзя без зеркалирования — но не замечает независимый дефект на + стороне вызова (см. #408). +- AC2 (отрицательный прогон обязателен) сформулирован по правилу «тест должен + уметь падать» и предлагает конкретный механизм (`scripts/mutation-gate.mjs`, + который в проекте уже используется по этому шаблону) — не декларация. +- Откат и release-артефакты соразмерны факту «видимого поведения нет» + (`User-Visible: no` подтверждён отсутствием изменений в UX/i18n разделах). + +## Чего не проверял + +- Гейты (`tsc`, `npm test`, `npm run build`, смоки, `golden:verify`, + `model-invariants`) не прогонял — этап spec review, кода задачи ещё нет, + прогонять нечего. Эмпирическая проверка ограничена одним изолированным + TS-файлом вне репозитория, подтверждающим находку M2-a (см. выше), — это не + прогон проектных гейтов. +- Не проверял `scripts/mutation-gate.mjs` на предмет того, заведутся ли + предложенные мутанты технически без ошибок раннера — это вопрос реализации, + а не ТЗ; на этапе спеки достаточно, что названный механизм существует и + используется в проекте по тому же образцу. +- Не проверял golden `test/golden-policy.test.mjs` целиком — находка по #408 + установлена чтением `scripts/golden-acceptance.mjs` и `demo/golden/accept.mjs` + напрямую; тестовое покрытие этой ветки (принимает ли текущий набор тестов + сценарий «все эталоны удалены») не сверял — это будет частью работы над #408, + не #405. +- Не оценивал вне двух Medium/одного Low другие возможные дефекты в + `scripts/docs-accept.mjs` целиком (не-`docs-acceptance.mjs`) — за пределами + того, что уже упомянуто как «не-скоуп» самим ТЗ. + +## Итог + +Один Medium **в скоупе** (M2-a) не даёт зелёный вердикт по §2.4 — ТЗ ведёт +прямиком к отказу гейта `tsc --noEmit`, если исполнитель буквально следует +таблице «Место». Плюс один Low (ложная ссылка на #406) для правки в этом же +цикле. Medium вне скоупа заведён отдельным issue (#408), в задаче #405 не +чинится. + +**Вердикт: жёлтый.** Возврат автору: дополнить таблицу «Место» и раздел +«Практически» строкой `src/houseplan-card.ts:7856`, поправить ссылку на #406.