mirror of
https://github.com/Matysh/houseplan-card
synced 2026-10-01 12:18:51 +00:00
claude[bot]
@@ -0,0 +1,113 @@
|
||||
# CODE-REVIEW #730 · заход r1
|
||||
|
||||
**Материал**: `981579aabbdbb93b4c514cad540735426e14b50e` (ветка `issue/730-derived-push-refusal`, рабочая копия на этом SHA; `git diff origin/dev...HEAD` / `git log --oneline origin/dev..HEAD`).
|
||||
Трек: `show`. Дешёвые гейты (`tsc --noEmit`, `npm test`, `npm run build` со сверкой бандла) подтверждены зелёным Validate на этом SHA: https://github.com/Matysh/houseplan-card/actions/runs/36790168783 — не перегонялись.
|
||||
|
||||
## Скоуп
|
||||
|
||||
Два workflow-тела (`_ship-review.yml` — публикация SHIP-REVIEW в `dev`, `_beta-derived.yml` — бот-коммит производных артефактов беты) и страж ребейза в `_process.yml` считали/не полностью разбирали отказ `git push`:
|
||||
|
||||
- **AC1.** `_ship-review.yml` и `_beta-derived.yml` разбирают stderr push через `merge-candidate.mjs --push-refusal=… --summary=…`: устаревший lease — прежний повтор/совет перезапустить; отказ GitHub — шаг останавливается без повторов, причина и ответ git без токена — в журнал и в сводку шага. Доказательство — исполнение настоящим bash/git во временных репозиториях.
|
||||
- **AC2.** Страж ребейза в `_process.yml` пишет причину отказа ещё и в сводку шага (`--summary`).
|
||||
- **AC3.** Без многострочного текста/heredoc в `run:`; тонкие `ship-review.yml`/`beta-derived.yml` не меняются, правка — только в телах `_*.yml`.
|
||||
|
||||
Дифф: `.github/workflows/_beta-derived.yml`, `.github/workflows/_process.yml`, `.github/workflows/_ship-review.yml`, `PROCESS.md`, `scripts/merge-candidate.mjs`, `test/publish-push-refusal.test.mjs`, `test/rebase-generated.test.mjs`. Продуктовый код (`src/**`) не тронут — изменение инфраструктурное, картам SCOPE.md/USER-GUIDE соответствует как часть «процесс сам себя не ломает» (трек show, #696), видимого пользователю поведения карточки нет.
|
||||
|
||||
## Как проверялось
|
||||
|
||||
1. Прочитан дифф целиком (453 строки), `PROCESS.md` §11 (кусок про слияние/страж ребейза), issue #730 и оба комментария автора.
|
||||
2. Прочитан `scripts/merge-candidate.mjs` — `describePushRefusal`, `PUBLISHED`, `refusalSummary`, `pushRefusalMain` (CLI `--push-refusal/--summary/--comment`).
|
||||
3. Прочитаны изменённые куски `_ship-review.yml`, `_beta-derived.yml`, `_process.yml` построчно, сверены с текстом тестов (что именно шаг пишет в `stdout`/`stderr`/`summary`/`dev`).
|
||||
4. Выполнено **дисциплину «тест должен уметь падать»**: временно откатил `.github/workflows/_beta-derived.yml`, `_ship-review.yml`, `_process.yml` и `scripts/merge-candidate.mjs` к `origin/dev` и прогнал новые тесты:
|
||||
- `test/publish-push-refusal.test.mjs` → 11 из 23 падают на старом коде (новые кейсы AC1/AC3 для ship-review и beta-derived).
|
||||
- `test/rebase-generated.test.mjs` → 2 из 19 падают на старом `_process.yml` (новые ассерты по сводке, AC2).
|
||||
Рабочая копия восстановлена на материал (`git checkout HEAD -- …`), `git status --short` и `git diff --stat` после восстановления пусты — рабочее дерево снова точно на материале.
|
||||
5. На восстановленном материале прогнан полный `npm test` (3348 тестов, 3347 pass, 1 skip, 0 fail) и отдельно `test/publish-push-refusal.test.mjs` + `test/rebase-generated.test.mjs` (42/42 pass) — для перепроверки после отката/восстановления файлов.
|
||||
6. `node scripts/smoke-select.mjs --base origin/dev --head HEAD` → «Исполняемого frontend-диффа нет», смоки не выбираются — браузерных смоков по диффу нет.
|
||||
7. Сверены трейлеры коммита: `Issue: #730` ✓, `User-Visible: no` ✓ (изменение не меняет поведение карточки, CHANGELOG/CHANGELOG.ru не тронуты — согласовано).
|
||||
8. Сверено AC3: `git diff origin/dev...HEAD --stat -- .github/workflows/ship-review.yml .github/workflows/beta-derived.yml` — пусто, тонкие файлы не тронуты; `grep -n "<<-?['\"]?[A-Za-z_]"` по новым телам — heredoc не найден (использован только для `test/publish-push-refusal.test.mjs`, который и проверяет его отсутствие в телах).
|
||||
9. Проверено, что `scripts/merge-candidate.mjs` в момент вызова берётся из `dev` (а не из тонкого вызывающего репозитория): `actions/checkout` с `ref: dev` в обоих job, плюс `git reset -q --hard origin/dev` перед каждой попыткой публикации ship-review — до разбора отказа.
|
||||
10. Признаков правки продуктового кода, PDF/geometry, `custom_components/**/*.py`, меток `ci:golden` нет — golden/pytest/invariants/perf вне применимости (см. «Чего не проверял»).
|
||||
|
||||
## AC · чем доказан · чем краснеет
|
||||
|
||||
| AC | Доказательство | Чем краснеет (проверено исполнением) |
|
||||
|---|---|---|
|
||||
| AC1 (разбор push, ship-review/beta-derived) | `test/publish-push-refusal.test.mjs`, 12 новых тестов (успех/lease-повтор/отказ GitHub×3 для каждого из двух тел + AC3-тест) | 11/23 тестов файла красные на `origin/dev`-версии `_ship-review.yml`/`_beta-derived.yml`/`merge-candidate.mjs` (проверено откатом и прогоном) |
|
||||
| AC2 (сводка стража ребейза) | `test/rebase-generated.test.mjs`, доп. ассерты на `r.summary`/`hook.summary` в двух существующих тестах | 2/19 тестов файла красные на `origin/dev`-версии `_process.yml` (проверено откатом и прогоном) |
|
||||
| AC3 (нет heredoc, тонкие файлы не тронуты) | `test/publish-push-refusal.test.mjs` — тест «#730 AC3: …» (grep по телам, слайс `run:`, неразрезанность последней строки); дифф-stat на тонких файлах | проверено чтением + статическая проверка (`grep`/`git diff --stat`), не ad-hoc исполнением — тест-ассерт сам есть защита от регрессии |
|
||||
|
||||
## Находки
|
||||
|
||||
### Medium (в скоупе AC2) — текст сводки «повтор и ребейз не помогут» вводит в заблуждение именно в сценарии, который страж ребейза обрабатывает как «не тупик»
|
||||
|
||||
`scripts/merge-candidate.mjs:552` (`refusalSummary`) формирует для ЛЮБОГО не-`stale` исхода одну и ту же фразу:
|
||||
|
||||
> «Это не сдвиг `${ref}`: повтор и ребейз не помогут, шаг остановлен без повторов.»
|
||||
|
||||
Эта фраза теперь (с #730) пишется в сводку шага и при `--stage=rebase`, причём именно для исхода `workflow`. Но `workflow` — это **единственный** исход, где по тексту того же PROCESS.md (§11, строки рядом с правкой этой задачи) ребейз-таки помогает: «Отказ по праву на workflow … называется отдельно: ребейз и push делает автор, либо владелец выдаёт право». Страж ребейза это знает и обрабатывает отдельно: `case workflow) → ::warning, exit 0`, задача возвращается автору именно для того, чтобы тот сделал ребейз сам.
|
||||
|
||||
Воспроизведено (не только чтением — исполнением):
|
||||
|
||||
```
|
||||
$ node -e "...classifyPushRefusal(...).kind === 'workflow'; refusalSummary(refusal, {ref:'issue/9-fix', stage:'rebase'})"
|
||||
### git push в `issue/9-fix` отклонён: workflow (#723)
|
||||
|
||||
Ребейз ветки на dev не опубликован в `issue/9-fix`. GitHub отклонил push по
|
||||
праву на workflow: у токена конвейера нет права создавать и менять
|
||||
`.github/workflows/`. Это не сдвиг `issue/9-fix`: повтор и ребейз не помогут,
|
||||
шаг остановлен без повторов.
|
||||
...
|
||||
```
|
||||
|
||||
Этот ровно текст и строку (`$RUNNER_TEMP/summary.md`) намертво фиксирует новый тест `test/rebase-generated.test.mjs:427-429` (`assert.match(r.summary, /…повтор и ребейз не помогут…/)` — косвенно, через более узкий якорь, но сама фраза в файле присутствует и тестом не оспаривается).
|
||||
|
||||
Это ровно тот риск, который сам автор назвал в комментарии к задаче и оставил непочиненным: «Общий текст сводки «ребейз не поможет» для стража ребейза неточен: при отказе по праву на workflow автор может сделать ребейз сам.» — т.е. дефект самоопознан, но не исправлен, хотя находится в скоупе AC2 (это та самая сводка, которую AC2 просит писать). Для `review-doc`/`release-review` (#723) эта же фраза при исходе `workflow` менее значима — там отказ по праву на workflow для публикации документа ревью не является штатным, часто повторяющимся сценарием, а для `rebase` это именно штатная, документированная ветка.
|
||||
|
||||
**Сценарий провала**: кандидат меняет `.github/workflows/*`, страж ребейза получает `! [remote rejected] … refusing to allow a Personal Access Token to create or update workflow`. Задача уходит в `S6-in-progress`, автор открывает сводку шага и читает «повтор и ребейз не помогут» — хотя корректное и единственное действие, которое ждёт от него процесс, это именно ребейз (своими правами) и push. Текст сбивает с толку в момент, когда автору и так нужно понять, что делать.
|
||||
|
||||
**Почему Medium, не High**: автоматика (код решения, метка, warning в логе отдельно от сводки) верна и не меняется этой находкой — ломается только пояснительный текст сводки, не поведение конвейера. Задача не блокируется функционально, но вводящий в заблуждение текст — это ровно то поведение, которое AC2 добавляет, и исправление тривиально (отдельная фраза для `workflow`-исхода, стадия `rebase` уже отличает его в `case`).
|
||||
|
||||
Это находка в скоупе задачи (AC2 как раз про текст сводки) → жёлтый вердикт, возврат автору, отдельный issue не заводится.
|
||||
|
||||
## Что проверено и корректно
|
||||
|
||||
- AC1: `_ship-review.yml` и `_beta-derived.yml` действительно разбирают `push`-отказ через `merge-candidate.mjs --push-refusal`, stale ведёт себя как раньше (повтор/совет), остальные исходы останавливают шаг с причиной и ответом git без токена в журнале и сводке — проверено исполнением, тесты умеют падать.
|
||||
- `scripts/merge-candidate.mjs`: добавление ключей `ship-review`/`beta-derived`/`rebase` в `PUBLISHED` корректно, `refusalSummary` и `pushRefusalMain` без побочных изменений в существующей логике для `review-doc`/`release-review` (дифф — только добавление записей и условного вызова `appendFileSync`, уже бывшего в #723 для других стадий).
|
||||
- AC2: сводка стража ребейза пишется для исходов `workflow`/`remote-rejected`/`unknown`, не пишется для `stale` — соответствует требованию «у стража ребейза тоже, #730» и проверено исполнением (2 падающих теста на старом коде).
|
||||
- AC3: heredoc в `_ship-review.yml` убран (сообщение коммита строится построчно в файл через `{ echo …; } > "$msg"`), многострочного текста в `run:` нет; тонкие `ship-review.yml`/`beta-derived.yml` не изменены — проверено статически (`git diff --stat`, `grep`) и тестом файла.
|
||||
- Скрипт `merge-candidate.mjs`, вызываемый из тел `_ship-review.yml`/`_beta-derived.yml`, берётся из `dev` (оба job чекаутят `ref: dev`, ship-review дополнительно сбрасывается на `origin/dev` перед каждой попыткой до вызова разбора) — не из тонкого вызывающего репозитория, что было бы несогласованно с остальной архитектурой (#705/#723).
|
||||
- Токен не попадает ни в журнал, ни в сводку, ни в комментарий (`noisySecretsGone` проверяет во всех новых тестах, включая `summary`).
|
||||
- Трейлеры коммита: `Issue: #730` и `User-Visible: no` на месте; `User-Visible: no` оправдан — изменение не затрагивает видимое поведение карточки, changelog-файлы не тронуты.
|
||||
- PROCESS.md обновлён точно и без лишнего — перечисляет новые стадии рядом с #723 и явно называет, что страж ребейза тоже пишет сводку (#730).
|
||||
- Полный `npm test` на материале зелёный (3347/3348, 1 skip, 0 fail), после отката/восстановления файлов рабочая копия точно совпадает с материалом (`git status --short`, `git diff --stat` пусты).
|
||||
|
||||
## Чего не проверял
|
||||
|
||||
- `tsc --noEmit`, `npm run build` + сверка трёх копий бандла — не перегонял: уже подтверждены зелёным Validate на этом же SHA (`981579aa`, https://github.com/Matysh/houseplan-card/actions/runs/36790168783). `npm test` я тем не менее перегнал сам (попутно с дисциплиной «тест должен уметь падать»), совпадает с Validate.
|
||||
- `actionlint` — автор заявил «чистый», сам CLI в окружении ревью недоступен (не найден бинарник); полагаюсь на заявление автора плюс на то, что синтаксическая валидность YAML проверяется тем же Validate-прогоном, который уже зелёный на этом SHA.
|
||||
- Браузерные смоки — `node scripts/smoke-select.mjs --base origin/dev --head HEAD` вернул «исполняемого frontend-диффа нет», смоки не выбираются; `src/**` не тронут. Решение: не прогонять — скрипт прямо сказал «нечего выбирать», это не НЕОПРЕДЕЛЁННОСТЬ.
|
||||
- `npm run golden:verify` — метки `ci:golden` на issue нет, рендер не затронут, не прогонял.
|
||||
- `python -m pytest tests_backend -q` — `custom_components/**/*.py` не менялся, не прогонял.
|
||||
- `npm run invariants -- --config …` — геометрия и ссылки на неё не менялись, не прогонял.
|
||||
- Performance-профили — не названы в AC, не прогонял.
|
||||
- Ручная проверка живого запуска `_ship-review.yml`/`_beta-derived.yml`/`_process.yml` в GitHub Actions (реальный push, реальный GitHub-отказ) — не делал; разбор строится на исполнении тел настоящим bash/git во временных локальных репозиториях (как в `test/publish-push-refusal.test.mjs`/`test/rebase-generated.test.mjs`), это стандартный для этого набора тестов способ доказательства и ранее был достаточен для #705/#723.
|
||||
- Мутанты по диффу — не запрашивались (трек show, #696), не прогонял.
|
||||
|
||||
## Вердикт
|
||||
|
||||
Жёлтый. Единственная находка — Medium, в скоупе AC2, не блокирует автоматику, но вводит автора в заблуждение ровно в документированном сценарии «отказ по праву на workflow → ребейз делает автор сам». Правится в этой же ветке.
|
||||
|
||||
---
|
||||
|
||||
<!-- material-anchors: сгенерировано конвейером (#414) -->
|
||||
|
||||
## Материал раунда
|
||||
|
||||
- Ветка: `issue/730-derived-push-refusal`, коммит `981579aabbdb` — ребейз его осиротит, и это нормально: ниже якоря, которые ребейз не меняет.
|
||||
- Дерево материала: `a759895ac00651b0e73a58be29dd56ad87baf69b`
|
||||
```
|
||||
git log --all --format='%H %T' | grep a759895ac006
|
||||
```
|
||||
- Тело issue: `3d77fb56ed2e308d5df6d329563f8143ebf6d19f6a36b320a92fdb76ee31d8fd`
|
||||
- Вердикт конвейера: `yellow` · High 0
|
||||
Reference in New Issue
Block a user