diff --git a/docs/reviews/SPEC-REVIEW-510-r4.md b/docs/reviews/SPEC-REVIEW-510-r4.md new file mode 100644 index 00000000..e05b5c18 --- /dev/null +++ b/docs/reviews/SPEC-REVIEW-510-r4.md @@ -0,0 +1,92 @@ +# SPEC-REVIEW-510-r4 + +- **Issue:** https://github.com/Matysh/houseplan-card/issues/510 +- **ТЗ:** `docs/specs/510-mutants-on-candidate-and-review-waits-validate.md` +- **Этап:** spec (S4-spec-review) · заход r4 · блокирующих циклов израсходовано 3/4 до этого раунда +- **Материал:** ТЗ на HEAD рабочей копии (`23641f4ec1764b5654d3893bb38e6d4904909db8`), дельта против материала r3 (`3854fe72ac59`) +- **Предыдущий раунд:** SPEC-REVIEW-510-r3, вердикт жёлтый, SHA материала `3854fe72ac59`, High 0 · Medium 1 · Low 0 + +## Скоуп ревью + +Разбор — по дельте (PROCESS.md §2.9/§2.10, issue #214). Дельта локальна: та же ветка, тот же файл, без ребейза, без смены подсистемы; `git diff 3854fe72..23641f4e --stat` — 1 файл, 4 вставки/4 удаления, один абзац §5.2 плюс одна строка описания теста в §8. Это прямая правка на находку r3 (Medium 1 «в скоупе»), сформулированную предельно узко: три места назвали контракт `proceed`/`reuse`-конъюнкта по-разному. Проверка сведена к (1) построчному закрытию находки r3 и (2) сверке новой формулировки с реальным `process.yml`, как и в r2/r3 — именно там нашлись предыдущие находки. Разделы, которых дельта не касается текстуально и чей контракт не меняется этой правкой (§1–§4, §5.1, §5.2 кроме первого абзаца, §5.3, §6, §7, §9, §10.0, §10.1, §11, §12), не перепроверялись — унаследованы (см. ниже). + +## Как проверялось + +- `git diff 3854fe72..23641f4e -- docs/specs/510-mutants-on-candidate-and-review-waits-validate.md` — вся дельта, один хунк в §5.2 (первый абзац и следующая за ним строка про «все последующие шаги ревью») плюс одна строка в §8. +- `git show 23641f4e` — сообщение коммита прочитано целиком: «`proceed` replaces the `rebase.conflict != 'true'` conjunct alone; the existing `reuse != 'true'` (#499), stage and decide conjuncts stay on every step that has them. The "single variable" claim is narrowed accordingly» — совпадает с текстом правки, а не расходится с ним. +- Реальный `process.yml` перечитан построчно (`grep -n` по именам всех перечисленных в ТЗ шагов и по `steps.rebase.outputs.conflict`) для каждого шага, упомянутого в новой формулировке: + - `validated` («Зелёные гейты на этом SHA», строка 486) — сегодня один конъюнкт `rebase.conflict != 'true'` → корректно заменяется на `gate.outputs.proceed == 'true'` целиком, других конъюнктов терять негде; + - «Установить зависимости» (515), кэш/установка Chromium (522, 529 — плюс `pw.cache-hit`), «Установить Claude Code» (545), `Review` (562), «Опубликовать документ ревью» (786), «Материал раунда воспроизводим» (948) — везде `rebase.conflict != 'true' && reuse.reuse != 'true'` (кое-где ещё третий конъюнкт вроде `pw.cache-hit`); ТЗ теперь прямо говорит «заменяет только конъюнкт `rebase.conflict`», остальные не трогаются — совпадает построчно; + - «Решение по вердикту» (965), «Переставить метку» (1066) — один конъюнкт `rebase.conflict != 'true'` → заменяется целиком, как и `validated`; + - «dev ушёл вперёд, пока шло ревью» (1020, не названа по имени в перечне ТЗ, но покрыта общим правилом «`stage == 'code'` там, где он есть») — `rebase.conflict != 'true' && stage == 'code'` → по общему правилу конъюнкт `stage` остаётся, заменяется только `rebase.conflict`; правило в тексте общее, а не только для перечисленных примеров, так что этот непоименованный шаг им тоже корректно покрывается; + - «Слить ветку в dev» (1049) — сегодня `stage == 'code' && decide.green == 'true'`, конъюнкта `rebase.conflict` в этом条ии вообще нет; ТЗ отдельно это оговаривает («по-прежнему `decide.outputs.green == 'true'`») и поясняет, что защита приходит транзитивно через `decide` (который сам гейтится через `proceed`) — логически корректно: если `gate` красный, `decide` не выполняется, `decide.outputs.green` пуст ≠ `'true'`, слияние не происходит. +- `test/review-doc-guard.test.mjs:592-603` (существующий тест #499 «конвейер: зелёный вердикт применяется повторно без модели») перечитан ещё раз: `assert.match(reviewStep, /steps\.reuse\.outputs\.reuse != 'true'/, ...)` — новая формулировка ТЗ прямо называет этот тест («тест #499 ... остаётся зелёным») и требование `reuse != 'true'` у шага `Review` сохранено текстом правки — проверено, что ссылка на тест точна (номер строки, имя теста, ассерт совпадают). +- Перечитан весь абзац §5.2 целиком (строки 69–76 в старой нумерации), включая непереписанную с r2 фразу про `validated`, «при `proceed == 'true'` на этапе code» — на предмет того, снимает ли новая (более узкая) формулировка «единственной переменной» второе, менее заметное замечание r3 (строка 73 против строки 69). + +## Закрытие раунда r3 + +| Находка r3 | Чем закрыта | Где это видно | +|---|---|---| +| Medium 1 — унификация «всё через `proceed`» для группы «продолжение ревью» теряла существующий конъюнкт `steps.reuse.outputs.reuse != 'true'`, буквальная реализация открыла бы повторный вызов модели и установку зависимостей/Chromium на reuse fast-path (#499), в противоречии с `test/review-doc-guard.test.mjs:596` | **Закрыта.** Абзац переписан: «Гейт добавляет в условия последующих шагов ровно одну переменную — `proceed`... и она **заменяет только конъюнкт** `steps.rebase.outputs.conflict != 'true'`; все прочие конъюнкты существующих условий (`reuse != 'true'` у зависимостей/Chromium/`Review`; `stage == 'code'` там, где он есть; `decide.outputs.green == 'true'` у слияния) остаются как есть». Пример «Установить зависимости» прямо выписан как `proceed == 'true' && reuse != 'true'`. Проверено построчно против реального `process.yml` (см. «Как проверялось») — совпадает для всех перечисленных и для непоименованного, но структурно идентичного шага «dev ушёл вперёд». | `docs/specs/510-*.md`, §5.2, абзац "Гейт добавляет..." и следующий пункт списка | +| — та же находка, встроенное противоречие: строка 73 («`validated`... при `proceed == 'true'` **на этапе code**») опровергала заявленную в старой строке 69 абсолютную «единственную переменную» | **Закрыта снятием избыточного обобщения.** Формулировка «единственная переменная» больше не заявлена как абсолютная — теперь это «ровно одна переменная, которую добавляет гейт, и она заменяет один конкретный конъюнкт»; существование других, ранее существовавших переменных (`stage`, `reuse`, `decide.green`) explicitно признано той же фразой. Фраза про `validated` «на этапе code» описывает внутреннюю логику скрипта (что ищет `validated`), а не второй конъюнкт `if:` — она сегодня не входит в `if:`-условие шага (проверено: у `validated` в реальном коде только `rebase.conflict != 'true'`), так что противоречия с «`if:` меняется только по конъюнкту `rebase.conflict`» больше нет. | `docs/specs/510-*.md`, §5.2, тот же абзац; `process.yml:486` (`if:` шага `validated`) | + +Обе части находки r3 закрыты в этом раунде без побочных отступлений: правка сузила формулировку до того, что реально проверяемо и совпадает с кодом, вместо того чтобы добавлять новую логику. + +## Находки + +Нет находок в скоупе этого раунда. Дельта r3→r4 — точечное уточнение формулировки, оно не вводит новых технических утверждений, не проверенных против кода: пример-конъюнкт для «Установить зависимости»/`Review`, ссылка на существующий тест `#596` и оговорка про «Слить ветку в dev» — все три сверены построчно выше и совпадают. + +## Что проверено и корректно + +- Абзац §5.2 «Гейт добавляет...» теперь описывает механическое, а не смысловое правило («замени один конкретный конъюнкт, остальные не трогай»), и это правило проверено на каждом реально существующем шаге с конъюнктом `rebase.conflict` в `process.yml` — совпадает без исключений, включая шаг, не названный по имени в перечне ТЗ («dev ушёл вперёд»). +- Ссылка на `test/review-doc-guard.test.mjs:596` в тексте ТЗ точна: тест существует, имя и ассерт совпадают, и правка ТЗ сохраняет ровно то условие (`reuse != 'true'` у `Review`), которое тест жёстко проверяет. +- Оговорка про «Слить ветку в dev» корректна: у этого шага сегодня нет конъюнкта `rebase.conflict`, поэтому нечего заменять напрямую, а защита от красного гейта приходит транзитивно через `decide`. +- AC1–AC6 дельтой этого раунда не задеты по существу — унаследованы. +- Обязательные разделы ТЗ (§7.1) присутствуют, задача не помечена `small`, ТЗ обоснованно живёт в `docs/specs/` — не менялось с r1. + +## Чего не проверял + +- `npx tsc --noEmit`, `npm test`, `npm run build`, `node scripts/check-docs.mjs` — не запускал: дельта r3→r4 — 4 вставки/4 удаления в одном `.md`-файле вне `src/**`/`scripts/**`/`test/**`, класс C (PROCESS.md §1), гейты неприменимы; то же было верно для r1–r3, поведение не изменилось. Задача не трогает `src/**`, поэтому `check-docs.mjs` тоже не запускается. +- Golden/смоки/perf/backend/инварианты модели — задача не трогает геометрию, рендер, состояние или бэкенд; не изменилось с r1. +- Права токена `HP_PROCESS_TOKEN`, риски §10.1 — не менялись дельтой, унаследованы. +- Реальная реализация `process.yml`/`validate-gate.mjs` — её ещё нет, этап spec её не предполагает; всё выше проверено чтением существующего `process.yml` и существующих тестов, не запуском несуществующего кода. + +## Унаследовано из r3 + +Без повторной проверки в этом раунде — дельта их не касается: + +- Обязательные разделы §7.1 присутствуют и на своём месте — из SPEC-REVIEW-510-r1 (SHA `34bf81a1`), подтверждено в r2 (SHA `5fc235c0`) и r3 (SHA `3854fe72`). +- Скоуп/не-скоуп (§2/§3) и совместимость условия job `changed_mutants` (§4) с существующим механизмом `--changed` — из SPEC-REVIEW-510-r2 (SHA `5fc235c0`); дельта r3 и r4 §4 не касаются. +- Раздел §10.0 (UX/i18n/модель данных не затрагиваются) — из SPEC-REVIEW-510-r2 (SHA `5fc235c0`); текст не менялся. +- Порядок шага `gate` относительно `material`/`reuse`/«Конфликт с dev» (§5.2, первое предложение) — сверен в r2 с реальными строками `process.yml` (414/430/445/485), не тронут дельтами r3 и r4 — из SPEC-REVIEW-510-r2 (SHA `5fc235c0`). +- Алгоритм `scripts/validate-gate.mjs` (§5.1), включая закрытие Medium 2 из r1 («доказательство только при исполненных и зелёных job мутантов») — из SPEC-REVIEW-510-r1 (SHA `34bf81a1`) и SPEC-REVIEW-510-r2 (SHA `5fc235c0`), текст §5.1 не менялся ни в r3, ни в r4. +- Красный-возврат (§5.2, `proceed != 'true'`) — исправлен и подтверждён верным ещё в r3 (SHA `3854fe72`), дельтой r4 не тронут (диф этого раунда его не касается). +- §6 (слияние кандидата), §7 (правила хендоффа), §9 (откат), §10.1 (риски), §11 (затронутые файлы), §12 (предположения) — из SPEC-REVIEW-510-r1/r2, текст не менялся ни в r3, ни в r4. +- AC1, AC3, AC4, AC5, AC6 — из SPEC-REVIEW-510-r1/r2, способ доказательства не изменился. + +## Вердикт + +High: 0 · Medium: 0 · Low: 0. + +Единственная находка r3 закрыта полностью и без побочных эффектов: формулировка §5.2 больше не заявляет тотальную замену условия на `proceed` — она сужена до «замена ровно одного конъюнкта, остальные существующие конъюнкты (`reuse`, `stage`, `decide.green`) сохраняются», что построчно совпадает с реальным `process.yml` для каждого перечисленного шага и для непоименованного, но структурно идентичного шага «dev ушёл вперёд». Ссылка на защитный тест `#499` (`test/review-doc-guard.test.mjs:596`) точна. Новых технических утверждений, не проверенных против кода, дельта не вводит. + +**Вердикт: зелёный.** + +--- + +--- + + + +## Материал раунда + +- Ветка: `issue/510-mutants-on-candidate-and-review-gate`, коммит `23641f4ec176` — ребейз его осиротит, и это нормально: ниже якоря, которые ребейз не меняет. +- Дерево материала: `26bb739d6572ce509b6a9442c2680196b363b233` + ``` + git log --all --format='%H %T' | grep 26bb739d6572 + ``` +- ТЗ `docs/specs/510-mutants-on-candidate-and-review-waits-validate.md`, блоб `df99bed3c772e12fcfde5439176d6827fe6d79dd` + ``` + git log --all --find-object=df99bed3c772e12fcfde5439176d6827fe6d79dd -- docs/specs/510-mutants-on-candidate-and-review-waits-validate.md + ``` +- Вердикт конвейера: `green` · High 0