Skip to content

feat(masters): add direct masters adimages add - #676

Merged
axisrow merged 1 commit into
mainfrom
pr/masters-adimages-add
Aug 3, 2026
Merged

feat(masters): add direct masters adimages add#676
axisrow merged 1 commit into
mainfrom
pr/masters-adimages-add

Conversation

@axisrow

@axisrow axisrow commented Aug 3, 2026

Copy link
Copy Markdown
Owner

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

masters update --image "N=path" (#672) умеет только ЗАМЕНИТЬ уже существующее изображение и отказывается работать, когда у кампании их нет. Добавить изображение сверх текущего набора или поставить первое на кампанию с нулём — невозможно.

Добавляет дописывающую половину мутирующей стороны:

direct masters adimages add 72349978 --image-file a.png --image-file b.png

Отказывается, если текущее количество плюс новые файлы превысят лимит Яндекса в 5. Работает от пустого набора — изображения на кампании МК опциональны, ровно как ad images у текстового объявления через API. Принимает --launch (та же семантика публикации черновика, что у masters update).

_apply_image_operations — новый bulk-примитив, на котором это построено: открыть модалку менеджера изображений ОДИН раз, применить все удаления и все загрузки, нажать «Сохранить» один раз. Он принимает и список удалений, и список загрузок, хотя add всегда передаёт только загрузки — путь удаления и делает его примитивом, а не хелпером формы add, и покрыт собственными тестами здесь же. Удаления адресуются по thumb URL, снятым ДО первого удаления, чтобы более позднее удаление в том же батче не сбилось из-за переиндексации панели по мере исчезновения ранних карточек. Загрузки опрашивают АБСОЛЮТНЫЙ ожидаемый размер панели, а не относительную проверку _set_image «вырос обратно до исходного размера», которая работает лишь для точного обмена 1-в-1. До единственного «Сохранить» ничего не коммитится, поэтому любой более ранний сбой оставляет сохранённый набор нетронутым. _set_image — то, что использует update --image — не тронут.

_save_and_verify_images оборачивает общий послесохраняющий хвост: label кнопки с учётом черновика, клик, проверка против абсолютного ожидаемого конечного состояния (через _verify_image_set_mismatches из предыдущего PR) и трансляция истёкшей посреди верификации сессии в ошибку «НЕ повторять» — загрузки не идемпотентны, поэтому авторетрай _with_session не должен их переигрывать.

Классифицировано DANGEROUS: песочницы для Мастера кампаний не существует, загружаются реальные файлы, а повторённый add допишет ещё раз.

Живая проверка 2026-08-03 на DRAFT-кампании 713234191, включая загрузку в заведомо пустой набор.

Refs #648

🤖 Generated with Claude Code

@axisrow

axisrow commented Aug 3, 2026

Copy link
Copy Markdown
Owner Author

@claude @codex review PR #676 at exact head f4d4736 (round 99949e75-f57b-4cd9-a7e1-3e556ffb532c). 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 Bot commented Aug 3, 2026

Copy link
Copy Markdown

Claude finished @axisrow's task in 3m 35s —— View job


Claude review — direct masters adimages add

  • Read the PR diff vs origin/pr/masters-adimages-get
  • Review browser/masters.py bulk primitive & save/verify
  • Review commands/masters.py CLI + limit checks
  • Review tests & smoke classification
  • Post findings

No critical issues found. The mutating half is built carefully and the risky parts (idempotency, session expiry, the absolute-count wait, the single-commit invariant) are each handled deliberately and backed by tests. Details below.

What holds up under scrutiny

  • Single-commit / abandon-safe invariant. _apply_image_operations (browser/masters.py:2667) opens the modal once, does all removals + uploads, and clicks Save exactly once. Any earlier failure raises before that Save, so the saved set is never left half-mutated — same property _set_image relies on, and it's exercised by TestApplyImageOperations.
  • Cap enforcement is on the real current count. The CLI pre-flight (commands/masters.py) rejects >5 --image-file flags before a session opens, and add_master_images (masters.py:1902) re-checks len(before_ids) + len(paths) > _IMAGES_MAX_COUNT against the live set before opening the modal — so appending onto a partially-full campaign can't silently blow past 5. Covered by test_exceeding_the_cap_raises_and_never_opens_the_modal.
  • Non-idempotent retry hazard is correctly defused. _save_and_verify_images (masters.py:2921) catches a BrowserAuthError raised during post-save verification and re-raises it as a plain BrowserSessionError, which _with_session does not auto-retry. This is the right call — uploads aren't idempotent, and letting tier-3 replay the whole op would double-append. Verified against the retry semantics in _with_session (masters.py:237) and tested by test_auth_error_during_verification_is_re_raised_non_idempotent.
  • Absolute-size upload wait is the right fix. Polling len(panel) >= base_count + i instead of _set_image's relative "grew back to original size" check is correct for the add case (0 removed, N added), where the relative check would never trigger. The final re-navigate verification (_verify_image_set_mismatches) is an independent safety net keyed on the absolute expected size, so even a premature per-upload poll can't produce a silent wrong-count success — it would surface as an explicit mismatch error.
  • Closure capture is safe. Both poll lambdas bind their loop variable as a default (lambda target_url=target_url: / lambda expected_count=expected_count:), avoiding the classic late-binding bug.
  • Removals snapshot thumb URLs up front (url_by_content_id built before the first removal), so panel re-indexing during a batch delete can't misaddress a later removal — though add only ever passes uploads, this is tested on the primitive directly.
  • Housekeeping is complete and correct: DANGEROUS classification in smoke_matrix.py (masters.adimages.add), the manual-only checklist entry, and the DRY_RUN_EXCEPTIONS allow-listing in test_cli_contract.py — all consistent with masters update's existing browser-driven precedent (no --dry-run, no sandbox).

Minor observations (non-blocking, not defects)

  • Duplicate-file upload (--image-file a.png --image-file a.png): if Yandex silently de-dupes identical uploads, the final count would come up one short and _verify_image_set_mismatches raises a clear mismatch rather than lying — i.e. it fails safe. Worth a note in docs eventually, but behavior is correct as-is.
  • Multi-path set_input_files is intentionally avoided (one path per call) per the docstring, since PR feat(masters): add masters update --image — Этап D point-replacement (#670) #672's recon never exercised the list form. Reasonable conservatism; no action needed.

Everything traces back to live-verified behavior (DRAFT campaign 713234191, including the empty-set case) and the test coverage mirrors the claims. LGTM from a correctness/safety standpoint.
· pr/masters-adimages-add

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. Swish!

Reviewed commit: f4d47364f2

ℹ️ 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".

@axisrow
axisrow force-pushed the pr/masters-adimages-get branch from b17713e to 9d4bf59 Compare August 3, 2026 02:58
@axisrow
axisrow changed the base branch from pr/masters-adimages-get to main August 3, 2026 02:58
@axisrow
axisrow force-pushed the pr/masters-adimages-add branch from f4d4736 to fd0ac00 Compare August 3, 2026 02:58
`masters update --image "N=path"` (#672) can only REPLACE an image that
already exists, and refuses outright when a campaign has none. There is no
way to add an image beyond the current set, or to put the first one on a
campaign that has zero.

Adds the append half of the mutating side:

  direct masters adimages add 72349978 --image-file a.png --image-file b.png

Refuses if the current count plus the new files would exceed Yandex's cap
of 5. Works from an empty set — images are optional on a Мастер кампаний
campaign, exactly like ad images on a text ad via the API. Accepts
`--launch` (same draft-publishing semantics as `masters update`).

`_apply_image_operations` is the new bulk primitive this is built on: open
the image manager modal ONCE, apply every removal and every upload, click
Save once. It takes both a removal list and an upload list even though
`add` only ever passes uploads — the removal path is what makes it a
primitive rather than an `add`-shaped helper, and it is exercised by its
own tests here. Removals are located by thumb URLs captured before any
removal, so a later removal in the same batch is not thrown off by the
panel re-indexing as earlier cards disappear. Uploads poll an ABSOLUTE
expected panel size rather than `_set_image`'s relative "grew back to the
original size" check, which only happens to work for an exact 1-for-1
swap. Nothing commits before the single Save, so any earlier failure
leaves the saved set untouched. `_set_image` — what `update --image` uses
— is untouched.

`_save_and_verify_images` wraps the shared post-save tail: draft-aware
button label, click, verify against an absolute expected end state (via
`_verify_image_set_mismatches` from the preceding commit), and translate a
mid-verification session expiry into a "do NOT retry" error — uploads are
not idempotent, so `_with_session`'s auto-retry must not re-run them.

Classified DANGEROUS: no sandbox equivalent exists for Мастер кампаний,
real files are uploaded, and a retried `add` appends again.

Confirmed live 2026-08-03 on DRAFT campaign 713234191, including uploading
into a genuinely empty set.

Refs #648

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@axisrow
axisrow force-pushed the pr/masters-adimages-add branch from fd0ac00 to 4e9c8d4 Compare August 3, 2026 03:04
@axisrow
axisrow merged commit 419dc78 into main Aug 3, 2026
6 checks passed
@axisrow
axisrow deleted the pr/masters-adimages-add branch August 3, 2026 03:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant