feat(masters): add masters update --image — Этап D point-replacement (#670) - #672
Merged
Conversation
#670) Replaces the image at a given position of a Мастер кампаний campaign's image set with a local file, via the edit page's image manager modal. Unlike headlines/texts, Yandex has no "replace this slot" primitive for images at all — only "remove from the set" and "add to the set", both inside `ImageSuggestionsEditorModal`. `_set_image` composes the two into one synthetic point-replacement. Confirmed live 2026-08-02 (campaign 713234191, Save never clicked): a newly uploaded image is always appended to the END of the set, never inserted at the freed position — so replacing position 2 of [A, B, C, D] yields [A, C, D, NEW]. The set's order carries no product meaning, so this is cosmetic, but the CLI help says so rather than implying a true positional swap. Images are also structurally different from headlines/texts: there is no fixed slot count in the DOM, an empty set is a legitimate state, and the edit page is an SPA whose images section renders after `domcontentloaded`. Reading too early yields an empty list that is indistinguishable from "this campaign genuinely has no images" — live-confirmed to produce a false "no images" failure on campaigns that demonstrably had four. Hence `_wait_for_images_editor`, which reports a timeout as a hard error rather than silently treating it as "no images". Verification is set-membership rather than positional, matching the append-to-end behaviour above. Also folds in the shared scaffolding the feature earned: - `_poll_until` replaces five hand-rolled deadline/wait_for_timeout loops and their sentinel flags, and makes PlaywrightError suppression uniform across them. - `_read_testid_suffixes` collapses the two prefix-scraping readers (`_read_image_content_ids`, `_read_modal_selected_thumb_urls`). - `_IMAGE_UPLOAD_SUFFIXES` lives next to the file input it describes, so the CLI's fail-fast check can't drift from what the page accepts. - `_validate_image_paths` lifts the extension/existence check out of the Click callback body. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RtBiHRofaYua9pmistY6oz
…LOG (#670) Issue #670 requires the append-to-end reordering limitation to be stated in the flag's help text, the README AND the CHANGELOG ("Это обязано быть прямо сказано в help-тексте, README и CHANGELOG, иначе пользователь ждёт позиционной замены и не получит её"). The help text said so; the other two were missing entirely — `--image` was absent from both files. README (both the English and Russian mirror sections): adds the flag to the `masters update` example block and to the supported-flags paragraph, plus a dedicated paragraph on the reordering limitation, the read-from-page upper bound for N (no fixed slot count, unlike headlines/texts), the legitimate empty-set state and its own explicit error, pre-browser path/extension rejection, and the fact that a failure before Save leaves the saved set untouched. Also drops "изображения" from the not-implemented-yet list, which is now stale. CHANGELOG: an Unreleased entry mirroring the Этап B one's shape, recording the same limitation with the live-confirmation reference, plus the shared scaffolding (`_poll_until`, `_read_testid_suffixes`, `_validate_image_paths`) the feature folded in. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RtBiHRofaYua9pmistY6oz
Owner
Author
|
@codex review PR #672 at exact head eae33d8 (round 50c40ab1-d119-43f4-859c-967b3c6c0cab). Focus on critical issues: bugs, security vulnerabilities, logical errors, data loss risks, performance problems — especially the --image point-replacement logic in direct_cli/browser/masters.py (update_master's images loop, _set_image, _verify_image_mismatches) when multiple --image flags are given in one call. Do NOT nitpick style, naming conventions, minor formatting, or subjective preferences — only flag issues that could break functionality or cause real harm in production. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: eae33d8b57
ℹ️ 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".
Comment on lines
+1714
to
+1717
| for index, path in (images or {}).items(): | ||
| if index < len(images_before_ids): | ||
| images_replaced_ids.add(images_before_ids[index]) | ||
| _set_image(page, index, path) |
There was a problem hiding this comment.
When multiple --image flags target positions in ascending order, each _set_image call removes an image and appends its replacement, so subsequent indices refer to the already-mutated order rather than the positions supplied by the user. For example, replacing positions 2 and 3 in [A, B, C, D] first produces [A, C, D, NEW1], then replaces D instead of the originally requested C. _verify_image_mismatches detects the discrepancy only after _click_save has committed the form, leaving the campaign with the wrong image removed and merely returning an error afterward; use the snapshotted content IDs to locate each original target (or process positions in a safe order) before saving.
Useful? React with 👍 / 👎.
Owner
Author
🔍 Local review (cycle 1) — round 50c40ab1-d119-43f4-859c-967b3c6c0cabReviewed via
|
…sition (#670) Cycle-review round 1 of PR #672 — both /review and a Codex cloud fallback ping (local companion timed out twice) independently found the same bug: replacing more than one image position in a single `masters update --image` call resolved each `_set_image` against the LIVE, already-mutated set instead of the original snapshot. Since a replacement always appends to the end of the set (confirmed live, documented in `_set_image`'s docstring), the second and later `--image` flags in one call silently removed a DIFFERENT image than the one the caller named — reproduced here on `[a, b, c, d]`: `--image "1=x" --image "2=y"` removed `a` and `c`, leaving `b` (which the caller asked to replace) in place and destroying `c` (which the caller never mentioned). `_verify_image_mismatches` only reported the mismatch after the removal was already saved to the live campaign, with no `--sandbox` available for this command to make the damage recoverable. `_set_image` now takes the target's content ID (resolved once, by `update_master`, against the pre-batch snapshot `images_before_ids`) and locates that image inside the modal directly, rather than re-deriving a position against whatever the live set looks like when that particular call runs. `index` is kept only for user-facing error text. A content ID that has already been consumed by an earlier replacement in the same batch now raises explicitly instead of silently drifting. New regression test: `test_update_master_replaces_multiple_images_by_original_position` (confirmed red before the fix, for the documented reason). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RtBiHRofaYua9pmistY6oz
Owner
Author
Cycle 1 reply — SKIP: PR/commit description overclaimNoted: the original PR body and commit message describe |
#670) Cycle-review round 2 of PR #672 — Codex adversarial review flagged that the content-ID fix (round 1) closes intra-call index drift but leaves a separate, more severe gap: `_verify_saved`'s post-click reload calls `assert_authenticated`, which can raise `BrowserAuthError` for a session invalidated in the window between `_click_save` and the reload. That exception was left to propagate uncaught out of `update_master`, straight into `_with_session`'s existing "retry the whole operation once on BrowserAuthError" self-heal path (direct_cli/commands/masters.py). For headlines/texts the retry is merely wasteful (`_set_repeating_value` writes fixed slots idempotently). For images it is destructive: replacement always appends to the end of the set (no fixed slots — see `_set_image`'s docstring), so a retry re-snapshots the ALREADY-mutated set and reapplies the same `--image "N=..."` positions against it, removing further images the caller never named — on top of the ones already correctly replaced before the auth error. `copy_master` already has the identical guard around its own post-click verification (issue #659) for the same reason (a retry there would create a second campaign). `update_master` never got it. Wraps `_verify_saved` in `try/except BrowserAuthError: raise BrowserSessionError(...) from exc`, mirroring `copy_master`'s pattern exactly, so `_with_session`'s except-BrowserAuthError retry no longer matches once the terminal save has already committed. New regression test: `test_auth_error_during_post_save_image_verification_is_not_retried`, mirroring `TestCopyMaster`'s existing `test_auth_error_during_post_click_verification_is_not_retried` (confirmed red before the fix — BrowserAuthError propagated as-is). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RtBiHRofaYua9pmistY6oz
Owner
Author
🔍 Local review (cycle 2) — round cycle2-e27fcc59-74bc-4341-a8a6-7dfef8fb6a4fReviewed via
|
Owner
Author
🔍 Local review (cycle 3, final) — cleanBoth This is the final review cycle. Proceeding to CI watch. |
Owner
Author
📋 Review summary — all cycles
Totals: 2 FIX (both resolved), 1 SKIP, 0 UNVERIFIED. Reviewed in local mode ( |
|
To use Codex here, create an environment for this repo. |
This was referenced Aug 3, 2026
axisrow
added a commit
that referenced
this pull request
Aug 3, 2026
Мастер кампаний has no Yandex Direct API surface, so `masters update --image` (#672) is currently the only way to touch a campaign's images — and it can only REPLACE one existing image, refusing outright on an empty set. There is no way to even see what images a campaign has. Adds the read half of a browser-driven counterpart to the API-side `direct adimages` group, deliberately reusing its vocabulary: direct masters adimages get 72349978 Reports `Position` (1-based), `ContentId` and `ThumbUrl` per image, plus `Count` and `MaxCount`. An EMPTY set is a successful result (`Count: 0`), not an error: images are optional on a Мастер кампаний campaign, exactly like ad images on a text ad via the API — there is no "at least one" invariant of the kind headlines and texts have. Read-only. The thumbnail URL is not exposed on the edit page itself, so this opens the image manager modal to read it and abandons the modal without ever clicking Save — nothing commits (the same abandon-safe invariant `_set_image` already relies on). A campaign with no images skips the modal entirely. Also generalises the verifier: `_verify_image_mismatches` now delegates to a new `_verify_image_set_mismatches`, which asserts an ABSOLUTE expected end state instead of assuming "removed count == added count". A point replacement is that same check with the two counts pinned equal, so existing behaviour is unchanged — `TestVerifyImageMismatches` passes untouched. The generalisation is what lets a later "remove every image" caller assert the set is genuinely empty, which the old shape could not express. `masters adimages get` is the CLI's first three-level command; the walkers were made recursive in the preceding commit, so this only adds the SAFE matrix entry. Refs #648 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
axisrow
added a commit
that referenced
this pull request
Aug 3, 2026
`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
added a commit
that referenced
this pull request
Aug 3, 2026
Мастер кампаний has no Yandex Direct API surface, so `masters update --image` (#672) is currently the only way to touch a campaign's images — and it can only REPLACE one existing image, refusing outright on an empty set. There is no way to even see what images a campaign has. Adds the read half of a browser-driven counterpart to the API-side `direct adimages` group, deliberately reusing its vocabulary: direct masters adimages get 72349978 Reports `Position` (1-based), `ContentId` and `ThumbUrl` per image, plus `Count` and `MaxCount`. An EMPTY set is a successful result (`Count: 0`), not an error: images are optional on a Мастер кампаний campaign, exactly like ad images on a text ad via the API — there is no "at least one" invariant of the kind headlines and texts have. Read-only. The thumbnail URL is not exposed on the edit page itself, so this opens the image manager modal to read it and abandons the modal without ever clicking Save — nothing commits (the same abandon-safe invariant `_set_image` already relies on). A campaign with no images skips the modal entirely. Also generalises the verifier: `_verify_image_mismatches` now delegates to a new `_verify_image_set_mismatches`, which asserts an ABSOLUTE expected end state instead of assuming "removed count == added count". A point replacement is that same check with the two counts pinned equal, so existing behaviour is unchanged — `TestVerifyImageMismatches` passes untouched. The generalisation is what lets a later "remove every image" caller assert the set is genuinely empty, which the old shape could not express. `masters adimages get` is the CLI's first three-level command; the walkers were made recursive in the preceding commit, so this only adds the SAFE matrix entry. Refs #648 Co-authored-by: axisrow <axisrow@users.noreply.github.com> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
axisrow
added a commit
that referenced
this pull request
Aug 3, 2026
`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
added a commit
that referenced
this pull request
Aug 3, 2026
`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: axisrow <axisrow@users.noreply.github.com> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
This was referenced Aug 3, 2026
Closed
axisrow
added a commit
that referenced
this pull request
Aug 8, 2026
…813) Смерджено в ручную Re-recon (2026-08-08, DRAFT campaign 713234191) of everything `masters update --image`/`masters adimages get/add/delete/set` (#670/#672/#675-#677) depend on, per issue #648's Этап D follow-up instructions (same discipline video's #806->#811 correction required). - Every ImageSuggestionsEditor*/ImageSuggestionsEditorModal* testid constant matches the live DOM exactly — no corrections needed, unlike video's guessed-and-wrong testids. - _apply_image_operations's previously "not live-verified" risk — uploading multiple files via one set_input_files([path1, path2]) call — is now confirmed working live (panel grew 3 -> 5 after one such call). - No genuine new command was warranted: images already have a full variable-length add/remove surface via 'masters adimages add/delete/set' (#675-#677), a deliberate command-group split from 'masters update --image' point-replacement made when #670/#675-#677 were originally scoped — adding --add-image/--remove-image flags to 'update' on top of that would just create two competing APIs for the same operation. All live interactions were abandoned via the modal's Cancel button (never Save) — verified the campaign's saved image set is unchanged before and after. Part of #648. Claude-Session: https://claude.ai/code/session_01PcYzUsFvZbn6VLhLAFuYgv Co-authored-by: axisrow <axisrow@users.noreply.github.com> Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Replaces the image at a given position of a Мастер кампаний campaign's image set with a local file, via the edit page's image manager modal.
Closes #670
Why images aren't like headlines/texts
Yandex has no "replace this slot" primitive for images at all — only "remove from the set" and "add to the set", both inside
ImageSuggestionsEditorModal._set_imagecomposes the two into one synthetic point-replacement (open modal → remove card at position → upload → poll for the new card → Save).Confirmed live 2026-08-02 (campaign 713234191, real DOM mutation, Save never clicked, page abandoned — no campaign was mutated): a newly uploaded image is always appended to the END of the set, never inserted at the freed position. Replacing position 2 of
[A, B, C, D]yields[A, C, D, NEW], not[A, NEW, C, D]. The set's order carries no product meaning (Yandex rotates/biases images by performance regardless of position), so this is a cosmetic limitation — but the CLI help, README and CHANGELOG say so rather than implying a true positional swap.Images are structurally different in three more ways, all of which the code accounts for:
ContentImageelements as the campaign has images, keyed by a Yandex content ID, not an index._IMAGES_MAX_COUNTbounds CLI parsing only; the real ceiling is read fresh from the page.goto(..., wait_until="domcontentloaded")returns before the images section exists. Reading too early yields[], which is indistinguishable from "this campaign genuinely has no images" — live-confirmed to make--imagefail with a false "no images" on four consecutive campaigns that demonstrably had four images each. Hence_wait_for_images_editor, which reports a timeout as a hard error rather than silently treating it as "no images".Post-save verification is set-membership, not positional, matching the append-to-end behaviour above.
Because both the removal and the upload happen inside the same open modal, any failure before
Saveleaves the campaign's saved image set untouched — every error message says so explicitly.Shared scaffolding folded in
The feature earned four cleanups rather than adding its own copies:
_poll_untilreplaces five hand-rolleddeadline/wait_for_timeout(250)loops and the two sentinel flags (removed,uploaded) that only existed because the loops were inline. It also makesPlaywrightErrorsuppression uniform — previously only one of the five suppressed it._read_testid_suffixescollapses_read_image_content_idsand_read_modal_selected_thumb_urls, which were the same prefix-scraping body twice._IMAGE_UPLOAD_SUFFIXESlives in the browser layer next to the file input it describes (accept=image/png,image/jpeg,image/jpg,image/gif, confirmed live), imported by the CLI — same reasoning as_IMAGES_MAX_COUNT, so the fail-fast check can't drift from what the page accepts._validate_image_pathslifts the extension/existence check out of the Click callback body.Tests patch timeout constants with
patch.objectrather than assigning module globals with a hardcoded restore value, keeping the offline tier process-parallel-safe.Testing
pytest tests/test_masters.py— 278 passedpytest(full offline tier) — 2857 passed, 8 skippedblack/flake8cleanThe 62 errors in the live-write tier are pre-existing and identical on a pristine baseline (verified via
git stash) — they need credentials this run didn't have.Known limitation
Each image is replaced through its own open/remove/upload/Save modal cycle, so replacing N images costs N cycles. Batching them into a single modal session looks possible (
set_input_filesaccepts a list) but that claim is not live-verified, so it's deliberately left for a follow-up with proper recon rather than guessed at here.🤖 Generated with Claude Code
https://claude.ai/code/session_01RtBiHRofaYua9pmistY6oz