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

CODE-REVIEW-723-r1

Issue: #723 · Этап: code · Заход: r1 · Трек: show (PROCESS.md §5, #696) Материал ревью: b4e7eb86e6800ae4f7abdf0f9e52aa02855864d6 (ветка issue/723-publish-push-refusal, HEAD этой копии) База сравнения: origin/dev = 7188db8187bee4fb601a0edea0acdfa0934b53f6 (dev впереди на 6 коммитов, слияние без конфликта — материал не приводился к dev, трек show) Validate на материале: success, https://github.com/Matysh/houseplan-card/actions/runs/36784530059 (переиспользован, не перегонялся — #343)

Скоуп

Диапазон git log --oneline origin/dev..HEAD: один коммит b4e7eb86. Диф git diff origin/dev...HEAD --stat:

.github/workflows/_process.yml       |  44 +++-
.github/workflows/release-review.yml |  35 +++-
PROCESS.md                           |   5 +-
scripts/merge-candidate.mjs          |  44 +++-
test/publish-push-refusal.test.mjs   | 395 +++++++++++++++++++++++++++++++++++
test/release-review.test.mjs         |   4 +-
6 files changed, 503 insertions(+), 24 deletions(-)

Ровно поверхность, названная в ТЗ (release-review.yml, _process.yml, тесты) плюс переиспользуемый код разбора отказа в scripts/merge-candidate.mjs (тот же модуль, что несёт classifyPushRefusal/describePushRefusal для стража ребейза #705) и одно предложение в PROCESS.md. Выхода за скоуп нет.

Задача: два шага публикации коммита (документ ревью релиза в dev — release-review.yml; документ ревью в ветку задачи — _process.yml) считали любой отказ git push сдвигом ветки и слепо повторяли/ребейзили. Отказ самого GitHub (право на workflow, правило ветки, хук) под эту диагностику не попадал и превращался в бесполезный повтор без объяснения причины — тот же класс бага, что чинил #705 для стража ребейза.

AC · чем доказан · чем краснеет

AC	Чем доказан	Чем краснеет
AC1: оба шага разбирают stderr push через merge-candidate.mjs --push-refusal; устаревший lease → прежний повтор/ребейз; отказ GitHub → шаг останавливается без повторов, причина и ответ git без токена — в журнале и $GITHUB_STEP_SUMMARY	test/publish-push-refusal.test.mjs — оба шага исполняются настоящим bash и настоящим git во временных репозиториях (подменён только транспорт push); проверено чтением + исполнением	На старых (dev) версиях release-review.yml/_process.yml с тем же тестовым файлом падают 9 из 11 тестов (проверено мной лично, см. «Как проверялось»)
AC2: redactSecrets вычищает токен, URL с учётными данными и Authorization из текста сводки/журнала	noSecrets()/noisySecretsGone() в каждом тесте отказа + отдельный юнит-тест с заголовком Authorization: Bearer и чужим токеном в stderr	Тест использует настоящий токен-паттерн (ghs_…, ghp_…) и заголовок Authorization: Basic/Bearer …; без вызова redactSecrets/без правки regex тест не пройдёт — проверено локальным запуском
AC3: в run: обоих шагов нет многострочного текста/heredoc; сводку пишет код (refusalSummary), не строка в workflow	Отдельный тест регэкспами проверяет тело run: на отсутствие <<-heredoc и наличие вызова merge-candidate.mjs --push-refusal=… --summary=…; bash -n и python3 -c "yaml.safe_load(...)" на обоих файлах — мной, см. ниже	Тест ищет буквальный heredoc-паттерн и упадёт, если кто-то вернёт <<EOF; bash -n упал бы при обрыве блока run:

Все три AC — из категории «код должен делать одно, а не другое при отказе» (по сути защитный/разветвляющий сценарий), и для всех трёх столбец «чем краснеет» заполнен результатом реального прогона, а не словом «Verified».

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

1. Прочитан diff файл за файлом (.github/workflows/release-review.yml, .github/workflows/_process.yml, scripts/merge-candidate.mjs, PROCESS.md, test/release-review.test.mjs) — см. ниже по каждому.
2. Прогнан сам новый тест node --test test/publish-push-refusal.test.mjs на материале ревью: 11/11 зелёных.
3. Мутационная проверка вручную (тест должен уметь падать, #723 сам заявляет «на старых workflow красные 9 из 11» — заявление автора, не доказательство): поднял git worktree add /tmp/hp-dev-check origin/dev --detach, скопировал в него новый test/publish-push-refusal.test.mjs и новый scripts/merge-candidate.mjs (источник refusalSummary/--summary), но оставил старые .github/workflows/*.yml из dev. Результат: # pass 2 / # fail 9 — совпадает с заявлением автора дословно. Worktree удалён (git worktree remove --force), рабочая копия не изменена.
4. bash -n на извлечённом теле обоих шагов run: (тем же способом, что тест вырезает блок — через первую строку с отступом меньше десяти) — чисто на обоих.
5. python3 -c "yaml.safe_load(...)" на обоих workflow-файлах — валидный YAML.
6. node scripts/smoke-select.mjs --base origin/dev --head HEAD — «Исполняемого frontend-диффа нет… Browser-smoke этим диффом не выбираются — это не пропустить проверки, а выбирать нечего». Согласуется с диффом: ни один изменённый файл не относится к src/**.
7. Сверены трейлеры коммита b4e7eb86: Issue: #723, User-Visible: no — есть оба; правка инфраструктурная (workflow-шаги публикации, не пользовательское поведение карточки), classification User-Visible: no корректна, changelog не требуется.
8. Прослежен путь отказа kind != stale до места, где он проявится пользователю процесса: в _process.yml job уже имеет шаг «Позвать владельца, если стадия упала» (if: failure()), так что жёсткий exit 1 новой веткой не проваливается молча — сеть уведомления не порвана. Для release-review.yml (manual dispatch) красный прогон виден тому, кто его запустил — как и раньше, до этой правки, поведение при исчерпании трёх попыток было тем же (exit 1 в конце цикла).

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

• Разбор причины (AC1). Оба шага: push_err пишется через 2> "$push_err", далее kind=$(node …/merge-candidate.mjs --push-refusal=… --ref=… --stage=… --summary=…) || kind=unknown; kind != stale останавливает шаг с exit 1 и понятным ::error::. Совпадает с существующим паттерном стража ребейза (#705, _process.yml:565-591), включая то, что «прочий» отказ (в том числе unknown — сеть/аутентификация) не лечится повтором — то же решение, что там уже принято веткой case … *) exit 1.
• _process.yml берёт merge-candidate.mjs из dev (git archive origin/dev scripts | tar -x -C "$tools"), а не из рабочей копии ветки задачи — обоснованно: ветка задачи может быть старше и не нести новый CLI-флаг --summary/--stage. Тот же приём уже применяет страж ребейза.
• refusalSummary (scripts/merge-candidate.mjs:517-547) собирает текст кодом, не строкой в run:; unfence() защищает markdown-блок сводки от тройных бэктиков в ответе GitHub; вызывается из CLI только когда kind !== stale, то есть сводка появляется ровно тогда, когда шаг не повторяет. Порядок аргументов (ref, stage) совпадает на обоих сайтах вызова.
• AC2 (redactSecrets). Секреты вычищаются до того, как текст попадает и в console.error (журнал), и в файл --summary: оба берут уже классифицированный refusal.stderr/reason, которые classifyPushRefusal прогоняет через redactSecrets на входе. Тест специально кладёт «шумный» stderr с заголовком Authorization: Basic … и чужим токеном (noisyRejected) — оба вычищены.
• AC3 (heredoc → построчная запись). Оба места, ранее писавшие сообщение коммита через <<MSG/<<EOF с интерполяцией переменных ($TAG, $counts, $NUM) внутри heredoc, переведены на { echo …; echo …; } > "$msg" + commit -q -F "$msg". Это не только соответствие форме (PROCESS.md §10.4 п.4 описывает подрыв heredoc нулевым отступом), но и защита от более тонкого случая: unquoted heredoc интерполирует переменные до сравнения с терминатором, и значение, совпавшее с MSG/EOF на отдельной строке, оборвало бы heredoc раньше времени. Построчный echo "...$var..." этому не подвержен — переменная разворачивается один раз как аргумент, а не как часть текста, заново читаемого шеллом на терминатор. Остальные heredoc-и в репозитории (_process.yml, ship-review.yml, validate.yml, release.yml) не тронуты и не должны быть: они либо несут статичный текст, либо (<<'NODE') явно закавычены и не интерполируют переменные — риска, который правит именно этот AC, там нет.
• PROCESS.md. Одно предложение у стража ребейза, ссылается на #723 и описывает то же различение (lease vs отказ GitHub) для обоих шагов публикации — формулировка совпадает с реализацией.
• test/release-review.test.mjs. Точечная правка: старая проверка «текст трейлеров присутствует одной строкой» заменена на проверку «трейлеры пишет echo» — согласуется с тем, что точный текст обоих коммитов теперь проверяет новый файл, а не этот тест.
• Не выходит за трек show. Мутанты по диффу не запрошены и не требуются (#696); их отсутствие не находка.

Находки

Нет. Ни одной High/Medium/Low находки — ни в скоупе, ни вне его.

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

• actionlint — бинарь не установлен в этом окружении. Не перегонял; заменил на python3 yaml.safe_load (валидность YAML) и bash -n на теле каждого run:-шага (синтаксис bash) — оба чище. actionlint добавляет поверх этого только schema-проверки полей самого workflow (не затронутых диффом, кроме двух изменённых шагов) — риск низкий, но это не эквивалент заявленного автором прогона, и я его не воспроизвёл.
• npx tsc --noEmit, npm test (полный), npm run build — не перегонял: Validate уже зелёный на этом самом SHA (run 36784530059, #343), сверка бандла входит в его состав. Прогнал только точечно новый файл теста (node --test test/publish-push-refusal.test.mjs) и ручную мутацию на соседнем worktree — этого было достаточно для AC этой задачи.
• npm run invariants, npm run golden:verify, pytest tests_backend — не применимы: диф не трогает геометрию, рендер карточки, метки ci:golden или custom_components/**/*.py.
• Браузерные смоки — не прогонял; smoke-select.mjs подтвердил, что выбирать нечего (диф не касается src/**), и ни один AC задачи смоук не называет.
• mutation-gate --check — не перегонял; трек show не требует мутантов по диффу (#696), автор привёл число как факультативную информацию, свериться с ней вне скоупа ревью.
• Ручное исполнение реальных GitHub Actions ранов (сам прогон воркфлоу в CI с намеренным отказом push) не делал — это выходит за рамки дешёвых гейтов ревью; вместо этого исполнил тело run:-шагов как есть настоящим bash и git во временном песочничном репозитории (то же, что делает test/publish-push-refusal.test.mjs), включая обратную мутацию на старом коде dev для проверки «тест умеет падать».

Вердикт

Зелёный. Все три AC доказаны исполнением (не только чтением), тест воспроизводимо красен на коде до правки (9/11, подтверждено независимым прогоном на origin/dev), секреты вычищаются, heredoc уязвимость к интерполяции устранена по существу, скоуп не нарушен, трейлеры на месте.

---

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

• Ветка: issue/723-publish-push-refusal, коммит b4e7eb86e680 — ребейз его осиротит, и это нормально: ниже якоря, которые ребейз не меняет.
• Дерево материала: 03963a1231ca2501b3bde09aa1b595db2b618ced

  git log --all --format='%H %T' | grep 03963a1231ca
  

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