diff --git a/docs/reviews/CODE-REVIEW-454-r3.md b/docs/reviews/CODE-REVIEW-454-r3.md new file mode 100644 index 00000000..257092cc --- /dev/null +++ b/docs/reviews/CODE-REVIEW-454-r3.md @@ -0,0 +1,258 @@ +# CODE-REVIEW — issue #454 · заход r3 (см. «Заход vs факт» ниже) + +- Issue: https://github.com/Matysh/houseplan-card/issues/454 +- Материал: `git rev-parse HEAD` = `4b443d8c` (ветка `issue/454-review-round-counter`, + ранее реализация была на `634478a5`; конвейер перед ревью привёл её к `dev` + через ребейз (+12 коммитов `dev`, включая `05ef3181`, `aa51ed2b`, `8935a736`, + `169fed01`, `4203a01b`, `2202b207` и др.) — после ребейза это другой код + (PROCESS.md §7.2), поэтому разбор ниже **полный**, не по дельте. +- `git diff origin/dev...HEAD --stat`: 8 файлов, +1226/−3 — + `.github/workflows/process.yml`, `scripts/review-doc-guard.mjs`, + `scripts/mutation-gate.mjs`, `test/review-doc-guard.test.mjs`, + `docs/specs/454-review-round-counter.md`, `docs/specs/README.md`, + `docs/reviews/SPEC-REVIEW-454-r1.md`, `-r2.md`. +- Класс изменения: B (инфраструктура), `User-Visible: no` на всех коммитах — + проверено `git show -s --format=%B` на каждом из 7 коммитов диапазона. + +## Заход vs факт (важно для чтения этого документа) + +Заголовок задачи называет это «заход r3». Проверено по таймлайну меток +(`gh api repos/.../issues/454/timeline`): метка `S7-code-review` навешена +на #454 **ровно один раз**, в 16:57:56. Это первый и единственный код-ревью +раунд #454 — фактически он `r1`. Номер `r3` в имени документа — не ошибка +приложенного шаблона, а живое проявление ровно того дефекта, который #454 +чинит: см. **M1** ниже, где это воспроизведено вплоть до реального лога guard +этого самого прогона. Оставляю нумерацию файла как задаст шаг публикации +(`CODE-REVIEW-454-r3.md`) — переименовывать его вручную значит повторить +ту же ошибку «не доверять факту конвейера», от которой и уходит #454. +Раздела «Унаследовано из r»/«Закрытие раунда r» в этом документе +нет: предыдущего код-ревью раунда #454 не существовало. + +## Скоуп + +`process.yml`: шаг `guard`/`decide` больше не считает `attempt`/`spent` +целиком по прозе комментариев issue — рядом встаёт независимый счёт по +опубликованным `docs/reviews/${marker}-${NUM}-r*.md` (через новый мелкий +checkout `ref: dev` + `gh api contents`). Логика вынесена в чистые функции +`scripts/review-doc-guard.mjs` (`reviewRoundsFromFiles`, `attemptFromRounds`, +`verdictDeclaration`, `blockingFromDocs`, `reviewCounters`, CLI `--counters`), +покрыта `test/review-doc-guard.test.mjs`, три новых мутанта в +`scripts/mutation-gate.mjs`. `docs/specs/README.md` — строка индекса. +Продуктовый код (`src/**`), i18n, бандл, touch — не задеты. + +## Как проверялось — гейты + +| Гейт | Команда | Результат | +|---|---|---| +| Typecheck | `npx tsc --noEmit` | чисто, без вывода | +| Unit-тесты | `npm test` | 1936 pass / 0 fail / 1 skip | +| Build | `npm run build` | сборка прошла (`rollup -c`), см. отдельное наблюдение о фингерпринте ниже — не относится к диффу #454 | +| Mutation gate | `node scripts/mutation-gate.mjs --check` | `EXIT=0`, все мутанты (включая три новых `review-round-*`) поймали своё | +| check-docs | `node scripts/check-docs.mjs` | passed (7 files, 12 links) — не обязателен для этого диффа (`src/**` не тронут), прогнан для полноты | + +Не гоняю: `golden:verify`, `smoke_*.mjs`, `pytest tests_backend`, performance — +диапазон не касается рендера, геометрии, бэкенда или perf-путей. Инварианты +модели (`npm run invariants`) не нужны — ни ребро, ни `layout`, ни +`marker.space` не затронуты. + +## Находки + +### M1 (Medium, в скоупе задачи) — страховка по комментариям может завысить и `attempt`, и `spent` через посторонний маркер в прозе; заявление «перерасчёт невозможен по построению» не выполняется на реальных данных + +**Воспроизведено исполнением на боевых данных #454, не на синтетике.** + +Guard этого самого прогона (job `101105199249`, run `33898021033`, +`gh api repos/Matysh/houseplan-card/actions/jobs/101105199249/logs`) +напечатал: + +``` +этап code, заход 3, блокирующих циклов 0 из 4 +``` + +при том, что по таймлайну меток это первый код-ревью раунд #454 (должно быть +`заход 1`). Причина — старая (пока не заменённая, `process.yml` живёт на +`main`, куда #454 ещё не смёржен) страховка по комментариям: + +```sh +of_stage="[.comments[] | select(.body | test(\"Вердикт:\")) | select(.body | test(\"CODE-REVIEW\"))]" +``` + +Прогон дословно этим выражением на реальных комментариях #454 +(`gh issue view 454 --json comments`, `jq`) даёт `of_stage.length = 2`: + +- `#issuecomment-5543499255` (16:29:05) — зелёный вердикт **SPEC-REVIEW** r2. + Матчит `Вердикт:` (свой) и `CODE-REVIEW` — потому что текст вердикта + разбирает историю #449 и содержит фразу «маркер `CODE-REVIEW` в нём есть, + но в прозе». Это не отсылка к собственному файлу, а цитата чужого маркера + внутри анализа самой этой задачи. +- `#issuecomment-5543835259` (16:57:54) — комментарий «Реализация готова», + вообще не вердикт. Матчит `Вердикт:` — потому что в тексте есть фраза + «строки `Вердикт:` в нём нет» (описание AC2 в обратных кавычках), и + `CODE-REVIEW` — потому что в тексте разбираются найденные баги на + `CODE-REVIEW-441-r1.md`/`CODE-REVIEW-230-r2.md`/«CODE-REVIEW 439». + +Оба совпадения — ложные: ни один из двух комментариев не является +код-ревью-вердиктом #454. `attempt = 2 + 1 = 3`, `spent = 0` (ни один из двух +не «жёлтый/красный» в своей строке) — совпадает с логом guard buкв в букву. + +**Тот же результат получен НОВЫМ кодом, который и рецензируется**, не только +старым shell (проверено исполнением на `HEAD`): + +``` +$ node -e 'import("./scripts/review-doc-guard.mjs").then(m => console.log( + m.reviewCounters({ rounds: [], docs: [], comments: { attempt: 3, spent: 0 } })))' +{ + attempt: 3, spent: 0, + attemptFiles: 1, spentFiles: 0, + attemptComments: 3, spentComments: 0, + blocking: [], unread: [] +} +``` + +`rounds: []` — потому что ни одного `CODE-REVIEW-454-r*.md` ещё не +опубликовано (проверено `git ls-tree -r HEAD --name-only | grep 454` — есть +только два `SPEC-REVIEW-454-*`). Файловая половина эту задачу считает +абсолютно верно (`attemptFiles=1`). Ломает её `Math.max` со второй, не +переписываемой этим ТЗ половиной. + +**Почему это не «синтетический край», а найденный вживую пример класса #89.** +ТЗ и AC5 формулируют защиту так: «Вердикт чужого этапа не влияет на счёт +(#89 не сломан)» — без оговорки. По факту это верно только для +`reviewRoundsFromFiles`/`attemptFromRounds` (доказано юнитом +«чужой этап и чужая задача в счёт не идут», `test/review-doc-guard.test.mjs:300`) +и НЕ верно для итогового `reviewCounters`, потому что `attemptComments` +считается прежним, недоказанным способом и участвует в `max()` наравне с +надёжным источником. В параграфе §5 самого ТЗ это фактически признано +оговоркой «по файлам» («Правило… по файлам оно выполняется строго»), но +таблица AC5 эту оговорку не несёт, а раздел §3 идёт дальше и заявляет +«перерасчёт невозможен по построению» — это неверно в общем виде: невозможно +удвоение счёта (сумма против максимума), но не невозможно завышение одной из +компонент максимума, и именно это сейчас произошло. + +**Чем краснеет сильнее, чем в этом раунде.** В этом прогоне `spentComments` +совпал с `spentFiles` (оба 0) только потому, что заражающий SPEC-REVIEW-вердикт +был зелёным. Формула `blocking` смотрит на цвет строки вердикта самого +комментария, а не на то, кому вердикт принадлежит: будь заражающий комментарий +жёлтым/красным вердиктом СВОЕГО (не CODE-REVIEW) этапа, `spentComments` вырос +бы, `max(spentFiles, spentComments)` унаследовал бы это, и код-ревью потерял +бы цикл бюджета §4 раньше, чем реально стартовал, — тот самый вред, ради +которого заведено #89 и который AC5 обещает не допустить. + +**Практический эффект сегодня** — не потеря артефакта (`docs/reviews/` пуст +для `CODE-REVIEW-454-*`, коллизии нет) и не потеря бюджета (`spent` верен), +а искажённое имя файла: шаг публикации назовёт этот документ +`CODE-REVIEW-454-r3.md`, оставив `r1`/`r2` навсегда пропущенными для этой +задачи. Не деструктивно, но прямо противоречит зафиксированному в ТЗ «После: +каждый заход получает собственное имя файла» — здесь заход получает чужое. + +**Почему Medium, а не High.** Первичный вред, ради которого заведено #454 — +уничтожение предыдущего артефакта ревью (#220/#365-класс) — этим случаем не +демонстрируется: коллизии имени нет, `spent` не искажён. Демонстрируется +недоказанность одного конкретного защитного заявления (AC5/§3) в комбинации +источников, а не поломка первичной гарантии. Это чинится в текущей задаче +(в скоупе — счёт раундов ровно то, чем #454 занимается), не отдельным issue. + +**Предлагаемое направление (не мандат, выбор за автором):** +1. Ужесточить страховку по комментариям: матчить не голый `CODE-REVIEW`, а + что-то вроде `CODE-REVIEW--r` — привязка к номеру issue резко снижает + шанс случайного совпадения в прозе, оставаясь той же «прозой», от которой + ТЗ явно отказывается полагаться как на основной источник, но убирает + наиболее грубый класс ложных срабатываний из резервного источника; + и/или +2. Добавить видимость расхождения: `::warning::`, когда + `attemptComments > attemptFiles + 1` без объясняющего файла — сегодня + расхождение тихое, лог `guard` печатает только итоговое число; + и/или +3. Смягчить формулировку «перерасчёт невозможен по построению» в + `docs/specs/454-review-round-counter.md` §3 и в комментарии `process.yml` + до того, что реально доказано («не даёт двойного счёта одного раунда», + а не «не может завысить»), и закрепить тестом текущее (несовершенное) + поведение вместо документирования отсутствующей гарантии. + +Любое из трёх снимает находку; я не настаиваю на конкретной реализации. + +## Наблюдения — не находки, не блокируют + +- **Бандл `custom_components/houseplan/frontend/houseplan-card.js` уже устарел + относительно фингерпринта исходников `dev`** (проверено: свежий + `npm run build` даёт другой `__HOUSEPLAN_BUILD_FINGERPRINT__`, чем + закоммиченный). Причина найдена: коммит `28a4cb5e` (issue #455, уже в + `dev`) поменял `package.json` — входит в `BUILD_INPUTS` + `scripts/source-fingerprint.mjs` — без последующего `bundle:sync`. К #454 + отношения не имеет (диф #454 не трогает `src/**`/`package.json`/`dist/**`), + предсуществует на `dev` независимо от этой задачи, и штатный механизм + (`rollup.config.mjs`: несовпавший фингерпринт обязан «fail closed» перед + golden/perf-съёмкой) рассчитан ровно на такой разрыв. Не завожу отдельный + issue: не деструктивно, самоисправится ближайшим `bundle:sync` перед любой + задачей, трогающей рендер/perf. Владельцу стоит знать на случай, если + ближайшая golden-задача упрётся в неожиданный отказ. +- `docs/specs/README.md`: строка `#454` вставлена перед `#440`/`#447` — + таблица в остальном идёт по возрастанию номера issue. Косметика, не правлю + находкой. + +## AC — таблица доказательств + +| AC | Критерий | Доказано | Комментарий | +|---|---|---|---| +| AC1 | `attempt` = max(номер)+1, не count+1 | unit, чтением | `test/review-doc-guard.test.mjs:288` — дыра `[1,3]→4` проверена | +| AC2 | «#449 как есть» → `attempt=3, spent=1` | unit на слепке реальных данных | перепроверено мной независимым `gh` снимком #449 — числа совпадают | +| AC2b | Реконструкция r1/r2/r3 → `attempt=4, spent=2` | unit | пройден | +| AC3 | Имя файла никогда не повторяется | unit + мутант `review-round-counts-files-not-max` | мутант краснеет корректно (проверено `--check`) | +| AC4 | Зелёный вердикт цикл не тратит | unit | `test/review-doc-guard.test.mjs` «зелёный вердикт цикла не тратит» | +| AC5 | Вердикт чужого этапа не влияет на счёт (#89) | unit — **только для `reviewRoundsFromFiles`** | **см. M1**: не доказано для `reviewCounters` в целом; комментарийная половина не покрыта тестом на это утверждение и на реальных данных нарушается | +| AC6 | Отказ публикации не занижает счёт | unit + мутант `review-round-drops-comment-insurance` | пройден | +| AC7 | Нет ветки/API недоступен → поведение как раньше | unit | пройден, включая «мусор на входе» | +| AC8 | `process.yml` идентичен в `main` и `dev` | существующий шаг Validate | сейчас идентичны (проверено `diff`); автор откладывает зеркалирование в `main` до после мержа — обоснованно, гейт сравнивает ветки между собой и покраснел бы раньше времени | +| AC9 | Логика в тестируемом модуле, не в inline-shell | unit + чтение | `test/review-doc-guard.test.mjs` проверяет сам текст шага `decide` на вызов `node scripts/review-doc-guard.mjs --counters` | + +## Что проверено и корректно + +- Файловая половина счёта (`reviewRoundsFromFiles`, `attemptFromRounds`, + `verdictDeclaration`, `blockingFromDocs`) — корректна, включая защиту от + цитирования чужого вердикта («описание чужого раунда не объявляет вердикт», + коммит `4b443d8c`, добавлен по итогам находки самим автором на корпусе 629 + документов) и от блока внутри тройных кавычек не по месту. +- Три новых мутанта реально ловятся штатным раннером (`--check`, exit 0). +- Трейлеры всех 7 коммитов диапазона: `Issue: #454`, `User-Visible: no` — + корректно для класса B, changelog не требуется. +- `process.yml` `main`↔`dev` совпадают сегодня; план зеркалирования после + мержа обоснован и не является дефектом. +- Гейты (typecheck/test/build/mutation/check-docs) зелёные на `HEAD`. + +## Чего не проверял + +- `golden:verify`, `smoke_*.mjs`, `pytest tests_backend`, performance-профили, + инварианты модели — diff их не касается. +- Реальный прогон `guard` С НОВЫМ кодом на живом GitHub Actions (сам workflow + ещё не в `main`/`dev`) — логика проверена юнитами и прямым вызовом + `reviewCounters`/`--counters` на данных, снятых с реального API, но не в + контексте самого `job` целиком (шаг разрешения ветки через + `git ls-remote`/`gh api matching-refs`, постраничный лимит 1000 файлов) — + эта часть осталась shell-кодом без юнитов по прямому требованию ТЗ + («guard остаётся лёгким», §4 ТЗ) и проверена только чтением. +- Не пересчитывал вручную мутацию `review-round-drops-file-source` построчно + за пределами штатного раннера — доверился `--check`. + +## Вердикт + +Один Medium в скоупе, без High. Возврат автору тем же issue (#454), не +отдельный issue. + +Документ: `docs/reviews/CODE-REVIEW-454-r3.md` (публикуется шагом конвейера; +см. раздел «Заход vs факт» насчёт номера). + +--- + + + +## Материал раунда + +- Ветка: `issue/454-review-round-counter`, коммит `634478a51077` — ребейз его осиротит, и это нормально: ниже якоря, которые ребейз не меняет. +- Дерево материала: `f8467cb113cd3c09cf3a10235bb7034ee9121063` + ``` + git log --all --format='%H %T' | grep f8467cb113cd + ``` +- ТЗ `docs/specs/454-review-round-counter.md`, блоб `b5af292101a495e4699d839397e837a3a2467c85` + ``` + git log --all --find-object=b5af292101a495e4699d839397e837a3a2467c85 -- docs/specs/454-review-round-counter.md + ```