houseplan-card/docs/reviews/CODE-REVIEW-727-r1.md

CODE-REVIEW-727-r1

Issue: #727 · этап code · трек ask · заход r1 · блокирующих циклов 0/4 Материал: git log --oneline origin/dev..HEAD / git diff origin/dev...HEAD, ровно 93be52ebe8c3f7860eeb955c5dda2a6d50bdbabe (рабочая копия уже на нём)

Скоуп

Один коммит поверх dev (52dc08a0): ночной режим пакетного ревью ship и переиспользование его результата гейтом беты по патч-набору (К1–К9, AC1–AC10 из ТЗ в теле #727). Классы изменений — B (scripts/**, .github/workflows/**, test/**) и C (PROCESS.md, docs/process/REVIEWER.md); файлов класса A нет. User-Visible: no — изменений в обоих CHANGELOG не требуется и нет (проверено: git diff --stat по docs/CHANGELOG*.md пуст).

Файлы: scripts/ship-review.mjs (+428/-…), scripts/reviews-archive.mjs, scripts/reviews-index.mjs, .github/workflows/_nightly.yml, .github/workflows/_ship-review.yml, PROCESS.md (§10.4, §11.7), docs/process/REVIEWER.md, test/ship-review.test.mjs, test/nightly-workflow.test.mjs, test/process-digests.test.mjs.

Спец-ревью пройдено за r1→r2 (единственная Medium-находка r1 по К7/АС7 — нереализуемость правила архивации для стабильной базы без примера в АС — закрыта в r2, вердикт зелёный, docs/reviews/SPEC-REVIEW-727-r2.md). Трек ask подтверждён верно: сложность/риск >3 (четыре поверхности) и публичный контракт гейта беты меняется.

Как проверялось

Прочитано построчно: scripts/ship-review.mjs целиком (591 строка, не только дифф), scripts/reviews-archive.mjs/reviews-index.mjs диффы с контекстом вызывающих функций (compareStable, stableTagsThrough, ordered), .github/workflows/_nightly.yml и _ship-review.yml целиком, PROCESS.md §10.4/§11.7 и docs/process/REVIEWER.md диффы, весь новый текст test/ship-review.test.mjs (580 новых строк) и test/nightly-workflow.test.mjs (137 новых строк).

Каждый AC1–AC9 сверен построчно с кодом (оракул AC → конкретная функция/шаг → тест, который его проверяет). AC10 — гейты, ниже.

Исполнено, не только прочитано:

• node --test test/ship-review.test.mjs test/nightly-workflow.test.mjs test/process-digests.test.mjs test/default-branch-workflows.test.mjs — 29 + … тестов, все зелёные (включая тесты на настоящем bash/git во временных репозиториях — shipSandbox, tempRepo, runStep).
• node scripts/mutation-gate.mjs --check — exit 0, 3 предупреждения (те же три имени, что называет автор как «как на dev»: corpus-loses-its-short-edge, optimize-reports-work-it-did-not-do, nightly-reuse-accepts-stale-marker — все три про геометрический корпус/#650, не про этот дифф). Якорь ship-review-ignores-merge-marker найден и зелёный, новые мутанты ship-review-accepts-partial-coverage/ship-review-accepts-high в реестре есть и зелёные.
• node scripts/entry-cost.mjs --check — зелёный.
• node scripts/reviews-index.mjs --check — зелёный («INDEX.md свеж»).
• node scripts/smoke-select.mjs --base origin/dev --head HEAD — «исполняемого frontend-диффа нет, browser-smoke не выбираются»: src/**/*.ts не тронут, смоки объективно нечего выбирать, а не решение их пропустить.
• Мутационная проверка вручную (дисциплина «тест умеет падать») на двух участках с наибольшим риском тихой порчи:
  • samePatchSet → return true (покрытие никогда не станет stale): упал AC3, AC4, AC5 и интеграционный AC2/AC5-тест — 4 из 19 тестов файла.
  • archivePlan: ordered.find(...) → [...ordered].reverse().find(...) (брать не ближайшую линию новее базы, а самую дальнюю — тестовый случай АС7 (в), [v1.78.0, v1.78.1, v1.79.0] должен дать v1.78.1): упал ровно тест АС7. Файл восстановлен cp из бэкапа после обеих проверок, git diff после — пуст.

Не прогонял и почему: actionlint — бинарь недоступен в этой песочнице; автор заявляет «чистый по изменённым файлам», не перепроверено независимо — риск низкий: YAML run:-блоки этого диффа уже проверяются bash -n в исполняемых тестах (test/ship-review.test.mjs, test/nightly-workflow.test.mjs), а структурные инварианты (поля job, порядок шагов, needs/if/permissions) — текстовыми assert'ами на реальном файле. npx tsc --noEmit / npm test (полный) / npm run build — не перегонял: Validate зелёный на этом SHA (ссылка в промпте ревью), дешёвые гейты закрыты им. Python/pytest tests_backend — не трогается, нет правки custom_components/**/*.py. npm run golden:verify и invariants — нет ci:golden, нет правки геометрии/рендера. Performance — не назван в AC.

Находки

Нет High. Нет Medium в скоупе задачи.

Low (наблюдение, не блокирует, не правлю и не снимаю — требует решения владельца о сроке). ТЗ (раздел «Разбиение») инструктирует: «Новый issue E (завести в S1-new, ссылка на #727): «Красная ночь: комментарий в задачи, слитые после последней зелёной»». Поиском по заголовку/метке S1-new такой issue в репозитории не найден — похоже, не заведён. Это явно вне скоупа самого #727 (раздел «Не-скоуп»: «Красная ночь — issue E»), поэтому не влияет ни на один AC1–AC10 и не блокирует это ревью; но без заведённой задачи инструкция ТЗ технически не выполнена полностью, и решение, которое владелец принял при развилке #707, рискует потеряться. Не завожу отдельным issue сам: это не найденный попутный дефект кода (критерий §12 «вне скоупа»), а административный шаг, порядок исполнения которого (сейчас / при следующей правке конвейера) решает не ревьюер.

Low (наблюдение). readRangeDocs — новая, нетривиальная функция (сортировка по глубине коммита через rev-list --count, дедупликация документа с одним именем между candidate и devRef в пользу devRef, отсечение чужой базы для ночных имён по regex). Прямого unit-теста на саму функцию нет: она проверяется только через интеграционный shipSandbox-тест, где сценарий «один и тот же документ с разным содержимым одновременно в дереве кандидата и в origin/dev» не встречается (там документ либо ещё не существует, либо уже запушен в оба места с одним и тем же содержимым). Логика — простая перезапись Map по порядку обхода [candidate, devRef], поведение прочитано и совпадает с докстрингом; риск низкий, повторной сессии не прошу.

Проверка критериев приёмки

AC	Что	Доказательство в коде	Тест	Вердикт
AC1 К1	Патч-набор: git patch-id --stable, без Release:, без коммитов только docs/reviews/**, порядок не важен, cherry-pick даёт тот же id	hasReleaseTrailer, countsForPatchSet, commitPatchIds (явные опции диффа — PATCH_DIFF), issuePatchSets	#727 AC1 — временный git-репозиторий, реальный git patch-id	доказано исполнением
AC2 К2	tag=nightly требует candidate; имя документа по базе+SHA12; prepare отдаёт только none/stale; блок несёт mode/patches в хвосте	shipReviewMode, nightlyDocPath, shipDocPath, anchorBlock/parseAnchorBlock	#727 AC2 (unit) + #727 AC2/AC5 и #727 AC2 _ship-review.yml (реальный bash, реальный workflow-файл через stepRun)	доказано исполнением
AC3 К3	Четыре статуса покрытия; «последний документ главнее»; чужая база не в счёт; документ без patches — по номеру	shipCoverage	#727 AC3 — все переходы статусов, включая «high отменяет ранний clean» и наоборот	доказано исполнением; мутация samePatchSet→true ловится тестом
AC4 К4	Гейт: всё clean без документа тега проходит; none/stale/high — отказ с командой (force только когда нужен)	shipReviewProblems	#727 AC4	доказано исполнением
AC5 К5	Бета читает дельту; бриф называет прочитанное ночью; force=true — всё; пустая дельта — без модели	planShipReview, renderShipBrief	#727 AC5 (unit) + #727 AC2/AC5 (сквозной сценарий на реальном workflow)	доказано исполнением
AC6 К6	Новая job после Validate, if: always(), не красит ночь, ждёт только появления прогона (18×10с)	.github/workflows/_nightly.yml (ship_review job)	#727 AC6 (структура YAML) + два теста на реальном bash (runStep) — вывод SHA до ожидания, красный Validate красит именно job ожидания, dispatch/appearance таймауты — предупреждение, не красная ночь	доказано исполнением
AC7 К7	parseDocName узнаёт ночное имя; archivePlan: база-бета → своя линия, стабильная база → ближайшая новее, без линии новее — kept	parseDocName (SHIP_DOC_NAME с опциональной -dev-sha12 группой), archivePlan (новая ветка doc.nightly && STABLE_TAG_RE), renderIndex (сортировка ночь/небета, подпись «ночь после»)	#727 AC7 — все четыре примера (а)-(г) из АС дословно	доказано исполнением; мутация find→reverse().find ловится тестом (в)
AC8 К8	Строка о High только ночью, с меткой документа, без повтора на тот же документ	highCommentBody, highCommentTargets, шаг «High ночью» в _ship-review.yml (if: needs.prepare.outputs.mode == 'nightly')	#727 AC8 (unit) + #727 AC8 _ship-review.yml (реальный bash: beta не пишет, nightly без High не пишет Medium/Low, nightly с High — только в задачу без метки)	доказано исполнением
AC9 К9	Канон — PROCESS.md §11.7/§10.4, REVIEWER.md, ключевое правило в process-digests	диффы PROCESS.md/REVIEWER.md прочитаны целиком; test/process-digests.test.mjs	node --test test/process-digests.test.mjs зелёный; entry-cost --check зелёный	доказано исполнением
AC10	Гейты	—	gate:small (заявлен автором, зелёный Validate подтверждает дешёвую часть), mutation-gate --check, reviews-index --check — перепрогнаны мной, зелёные; якорь найден	доказано исполнением (частично — моим прогоном)

Что проверено и корректно

• Патч-набор исключает ровно то, что нужно: Release:-коммит кандидата беты (несущий Issue: всей линии) и чисто-доковые коммиты — иначе ночной набор никогда бы не совпал с набором на кандидате беты (тест это явно демонстрирует: один и тот же набор до и после Release:-коммита).
• shipReviewProblems корректно различает «документа тега нет вовсе» (нет force в команде) от «документ тега есть, но не всё покрывает» (force обязателен) — оба случая сведены к конкретным regex-проверкам в тестах.
• archivePlan: ветвление «база-бета» (точное совпадение, прежняя логика) и «стабильная база» (поиск ближайшей линии строго новее, новая логика через уже существующий compareStable) не пересекаются и не регрессируют старый deepEqual-тест #696 (проверено прогоном).
• _nightly.yml: SHA прогона Validate выводится шагом id: validate до шага ожидания (gh run watch --exit-status), поэтому красный Validate не теряет кандидата для ночного ship-ревью — ключевое свойство АС6, прочитано и подтверждено порядком шагов и outputs: на уровне job. ship_review job зависит только от dispatch (не от его успеха — if: always()) и несёт continue-on-error: true, так что сбой диспетчеризации ship-ревью не красит ночь — ровно то поведение, которое требует АС6 («предупреждение, не красная ночь»).
• _ship-review.yml: mode/patches публикуемого блока берутся из выходов prepare (детерминированный скрипт), а не из JSON-результата модели — единственный источник числа patch-id не раздваивается между моделью и шагом публикации (§8, «одно число — один источник»); явно проверено тестом с «подложным» patches в результате модели, который публикация игнорирует.
• Трейлеры коммита корректны: Issue: #727, User-Visible: no, Co-Authored-By/Claude-Session — на месте.

Унаследовано из спец-ревью (без повторной проверки)

Продуктовых вопросов владельцу по ТЗ не было (спец-ревью r2, зелёный, docs/reviews/SPEC-REVIEW-727-r2.md, материал a49f7095). Код-ревью принимает формулировки К1–К9 и границы скоупа/не-скоупа как данность и проверяет только соответствие кода этим формулировкам — что и сделано выше.

Чего не проверял

• actionlint — бинарь недоступен в песочнице ревью; заявление автора не перепроверено независимо (риск низкий, см. «Как проверялось»).
• Полные npx tsc --noEmit / npm test / npm run build — не перегонял: зелёный Validate на 93be52eb (ссылка в промпте) закрывает дешёвые гейты для этого SHA.
• Живой ночной прогон (_nightly.yml → ship-review.yml -f tag=nightly на реальном GitHub Actions, включая git describe с настоящими тегами в checkout публикации) — автор сам называет это риском и предлагает понаблюдать первую живую ночь; контрактные тесты на реальном bash/git это закрывают настолько, насколько возможно до прод-прогона, но не заменяют его.
• Browser-smoke, golden, performance, HA-pytest — объективно не применимы к этому диффу (обоснование — «Как проверялось»).

Вердикт

Все AC1–AC10 доказаны исполнением (не только чтением), два defensive-правила (stale-покрытие, выбор ближайшей архивной линии) проверены мутацией вручную и тест их ловит. High и Medium в скоупе нет. Одно Low-наблюдение (issue E из ТЗ не заведён) — вне скоупа #727, не блокирует.

Зелёный.

---

Материал раунда

• Ветка: issue/727-nightly-ship-review, коммит 93be52ebe8c3 — ребейз его осиротит, и это нормально: ниже якоря, которые ребейз не меняет.
• Дерево материала: 990dd76b955a9fc9d630dd09709eaa5d4f052e05

  git log --all --format='%H %T' | grep 990dd76b955a
  

• Тело issue: 8ce2942ca915bc938c8c5b4a720bb71d5a06d18c13db0692ee802f6a55662b7a
• Вердикт конвейера: green · High 0