|
|
|
@@ -0,0 +1,90 @@
|
|
|
|
|
# CODE-REVIEW-368-r1
|
|
|
|
|
|
|
|
|
|
- Issue: [#368](https://github.com/Matysh/houseplan-card/issues/368)
|
|
|
|
|
- Трек: `trivial` (короткий трек, §5.1 PROCESS.md) — S2-analysis → S5-ready, AC зафиксированы в теле issue при переходе.
|
|
|
|
|
- Ветка: `issue/368-expected-rev-docs`
|
|
|
|
|
- SHA материала ревью: `774388c2e573af3043ead8995358c2930a44c00f` (единственный коммит диапазона `origin/dev..HEAD`, совпадает с SHA, названным автором в хендофф-комментарии)
|
|
|
|
|
- Заход: r1 · блокирующих циклов израсходовано 0/2 (лимит короткого трека)
|
|
|
|
|
|
|
|
|
|
## Скоуп
|
|
|
|
|
|
|
|
|
|
Диапазон `git diff origin/dev...HEAD` — 4 файла, только правки текста, поведение не меняется:
|
|
|
|
|
|
|
|
|
|
```
|
|
|
|
|
custom_components/houseplan/websocket_api.py | 10 ++++++----
|
|
|
|
|
docs/ARCHITECTURE.md | 7 ++++++-
|
|
|
|
|
docs/CHANGELOG.md | 10 ++++++++++
|
|
|
|
|
docs/CHANGELOG.ru.md | 10 ++++++++++
|
|
|
|
|
4 files changed, 32 insertions(+), 5 deletions(-)
|
|
|
|
|
```
|
|
|
|
|
|
|
|
|
|
Класс изменений: A (одна строка кода в `websocket_api.py` — текст f-string) + C (документация/changelog). Задача сама по себе — исправление задокументированности уже введённого в #340/#356 требования `expected_rev`, без изменения логики. AC из тела issue:
|
|
|
|
|
|
|
|
|
|
1. Оба changelog содержат breaking-абзац со ссылками на #340/#356.
|
|
|
|
|
2. Документация WS-команд (если файл существует) описывает требование и пример цикла для внешних клиентов.
|
|
|
|
|
3. `conflict`-ответ несёт actionable-подсказку (или зафиксировано, что уже несёт).
|
|
|
|
|
|
|
|
|
|
Продуктовая рамка (`docs/SCOPE.md`): задача не добавляет и не убирает пользовательскую функциональность — она делает уже принятое решение (#340/#356, безопасность записи через `expected_rev`) видимым для внешних интеграторов, что соответствует J6 («Keep the plan true as the home evolves» — optimistic locking) и не создаёт нового отдельного user job. Вне скоупа: сама механика CAS-проверки не меняется и не пересматривается.
|
|
|
|
|
|
|
|
|
|
## Как проверялось
|
|
|
|
|
|
|
|
|
|
Гейты прогнаны локально (зелёного Validate на этом SHA не найдено — прогон делаю сам, набор дешёвый):
|
|
|
|
|
|
|
|
|
|
| Гейт | Команда | Результат |
|
|
|
|
|
|---|---|---|
|
|
|
|
|
| Typecheck | `npx tsc --noEmit` (запускался также как часть `npm run build`) | OK, 0 ошибок |
|
|
|
|
|
| Unit-тесты (JS/TS) | `npm test` | `# tests 1538 / pass 1537 / fail 0 / skipped 1` — зелёный |
|
|
|
|
|
| Build + сверка бандла | `npm run build && cmp dist/houseplan-card.js custom_components/houseplan/frontend/houseplan-card.js` | build OK; `cmp` — байт-в-байт совпадение (frontend не менялся, поэтому бандл идентичен) |
|
|
|
|
|
| `demo/srv/assets/houseplan-card.js` | `cmp dist/... demo/srv/assets/...` | файла нет — ожидаемо, копия стенда не коммитится с #255 (класс D, `AGENTS.md` §Change classes) |
|
|
|
|
|
| `node scripts/check-docs.mjs` | не запускал | diff не трогает `src/**` (0 файлов), правило AGENTS.md/§8 PROCESS.md требует его только при правке фронтенда — не применимо |
|
|
|
|
|
| Browser-smokes | `node scripts/smoke-select.mjs --base origin/dev --head HEAD` | вывод: «Исполняемого frontend-диффа нет (src/**/*.ts не тронут). Browser-smoke этим диффом не выбираются... Тронуто файлов: 4.» → инструмент прямо сообщает «нечего выбирать»; сама карточка (собранный бандл) не менялась, смоки бы тестировали код, который не тронут — не прогонял |
|
|
|
|
|
| `npm run golden:verify` | не запускал | diff не меняет рендер/геометрию/стили — визуального результата нет |
|
|
|
|
|
| `python -m pytest tests_backend -q` | **не прогонял исполнением** | в этом окружении нет ни `pytest`, ни модуля `homeassistant` (`ModuleNotFoundError`, нет `.venv-backend`) —harness недоступен. Изменение в `websocket_api.py` разобрано **чтением, не исполнением** (см. ниже) вместо этого |
|
|
|
|
|
| `node scripts/model-invariants.mjs` | не запускал | diff не трогает геометрию, `layout`, `marker.space`, `open_spans`, рёбра/толщину стен — не применимо |
|
|
|
|
|
| Performance-профили | не запускал | не названы в AC, путь не является чувствительным к перфу (текст ошибки/документация) |
|
|
|
|
|
| `node scripts/process-gate.mjs` | запускал | `гейт пройден, предупреждений 1` — единственное предупреждение: нет `docs/specs/368-*.md`, что ожидаемо и допустимо для `small`/`trivial` (§5.1) |
|
|
|
|
|
| `python3 -m py_compile custom_components/houseplan/websocket_api.py` | запускал | `COMPILE OK` — синтаксическая валидность правки f-строк подтверждена |
|
|
|
|
|
|
|
|
|
|
### Разбор по AC (доказательство)
|
|
|
|
|
|
|
|
|
|
**AC1 — оба changelog содержат breaking-абзац со ссылками на #340/#356.**
|
|
|
|
|
Проверено чтением диффа: `docs/CHANGELOG.md` и `docs/CHANGELOG.ru.md` получили идентичный по смыслу абзац в `## Unreleased`, оба со ссылками на #340, #356 и #368, оба в одном коммите `774388c2` (трейлер `User-Visible: yes` соблюдён — правило требует ровно этого). Формулировка сообщает: `config/set`/`layout/set` над непустым store требуют `expected_rev`, откуда брать `rev`, и что сама карточка не затронута. **Доказано чтением.**
|
|
|
|
|
|
|
|
|
|
Отдельно проверил фактическую точность формулировки «карточка шлёт ревизию с v1.60»: `src/houseplan-card.ts:6949` подтверждает `expected_rev: this._cfgRev` в вызове `config/set`. Для `layout/set` в `src/**` вообще нет ни одного вызова этого типа сообщения — карточка с v1.10.0 (`git log -S"houseplan/layout/set"`, коммит `e7cf0416`, "точечный layout/update вместо layout/set (анти last-writer-wins)") сознательно перешла на поточечный `layout/update`, который в `expected_rev` не нуждается. Утверждение «карточка не затронута» в CHANGELOG остаётся верным (по обеим причинам сразу — и для `config/set`, и потому что `layout/set` картой не используется), но не по причине «шлёт ревизию с v1.60» в буквальном смысле для layout. Это не искажает наблюдаемое поведение и не вводит пользователя в заблуждение относительно сути изменения (собственно breaking для внешних писателей описан верно), поэтому квалифицирую как **Low**, не блокирует: формулировка обобщает две разные причины неуязвимости под одну фразу. Формально почвы для возврата нет — снимаю без правки, с записью здесь.
|
|
|
|
|
|
|
|
|
|
**AC2 — документация WS-команд описывает требование и пример.**
|
|
|
|
|
`docs/ARCHITECTURE.md:889-899` (существующий раздел «Integration WS API», уже нёсший абзац о #340) расширен: явно распространяет то же правило на `layout/set` (#356) и добавляет цикл для внешних клиентов — прочитать `rev` через `config/get`/`layout/get`, отправить его как `expected_rev`, на `conflict` перечитать и повторить. Таблица команд (строки 866-887) не менялась — и не должна была: столбцы `Parameters`/`Response` уже перечисляли `expected_rev?` для обеих команд. Проверено чтением текста и сверкой с уже существующей до диффа таблицей и её описанием у `ws_layout_set`/`ws_config_set` (докстринг `websocket_api.py:568-574` соответствует). **Доказано чтением.**
|
|
|
|
|
|
|
|
|
|
**AC3 — `conflict`-ответ несёт actionable-подсказку.**
|
|
|
|
|
`custom_components/houseplan/websocket_api.py:594-600` (layout) и `:1323-1332` (config, по номерам до диффа — после сдвига та же пара) — оба сообщения об ошибке дополнены фразой `"... or — for external clients — include expected_rev from houseplan/config|layout/get (current rev N)"`, сохранив исходное `"reload the ... (current rev N)"` для вкладок. Логика веток (`if "expected_rev" not in msg and current_rev:` и вторая проверка сравнения ревизий) не тронута — сравнение диффа построчно подтверждает: изменились только литералы f-строк, не условия. `python3 -m py_compile` подтверждает синтаксическую корректность. **Доказано чтением + компиляцией**, а также юнит-тестами (см. ниже) для инварианта «подстрока не потерялась».
|
|
|
|
|
|
|
|
|
|
**Автотест, который умеет падать:** `tests_backend/test_ha_websocket.py:516` и `:596` — `assert "revision is required" in rejected["error"]["message"].lower()`. Прочитал оба теста целиком (контекст вокруг присланных строк): они гоняют полный сценарий CAS-конфликта (stale-клиент шлёт `config/set`/`layout/set` без `expected_rev` поверх ненулевой ревизии) и проверяют код ошибки `conflict` плюс присутствие фиксированной подстроки. Тест **умеет падать** — упадёт, если подстрока `"revision is required"` исчезнет или код ошибки перестанет быть `conflict"`; ровно то, что могло случиться при неаккуратной правке текста. Не смог **исполнить** эти тесты в данном окружении (нет `pytest`, нет `homeassistant`, `.venv-backend` отсутствует — AGENTS.md подтверждает, что харнесс есть только в облачных агентах и на машине владельца/WSL). Разобрал изменение построчным сравнением диффа: правка меняет исключительно текст после уже проверяемой тестом подстроки, оставляя её начало (`f"Layout revision is required; reload the layout, or — for "`) и весь код условий нетронутыми. Это разбор **чтением, не исполнением**, как и требует §2.7 PROCESS.md, когда тест недоступен для прогона.
|
|
|
|
|
|
|
|
|
|
## Находки
|
|
|
|
|
|
|
|
|
|
Нет High. Нет Medium. Одна снятая Low (см. AC1 выше — не требует правки, оставлена как объяснённое решение ревьюера).
|
|
|
|
|
|
|
|
|
|
## Что проверено и корректно
|
|
|
|
|
|
|
|
|
|
- Единственная логическая ветка кода не изменилась — изменился только текст ошибки; условия `if "expected_rev" not in msg and current_rev` и сравнение `msg["expected_rev"] != current_rev` идентичны байт-в-байт до и после.
|
|
|
|
|
- Обе строки-подсказки консистентны между `layout/set` и `config/set` (симметричная формулировка, оба указывают правильную парную команду `get`).
|
|
|
|
|
- `docs/ARCHITECTURE.md` не дублирует и не противоречит уже существующему абзацу про #340 — расширяет его на #356 и добавляет практический цикл, не переписывая существующую таблицу команд.
|
|
|
|
|
- Оба changelog правлены в одном коммите с `User-Visible: yes`, трейлер `Issue: #368` на месте, ветка называется `issue/368-expected-rev-docs` — соответствует §10 PROCESS.md и `scripts/validate-commit-provenance.mjs`/`process-gate.mjs` (прогнан, гейт пройден).
|
|
|
|
|
- `process-gate.mjs` — 0 ошибок, 1 ожидаемое предупреждение (нет `docs/specs/368-*.md`, что верно для `trivial`).
|
|
|
|
|
- «Одно число — один источник»: в этом диффе новых пользовательских величин нет (только текст сообщений и changelog-проза, без числовых значений, кроме уже существующего `current_rev`, который берётся из той же переменной, что и раньше — источник не размножился).
|
|
|
|
|
- Diff не задевает геометрию, конфиг-миграцию, i18n, touch, perf — инварианты модели и браузерные смоки закономерно не требуются, что подтверждает и вывод `smoke-select.mjs`.
|
|
|
|
|
|
|
|
|
|
## Чего не проверял и почему
|
|
|
|
|
|
|
|
|
|
- **`python -m pytest tests_backend -q` не исполнялся** — окружение ревью не имеет ни `pytest`, ни `homeassistant`, ни `.venv-backend`. Компенсировано построчным чтением диффа и обоих тестов, пинящих подстроку сообщения (см. AC3). Рекомендация: если у исполнителя/владельца есть доступ к рабочему харнессу — было бы полезно фактически прогнать `test_ha_websocket.py::test_config_set_stale_conflict`-подобные кейсы разово, но для этой правки (только текст, логика нетронута) риск регрессии оцениваю как исчезающе малый, блокировать не буду.
|
|
|
|
|
- **Browser-smokes (полный список, 202 файла) не прогонялись** — diff не трогает `src/**`, `smoke-select.mjs` прямо сообщает «выбирать нечего»; прогон всего набора не соразмерен задаче (PROCESS.md §8, «полные наборы — предрелизный гейт»).
|
|
|
|
|
- **`golden:verify`** — не запускал, нет визуальных изменений.
|
|
|
|
|
- **`model-invariants.mjs`** — не запускал, геометрия не тронута.
|
|
|
|
|
- **Performance-профили** — не запускал, не названы в AC и путь не перфочувствителен.
|
|
|
|
|
- **`check-docs.mjs`** — не запускал, diff не трогает `src/**`.
|
|
|
|
|
|
|
|
|
|
## Вывод
|
|
|
|
|
|
|
|
|
|
Все три AC выполнены и доказаны (две — прямым чтением текста, третья — чтением + компиляцией + существующим автотестом, который умеет падать и логически не мог быть задет правкой). Единственное замечание — Low на неточную формулировку одной фразы в changelog про `layout/set` — не искажает смысла для пользователя и снято без правки. Задача решает заявленный сценарий (документационная прозрачность breaking-изменения #340/#356 для внешних писателей) и не деградирует ничего смежного.
|
|
|
|
|
|
|
|
|
|
**Вердикт: зелёный.**
|
claude[bot]