diff --git a/docs/reviews/SPEC-REVIEW-434-r2.md b/docs/reviews/SPEC-REVIEW-434-r2.md new file mode 100644 index 00000000..cb233272 --- /dev/null +++ b/docs/reviews/SPEC-REVIEW-434-r2.md @@ -0,0 +1,182 @@ +# SPEC-REVIEW-434-r2 + +- Issue: https://github.com/Matysh/houseplan-card/issues/434 +- Этап: ревью ТЗ (PROCESS.md §2.4) +- ТЗ: `docs/specs/434-v171-polish-audit.md` +- Проверяемый SHA: `904a989af70481eb7f34c1e3cfc99de115ba869f` (= `git rev-parse HEAD`, + совпадает с SHA, названным автором в хендоффе r2) +- Заход: r2 · блокирующих циклов израсходовано 1 из 4 (полный трек, лимит 4) +- Вердикт: **зелёный** + +## Скоуп раунда + +Повторный раунд по §2.9/2.10: предмет разбора — дельта ТЗ между материалом r1 +(`5566d6f9898e3c6d21f3e92a2ddf94536c86edc4`) и текущим `904a989a`, а не задача +целиком. Дельта проверена командой: + +``` +git diff 5566d6f9898e3c6d21f3e92a2ddf94536c86edc4..904a989af70481eb7f34c1e3cfc99de115ba869f -- docs/specs/434-v171-polish-audit.md +``` + +`git diff --stat` на этом же диапазоне подтверждает, что дельта не касается +ничего, кроме файла ТЗ, — второй изменённый файл в диапазоне, +`docs/reviews/SPEC-REVIEW-434-r1.md`, это публикация документа предыдущего +раунда конвейером (класс C, не предмет разбора). + +Дельта локальна: она правит ровно одну тему — контракт job-level +`timeout-minutes` для job `smoke` (пункт 8 / AC9 / раздел 7.7 / «Производительность» +/ «Риски» / таблица «чем краснеет» / тест-план / «Принятые предположения»). +Условия «разбор остаётся полным» (ребейз на ушедший вперёд `dev`, смена +контракта поведения, новая подсистема, объём дельты сопоставим с исходной +задачей) не выполнены: ни ребейза, ни новой подсистемы, дельта — 40 строк +правки внутри уже существующего AC9, без изменения продуктового контракта +(изменение чисто CI/gate, не видимое пользователю). Сокращённый объём разбора +обоснован. + +## Закрытие раунда r1 + +| Находка r1 | Чем закрыта | Где это видно | +|---|---|---| +| [Medium, в скоупе] ТЗ утверждало неизменный `timeout-minutes: 20` у job `smoke`, хотя эти 20 минут принадлежат `performance_smoke`, а у `smoke` собственного лимита нет вовсе (дефолт GitHub 360 мин) | Текст ТЗ переписан по всем перечисленным в находке местам: п.8 «Подтверждённые причины», раздел 7.7 «Bounded smoke execution», «Производительность», «Риски», AC9, «чем краснеет» (AC9), тест-план (шаг 8), «Принятые технические предположения». Факт теперь сформулирован верно, и **в скоуп задачи добавлено решение**: job `smoke` получает собственный `timeout-minutes: 20` — новая независимая граница, отдельная от `performance_smoke` | `docs/specs/434-v171-polish-audit.md:73-76` («сама job `smoke` не имеет `timeout-minutes` и потому наследует 360-минутный default… Единственные 20 минут в workflow относятся к другой job — `performance_smoke`»); `:247-250` («Job `smoke` получает собственный `timeout-minutes: 20`: это отдельная граница, которой сейчас нет…»); `:384-389` (AC9 текст); `:420` (таблица «чем краснеет», мутация «удаляет `smoke.timeout-minutes`»); `:446-448` (тест-план п.8); `:470-472` (риски); `:517-519` (принятые предположения) | + +Факт перепроверен независимо от заявления автора: `grep -n timeout +.github/workflows/validate.yml` на актуальном HEAD даёт ровно одно +совпадение — строка 715, job `performance_smoke` (заголовок job на строке 705, +`timeout-minutes: 20` на 715). Job `smoke` (заголовок строка 486, цикл +`for f in demo/smoke_*.mjs` строка 554) действительно не содержит +`timeout-minutes` нигде в своём теле. Формулировка ТЗ после правки точна. + +## Проверка дельты по существу (не только «текст поменялся») + +Так как дельта меняет именно AC9 (появляется новое обязательство — job-level +timeout), доказательство которого дельта задевает, AC9 разобран заново, а не +унаследован: + +- **Согласованность формулировок.** Все шесть затронутых мест (см. таблицу + выше) описывают одну и ту же границу одинаково: 20 минут, независимая от + `performance_smoke`, покрывает весь shard «при системном зависании либо + серии отдельных отказов» (`:470-472`), а не заменяет per-file timeout. + Противоречий между разделами не найдено. +- **Реалистичность значения 20 минут.** Комментарий в самом + `.github/workflows/validate.yml:492-494` фиксирует, что последовательный + прогон всех смоков занимал ~7.5 минут до шардирования на 3 части, то есть + штатный шард сейчас укладывается в ~2.5 минуты — у 20-минутного лимита + восьмикратный запас на холодный раннер, сопоставимый с обоснованием того же + значения у `performance_smoke` (`:711-713`, «15 минут не хватало... холодный + кэш»). Патологический сценарий, где job-level timeout прервёт shard до того, + как последовательно доработают все per-file timeout (215 файлов / 3 шарда + ≈ 72 на шард × 190 c ≈ 228 минут в предельном случае «все зависли») — + ТЗ его не скрывает, а называет прямо как ожидаемое поведение защиты «при + серии отдельных отказов», а не как гарантию довести до конца каждый файл. + Технической ошибки или недосказанности здесь нет. +- **Проверяемость AC9 после правки.** «Чем краснеет» (`:420`) называет два + независимых свидетеля — мутацию per-file wrapper (`plain node "$f"`) и + отдельно мутацию `smoke.timeout-minutes` (удаление поля) — и требует, чтобы + workflow-контрактный тест реагировал на оба независимо + («не находит одну из двух независимых границ»). Это не создаёт пустого + столбца и не путает test-time симуляцию (per-file probe) со статическим + YAML-контрактом (job-level timeout, аналог которому — `test/validate-workflow.test.mjs`, + подтверждённый как рабочий прецедент ещё в r1). Инженерно реализуемо тем же + способом, что и остальной AC9. +- **Затронутые модули учитывают файл workflow.** `docs/specs/434-v171-polish-audit.md:339-340` + прямо называет `.github/workflows/validate.yml` в списке затронутых модулей — + правка AC9 не «повисает» без соответствующего файла в скоупе. + +Новых дефектов дельта не вносит. + +## Унаследовано из r1 + +Всё, что не задето дельтой, принимается без повторной проверки — документ +`docs/reviews/SPEC-REVIEW-434-r1.md` (материал: ветка `issue/434-v171-polish-audit`, +коммит `5566d6f9898e`, дерево `1e1861969610794ffa6a9d8458b52ef3695496b8`, +блоб ТЗ `e0229c0cd6550a1c44b978335c52bd2b264b1c52`): + +- Обязательные разделы §7.1 присутствуют полностью; продуктовые «Сценарий» и + «Что человек увидит до и после» отвечают на оба обязательных вопроса + (персона/поверхность/момент; видимое изменение без терминов реализации) — + и эти разделы дельтой r2 не тронуты (см. diff — правки только в §7.7, + «Производительность», «Риски», AC9, таблице и тест-плане). +- Восемь из девяти пунктов «Подтверждённые причины» (все, кроме п.8, который + и есть предмет дельты) построчно сверены с `origin/dev` двумя независимыми + агентами и подтверждены дословно, включая: `read_catalog`/`blob.is_file()` + (п.1), отсутствие capability-гарда в `space-card.ts:732` (п.2), кэш по + id-set без ревизии config (п.3), отсутствующий негативный тест «sidecar без + blob» (п.4), `reused:true` без catalog-записи (п.5), locale gate снимок + прошлого рендера (п.6, номера строк помечены как приблизительные), отсутствие + отдельного witness у `snapshotBindings.has(binding)` (п.7), throw до discard + в support preview (п.9). +- AC1–AC8, AC10–AC12 однозначны, способ доказательства назван для каждого; + таблица «чем краснеет» (#435) заполнена без пустых столбцов для всех, кроме + AC9, которую r2 перепроверил заново (см. выше). +- Маршрут (full) обоснован верно — критерий лёгкого трека не проходит из-за + нескольких независимых поверхностей и подсистем. +- Не-скоуп корректно исключает соседние более крупные рефакторинги + (LanguageRuntime/support pipeline/шардирование, изменения schema/API version, + видимого текста/UI). +- Изменение семантики `reused` не является изменением видимого контракта — + поле нигде не читается в `src/**`. +- AC3 (удаление orphan-блобов) не противоречит правилу SCOPE.md «никогда не + удалять файл по предположению»: причина удаления — явный вызов + `houseplan/assets/delete` с точным id, а не вывод из отсутствия ссылок. +- Раздел «Откат» корректно называет границы (миграции нет, восстановленные + sidecar остаются валидными записями); эти строки дельтой не менялись. +- Технические прецеденты (`scripts/mutation-gate.mjs` уже содержит Python-мутанты, + `test/validate-workflow.test.mjs` — рабочий образец YAML-контрактного теста) + подтверждены на `origin/dev` в r1 и остаются в силе — AC9 после правки + использует тот же прецедент. + +## Что проверено в r2 (сверх наследования) + +- Дельта ТЗ (`docs/specs/434-v171-polish-audit.md`, r1→r2) построчно прочитана + целиком. +- Факт из находки r1 перепроверен заново на актуальном HEAD: + `grep -n timeout .github/workflows/validate.yml` (1 совпадение, строка 715, + job `performance_smoke`); контекст job `smoke` (строки 486–586) и + `performance_smoke` (строки 705–720) прочитан целиком, `timeout-minutes` в + теле `smoke` действительно отсутствует. +- Реалистичность выбранного значения 20 минут сверена с комментарием в самом + workflow-файле (фактическое время шардированного прогона) и числом реальных + smoke-файлов (`ls demo/smoke_*.mjs | wc -l` → 215). +- Согласованность формулировки новой границы across всех шести затронутых + разделов ТЗ. +- `git diff --stat` между SHA r1 и r2 — подтверждено, что кроме ТЗ и + публикации документа r1 больше ничего не менялось (продуктовый код не + затронут, разбор кода в этом раунде не требуется). + +## Чего не проверял + +- Гейты (`typecheck`/`test`/`build`/backend pytest) не запускались: диапазон + правок — документация класса C (ТЗ + документ предыдущего ревью), кода нет. +- Не проверялся сам код реализации — его по-прежнему нет, это ревью ТЗ. +- Разделы, не затронутые дельтой (см. «Унаследовано из r1»), не разбирались + заново по существу — они наследуются от r1 согласно §2.10. +- Не оценивался точный сценарий частичного отказа (например, ровно 6–7 + зависших файлов из 72 в шарде) на предмет того, какие именно файлы успеют + отработать до срабатывания job-level timeout — ТЗ прямо не гарантирует + «довести шард до конца при серии отказов», и это осознанно названо + принятым поведением защиты, а не заявленным AC. + +## Материал раунда + +- Ветка: `issue/434-v171-polish-audit`, SHA `904a989af70481eb7f34c1e3cfc99de115ba869f`. +- Дельта: `git diff 5566d6f9898e3c6d21f3e92a2ddf94536c86edc4..904a989af70481eb7f34c1e3cfc99de115ba869f -- docs/specs/434-v171-polish-audit.md`. +- Предыдущий раунд: `docs/reviews/SPEC-REVIEW-434-r1.md`, материал — + ветка `issue/434-v171-polish-audit`, коммит `5566d6f9898e`, дерево + `1e1861969610794ffa6a9d8458b52ef3695496b8`, блоб ТЗ + `e0229c0cd6550a1c44b978335c52bd2b264b1c52`. + +--- + + + +## Материал раунда + +- Ветка: `issue/434-v171-polish-audit`, коммит `904a989af704` — ребейз его осиротит, и это нормально: ниже якоря, которые ребейз не меняет. +- Дерево материала: `37d24151e91b534280e7ce0d4a437cd082afdc30` + ``` + git log --all --format='%H %T' | grep 37d24151e91b + ``` +- ТЗ `docs/specs/434-v171-polish-audit.md`, блоб `e235fcef2bcd817c39c0c5a9134fd2ec8f6eb2f1` + ``` + git log --all --find-object=e235fcef2bcd817c39c0c5a9134fd2ec8f6eb2f1 -- docs/specs/434-v171-polish-audit.md + ```