16 KiB
CODE-REVIEW — issue #656, заход r1
Материал раунда
- SHA материала:
43fab645b0d07a5ead78341ebca5f788db13a0fc(веткаissue/656-release-proof), ровно один коммит поверхorigin/dev. - Validate на этом SHA: green — https://github.com/Matysh/houseplan-card/actions/runs/36225639293.
- Класс изменений: B (
scripts/ci-proof.mjs,scripts/release-gate.mjs,.github/workflows/performance.yml,test/**) + C (PROCESS.md,docs/DEVELOPMENT.md,docs/STATUS.md). Класса A нет — инфраструктурная задача, вход сразу наS7-code-reviewподтверждён. - Трейлеры коммита:
Issue: #656,User-Visible: no— присутствуют и верны (изменение не видимо пользователю продукта).
Скоуп
Issue описывает два инцидента аудита 26.09 и просит по ним 4 пункта в разделе «Ожидается»:
- Зафиксировать и реализовать политику proof (fail-open → fail-closed) — вариант (а) или (б), с тестами на оба случая.
- Починить/обойти красный Validate на
mainдо промоушена 1.78.0. - Сухой прогон
scripts/release-gate.mjsна SHA кандидата 1.78.0 с результатом в issue до промоушена. performance.yml:paths-ignoreдля.github/workflows/**иdocs/**.
Материал этого коммита закрывает пункты 1 и 4. Пункты 2 и 3 в диффе отсутствуют — см. «Открытые пункты» ниже; это не дефект кода под ревью, а разбор того, что именно доказано этим коммитом.
Как проверялось
Дешёвые гейты (typecheck/test/build) подтверждены зелёным Validate на этом SHA — не перегонял. Диф не трогает src/**, поэтому check-docs.mjs не требуется. Дополнительно прогнал целевые проверки и мутационную проверку «тест умеет падать»:
| Гейт | Прогнан | Результат |
|---|---|---|
| Validate (typecheck/test/build/docs/provenance/process-gate) | нет, зачтён по зелёной ссылке на этот SHA | green |
node --test test/ci-proof.test.mjs test/release-gate.test.mjs |
да | 31/31 green |
node --test test/performance-workflow.test.mjs |
да | 6/6 green |
node scripts/smoke-select.mjs --base HEAD~1 --head HEAD |
да | «Browser-smoke этим диффом не выбираются» — нет src/** в диффе, смоки не нужны |
Мутация: откат scripts/ci-proof.mjs + scripts/release-gate.mjs к родителю (9287f798), тесты того же файла |
да | ключевой тест #656: a newer failed full proof blocks an older green proof on the exact SHA красный ('green' !== 'failed'), остальные 29/31 green |
Мутация: откат .github/workflows/performance.yml к родителю |
да | test/performance-workflow.test.mjs 5/6 green, 1 fail (искомая строка paths-ignore отсутствует) |
golden:verify, pytest tests_backend, npm run invariants, performance-профили |
нет | диф не меняет src/**, Python, геометрию; в AC не названы |
node scripts/check-docs.mjs |
нет | диф не трогает src/** |
Рабочая копия после обеих мутационных проверок восстановлена до состояния коммита (git status --short — чисто), файлы правились только временно для проверки и возвращены.
AC · чем доказан · чем краснеет (защитные AC)
| AC | Чем доказан | Чем краснеет |
|---|---|---|
Среди совместимых полных Validate-прогонов на одном SHA решает новейший: поздний failed/missing/pending блокирует более ранний зелёный |
test/release-gate.test.mjs: «#656: a newer failed full proof blocks an older green proof on the exact SHA» |
Мутация — откат ci-proof.mjs/release-gate.mjs к родительскому коммиту: тест краснеет, возвращая green вместо ожидаемого failed (проверено мной прогоном, см. таблицу выше) |
light/stale/cancelled прогон не вытесняет более раннее совместимое доказательство |
test/release-gate.test.mjs: «#656: a newer failed light proof cannot hide an older compatible green proof»; test/ci-proof.test.mjs: «#656 AC: newest compatible full proof decides; light proofs do not» |
Та же мутация — на старом коде эти тесты уже проходили (это старое поведение #541/#511), различие видно на первом ряду таблицы; сам новый тест дополнительно проверен на актуальном коде (green) |
| Новый полный зелёный прогон обновляет старый красный | test/release-gate.test.mjs: «#541: a later complete full proof refreshes an older red release candidate» |
Существовавший тест, поведение не менялось этим коммитом — проверено чтением, не мутировал повторно |
performance.yml не запускает полные бенчмарки на push в main, тронувший только .github/workflows/**/docs/** |
test/performance-workflow.test.mjs проверяет наличие paths-ignore в YAML под push: |
Мутация — откат performance.yml к родителю: тест краснеет (1 fail из 6), проверено мной прогоном |
Пустых третьих столбцов нет.
Что проверено и корректно
- Логика
classifyValidateProofs(scripts/release-gate.mjs:56-86): прогоны читаются от нового к старому (newestFirst), цикл продолжает сквозьcancelled/staleи останавливается на первом «решающем» статусе (green/failed/missing/pending), после чегоselectCiProofVerdict(scripts/ci-proof.mjs:442-448) просто берёт первый нецензурированный элемент. Разобрал построчно и мысленно прогнал сценарий из инцидента («зелёный push + красныйworkflow_dispatchна том же дереве») — новый код возвращаетfailed, старый возвращалgreen; воспроизвёл это прогоном (см. мутацию выше), а не только чтением. - Стабильность набора job между повторными прогонами одного SHA:
selection(backend/geometry_parity/…) вvalidate.ymlсчитается черезgit merge-base origin/dev "$HEAD_SHA"(.github/workflows/validate.yml:318) — для фиксированного кандидата наdevmerge-base не плывёт при повторных прогонах, то есть «набор запрошенных job» между двумя полными прогонами одного SHA не может разойтись за счёт дрейфа диффа. Проверено чтением, не исполнением — вариант (а) из issue («разница в наборе job» как исключение) поэтому корректно свёрнут к единственной практически значимой оси —full/light(черезpolicy.full), отдельного сравнения списков не требуется. review/mergeне затронуты и не должны быть:scripts/validate-gate.mjsиscripts/merge-candidate.mjsвызываютevaluateCiProofна одном конкретном (только что продиспатченном) прогоне, а не сканируют историю черезclassifyValidateProofs/selectCiProofVerdict— семантика #541 для них действительно не меняется, как заявлено вdocs/DEVELOPMENT.md.- Документация — одно число, один источник:
PROCESS.md§11.6 (новый раздел),docs/DEVELOPMENT.md:624-630иdocs/STATUS.md(строка CI) описывают правило одинаковыми словами («newest compatible full run… a later failed full run blocks an older green proof… a later complete green full run can refresh an older failure»); проверилgrepпо репозиторию — старая формулировка «any complete green full proof… is sufficient» нигде больше не осталась активной (только в архивныхdocs/reviews/CODE-REVIEW-619-r1.md, что законно — исторический документ прошлого раунда). Тестtest/release-gate.test.mjs(«#541: the release documents describe proof semantics») сверяет актуальность текстаdocs/DEVELOPMENT.mdс кодом. performance.yml(.github/workflows/performance.yml:6-11):paths-ignoreвложен корректно — только подpush, не затрагиваетschedule/workflow_dispatch, так что еженедельный и ручной прогоны не теряются. Реальный промоушен-коммит вmain(version fields,dist/**,CHANGELOG*, по «Promotion rule» AGENTS.md) не попадает под.github/workflows/**/docs/**, поэтому Full Performance по-прежнему запустится на настоящем кандидате — паттерн не рискует молча пропустить нужный прогон.- Тесты
test/ci-proof.test.mjs/test/release-gate.test.mjsпереименованы и переписаны так, что старые формулировки (#619: … survives a newer failed duplicate) заменены на противоположное по смыслу утверждение (#656: … blocks …) — это осознанная смена контракта, а не тихая правка ожидания; в отличие от «правки теста, чтобы он перестал падать», здесь меняется сама политика, зафиксированная в PROCESS.md, и это прослеживается по issue.
Находки
Пусто — блокирующих (High) и находок Medium в скоупе нет.
Открытые пункты (не находки, для протокола)
Issue перечисляет 4 пункта «Ожидается»; этот коммит закрывает 1 и 4. Пункты 2 (красный Validate на main) и 3 (сухой прогон release-gate.mjs на кандидате 1.78.0, запись в issue) в материале отсутствуют:
- Оба пункта в тексте issue явно привязаны к моменту «до промоушена» stable-релиза, а не к этому коммиту. На дереве уже есть беты
v1.78.0-beta.1..3, стабильного тега ещё нет — то есть кандидат-SHA для пункта 3 пока не выбран, прогонять дословно нечего. - Пункт 2 сам называет допустимым решением «промоушен вскоре после починки п.1» (после fast-forward
mainунаследует текущийaction-pins.mjs/тесты изdev, и красный уйдёт сам) — это операционное решение владельца о релизе, не код этой задачи. - Ни то ни другое не меняет вердикт по коду, который здесь под ревью: это не Medium «в скоупе» (нечего чинить кодом сейчас) и не Medium «вне скоупа» (не посторонний дефект — это тот же issue, следующий шаг которого явно назначен на другое время). Фиксирую это здесь, чтобы «оставили в тексте ревью» не выглядело как основание не возвращаться к пунктам 2/3 — они остаются в issue #656 до его закрытия (батч на релизе, не на этом код-ревью).
Чего не проверял
- Живой прогон
scripts/release-gate.mjs/waitForGreenWorkflowпротив настоящего GitHub API — код проверен модульными тестами (fixtures), не end-to-end диспетчем; это соответствует объёму гейтов задачи такого размера. golden:verify,pytest tests_backend,npm run invariants, performance-профили — не прогонял, диф не трогаетsrc/**, Python-бэкенд или геометрию, и ни один из них не назван в AC.- Пункты 2 и 3 из «Ожидается» — не проверял по существу (см. «Открытые пункты»): для пункта 3 попросту нет ещё кандидата, для пункта 2 нет назначенной даты промоушена в материале.
- Ручное тестирование UI — не применимо, изменение не пользовательское (CI/процесс).
Вердикт
Зелёный. Пункт 1 (fail-closed политика) и пункт 4 (performance.yml paths-ignore) реализованы полно, тесты доказывают AC и умеют падать (проверено мутацией на обоих затронутых гейтах), документация синхронна (PROCESS.md/DEVELOPMENT.md/STATUS.md — одна формулировка, один источник). Пункты 2 и 3 не входят в этот коммит по замыслу самого issue (действия «до промоушена», а не код этой задачи) и не образуют находку.
Материал раунда
- Ветка:
issue/656-release-proof, коммит43fab645b0d0— ребейз его осиротит, и это нормально: ниже якоря, которые ребейз не меняет. - Дерево материала:
34573345f65403cf240cd945b7662dd2aca74191git log --all --format='%H %T' | grep 34573345f654 - Тело issue:
e72159f00e412e8dea5068bb2bb071a621ee0c4fc029c67aef1d67d71249b297 - Вердикт конвейера:
green· High 0