From e3073f6840e50394d10c829fc8b4c70cd74391a2 Mon Sep 17 00:00:00 2001 From: "claude[bot]" <209825114+claude[bot]@users.noreply.github.com> Date: Wed, 2 Sep 2026 14:05:01 +0000 Subject: [PATCH] docs: review document for #418 Issue: #418 User-Visible: no --- docs/reviews/SPEC-REVIEW-418-r1.md | 195 +++++++++++++++++++++++++++++ 1 file changed, 195 insertions(+) create mode 100644 docs/reviews/SPEC-REVIEW-418-r1.md diff --git a/docs/reviews/SPEC-REVIEW-418-r1.md b/docs/reviews/SPEC-REVIEW-418-r1.md new file mode 100644 index 00000000..b6bc0229 --- /dev/null +++ b/docs/reviews/SPEC-REVIEW-418-r1.md @@ -0,0 +1,195 @@ +# SPEC-REVIEW-418-r1 + +Issue: #418 · Этап: S4-spec-review (лёгкий трек, `small`) · Заход: r1 · Бюджет циклов: 0/2 +Ревьюер: Claude (роль «ревьюер ТЗ», сессия отдельна от автора) +Материал: тело issue #418 на момент ревью (ТЗ живёт в теле issue, файла в `docs/specs/` нет — +верно для `small`) плюс комментарии `Аналитика` и `Взял`. Код на `dev` +(`b71bc39b`, ветка задачи ещё не заведена — стадия S3→S4, кода нет). + +## Скоуп задачи + +Баг-репорт + ТЗ по двум гонкам состояний в диалоге Help & Feedback (#43): + +1. Поздний ответ `houseplan/support/preview` может применить точную геометрию + после того, как пользователь снял «Приложить данные». +2. Правка текста после неуспешной фактической отправки не меняет + `idempotency_key`, из-за чего relay (24-часовой дедуп, §9.2 #43) возвращает + исходный `report_id`, а исправленный текст не уходит. + +Задача не добавляет функциональность — восстанавливает уже опубликованные +гарантии `docs/SUPPORT-PRIVACY.md` и §6.4/§6.5 `docs/specs/043-private-support-report.md`. +Диалог помощи не входит буквальной строкой в J1–J7 `docs/SCOPE.md`, но он — +существующая принятая поверхность (#43, ревью пройдено ранее), и задача её не +расширяет, а чинит уже данное обещание приватности и доставки. Возражений по +`docs/SCOPE.md` нет. + +Лёгкий трек (`small`) заявлен и подтверждён по факту: одна поверхность (диалог +Help & Feedback, два файла одной фичи — `houseplan-editor-runtime.ts` и +`support-feedback.ts`), нет миграции конфига, нет нового UX-контракта (правится +поведение в рамках уже описанного #43 контракта, а не добавляется новое), +нет влияния на perf/touch. Автор сам назвал, почему не `trivial`: два +независимых, содержательно разных перехода состояния (не «одна строчка»). + +## Как проверялось + +Ревью ТЗ на этапе S4 не имеет диффа продуктового кода — задача ещё не в +разработке. Материал ревью — текст ТЗ; проверка состояла в сверке каждого +фактического утверждения ТЗ с текущим состоянием кода на `dev` и с +каноническими документами, а не в исполнении тестов. + +Прочитано и сверено: + +- `docs/SCOPE.md`, `PROCESS.md` (§2.3–2.5, §5, §7.1), `AGENTS.md` — рамка + процесса и лёгкого трека. +- `docs/SUPPORT-PRIVACY.md` — подтверждает «disabling the attachment... removes + the token», совпадает с формулировкой ТЗ. +- `docs/specs/043-private-support-report.md` §6.4, §6.5, §9.2 — источник цитат + «снятие чекбокса немедленно скрывает raw preview и удаляет token», «один клик + создаёт один idempotency key», «idempotency key retained 24 h and returns the + original report id». Все три цитаты в ТЗ #418 дословно совпадают с текстом + канонического документа — не выдуманы. +- `src/houseplan-editor-runtime.ts:9077-9349` — весь код диалога: `_supportPatch`, + `_updateSupportDraft`, `_buildSupportPreview`, `_setSupportAttachment`, + `_submitSupport`, `_scheduleSupportExpiry`, разметка `.supportpreview` + (:9436-9437). +- `src/support-feedback.ts` целиком — `newSupportDialogState`, `supportDraftError`, + `supportCanSubmit`; `idempotencyKey` генерируется один раз при + `newSupportDialogState()` и нигде в рантайме не переприсваивается — подтверждает + находку (2) как реальный, не выдуманный дефект. +- `test/support-feedback.test.mjs`, `demo/smoke_support_feedback.mjs` — существующее + покрытие; `retryKey`/`retryRequests` (:223, :243) уже проверяют «повтор без + правки сохраняет key», это ровно то, что AC3 требует не сломать. +- `scripts/mutation-gate.mjs` — реестр мутаций существует и работает по + документированному паттерну «патч продуктового кода → тест обязан покраснеть»; + требование ТЗ «доказательство AC1/AC2 включает mutation» не изобретает новый + механизм, а использует принятый в проекте. + +**Гейты (tsc/test/build/smokes) не прогонялись** — они не относятся к материалу +этого раунда: диффа продуктового кода нет, ветка `issue/418-*` не заведена, +проверять нечего. Это решение, а не пропуск: на стадии S4-spec-review гейты +запускаются в цикле реализации (S6) и на код-ревью (S7), не здесь. + +### Проверка конкретных фактических утверждений ТЗ + +1. **«Поздний ответ применяется как ни в чём не бывало»** — подтверждено чтением. + `_buildSupportPreview` (:9193) проверяет `current.attach` только **до** + `await this.host.hass.callWS(...)` (:9195). После await единственная проверка + в `_supportPatch` (:9089) — совпадение `draftId`, без повторной проверки + `attach`. Значит успешный поздний ответ действительно перезаписывает + `status:'idle', preview:null`, поставленные `_setSupportAttachment(false)` + (:9263-9269), на `status:'ready', preview:` (:9237-9241). Реальный + дефект, не домысел. +2. **«Блок `.supportpreview` завязан на `state.preview`, а не на `state.attach`»** + — подтверждено чтением, :9436: `${state.preview ? html\`
...`. + Действительно не проверяет `state.attach`. +3. **«Смягчающее обстоятельство»: сегодня недостижимо через UI, чекбокс задизейблен + во время building** — подтверждено чтением, :9422: + `?disabled=${busy || state.status === 'success'}`, где `busy` включает + `'building'`. Между `_setSupportAttachment(true)` и синхронной установкой + `status:'building'` внутри `_buildSupportPreview` нет `await`, то есть окна для + клика физически нет. Автор не преувеличил риск и честно снизил серьёзность + находки (1) указанием на недостижимость через штатный UI — это корректная, + проверяемая калибровка, а не самоцель для более высокого приоритета. +4. **«`_updateSupportDraft` не блокирует правку в статусе `error`»** — подтверждено, + :9102: guard блокирует только `'building' | 'sending' | 'success'`, `'error'` + в нём нет — правка после неудачи действительно возможна через штатный UI (в + отличие от находки 1, эта реально достижима). +5. **«Ключ идемпотентности создаётся один раз на открытие диалога» и «нигде не + меняется»** — подтверждено: `newSupportDialogState()` (`support-feedback.ts:57`) + присваивает `idempotencyKey` один раз; ни один вызов `_supportPatch` в + `houseplan-editor-runtime.ts` не содержит поля `idempotencyKey`. Дефект (2) + реален буквально так, как описан. +6. **AC3 «retry неизменённого обращения с тем же key» уже покрыт существующим + smoke** — подтверждено, `demo/smoke_support_feedback.mjs:223,243`. + +Ни одно утверждение ТЗ не оказалось домыслом, выданным за факт: там, где автор +делает техническое допущение (сравнение по trimmed-значениям, смена +preview-token = смена payload, best-effort cleanup), оно явно вынесено в раздел +«Принятые технические предположения» и помечено как решаемое реализацией +свободно. + +## Находки + +### Low — нет отдельных разделов «Сценарий» и «Что человек увидит до/после» (waived) + +`PROCESS.md` §7.1 называет их первыми двумя обязательными разделами ТЗ. +В теле issue #418 их нет как отдельных заголовков: «Проблема» написана в +терминах кода (номера строк, имена функций), а не «какая персона на какой +поверхности в какой момент». + +Решение ревьюера: **снимается без правки**. Причины: +- §5 лёгкого трека прямо сужает обязательный шаблон тела issue до + «проблема · контракт · AC1…ACn с доказательством · откат» — сценарий и + «что человек увидит» в этот список не входят; +- персона и поверхность здесь не могут быть предметом спора: диалог Help & + Feedback открывается только при `_canEdit` (:9078), то есть исключительно + Home admin в редакторе на десктопе — ровно то, что `docs/SCOPE.md` называет + единственной персоной редакторов; +- момент и видимое пользователю поведение фактически описаны — раздел + «Ожидаемое поведение» в начале issue и honesty-репорт «смягчающее + обстоятельство» дают то же самое другими словами, включая точную оговорку о + недостижимости первой гонки через штатный UI. + +Не превращается в отдельный issue: находка внутри скоупа и ниже порога +Medium. + +## Что проверено и корректно + +- Обе фактические предпосылки ТЗ (поздний preview-ответ, статичный + idempotency key) подтверждены чтением текущего кода, а не поверены на слово + автору. +- Все три цитаты из #43/`SUPPORT-PRIVACY.md` дословны, не искажены. +- AC1–AC3 однозначны, у каждого назван способ доказательства (`smoke` + + `mutation`/`unit`), и способ реалистичен: существующий harness + `demo/serve.mjs` + мокнутый `callWS` (`demo/smoke_support_feedback.mjs:46-76`) + уже даёт управляемые ответы на `houseplan/support/preview` и `.../submit`, + так что «browser smoke с управляемыми Promise» и «off → on ответы в обратном + порядке» реализуемы без нового харнесса. +- AC3 явно защищает регресс существующего контракта #43 (fresh defaults, + exact bytes preview/download, success, manual recovery, disabled busy + controls, retry с тем же key, responsive 320×760/760×320) — сокращения + скоупа под видом починки нет. +- Технические решения (где живёт generation-счётчик, имя fingerprint-хелпера) + корректно оставлены реализации с пометкой «assumed» там, где это + предположение, а не факт. +- Откат — простой revert, без миграции; согласуется с тем, что + config/schema/backend protocol не меняются. +- Лёгкий трек подтверждён по всем пяти критериям §5 одновременно, не только + заявлен. +- Автор честно откалибровал приоритет находки (1): указал, что через штатный + UI гонка сегодня недостижима, не спрятал это в защиту серьёзности бага. + +## Чего не проверял + +- Гейты `tsc`/`test`/`build`/`smoke`/`golden`/`check-docs`/`no-new-any` — не + прогонял: на S4 нет диффа продуктового кода, гейты относятся к S6/S7. +- Поведение relay (`support.houseplan.tech`) не в этом репозитории; утверждение + «relay вправе вернуть результат первого payload» проверено только по + документации (`docs/specs/043...md` §9.2), не по коду relay — она вне + контроля этого репозитория и вне скоупа ревью. +- Мутанты `scripts/mutation-gate.mjs`, которые ТЗ требует добавить, ещё не + существуют (появятся в реализации) — не мог проверить, что они «обязаны + покрасить smoke», это станет предметом код-ревью (§2.10-подобная проверка + «тест умеет падать»). +- Реальную доступность диалога с клавиатуры/screen reader — не относится к + предмету этого ТЗ (диалог существует с #43, доступность не меняется). + +## Материал раунда + +- SHA `dev`: `b71bc39b` (ветка задачи `issue/418-*` не создана — на момент + ревью задача в S3→S4, кода нет). +- Материал ТЗ: тело issue #418 на момент чтения (комментарии `Аналитика`, + `Взял` — без изменения текста ТЗ). + +## Вердикт + +Зелёный. High: 0 · Medium: 0 (Low снят решением ревьюера, см. выше). + +--- + + + +## Материал раунда + +- Ветка: `dev`, коммит `` — ребейз его осиротит, и это нормально: ниже якоря, которые ребейз не меняет. +- Якоря снять не удалось: ветки задачи нет, материал читался по `dev`.