From 310bc6e4f823f252319640ad8e22b663357f559e Mon Sep 17 00:00:00 2001 From: "claude[bot]" <209825114+claude[bot]@users.noreply.github.com> Date: Mon, 28 Sep 2026 22:39:08 +0000 Subject: [PATCH] docs: review document for #700 Issue: #700 User-Visible: no --- docs/reviews/CODE-REVIEW-700-r4.md | 103 +++++++++++++++++++++++++++++ 1 file changed, 103 insertions(+) create mode 100644 docs/reviews/CODE-REVIEW-700-r4.md diff --git a/docs/reviews/CODE-REVIEW-700-r4.md b/docs/reviews/CODE-REVIEW-700-r4.md new file mode 100644 index 00000000..ef881b6e --- /dev/null +++ b/docs/reviews/CODE-REVIEW-700-r4.md @@ -0,0 +1,103 @@ +# CODE-REVIEW-700-r4 + +## Материал раунда + +- Issue: [#700](https://github.com/Matysh/houseplan-card/issues/700), трек `track:show`, инфраструктурный маршрут (класс B: `.github/workflows/**`, `scripts/**`, `test/**`; класс C: `PROCESS.md`, `docs/reviews/**`). Файлов класса A нет. +- SHA материала: **`1aa52d21071705574c6cccb1f19ccc3a8d5b9083`** (рабочая копия уже на нём, `git status` чист). +- Диапазон: `git log --oneline origin/dev..HEAD` — 4 коммита: + `e45bc87c` (задача, r0) → `a005aae5` (докс r1) → `f6e317d8` (задача, правка Medium из r1) → `1aa52d21` (докс r2). +- `git diff origin/dev...HEAD --stat`: `validate.yml` +66/-12, `PROCESS.md` +7/-1, `scripts/check-docs.mjs` +12/-1, `scripts/mutation-registry.mjs` +34, три тестовых файла, два committed документа ревью (`CODE-REVIEW-700-r1.md`, `-r2.md`). +- Validate на этом SHA: **success**, https://github.com/Matysh/houseplan-card/actions/runs/36492360526 — дешёвые гейты (`tsc --noEmit`, `npm test`, `npm run build` + сверка бандла) подтверждены этим прогоном, повторно не гонял. + +## Почему разбор полный, а не по дельте (§2.10) + +Формально с r2 (доказанно зелёный, `docs/reviews/CODE-REVIEW-700-r2.md`) код задачи не менялся ни байтом: r3 применил тот же вердикт повторно без вызова модели, потому что дерево `a6670a47` совпадало с проверенным. Но между r3 и r4 ветку **пришлось перебазировать** на ушедший на 11 коммитов вперёд `dev` (`e1700757`) — push кандидата рвался токеном конвейера без права `workflow` (#705), и автор сам пометил: «дифф против `dev` сменил контекст `validate.yml` … ожидаю новый заход ревью, а не повтор вердикта r2». Это прямо описанное в задании исключение — «ребейз на ушедший вперёд `dev`» — где разбор остаётся полным. Сделал полный разбор: перечитал итоговый `preflight` job целиком (не только хунки диффа), проверил, как правка `#700` сочетается с соседними правками `dev` (`#696` — цена захода по треку/мутанты по диффу; `#697` — режим скриншотов на ветке задачи), и заново прогнал целевые тесты и три зарегистрированных мутанта. + +## Как проверялось + +1. Прочитан весь `.github/workflows/validate.yml` job `preflight` в его текущем виде (после ребейза), не только диф-хунки — искал конфликт правки `#700` с окружающим контекстом от `#696`/`#697`. +2. Прочитан `scripts/check-docs.mjs` целиком вокруг флага `--external`/`--external=warn` и путь `errors`/`warnings`/`process.exitCode`. +3. Прочитаны все три тестовых диффа (`test/validate-workflow.test.mjs`, `test/docs-freshness.test.mjs`, `test/classify-changes.test.mjs`) и правка `PROCESS.md`. +4. Прогнаны целевые юниты: `node --test test/validate-workflow.test.mjs test/docs-freshness.test.mjs test/classify-changes.test.mjs` — 52/52 зелёных. +5. Для каждого из трёх мутантов `#700` в `scripts/mutation-registry.mjs` (`task-branch-workflow-sync-red-again`, `workflow-sync-issue-duplicated-on-read-failure`, `external-link-warn-mode-ignored`) применил патч руками, перезапустил соответствующий `--test-name-pattern`, убедился, что тест краснеет, откатил файл (`git status` после — чист). Это не требуется на `track:show` (мутанты по диффу не запрашиваются), но дёшево и напрямую проверяет дисциплину «тест умеет падать» для тестов, которые я использую как доказательство. +6. `node scripts/smoke-select.mjs --base origin/dev --head HEAD` → «Исполняемого frontend-диффа нет (`src/**/*.ts` не тронут)» — прямое совпадение «нечего выбирать», не решение пропустить. +7. Сверил трейлеры всех 4 коммитов (`git log -1 --format=%B`): `Issue: #700` на каждом; `User-Visible: no` на задачных — корректно, изменение не видно продукту; на докс-коммитах трейлеры избыточны, но не вредят. +8. Сверил, что лейблы `infra`/`process`, использованные в новом шаге создания issue, существуют в репозитории (`gh label list`). + +## Гейты: что прогнал, что нет, почему + +| Гейт | Статус | Почему | +|---|---|---| +| `npx tsc --noEmit`, `npm test`, `npm run build` + сверка 3 копий бандла | не гонял повторно | зелёный Validate на этом же SHA (run 36492360526) уже подтвердил — цена та же, повторный прогон ничего нового не даст (#343) | +| `node --test test/validate-workflow.test.mjs test/docs-freshness.test.mjs test/classify-changes.test.mjs` | прогнал сам | целевые тесты по диффу; 52/52 | +| 3 мутанта `#700` из `mutation-registry.mjs` вручную | прогнал сам | не требуется на `show`, но дёшево и это единственное прямое доказательство «тест краснеет» на защитных AC | +| `actionlint validate.yml` | не гонял (инструмент не установлен в среде) | синтаксис YAML фактически подтверждён тем, что Validate на этом SHA успешно исполнил job — невалидный YAML GitHub Actions не запустил бы вовсе | +| `node scripts/smoke-select.mjs --base origin/dev --head HEAD` | прогнал | требование прогона по диффу; результат — «нечего выбирать» (нет `src/**`) | +| `npm run golden:verify` | не гонял | нет метки `ci:golden`, golden-файлы не тронуты | +| `python -m pytest tests_backend -q` | не гонял | `custom_components/**/*.py` не тронут | +| `npm run invariants -- --config ` | не гонял | геометрия и ссылки на неё не тронуты | +| performance-профили | не гонял | не названы в AC | +| Поведение на push в `main` / тег релиза (не `dev`, не `issue/*`) | проверено чтением, не исполнением | в среде ревью нет доступа поднять реальный push-событие на `main`; `case "$REF" in refs/heads/issue/*)` — единственная новая ветка условия, любой другой `ref` (включая `main`/теги) идёт по прежнему строгому пути `check()`/`--external` без изменений в этой ветке кода | + +## AC (тело issue, раздел «Предложение», 3 пункта) + +| AC | Чем доказан | Чем краснеет | +|---|---|---| +| 1. На `issue/*` расхождение зеркала и упавшая внешняя ссылка — предупреждение в сводку, без красного job | Автотест `test/validate-workflow.test.mjs` («#700: на ветке задачи…») читает `advise()`/`check()`-ветвление и `--external=warn`; я применил мутант `task-branch-workflow-sync-red-again` (`advise`→`check`) — тест упал; применил `external-link-warn-mode-ignored` — тест упал | Оба мутанта воспроизведены вручную и убиты; откат подтверждён `git status` | +| 2. Блокируют только push в `dev`, кандидат беты, релиз | Прочитано по коду: `case "$REF" in refs/heads/issue/*)` — единственное исключение; `check-docs.mjs` то же самое условие в `validate.yml`. Кандидат беты — коммит с `Release:` на `dev` (тот же `ref`, тот же строгий путь), не отдельная ветка | Проверено чтением, не исполнением (нет доступа поднять push-событие на `main`/тег в среде ревью) | +| 3. Расхождение зеркала на `dev` заводит одно issue, если такого ещё нет (как ночной `#472`) | Шаг «Расхождение зеркала на dev — issue владельцу», `if: push && ref==dev && workflow_sync.outcome=='failure'`; мутант `workflow-sync-issue-duplicated-on-read-failure` (снимает фикс r1: `exit 0`→`:`) убит — тест `#700: на ветке задачи…` красный на мутанте, зелёный на исходнике | Мутант воспроизведён вручную и убит; логика идентична принятому прецеденту `_mutation-gate.yml:317-333` (одно issue, комментарий к открытому) | + +Все три AC — track `show`, до трёх штук в теле issue, лимит соблюдён. + +## Рассмотрено и отклонено как находка + +**Гонка/ложное срабатывание при сбое `git fetch` внутри шага `workflow_sync`.** Шаг начинается с `git fetch --quiet origin main dev`; в GitHub Actions шаги `run:` по умолчанию исполняются с `bash -eo pipefail`, поэтому сбой сети на этой строке уронит шаг **до** цикла `diff`, и `steps.workflow_sync.outcome` станет `failure` без реального расхождения зеркала. После `#700` это не просто красит один прогон (как было раньше) — на push в `dev` это теперь автоматически заводит владельцу постоянный GitHub issue `[workflow-sync]`, который придётся закрывать вручную, хотя расхождения нет. + +Не поднимаю как Medium: это тот же дизайн, который уже принят и явно задокументирован в прецеденте, на который ссылается сама задача — `_mutation-gate.yml:275` открытым текстом говорит «любой другой пропуск шардов (упал `material`) по-прежнему заводит issue», то есть владелец уже согласился не различать «содержательный отказ» и «инфраструктурный сбой» ради простоты. Задача `#700` не меняет и не ухудшает это поведение — она копирует уже принятый паттерн один в один. Расширять скоуп до различения причин отказа шага — не работа этой задачи. + +**Гонка двух параллельных push в `dev`, оба находят issue не открытым и оба создают дубликат.** Технически возможно между `gh issue list` и `gh issue create` в двух разных прогонах, но `concurrency: group: validate-...${{ github.ref }}` с `cancel-in-progress: true` отменяет предыдущий прогон на том же `ref` при новом push — два прогона `preflight` для `dev` одновременно не живут в штатном случае. Не нахожу воспроизводимого сценария в рамках обычного использования; Low, не блокирует. + +## Что проверено и корректно + +- `advise()` не устанавливает `fail=1` — предупреждение не красит итоговый вердикт job (проверено чтением и мутантом). +- `check-docs.mjs`: при `--external=warn` отказы внешних ссылок уходят в `warnings`, а не в `errors`, значит `process.exitCode` не выставляется этой причиной — шаг `docs` не падает от чужого сайта на ветке задачи. Остальные проверки документации (пропущенный alt, отсутствующий якорь, скриншот-манифест и т. д.) остаются в `errors` независимо от ветки — AC не про них, и они по-прежнему красят. +- `permissions: issues: write` добавлено на уровне job, а не workflow — оценил риск: `validate.yml` **не входит** в список из шести «тонких» файлов, зеркалируемых в `main` (`process.yml`, `mutation-gate.yml`, `process-resume.yml`, `nightly.yml`, `process-reconcile.yml`, `process-metrics.yml`), значит новое право не нужно синхронизировать в `main` — согласуется с тем, что сам список этот файл не включает. +- Фикс r1 (`gh issue list … || true` → `if ! existing=$(…); then …; exit 0; fi`) закрыт и не регрессировал при ребейзе: воспроизведён мутант, тест падает. +- Трейлеры `Issue:`/`User-Visible:` на месте на обоих коммитах класса B; `User-Visible: no` корректен — изменение невидимо продукту, оба changelog не требуются. +- Одно число — один источник (§8): в диффе нет пользовательски видимых чисел (внутренний CI-процесс), пункт неприменим. +- Лейблы `infra`, `process`, используемые в `gh issue create`, существуют в репозитории. + +## Чего не проверял + +- `actionlint` сам не гонял (инструмент недоступен в среде ревью) — полагаюсь на успешный прогон Validate на этом SHA как косвенное доказательство валидности YAML. +- Реальное поведение на push в `main` и на релизном теге не воспроизводил исполнением — только чтением кода (нет средства поднять такое событие в среде ревью); риск минимален, так как единственная новая ветка условия — `issue/*`, остальное не тронуто. +- Полные наборы (golden, HA pytest, инварианты, performance) не гонял — ни один AC их не требует, дифф их не касается. +- Мутанты по диффу в `mutation-gate.yml`-смысле («на этом SHA») не запрашивал — не требуется на `track:show`; три мутанта из реестра, которые сам автор завёл под `#700`, проверил вручную вместо этого (не обязательное, но дешёвое усиление). + +## Закрытие раунда r3 + +r3 не вносил новых находок: он применил зелёный вердикт r2 повторно без вызова модели, потому что дерево `a6670a47` было побайтово тем же, что проверялось в r2. Закрывать нечего. + +## Унаследовано из r3 / переподтверждено в r4 + +Код задачи (`validate.yml`, `check-docs.mjs`, три тестовых файла, `mutation-registry.mjs`, `PROCESS.md`) текстуально идентичен тому, что получило зелёный вердикт в `docs/reviews/CODE-REVIEW-700-r2.md` (SHA материала r2 — `ed3d63e2`/`a6670a47`). Несмотря на это, **не переносил вердикт по правилу «дерево не изменилось»**, а провёл полный разбор заново: рабочая копия ребейзнута на 11 коммитов вперёд `dev` (§2.10, «ребейз на ушедший вперёд `dev`» — явное исключение из сокращённого объёма), и сам автор указал, что итоговый контекст `validate.yml` внутри `preflight` сменился из-за параллельных правок `#696`/`#697`. Результат независимого полного разбора совпал с r2: 0 High, 0 Medium. + +## Итог + +High: 0. Medium (в скоупе): 0. Medium (вне скоупа): 0. Low: 1 (гонка двух параллельных push в `dev`, отклонена как невоспроизводимая при текущем `concurrency`, не правится и не заводится отдельно). Все три AC доказаны — два исполнением с падающим мутантом, один чтением с явной пометкой «проверено чтением, не исполнением» из-за ограничений среды ревью, а не из-за отсутствия автотеста. + +**Вердикт: зелёный.** + +--- + + + +## Материал раунда + +- Ветка: `issue/700-preflight-warnings`, коммит `1aa52d210717` — ребейз его осиротит, и это нормально: ниже якоря, которые ребейз не меняет. +- Дерево материала: `72952e142f2868e6a1cf473135996b79e5eacb5e` + ``` + git log --all --format='%H %T' | grep 72952e142f28 + ``` +- Тело issue: `a0c40d40c1401844c3d04ff0695dd347e76172d990004ea2799cc4df38729fce` +- Вердикт конвейера: `green` · High 0