feat(masters): add --clear-headline/--clear-text to masters update (#786) - #787
Conversation
…786) Closes the Этап B follow-up left open by #665: `--headline`/`--text` refuse an empty replacement (indistinguishable from a delete to the post-save verification), and deleting a variant was deferred rather than folded into that flag. - New `_clear_repeating_value` (browser layer) clicks a slot's own `.clear` button (`CampaignTitles{N}.clear`/`CampaignTexts{N}.clear`), confirmed live and documented in tests/fixtures/masters_wizard_edit_stage_b.html's "BONUS FINDING". - New `--clear-headline`/`--clear-text` CLI flags (repeatable, 1-based slot number, mirrors --headline/--text numbering). - A slot passed to both a set flag and its clear counterpart in the same call is rejected as a UsageError before any browser session opens. Clearing an already-empty slot is refused too. - Verification reuses the existing _verify_repeating_value_mismatches re-read-and-compare check (a cleared slot is expected to read back as ""). Out of scope, confirmed by live recon rather than merely deferred: adding a brand-new variant (Yandex's edit page has a fixed slot count, no "add another" control) and per-variant weights (Мастер кампаний has no weight/priority UI at all for these slots, unlike ordinary text-campaign ad variants). Part of the #648 umbrella (Этап B done here; Этап D media uploads remains). Offline-only in this PR — live verification queued separately per the project's serialized browser-session access. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VGxHgqUmX2YeA1ignZSef1
…ble save _clear_repeating_value clicked its per-slot .clear button and returned immediately, with no confirmation the textarea actually emptied before the caller's _click_save fires. This module already learned (issue #681) that a button click on this same edit page is async and "click alone isn't proof" — _remove_audience_tag/_add_audience_tag poll BEFORE save and raise on a non-committing click, precisely to avoid persisting a half-committed state. The new clear path lacked that guard, so a click that hadn't committed could reach the irreversible save; the mismatch would only surface afterwards via the post-save verification re-read, by which point the save had already happened. Add the same post-click poll-until-empty guard, with the same _AUDIENCE_TAG_SUGGEST_TIMEOUT_MS budget, and raise BrowserSessionError before the caller ever gets to save if the slot never empties. Found by cycle-review (Codex adversarial review) on PR #787. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VGxHgqUmX2YeA1ignZSef1
🔍 Local review (cycle 1) — round 936a027b-ed1d-4497-9086-d87e38bf1078Reviewed locally: built-in
Resolution: fixed in 6cf8e5b — added a post-click poll-until-empty guard (same budget/pattern as the audience-tag add/remove loop, issue #681) so a non-committing clear click is caught before |
axisrow
left a comment
axisrow
left a comment
There was a problem hiding this comment.
Ревью PR #787
Вердикт: approve. Замечаний, требующих изменений, не нашёл.
Что проверено
- Diff
direct_cli/browser/masters.py/direct_cli/commands/masters.py/tests/test_masters.pyотносительноorigin/mainна head-коммите6cf8e5b. _clear_repeating_value: bounds-check, чтение текущего значения, guard на уже пустой слот, клик по.clear, и — что особенно важно — post-click commit-check с poll-циклом через_clock.now()/page.wait_for_timeout(не rawtime), то есть соблюдён контракт_raw_clock_calls/TestBrowserPackageClockдля poll-циклов вbrowser/. Второй коммит (6cf8e5b) добавляет именно этот commit-check как фикс по cycle-review-находке — обоснованно: клик по асинхронной кнопке без проверки, что состояние действительно применилось, до необратимого_click_save— реальный риск, ровно как в прецеденте с audience-tag close button (#681).- CLI-граница (
_parse_clear_slot_options,_reject_overlapping_slots) отклоняет: слот вне диапазона, дубликаты, пересечение--headline/--clear-headlineна одном слоте — до открытия браузерной сессии (подтверждено тестомtest_out_of_range_clear_slot_does_not_open_a_browser_session). update_master:verify_headlines/verify_textsкорректно мёржатheadlines/clear_headlinesв единую карту ожидаемых значений для_verify_repeating_value_mismatches— очищенный слот должен читаться как"", что уже покрывается существующей проверкой равенства.- Оverlap-проверка есть только на CLI-границе, не дублируется в browser-слое — задокументировано и последовательно с остальными парными опциями модуля (
target_action_prices/add_target_actions/remove_target_action_goal_ids). README.md/CHANGELOG.mdобновлены, out-of-scope (веса, добавление новых вариантов) явно задокументированы со ссылкой на живой recon.
Верификация окружения
git diff origin/main...6cf8e5b— совпадает с описанием PR.pytest -qна чистом PR head (без незакоммиченных изменений в чекауте): 3348 passed, 23 skipped, 39 subtests passed.black --check/flake8на изменённых файлах — чисто.
Живая верификация --clear-headline/--clear-text против реальной кампании отложена по описанию PR (обоснованно — сериализованная очередь на общую master-сессию), это не блокирует офлайн-мерж согласно предыдущей практике (#665 тоже смержен офлайн первым).
…ting_value The commit-check loop added in 6cf8e5b guarded its FIRST inner_text() read (the pre-click "is it already empty?" check) with try/except PlaywrightError -> BrowserSessionError, but the post-click reads inside the poll loop itself were left unguarded. A transient DOM detach during the poll (Yandex re-rendering the slot row after the .clear handler runs) would surface a raw PlaywrightError traceback instead of this function's documented "click may not have committed" error — in the exact failure mode the commit-check mechanism exists to make legible. Extract both post-click reads into a helper with the same PlaywrightError -> BrowserSessionError conversion as the initial read. Found by cycle-review (Codex adversarial review, round 2) on PR #787. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VGxHgqUmX2YeA1ignZSef1
🔍 Local review (cycle 2) — round 366fb45b-cbbc-413f-89c8-b7904ce96e58Reviewed locally: built-in
Resolution: fixed in d81b851 — extracted the post-click reads into a helper with the same |
🔍 Local review (cycle 3) — round 35c76f01-8abc-4917-aa08-16fa7c0dc80fReviewed locally: built-in Verdict: approve, no findings. Codex traced the clear-slot path end-to-end (false-success, partial-failure, and irreversibility scenarios) and found every failure boundary guarded: CLI validation, pre-click already-empty check, post-click commit poll with detach handling (fixed in cycle 2), and post-save re-verification of cleared slots. No new FIX this round — this closes the review cycle. |
📋 Review summary — all cycles
Totals: 2 FIX (both resolved), 0 SKIP, 0 UNVERIFIED, 0 HALLUCINATION. Note: the built-in |
…llback test_raises_browser_session_error_when_commit_check_read_detaches (d81b851) asserted assertNotIsInstance(ctx.exception, PlaywrightError). CI runs without the playwright package installed, where masters.py's own ImportError guard makes PlaywrightError an alias for bare Exception — so the assertion became "BrowserSessionError is not an Exception", which is always false, failing the test in exactly the environment this project's CI uses (confirmed: local run has playwright installed and passed, CI does not and failed with the same message reported here). assertRaises(BrowserSessionError) already proves the exception IS a BrowserSessionError; replace the redundant-and-environment-dependent check with an exact-type assertion, which needs no PlaywrightError reference and is stable regardless of whether playwright is installed. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VGxHgqUmX2YeA1ignZSef1
🔍 Local re-review (after CI-fix commit) — round 76f9e41b-cdf5-49c0-9a31-7e071d206064Triggered by a post-review CI-fix push (30866d3 — a test assertion relied on Reviewed locally: built-in Verdict: approve, no findings. Codex re-traced the full clear-slot path against the new head (including compaction/reordering scenarios, partial-failure/retry safety, and the CLI-vs-browser-layer overlap-validation boundary) and found no material issue — every failure path fails closed before the irreversible save or is caught by post-save verification. |
1 participant
Summary
Closes the Этап B follow-up left open by #665 (
masters update --headline/--text): those flags refuse an empty replacement (indistinguishable from a delete request to the post-save verification), and deleting a variant was tracked as a separate follow-up rather than folded into them. This PR adds that delete path.Part of the umbrella issue #648 — specifically the remaining Этап B scope. Per live recon already on file (
tests/fixtures/masters_wizard_edit_stage_b.html), Мастер кампаний has no per-variant weight/priority UI at all, so "списки с весами" from the original #631/#648 wording does not apply here — there is nothing for a weight flag to set. Adding brand-new variants is also out of scope: the edit page has a fixed slot count (5 headlines / 3 texts) with no "add another" control.Changes
direct_cli/browser/masters.py: new_clear_repeating_valueclicks a slot's own.clearbutton (CampaignTitles{N}.clear/CampaignTexts{N}.clear— confirmed live in the existing fixture's "BONUS FINDING", previously unused).update_mastergainedclear_headlines/clear_textskwargs, wired into the mutation loop and into_verify_saved(a cleared slot is expected to read back as"", reusing the existing_verify_repeating_value_mismatchescheck).direct_cli/commands/masters.py: new repeatable--clear-headline/--clear-textoptions (1-based slot number, same numbering as--headline/--text). New_parse_clear_slot_options(bounds/duplicate checks) and_reject_overlapping_slots(a slot can't be both set and cleared in the same call — rejected asUsageErrorbefore any browser session opens).tests/test_masters.py: offline unit coverage for_clear_repeating_value(browser layer, fake Page/Locator) and the new CLI flags/parsers/guards (~30 new tests).README.md/CHANGELOG.mdupdated.Verification
Offline: full suite green (
pytest, 3346+ passed).black/flake8clean.Live: not yet done in this PR — per project convention (
feedback_live_verification_before_tests) and the current serialized queue for the shared test master/browser session (other sessions #782/#783/#776/#781 active), live verification of--clear-headline/--clear-textagainst a real campaign is queued separately and will follow before this is considered fully verified, mirroring how #665 itself shipped its offline PR first and was live-verified afterward.Out of scope (confirmed, not merely deferred)
🤖 Generated with Claude Code