feat(masters): add direct masters adimages delete/set

[axisrow]

Стек: базируется на #676 (pr/masters-adimages-add). Ретаргетить на main после мержа стека.

Завершает подгруппу masters adimages: с уже имеющимися get (чтение) и add (дозапись) эти две команды закрывают удаление и замену всего набора, так что изображениями кампании можно управлять от начала до конца, а не только точечно заменять через masters update --image.

direct masters adimages delete 72349978 --position 2
direct masters adimages delete 72349978 --all
direct masters adimages set 72349978 --image-file a.png

delete адресует изображения по --position (1-based, как показывает adimages get), по --content-id или через --all. --all на уже пустом наборе — идемпотентный no-op: модалка не открывается и ничего не сохраняется, — тогда как указание несуществующей позиции или content ID всегда ошибка. --all нельзя комбинировать с --position/--content-id: комбинация неоднозначна и рискует молчаливой потерей данных, поэтому это UsageError, а не тихое «проигнорируем более узкие».

set заменяет весь набор внутри ОДНОГО вызова _apply_image_operations (удалить всё, затем загрузить всё), так что замена 5→5 никогда не превышает лимит Яндекса транзиентно, как это сделала бы двухфазная связка delete-then-add. Без единого --image-file команда опустошила бы набор, поэтому она требует явного --allow-empty — защита от случайно пустого шелл-глоба, а не ограничение: пустое конечное состояние остаётся достижимым, просто осознанно, а delete --all говорит то же самое прямее.

Кампания с нулём изображений — валидное конечное состояние, и именно ради этого существует абсолютная проверка конечного состояния в _verify_image_set_mismatches: delete --all действительно утверждает, что сохранённый набор теперь пуст, а не выводит это из дельты счётчиков, как приходилось старому верификатору «удалено == добавлено».

Живая проверка 2026-08-03 на DRAFT-кампании 713234191: delete --position, delete --all, set (полная замена) и set --allow-empty — все прошли round-trip через свежую перезагрузку. Это закрывает единственный ранее непроверенный риск, отмеченный при написании примитива: контрол «Сохранить» у Яндекса остаётся кликабельным, когда выбор в модалке сведён к нулю.

Refs #648

🤖 Generated with Claude Code

[axisrow]

@claude @codex review PR #677 at exact head 6046684 (round 75587fb9-94e5-442a-9404-3a6deef10f09). Focus on critical issues: bugs, security vulnerabilities, logical errors, data loss risks, performance problems. Do NOT nitpick style, naming conventions, minor formatting, or subjective preferences — only flag issues that could break functionality or cause real harm in production.

[claude]

Claude finished @axisrow's task in 2m 54s —— View job

---

Review: masters adimages delete/set — head 6046684

Reviewed the delete/set primitives, CLI validation, the generalized verifier, and the test coverage against base pr/masters-adimages-add. No critical bugs, security issues, or data-loss risks found. The design is careful and the risky paths are well guarded.

What holds up under scrutiny

• set never transiently exceeds the cap. Removals and uploads happen inside one _apply_image_operations call (browser/masters.py:2010), and the cap check is len(paths) > _IMAGES_MAX_COUNT — since every current image is removed first (base_count = len(panel_urls) - len(remove_content_ids), masters.py:2898), a 5→5 replacement never peaks above 5. Correct.
• Abandon-safe. Nothing commits until the single Save click at the end of _apply_image_operations. A failed upload/removal raises BrowserSessionError before Save, leaving the saved set untouched. So a partial set failure cannot lose the campaign's existing images.
• Absolute end-state verification. _verify_image_set_mismatches (masters.py:2995) asserts the exact final size (len(expected_kept_ids) + expected_added_count) and does not early-return on empty removed_ids, so delete --all genuinely proves the set is empty rather than inferring it from a delta. This is the right generalization of the old removed==added verifier.
• Non-idempotent retry guard preserved. _save_and_verify_images translates a mid-verify session expiry into a do-NOT-retry BrowserSessionError (masters.py:3088), so _with_session's auto-retry can't re-apply a delete/set.
• Validation is layered correctly. --all + --position/--content-id → UsageError (not a silent narrowing); duplicate --position rejected; out-of-range position caught both at the CLI (1..MAX) and per-campaign in delete_master_images (position >= len(before_ids)). set with no files requires explicit --allow-empty. Target dedup (if cid not in targets) means a position and content-id pointing at the same image won't double-remove.
• Correctly classified as DANGEROUS in smoke_matrix.py, added to test_dangerous_commands.sh and DRY_RUN_EXCEPTIONS, and never auto-run.
• Tests cover the meaningful branches (zero-based position translation, --all no-op on empty set, out-of-range/unknown-id errors, full replacement in one modal, empty-set via set, cap overflow).

Non-blocking observations (not bugs, no action required)

