mirror of
https://github.com/Matysh/houseplan-card
synced 2026-10-01 04:09:17 +00:00
claude[bot]
@@ -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`.
|
||||
|
||||
---
|
||||
|
||||
<!-- material-anchors: сгенерировано конвейером (#414) -->
|
||||
|
||||
## Материал раунда
|
||||
|
||||
- Ветка: `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
|
||||
```
|
||||
Reference in New Issue
Block a user