diff --git a/docs/reviews/CODE-REVIEW-657-r1.md b/docs/reviews/CODE-REVIEW-657-r1.md new file mode 100644 index 00000000..234a7283 --- /dev/null +++ b/docs/reviews/CODE-REVIEW-657-r1.md @@ -0,0 +1,246 @@ +# CODE-REVIEW-657-r1 + +- Issue: https://github.com/Matysh/houseplan-card/issues/657 +- Материал: `git diff origin/dev...HEAD` на `339379f504f7ab5c9142bbca887c363fc0645f1e` + (ветка `issue/657-generated-at-merge`, HEAD detached), 35 файлов, +1021/−136. + Якоря совпадают: `head=339379f5`, `tree=b63cbf25…`, `base=6d3738ad` (origin/dev). +- Класс: B (`scripts/**`, `.github/workflows/**`, `test/**`) + C (`PROCESS.md`, + `AGENTS.md`, `docs/**`) — класса A нет. +- Этап: код-ревью (PROCESS.md §2.7), заход r1, блокирующих циклов израсходовано 0/4. +- **Вердикт: красный.** + +## Скоуп + +Задача закрывает инфраструктурную часть SCOPE (конвейер ревью — не пользовательский +джоб, а обслуживание J1–J7 опосредованно, через скорость доставки; сама задача это +и объявляет как «инфраструктурная, вход сразу на S7»). Решения владельца: **1б** — +`docs/reviews/INDEX.md` пересобирается только коммитами, идущими прямо в `dev` +(слияние кандидата, публикация SPEC-REVIEW), а не в ветке задачи; **2б** — бандл +(`dist/**`, `custom_components/houseplan/frontend/**`) в дереве `dev` меняет только +релизный кандидат/бета (трейлер `Release:`), стенд `dev.houseplan.tech` берёт бандл +головы `dev` из артефакта Validate через служебную ветку `dev-build`. + +AC из хендоффа — семь строк «AC · чем доказан · чем краснеет» (см. комментарий +автора от 2026-09-26T05:59:00Z). Разбор велся по каждой строке отдельно чтением +кода и, где это было практично, реальным исполнением (не моками) сценариев, +которые юнит-тесты задачи проверяют только частично. + +## Как проверялось + +Дешёвые гейты подтверждены зелёным Validate на этом SHA +(https://github.com/Matysh/houseplan-card/actions/runs/36222411365, push) — +`typecheck`, `npm test`, `npm run build` + `bundle-policy --verify`, мутанты +(сharded) все прошли. Автор дополнительно прогнал полный `node --test test/*.test.mjs` +локально (шесть частей) и вручную все 11 новых мутантов (`assertion-killed`). +Перегонять эти гейты не стал — правило #343/условие честности сужения. + +Дополнительно прогнал сам (не входит в стандартный набор для B/C-диффа, но +необходимо для проверки защитного AC «индекс свеж после integrate»): + +| Гейт | Команда | Результат | +|---|---|---| +| smoke-select | `node scripts/smoke-select.mjs --base origin/dev --head HEAD` | «Исполняемого frontend-диффа нет» — смоки не выбираются, диф не трогает `src/**` | +| воспроизведение сценария fast-forward (см. находку H1) | реальный git-репозиторий, оригинальный `scripts/reviews-index.mjs` без правок, без моков | индекс красится — см. ниже | + +**Не прогонял:** golden (`npm run golden:verify`), `pytest tests_backend`, +performance — диф не трогает `demo/**` (визуал), `custom_components/**/*.py` +или профили; AC их не называет. `check-docs.mjs` не обязателен: диф не +трогает `src/**` (§8, REVIEWER.md «Объём гейтов»). + +## Находки + +### H1 — после fast-forward слияния (dev не двигался за время ревью) `docs/reviews/INDEX.md` остаётся неполным, и Validate на `dev` красится на первом же таком merge + +**Заявленный AC** (таблица хендоффа): «Индекс не пересобирается в ветке задачи; +после `integrate` свеж» — доказательство: `test/reviews-index.test.mjs` +«#635/#657 (1б)», `rebase-generated` (порядок шагов); краснеет — мутант +`process-index-on-task-branch`. + +**Что реально происходит.** До этой задачи шаг «Опубликовать документ ревью» +(`_process.yml`) пересобирал `docs/reviews/INDEX.md` **тем же коммитом**, что и +сам документ, независимо от цели (`if [ -f "$doc" ]; then … fi`) — то есть каждый +код-ревью-коммит в ветке задачи уже нёс свежий индекс. Этот диф сузил условие до +`[ "$target" = "dev" ]` (истинно только для SPEC-REVIEW, публикуемого прямо в +`dev`; для CODE-REVIEW цель — ветка задачи, и индекс там больше не трогается — +это и есть решение 1б). Расчёт был на то, что `merge-candidate.mjs` дособерёт +индекс при слиянии («слиянием кандидата (`merge-candidate.mjs`, уже умеет)» — из +решения владельца). Но `merge-candidate.mjs` в этой задаче **не менялся** (нет в +диффе), и пересборка индекса (`--commit-if-stale`) в нём вызывается **только** +внутри `rebaseOnto()` — то есть только когда `devMoved === true` и требуется +ребейз. Когда `dev` не сдвинулся за время ревью (`decideMerge({devMoved: false})` +→ `fast-forward`, ветка `merge-candidate.mjs:250-253`), происходит прямой +`pushWithLease(tip, 'dev', devNow)` — индекс не пересобирается вообще. Это не +экзотика: это отдельная, специально тестируемая ветка решения +(`test/merge-candidate.test.mjs`: «dev не двигался: push кандидата как есть...»), +которая наступает всегда, когда во время ревью текущей задачи ни одна другая +задача не слилась в `dev` (тихие периоды, ночь, единственная задача в очереди — +именно то, чего #657 и добивается, снижая частоту параллельных слияний). + +**Воспроизведение** (настоящий git, неизменённый `scripts/reviews-index.mjs` из +этой ветки — без моков, без правок): + +``` +git init -q -b dev +# … docs/reviews/CODE-REVIEW-1-r1.md + `reviews-index.mjs` → коммит, индекс свеж +node scripts/reviews-index.mjs --dir=docs/reviews --check # OK, «индекс свеж» + +git checkout -q -b issue/700-x +# добавлен docs/reviews/CODE-REVIEW-700-r1.md, INDEX.md НЕ тронут (= поведение +# публикации документа в ветку задачи после этого диффа) +git commit -q -m "docs: review document for #700" + +git checkout -q dev +git merge --ff-only -q issue/700-x # = decideMerge({devMoved:false}) → fast-forward + +node scripts/reviews-index.mjs --dir=docs/reviews --check +# → "docs/reviews/INDEX.md устарел — пересобрать: node scripts/reviews-index.mjs" +# exit code: 1 +``` + +Это ровно тот же гейт, что стоит в `validate.yml` на `push` в `dev` +(`id: reviews_index`, шаг «Индекс ревью совпадает с каталогом docs/reviews», +`validate.yml:104-107`), и его провал **не** «continue-on-error» на уровне +итогового вердикта: шаг «Вердикт предполётных проверок» (`validate.yml:211-234`) +явно делает `check "индекс ревью совпадает с каталогом" "$REVIEWS_INDEX"` и +`exit $fail` — красный шаг красит весь `preflight` job, а значит и весь прогон +Validate на голове `dev`. + +**Почему это находка именно этой задачи, а не старый долг.** До #657 индекс уже +приезжал в `dev` свежим при fast-forward, потому что ветка сама несла +самодостаточный INDEX-коммит (см. выше). #657 убрал этот путь для CODE-REVIEW +(единственный тип документа, идущий через `merge-candidate.mjs`), не добавив +взамен пересборку в фаст-форвардной ветке слияния. AC таблицы хендоффа +проверяет ровно две вещи — что публикация в ветку задачи индекс НЕ трогает +(верно) и что `merge-candidate.mjs` «всё ещё» вызывает `--commit-if-stale» (тоже +формально верно — вызывает, но не на этом пути) — и не проверяет само +условие «после `integrate` свеж» для фаст-форвардного слияния, потому что +`test/merge-candidate.test.mjs` работает на fakeOps и не в состоянии увидеть +файловые последствия для `docs/reviews/`. + +**Чем красится:** сценарий выше — реальным `git merge --ff-only` + +`reviews-index.mjs --check` на неизменённом коде репозитория; это не +гипотетическая, а прямо воспроизводимая последовательность действий +конвейера при следующем же тихом слиянии. + +**Что нужно поправить (не мой выбор, но для ориентира владельцу):** в +`decideMerge`/`mergeCandidate` фаст-forward ветка обязана либо тоже дергать +`reviews-index --commit-if-stale --issue=$issue` на `tip` перед +`pushWithLease`, либо публикация документа в ветке задачи обязана сама +включать свежий INDEX-коммит для случая, когда слияние окажется +fast-forward (тогда придётся вернуть часть анализа конфликтов, которого 1б +избегает). Выбор архитектуры — авторский; здесь фиксируется только то, что +текущий код инвариант не держит. + +**Серьёзность:** High (блокирует). AC таблицы хендоффа для этой строки не +выполнен на реальном, регулярно достижимом пути; последствие — не тестовый +провал в ветке задачи, а красный Validate на `dev` после штатного слияния, +без явной причины в диффе того же дня (следующий разработчик будет +разбираться, что сломало `dev`, не находя в своём коммите ничего relevant). + +## Что проверено и корректно + +- **Бандл-политика (2б), правило коммита.** `scripts/bundle-policy.mjs` + (`bundleCommitErrors`, `isBundlePath`, `releaseTrailers`) и его подключение в + `validate-commit-provenance.mjs` (и хук `commit-msg`, и история в CI) — + прочитано полностью, логика согласована с `test/bundle-policy.test.mjs`. + Хук судит без даты автора (`authorDate` не передаётся) — значит, судится + всегда, а разграничение по `BUNDLE_RELEASE_ONLY_SINCE` работает только для + ретроспективной проверки истории в CI, что и заявлено («в хуке даты нет — + судится всегда»). Проверено чтением и сверкой с тестом, тест умеет падать + (мутанты `bundle-policy-rule-off`, `bundle-policy-cutoff-inverted`). +- **`--verify`/`--must-match` (сверка копий только на кандидате).** + `committedBundleMustMatch` корректно решает по путям коммита ИЛИ по + `isCandidateSubject` (кандидат, забывший пересобрать бандл, всё равно + сверяется) — прочитано, обосновано тестом (`bundle-policy-always-compares`, + `bundle-policy-candidate-not-compared`). Отдельно проверил вручную: + `git diff-tree -r --no-commit-id --name-only HEAD` для настоящего + двухродительского merge-коммита возвращает пустой список (эксперимент в + `/tmp/mergetest`) — то есть `commitShape()` в этом (маловероятном при линейной + модели `dev`) случае решит «копии не сверяются» даже если бандл менялся. + Это не считаю находкой уровня Medium: `merge-candidate.mjs` не создаёт + двухродительских коммитов в `dev` (только fast-forward/rebase-push), а + `validateCommitMessage`'s собственный range-check уже сознательно пропускает + `parentCount > 1` тем же способом (`continue`) — новый код просто следует + уже принятому в кодовой базе допущению. Фиксирую как наблюдение, не находку. +- **`assertCommittedBundleFresh` / публикация беты.** Читает манифест из + закоммиченного дерева публикуемого SHA (не с диска), сверяет с + `sourceFingerprint(root)` при чистом дереве на этом SHA — корректно; + мутант `release-prerelease-skips-fresh-bundle` доказывает, что вызов не + выпадает молча. +- **`bundle-sync --release` / `bundle:clean`.** `TARGETS` формируется по + наличию `--release`; без флага HACS-копия не трогается — проверено чтением + и тестом `test/bundle-sync.test.mjs` (реальный `spawnSync`, не мок). + `bundle-policy.mjs --clean` (`git checkout --`, `git clean -fdq`) возвращает + ровно корни бандла, не трогая остальное дерево — тест это подтверждает + (`notes.txt` остаётся). +- **`dev-build.mjs` / стенд.** `publishDevBuild` строит коммит без родителя во + временном индексе (не трогая рабочее дерево источника), публикует только если + `expectRef` не ушёл вперёд, иначе тихо уступает следующему прогону — прочитано + и подтверждено `test/dev-build.test.mjs` на настоящих git-репозиториях (bare + + клон), включая сценарий стенда (`update-dev-bundle.sh --reset` / без флага). + `validate.yml`: `dev_build` job условие `push && ref == refs/heads/dev`, + `continue-on-error: true`, не входит в `needs` job `proof` — подтверждено и + тестом `test/validate-workflow.test.mjs`, и чтением самого workflow. +- **`rebase-on-dev.mjs`.** Конфликт по бандлу теперь берёт версию `dev` без + пересборки/amend — согласовано с тем, что ветка бандл не несёт; `GENERATED_ROOTS` + теперь читается из `bundle-policy.BUNDLE_ROOTS`, единая точка. Тесты + (`test/rebase-on-dev.test.mjs`) переписаны без фальшивой пересборки и это + корректно отражает новое поведение. +- **`docs/reviews` — переключение публикации на `target=dev`.** Само условие в + `_process.yml` (строка `if [ -f "$doc" ] && [ "$target" = "dev" ]; then`) и + соответствующий мутант (`process-index-on-task-branch`) корректны как + локальное решение; ограничение находки H1 — не в этом условии, а в + отсутствующей компенсации на стороне `merge-candidate.mjs`. +- **Второй коммит (`339379f5`) — понижение базы `bundleBytes`.** Причинно + обосновано в теле коммита (Validate на первом коммите падал на храповике + связности монолита из-за постороннего дрейфа размера `dist` после хотфиксов + #649, не из-за этой задачи); не расширяет скоуп. +- **Трейлеры.** Оба коммита несут `Issue: #657` и `User-Visible: no` — + корректно: класса A нет, видимого пользователю поведения нет, changelog не + требуется. +- **Одно число — один источник.** Число job-загрузок артефакта `card-bundle` + (было 4, стало 5 — новая `dev_build`) сверено вручную по самому workflow + (`grep -n "name: card-bundle"` — ровно 5 вхождений: 1 upload + 4 download: + `dev_build`, `smoke`, и два прочих браузерных job) и совпадает с тем, что + проверяет `test/validate-workflow.test.mjs`. + +## Чего не проверял + +- **Golden, pytest, performance-профили** — диф не трогает визуал/Python/ + профили, AC их не называет. +- **`check-docs.mjs`** — не обязателен (диф не по `src/**`); заявление автора, + что этот гейт независимо красный на скриншотах и на базе `dev`, не + перепроверял (не относится к предмету ревью). +- **Живой прогон `merge-candidate.mjs` против настоящего GitHub API** — не + запускал (нужны токен и реальный репозиторий); H1 доказан на уровне логики + и на уровне `docs/reviews/INDEX.md`-инварианта напрямую, что для дефекта + этого типа достаточно и без интеграции с `gh`. +- **Двухродительские merge-коммиты в `dev`** — отмечено выше как + воспроизводимое, но не относящееся к используемому в проекте потоку + (см. «Что проверено и корректно»); не поднимаю до находки. + +## Итог + +AC "индекс ревью не пересобирается в ветке задачи; после `integrate` свеж" +не выполняется для fast-forward пути слияния — реального, тестируемого и, +по всей вероятности, частого исхода `decideMerge()`. Это ломает гейт +"индекс ревью совпадает с каталогом" на `dev` без видимой причины в +последующих диффах. Остальная часть задачи (бандл-политика 2б, стенд dev, +переход публикации документа на `target=dev`) прочитана и перепроверена +(частично исполнением) без находок. + +**Вердикт: красный · заход r1 · блокирующих циклов 0/4 · High: 1 · Medium: 0 → в задаче · Документ: docs/reviews/CODE-REVIEW-657-r1.md** + +--- + + + +## Материал раунда + +- Ветка: `issue/657-generated-at-merge`, коммит `339379f504f7` — ребейз его осиротит, и это нормально: ниже якоря, которые ребейз не меняет. +- Дерево материала: `b63cbf255cc76f784688d9ad7553444f9a89e337` + ``` + git log --all --format='%H %T' | grep b63cbf255cc7 + ``` +- Тело issue: `e15e73b22b32d0524d13035dc170c366dd9aa8a038a6ebb5dab193b2868f5043` +- Вердикт конвейера: `red` · High 1 diff --git a/docs/reviews/INDEX.md b/docs/reviews/INDEX.md index 8d805454..0dde5cdd 100644 --- a/docs/reviews/INDEX.md +++ b/docs/reviews/INDEX.md @@ -1,10 +1,11 @@ # Индекс ревью -Генерируется `node scripts/reviews-index.mjs` (#635) — не редактировать руками. Документов: 1073, issue: 379. Вердикт: 🟢 зелёный · 🟡 жёлтый · 🔴 красный · ⚪ не распознан (свободная форма старых документов). H/M — число High/Medium по строке вердикта или заголовкам находок. Файлы — пути, названные в находках; ищите по имени файла: `grep form-kit INDEX.md`. +Генерируется `node scripts/reviews-index.mjs` (#635) — не редактировать руками. Документов: 1074, issue: 380. Вердикт: 🟢 зелёный · 🟡 жёлтый · 🔴 красный · ⚪ не распознан (свободная форма старых документов). H/M — число High/Medium по строке вердикта или заголовкам находок. Файлы — пути, названные в находках; ищите по имени файла: `grep form-kit INDEX.md`. | Issue | Документ | Этап · раунд | Вердикт | H | M | Находки | Файлы | |---|---|---|---|---:|---:|---|---| | #660 | [SPEC-REVIEW-660-r1.md](SPEC-REVIEW-660-r1.md) | spec · r1 | 🔴 красный | 2 | 1 | Раздел ## ТЗ в теле issue отсутствует целиком; Изменение прямо противоречит двум местам; AC «расстояние уменьшено ровно вдвое» не | `docs/process/AUTHOR.md` `REVIEWER.md` `test/core-file-budget.test.mjs` `scripts/smoke-select.mjs` `demo/helpers/hp-test.mjs` `docs/UX-MODES.md` `docs/reviews/SPEC-REVIEW-647-r1.md` | +| #657 | [CODE-REVIEW-657-r1.md](CODE-REVIEW-657-r1.md) | code · r1 | 🔴 красный | 1 | 0 | после fast-forward слияния (dev не двигался за время ревью) docs/reviews/INDEX.md остаё… | `docs/reviews/INDEX.md` `test/reviews-index.test.mjs` `_process.yml` `merge-candidate.mjs` `test/merge-candidate.test.mjs` `scripts/reviews-index.mjs` | | #656 | [CODE-REVIEW-656-r1.md](CODE-REVIEW-656-r1.md) | code · r1 | 🟢 зелёный | 0 | 0 | — | — | | #654 | [SPEC-REVIEW-654-r1.md](SPEC-REVIEW-654-r1.md) | spec · r1 | 🟡 жёлтый | 0 | 1 | «Release-артефакты» не называют обновление docs/ISOMETRIC.md | `docs/ISOMETRIC.md` `docs/CHANGELOG.md` `docs/CHANGELOG.ru.md` `docs/reviews/INDEX.md` | | #654 | [SPEC-REVIEW-654-r2.md](SPEC-REVIEW-654-r2.md) | spec · r2 | 🟢 зелёный | 0 | 0 | — | — |