1. Stale docstring vs. PR claim. _apply_image_operations (browser/masters.py:2818-2822) still says the zero-selection-Save case is "Not live-verified (flag for live smoke before relying on this in production)", but the PR description states this exact case (delete --all, set full replace) was live-verified on 2026-08-03 against campaign 713234191. Worth updating the docstring so future maintainers don't re-treat a verified path as unverified. Fix this →
2. Asymmetric duplicate handling. Duplicate --position values raise UsageError, but duplicate --content-id values are silently deduped in delete_master_images. Harmless (dedup prevents any double-remove), just slightly inconsistent UX — the reported Deleted count reflects unique targets, which is the correct/safe behavior.

Note: I could not execute the test suite in this environment (dependency install is sandbox-blocked), so the above is a static review. The added tests read as correct and comprehensive.

Overall: LGTM. Nothing here would break functionality or cause harm in production.
• branch pr/masters-adimages-delete-set-pr

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 6046684aa7

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[chatgpt-codex-connector]

Add dry-run protection to destructive image commands

When an operator supplies the wrong campaign ID or accidentally uses --all, these browser-driven commands can immediately remove the entire production image set without a preview checkpoint. Adding masters.adimages.delete and masters.adimages.set to DRY_RUN_EXCEPTIONS bypasses the repository's required safety contract; implement --dry-run so it resolves the current set and reports the planned removals/uploads without saving instead of exempting these destructive commands.

AGENTS.md reference: AGENTS.md:L493-L496

Useful? React with 👍 / 👎.

[chatgpt-codex-connector]

Honor --launch when the image set is already empty

When --all --launch targets an empty DRAFT campaign, this early return skips _save_and_verify_images, so the publish button is never clicked even though the command reports success; the equivalent shortcut in set_master_images at lines 2031–2032 causes the same behavior for set --allow-empty --launch. Only treat the empty operation as a no-op when launch is false, otherwise continue through the draft-launch path.

Useful? React with 👍 / 👎.

[axisrow]

Триаж замечаний Codex

1. P1 tests/test_cli_contract.py:54 — «добавить --dry-run вместо освобождения» → HALLUCINATION

Претензия «добавление в DRY_RUN_EXCEPTIONS обходит обязательный контракт безопасности» не подтверждается:

• Прецедент существует и задокументирован ДО этих PR-ов. На main в списке уже шесть browser-driven команд masters, включая деструктивную masters.archive и мутирующую masters.update. Комментарий, их оправдывающий, написан в коммите 046d4ad (PR feat(masters): add direct masters update — Этап A (issue #631) #646): «Мастер кампаний has no API surface at all — these click a button in a real browser session against production, there is no request payload to preview.» Новые записи следуют прецеденту, а не ломают его.
• Механизм технически неприменим. В API-командах --dry-run — чистый seam на локально собранном payload'е, ноль I/O до печати (campaigns.py:330). У masters adimages payload'а не существует: вся мутация — Playwright-сессия с расшифрованными Chrome-куками. Предложенное «резолвить текущий набор и печатать план» требует открыть браузер и загрузить живую production-страницу, что прямо противоречит семантике флага в этом репозитории (--dry-run = «Show request without sending»).
• Безопасность не «освобождена», а перенесена в другой слой: все три команды классифицированы DANGEROUS (smoke_matrix.py:210-212) — исключены из автотестов, попадают в ручной чек-лист; плюс локальные UsageError-гварды до открытия браузера (взаимоисключение --all с --position/--content-id, дубли позиций, диапазон, set без файлов требует --allow-empty).
• AGENTS.md L493-496 — раздел «Important Notes» с рекомендацией, а не hard-контракт; hard-контракт — сам тест test_mutating_commands_have_dry_run_or_explicit_exception, у которого by design есть ветка «or explicit exception».

Кода не меняю.

2. P2 browser/masters.py:1961 — --launch на пустом наборе → подтверждена, но вынесена за скоуп

Замечание фактически верное, проверено на коде: при delete --all --launch (и set --allow-empty --launch) на пустом DRAFT-е ранний возврат минует _save_and_verify_images, где launch=True кликает «Запустить кампанию» — команда рапортует успех, а черновик остаётся черновиком, вопреки хелпу флага («If CAMPAIGN_ID is currently a DRAFT, publish it while saving… Has no effect on a non-DRAFT campaign» — единственная оговорка про не-DRAFT, не про пустой набор).

Правка сознательно вынесена за рамки этого PR и будет сделана отдельно: это касается семантики --launch во всех командах группы, а не только delete/set, и заслуживает собственного ревью.

---

Замечание Claude про докстринг (не блокирующее)

Верное: _apply_image_operations всё ещё помечает случай нулевого выбора как «Not live-verified», хотя тело коммита утверждает обратное (живая проверка 2026-08-03 на кампании 713234191). Расхождение внутри одного коммита — исправлю вместе с правкой --launch, чтобы не двигать head этого PR ради комментария.