From 6b31e94501cdfb2719bc79842b5bfa83086e196e Mon Sep 17 00:00:00 2001 From: "claude[bot]" <209825114+claude[bot]@users.noreply.github.com> Date: Thu, 10 Sep 2026 06:54:51 +0000 Subject: [PATCH] docs: review document for #517 Issue: #517 User-Visible: no --- docs/reviews/SPEC-REVIEW-517-r1.md | 219 +++++++++++++++++++++++++++++ 1 file changed, 219 insertions(+) create mode 100644 docs/reviews/SPEC-REVIEW-517-r1.md diff --git a/docs/reviews/SPEC-REVIEW-517-r1.md b/docs/reviews/SPEC-REVIEW-517-r1.md new file mode 100644 index 00000000..a6fedf6e --- /dev/null +++ b/docs/reviews/SPEC-REVIEW-517-r1.md @@ -0,0 +1,219 @@ +# SPEC-REVIEW-517-r1 + +## Скоуп + +Issue #517 переносит хранение ТЗ полного трека из `docs/specs/-*.md` в тело +issue и переносит доказуемость «вердикт ревью вынесен на этом тексте» с блоба +файла на `sha256` нормализованного тела issue, записываемый в блок якорей +документов ревью. Пять поверхностей: (1) `process.yml` — новый шаг снятия +хеша тела и сравнение с записью последнего зелёного `SPEC-REVIEW`; (2) +`scripts/review-doc-guard.mjs` — `anchorIssueBodyFrom`/сравнение хешей; (3) +`scripts/process-gate.mjs` — замена проверки 3 (файл ТЗ) на проверку заголовка +`## ТЗ`/`AC1` в теле; (4) `scripts/task-packet.mjs` — смена приоритета +источника AC; (5) `PROCESS.md`/`AGENTS.md`/`docs/specs/README.md` — переписывание +правил. `User-Visible: no`, продукт и словари не затронуты. + +Issue помечен `small`; ТЗ по правилу лёгкого трека действительно лежит в теле +issue, файла `docs/specs/517-*.md` нет — соответствует ожиданию для этой метки. +Ветки `issue/517-*` на origin нет: кода ещё не существует, ревью — чисто по +тексту тела issue и его единственному комментарию (S2-аналитика). + +## Как проверялось + +- Прочитаны `docs/SCOPE.md`, `PROCESS.md` (целиком), `AGENTS.md`. +- Прочитано тело issue #517 и комментарий S2 (`gh issue view 517 --json body,comments`). +- Каждое фактическое утверждение ТЗ и S2-комментария о текущем коде сверено с + реальными файлами, а не принято на слово: + - `scripts/review-doc-guard.mjs`: `materialAnchorBlock` (строка 256), + `anchorTreeFrom`/`anchorVerdictFrom` (442, 451), `reusableGreenVerdict` + (480) — сигнатуры и поведение совпадают с описанием в ТЗ (У2, У3). + - `scripts/process-gate.mjs`: `checkSpecs` (305), `NO_SPEC_FILE` (76) — + совпадает с описанием гейта в плане и S2. + - `scripts/task-packet.mjs`: `extractAcceptanceCriteria` (49), `acSource` + тернарник (строка ~112: `specs.length ? specs... : issue.body`) — + поведение (AC из файла, тело — fallback) совпадает с тем, что план + собирается инвертировать (AC5). + - `.github/workflows/process.yml`: полностью прочитана структура job + `review` — шаги `Перейти на ветку задачи` (безусловный), `Привести ветку к + dev`/`rebase` (`if: stage=='code'`), `Зафиксировать SHA материала ревью`/ + `material` (`if: rebase.conflict != 'true'`, БЕЗ условия по stage), + `reuse` (#499, `if: stage=='code'`, сравнивает только дерево через + `reusableGreenVerdict`), шаг вызова модели `Review` (`if: gate.proceed && + reuse.reuse != 'true'`), шаг публикации документа (использует + `steps.material.outputs.*` без разбора по stage). +- Проверено отсутствие `docs/specs/517-*.md` (`ls docs/specs/517*` — нет + файлов), что подтверждает корректность выбора лёгкого треке ТЗ-в-теле. +- Проверены точные ссылки, которые ТЗ/S2 называют как подтверждённые фактом: + `docs/specs/README.md` (раздел «Обязательные release-артефакты», на который + ссылается `PROCESS.md:158`), список секций PROCESS.md (`grep '^## \|^### + '`) — секции §2.3, §5, §7.1, §7.3 п.1 существуют и релевантны; секция + «§10.5 п.3», которую S2-комментарий называет как место требования файла ТЗ в + гейте, **не существует** (максимальный номер подраздела — §10.4, дальше идёт + §11) — верная ссылка на этот гейт — §10.2 п.3. +- Гейты (`typecheck`/`test`/`build`) не прогонялись и не нужны на этом этапе: + кода нет, ветки нет, диффа нет — предмет ревью текстовый (тело issue), а не + дерево репозитория. + +## Находки + +### Medium-1 (в скоупе). Гарантия AC2 не держится при повторном применении зелёного вердикта (#499) + +**Файл:** тело issue #517, раздел «Уточнения S2», пункт У3, и AC2. + +**В чём дефект.** AC2 обещает: правка тела issue после зелёного ревью ТЗ +обязательно даёт код-ревью строку «ТЗ менялось после зелёного ревью ТЗ» и +полный разбор. У3 при этом утверждает: «reuse (#499) и страховка #414 не +затрагиваются» — то есть механизм повторного применения зелёного вердикта без +вызова модели остаётся как есть. Но именно этот механизм и есть дыра: +`reusableGreenVerdict` (`scripts/review-doc-guard.mjs:480`) признаёт вердикт +переиспользуемым по ТРЁМ условиям — записанный вердикт `green`/`High 0`, +наличие якоря дерева, и **отсутствие отличий git-дерева** от этого якоря +(`differs(tree)`) — и не знает ничего о теле issue. Шаг `reuse` +(`process.yml:436-447`) условен только на `stage == 'code'`, а шаг вызова +модели `Review` (~851) стоит под `if: gate.proceed && reuse.reuse != 'true'`. +Значит, когда `reuse=true`, модель **не вызывается вовсе** — а именно туда, по +плану ТЗ, должна попадать «находка, доставляемая ревьюеру в промпт». + +**Сценарий воспроизведения.** Код-ревью раунда r_k проходит зелёным (High 0), +дерево якорится. Слияние не удаётся (ребейз/страж #312) — задача возвращается +в `S6` и сразу в `S7` (ровно сценарий #437 r4, на который ссылается сам +`reusableGreenVerdict`). Между r_k и r_k+1 кто-то правит тело issue (уточняет +AC, меняет формулировку риска) — дерево кода при этом не меняется. На r_k+1 +`reuse` видит: дерево не отличается от якоря вне `docs/reviews/**` → вердикт +переиспользуется, модель не вызывается, найденная-по-плану строка «ТЗ +менялось» никогда не печатается. Раздел «Риски» ТЗ утверждает обратное: +«правка после этого попадёт в следующий заход» — для этого конкретного пути +это неверно: следующий заход — это переиспользование без чтения вообще. + +Важно: на ПЕРВОМ раунде код-ревью (переход `S5→S7`, live-проверка, которую +называет сам AC2) `reuse` в принципе не может сработать — предыдущего +`CODE-REVIEW`-документа ещё нет, `reusableGreenVerdict` вернёт `null`, модель +вызывается штатно. То есть заявленная в AC2 «живая проверка» пройдёт и +подтвердит AC — но не покроет описанный выше многораундовый путь, и ни один +названный в ТЗ тест/мутант его не ловит. + +**Почему это Medium, а не High.** Основной путь (первое код-ревью после +зелёного ревью ТЗ) действительно закрыт корректно — уязвим только повторный +раунд после rebase-only возврата, что само по себе не рядовая, а уже особая +ситуация (#437-класс). Дефект в скоупе задачи и правится в ней же: либо +`reusableGreenVerdict`/вызывающий его CLI получает дополнительный параметр +«хеш тела issue совпадает с якорем», либо в разделе «Риски» ТЗ явно +фиксируется этот остаточный пробел как принятый (а не отрицается фразой «не +затрагивается»). + +### Medium-2 (в скоупе). Метка `small` не соответствует собственному описанию задачи как полнотрековой + +**Файл:** метки issue #517; комментарий S2 («Трек: полный (три поверхности — +конвейер, гейт, документация процесса)... сегодня формально это трек +small-стиля, поэтому дополнительно ставлю метку small»). + +**В чём дефект.** §5 PROCESS.md требует ОДНОВРЕМЕННО: сложность/риск ≤3, ОДНУ +поверхность, отсутствие миграции, отсутствие нового UX-контракта, отсутствие +влияния на touch/perf. Автор сам называет три поверхности (пайплайн, гейт, +процесс-документация) и явно пишет «трек: полный» прямым текстом — это прямое +нарушение критерия «одна поверхность», названное самим автором, а не +предположение ревьюера. Несмотря на это, к issue добавлена метка `small`, и +заявленная причина — «чтобы `process-gate --issues` не требовал файла ТЗ на +класс A» — не соответствует коду: `checkSpecs` (`scripts/process-gate.mjs:311-312`, +`if (!c.classes.has('A') || c.isRelease) continue;`) проверяет только коммиты, +несущие класс A. У этой задачи класса A нет вовсе (только B: `process.yml`, +`scripts/**`; и C: `docs/**`) — проверка 3 не сработала бы независимо от +метки. Причина, которой оправдан выбор трека, ложная. + +**Последствие.** Метка `small` снижает бюджет циклов ревью ТЗ с 4 до 2 (§4: +«Для лёгкого трека лимит ревью ТЗ — 2 цикла») для задачи, которую сам автор +характеризует как трёхповерхностную и полнотрековую по существу — при том что +именно эта задача переписывает структурные гарантии всего конвейера ревью. +Обратное несоответствие (`trivial`/`small` — обоснование не выбора, а отказа +от него, #338) здесь работает в неверную сторону: критерий нарушен и назван +самим автором, но метка не снята. + +**Рекомендация.** Либо снять `small` (раз задача и без него не требует файла +ТЗ по факту отсутствия класса A — необходимости в метке нет вообще), либо +получить явное решение владельца принять урезанный бюджет циклов для этой +конкретной задачи, зафиксировав его в issue. + +### Low-1. У1 содержит неточное утверждение о текущем коде (не блокирует, для протокола) + +**Файл:** тело issue #517, «Уточнения S2», пункт У1. + +У1 утверждает: «на `spec` шага `material` нет (он под `if: stage == +'code'`-веткой ребейза), понадобится снимать хеш в отдельном шаге, +общем для обоих этапов». Проверка `process.yml` показывает обратное: шаг +`material` (`id: material`, строка 403) условен ТОЛЬКО на `steps.rebase.outputs.conflict +!= 'true'`; шаг `rebase`, который единственный несёт условие `stage=='code'`, +на этапе `spec` просто не выполняется, и его output пуст — а пустая строка `!= +'true'` истинна. Значит, шаг `material` **уже выполняется и на этапе spec** +(это же ревью — прямое доказательство: якорь ниже в этом документе получит +дерево/SHA, снятые этим самым шагом на spec-прогоне). Задача не блокируется +этим — решение, добавлять ли новый шаг `issue_body` или расширить уже +существующий `material` дополнительным выводом, относится к «чего пользователь +не наблюдает» (§7.1) и свободно решается разработчиком. Но фраза «всё, на что +опирается решение, сверено с кодом» в S2 не вполне точна: там же назван +несуществующий `§10.5 п.3` (реальный номер — §10.2 п.3, «Что проверяет +process-gate.mjs») и несуществующая функция `acceptanceFrom` в +`task-packet.mjs` (реальный механизм — тернарник `acSource` плюс +`extractAcceptanceCriteria`, поведение при этом описано верно). Снимается +записью: исправить формулировку У1 и, если сохраняется решение о новом шаге +`issue_body`, явно обосновать его отдельно от ошибочной посылки «шага нет». + +## Что проверено и корректно + +- AC1, AC3, AC4, AC5 — однозначны, доказательство названо для каждого + (unit/тест на фикстурах/тест на текст), формулировки не оставляют места для + трактовки. +- Обязательные разделы §7.1 присутствуют; неприменимые (UX, модель + данных/миграция, i18n, производительность) явно помечены неприменимыми с + обоснованием («изменения только в конвейере... продукт и словари не + затрагиваются»), а не молча пропущены. +- Блок принятых предположений оформлен по правилу (У4 — «принято + предположительно, менять свободно»), продуктовых открытых вопросов + владельцу нет и не должно быть: персона задачи — автор ТЗ/ревьюер, не + пользователь продукта (`docs/SCOPE.md` эту поверхность не описывает и не + обязан); единственный «технический вопрос» (нормализация тела) автор + корректно закрыл сам, не эскалируя владельцу — соответствует правилу «на + этапе аналитики технические вопросы не задаются». +- Обратная совместимость формата якорей (У3: новая строка `Тело issue:` + независима от `specs`, старые документы читаются как раньше через + `anchorIssueBodyFrom → null`) подтверждена по образцу уже существующих + `anchorTreeFrom`/`anchorVerdictFrom`, которые именно так и устроены. +- Откат описан конкретно (перечислены все файлы), риск лимита тела GitHub + (65 536 знаков) сопоставлен с реальным максимумом ТЗ проекта (21 КБ, + `docs/specs/162-*.md`) — не голословно. +- «Не входит» и скоуп/не-скоуп разделены явно, смешанных вопросов владельцу не + задано. +- SCOPE.md намеренно не применяется как рамка приёмки: задача не продуктовая + (`User-Visible: no`, персона — внутренний процесс), это соответствует + собственному правилу PROCESS.md об инфраструктурных задачах, а не пробел. + +## Чего не проверял + +- Не прогонялись `typecheck`/`test`/`build`/`process-gate` — кода нет, ветки + `issue/517-*` не существует на момент ревью; на этапе ТЗ это гейты + следующего этапа. +- Не проверялась синтаксическая корректность будущих правок YAML + (`process.yml`) — их ещё не существует; проверялась только текущая, ДО + правки, структура шагов, на которой основаны находки Medium-1/Low-1. +- Не оценивалось качество мутационных гардов (`review-anchor-drops-issue-body` + и т.д.) сверх того, что они названы и привязаны к конкретным чистым функциям + — их реализация появится в коде, а не в ТЗ. + +## Вердикт + +Жёлтый. High: 0, Medium: 2 (оба в скоупе задачи, чинятся в ней же, без +повторного ревью после исправления по существу — только новый заход того же +цикла ТЗ). Low: 1, снимается записью в этом документе, отдельного действия не +требует. + +--- + + + +## Материал раунда + +- Ветка: `dev`, коммит `a824acc1b18a` — ребейз его осиротит, и это нормально: ниже якоря, которые ребейз не меняет. +- Дерево материала: `91c557112388667494babbaaa83f5f6a75195628` + ``` + git log --all --format='%H %T' | grep 91c557112388 + ``` +- Вердикт конвейера: `yellow` · High 0