fix(masters): harden delete_master — TOCTOU race, transient verify errors, virtualized-grid scroll (#793, #791) - #794
Merged
Conversation
…verify errors Codex's adversarial review (cycle-review round 4) on PR #784 found two edge cases in delete_master (direct_cli/browser/masters.py), tracked as issue #793: - Finding 1 (TOCTOU): the DRAFT-only guard was checked once via _find_master_row, well before the irreversible DeleteCampaignAction click -- on a shared/agency account another session could move the campaign off DRAFT in that window. delete_master now re-reads the row immediately before the click and aborts ("Not clicking 'Удалить'") if the status changed or the row vanished entirely, instead of clicking against an unconfirmed state. - Finding 2 (transient verify errors): the post-click verify loop let any BrowserSessionError from fetch_masters_list (HTTP 5xx, non-JSON, captcha, mid-poll session expiry) propagate immediately, even though the click is immediate and irreversible and the campaign is almost certainly already gone. The loop now tolerates transient errors and keeps polling until _DELETE_VERIFY_TIMEOUT_MS; if every poll still fails, the error explicitly states the click already landed rather than reading like the delete itself failed. Adds 4 new tests to TestDeleteMaster covering both findings; all 722 tests in test_masters.py pass. Closes #793 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016N3w1Mg56sdiTeDa5SKbqw
…ried DRAFT row delete_master needs the target row's DOM node to click its menu, but the campaigns grid is a virtualized SPA (#639/#671) -- a row outside the grid's currently-rendered viewport was previously absent from the DOM entirely, and scroll_into_view_if_needed() cannot help: it only acts on a node that has already resolved, and cannot make an unrendered one exist. Live recon (direct_cli/browser/masters.py) found the grid's actual virtual-scroll container is a `[data-testid^="Grid.Row-"]` element's closest scrollable ancestor -- located structurally at runtime rather than by a hardcoded CSS-modules class (which Yandex's build regenerates on every deploy, same instability class as every other un-testid'd selector this module avoids). Directly assigning `scrollTop` plus dispatching a synthetic `scroll` event triggers the grid's own virtualization to re-render its visible row window, the same as a real user scroll would. New `_scroll_grid_to_row()` drives this container one `clientHeight` at a time until the target row appears in the DOM (confirmed live: 8 steps for a row ~90% down a ~6000px list), then falls through to the existing scroll_into_view_if_needed()/trigger-click retry loop unchanged -- a best-effort nudge that never raises on its own, mirroring that loop's own error-handling contract. Live-verified end-to-end 2026-08-06: created a fresh DRAFT campaign (713356270) via the create wizard UI, confirmed it was placed outside the grid's initial ~10-row render window by the grid's default cost-descending sort, then ran `masters delete 713356270 --yes` through this code unmodified -- it located, scrolled to, and deleted the campaign successfully; `masters list` confirmed it gone afterward. Adds TestScrollGridToRow (unit tests against page.evaluate() directly) and 2 new TestDeleteMaster integration tests (call order, best-effort failure handling). All 728 tests in test_masters.py pass. Closes #791 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016N3w1Mg56sdiTeDa5SKbqw
axisrow
changed the title
Owner
Author
🔍 Local review (cycle 1) — round 8c278a88-1971-4a2c-bacf-0e166294a2f0Reviewed locally: built-in 8-angle
Several other candidate findings from the finder angles (overlap-guard defense-in-depth, No FIX-level findings in this PR's own diff (#793, #791 scope). |
Owner
Author
📋 Review summary — all cyclesReviewed locally: 8-angle recall-biased
Totals: 0 FIX, 2 SKIP (1 tracked as follow-up issue #797), 0 UNVERIFIED. Note: several other candidate findings from the finder angles (update_master set/clear overlap defense-in-depth, _clear_repeating_value duplication/error-tolerance) trace to issue #786 code already on |
axisrow
added a commit
that referenced
this pull request
Aug 7, 2026
…ent verify errors (#799) Смерджено в ручную * fix(masters): harden archive_master/copy_master — TOCTOU race, transient verify errors Follow-up to PR #794 (delete_master hardening, #793/#791): local code review of that PR found archive_master and copy_master share the exact same "read row -> click -> poll _find_master_row" shape but were not hardened, tracked as issue #797. Finding 1 (TOCTOU race, both functions): the up-front status guard was read once, well before each function's own irreversible click -- on a shared/agency account another session could change the campaign's status in the real elapsed time spent navigating/opening the "⋮" menu. Both functions now re-verify status immediately before their click via a new shared _reverify_status_or_raise helper, aborting with an explicit message if the row vanished or its status changed, instead of clicking against an unconfirmed state. archive_master's re-check runs via _click_menu_item's new pre_click_check hook (after the menu opens, before the item click -- the narrowest possible window). copy_master requires the status to still match what it first read (no single required status exists for cloning, unlike archive's SUSPENDED). Finding 2 (non-tolerant post-click verify loop, copy_master worse): both post-click verify loops let a transient BrowserSessionError from _find_master_row (HTTP 5xx/captcha/auth blip) propagate raw and abort, even though the click had already landed. copy_master's loop only caught the narrower BrowserAuthError. Both now use a new shared _poll_master_row_tolerant helper (same tolerant-poll shape as delete_master's own fix) -- a timeout while every poll errored reports that the click already landed rather than reading like the action itself failed. This matters more for copy_master: it is explicitly non-idempotent, so a false failure risks a caller retrying into a duplicate campaign. Fixed 5 pre-existing test fixtures that used the raw grid status string "STOPPED" instead of fetch_masters_list's own normalized "SUSPENDED" -- harmless before this fix (archive_master never compared against the intermediate status), now caught by the new TOCTOU re-check. Adds 8 new tests (4 each to TestArchiveMaster/TestCopyMaster) covering both findings. All 736 tests in test_masters.py pass; full offline suite (3385 tests) green. Closes #797 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HstdqaLSqTV1uip4DvArjG * chore: trigger CI re-run for stuck API Coverage workflow (#799) * fix(masters): stop navigating away from the overview page in the archive/copy TOCTOU re-check The pre-click re-check in archive_master/copy_master (issue #797) called _find_master_row -> fetch_masters_list, which unconditionally navigates to the campaigns grid (page.goto(GRID_URL)) to capture its data request. Since the re-check runs right after the overview page's "⋮" menu is opened and right before the menu item is clicked, that navigation destroyed the menu the click was about to target -- every real archive/copy call would fail at the click. Unit tests never caught it because they mock fetch_masters_list wholesale, bypassing the real navigation. _reverify_status_or_raise now reads the same normalized status straight from the overview page's own body text (_read_status_text), exactly like suspend_master/resume_master already do, so the re-check never navigates at all -- this also fixes a related staleness risk (the grid's own status can lag the overview page by 45+ seconds per this module's own docs). Also re-raises BrowserAuthError from archive_master's post-click verify poll instead of letting _poll_master_row_tolerant swallow it as a generic transient error: unlike copy_master (not idempotent, so it deliberately keeps this wrapped), archive_master is idempotent and loses _with_session's fast auto-heal-and-retry otherwise. Found via cycle-review (Codex + /review) before merge. * chore(masters): pass not_found_hint to _reverify_status_or_raise callers Deferred /review finding from this PR's local cycle-review: the TOCTOU re-check helper defines a not_found_hint parameter (paired with changed_status_hint) but neither archive_master nor copy_master passed one, leaving the vanished-row error message without the next-step pointer its sibling changed-status branch has. Both callers now pass a short "check masters list" hint, matching the existing pattern. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HstdqaLSqTV1uip4DvArjG * fix(masters): tolerate status-text hydration lag in the archive/copy TOCTOU re-check _goto_overview_page only guarantees the overview page's title has rendered (issue #683), not its status text -- a separate render pass that _wait_for_recognised_status's own docstring documents as routinely reading as unrecognised for a moment right after. The TOCTOU re-check added in the previous commit read status via a single unguarded _read_status_text call, so a hydration lag right after navigating to the overview page could be misread as "another session changed it" and abort a perfectly legitimate archive/clone. _reverify_status_or_raise now polls via _wait_for_recognised_status, the same helper suspend_master/resume_master already use around their own status-dependent clicks, so a momentary lag is tolerated and only a persistently unrecognised status still aborts. Found via cycle-review round 2 (Codex). * fix(masters): compare copy_master's TOCTOU re-check against the overview page, not the grid copy_master's re-check passed the up-front grid-derived existing["Status"] as expected_status, but _reverify_status_or_raise compares against the overview page's own status text (_read_status_text's four-value vocabulary: SUSPENDED/ACTIVE/ MODERATION/ARCHIVED). The grid's primaryStatus vocabulary is broader (MODERATION_DENIED, RUN_WARN, TEMPORARILY_PAUSED, DRAFT, ...) and only STOPPED is normalized to match the overview's SUSPENDED marker -- any other grid status not in that four-value set deterministically false-aborted a legitimate, unchanged clone with a misleading "another session likely changed it" error. The grid is also documented elsewhere in this module to lag the overview page by 45+ seconds during a DRAFT->MODERATION transition, which could trigger the same false abort even for a recognised status. copy_master now reads its own starting status from the overview page (via _wait_for_recognised_status, right after _goto_overview_page) and compares the re-check against that instead -- both reads now come from the same source, exactly like archive_master already compares overview-SUSPENDED to overview-SUSPENDED. Found independently by both Codex and /review in cycle-review round 3. --------- 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.
Summary
Codex's adversarial review (cycle-review, round 4) on PR #784 found three edge cases in
delete_master(direct_cli/browser/masters.py), tracked as issues #793 and #791. This PR fixes all three.#793 Finding 1 — TOCTOU on DRAFT status before the irreversible click
delete_masterread the campaign's DRAFT status once via_find_master_row, well before theDeleteCampaignActionclick (the row-menu open/retry loop takes real time in between). On a shared/agency account, another session could transition the campaign (DRAFT → MODERATION/ACTIVE) inside that window.delete_masternow re-reads the row a second time immediately before the click and aborts — "Not clicking 'Удалить'" — if the status changed or the row vanished, instead of clickingDeleteCampaignActionagainst an unconfirmed state.#793 Finding 2 — transient verify-loop errors surfaced as hard failures
The post-click verify loop called
_find_master_row(page, campaign_id, status="all"), which can raiseBrowserSessionError(HTTP 5xx, non-JSON, captcha, mid-poll session expiry) on any individual poll — none of that was caught, so a transient error right after a successful, irreversible click propagated as if the delete itself had failed. The loop now tolerates transient errors and keeps polling until_DELETE_VERIFY_TIMEOUT_MS; if every poll still fails, the error explicitly states the click already landed, so a caller doesn't retry against a false premise.#791 — virtualized grid row unreachable when scrolled out of the initial render window
The campaigns grid is a virtualized SPA (#639/#671) —
deleteneeds the target row's DOM node to click its menu, and a row outside the grid's currently-rendered viewport was previously absent from the DOM entirely, with no way to make it appear (scroll_into_view_if_needed()only acts on a node that has already resolved).Live recon found the grid's virtual-scroll container is a
[data-testid^="Grid.Row-"]element's closest scrollable ancestor — located structurally at runtime, never by a hardcoded CSS-modules class (which Yandex's build regenerates on every deploy). Directly assigningscrollTopplus dispatching a syntheticscrollevent triggers the grid's own virtualization to re-render its visible row window, same as a real scroll would. New_scroll_grid_to_row()drives this oneclientHeightat a time until the target row appears, then falls through to the existing retry loop unchanged.Live verification
Both fixes required a live browser session (queued behind a concurrent session using the same account — coordinated via the orchestrator before any mutation):
FakePage/mockedfetch_masters_listonly — no live mutation needed.masters add --drafthit an unrelated, separately-tracked bug — see below), confirmed viapage.evaluaterecon that it rendered outside the first ~10 grid rows under the default cost-descending sort, then ranmasters delete 713356270 --yesthrough this PR's code unmodified. It located, scrolled to, and deleted the campaign successfully;masters list --status allconfirmed it gone afterward (79 campaigns, same as before creation).Side finding (not fixed here, filed separately)
While creating the test DRAFT campaign,
masters add --drafthit issue #796 (silent rejection, "Нужно указать хотя бы один регион" — a region is required by the create form's UI validation but wasn't being satisfied by the CLI's own flow in that session). Reproduced manually via the wizard UI to unblock this PR's live verification; the underlying CLI bug is out of scope here and filed as #796.Tests
TestDeleteMaster: 4 tests for masters delete: TOCTOU on DRAFT status + non-tolerant verify loop after irreversible click #793 (TOCTOU × 2, transient-verify-tolerance × 2) + 2 tests for masters delete: can't reach a DRAFT row virtualized out of the grid viewport #791 (call order, best-effort failure handling).TestScrollGridToRow: 4 unit tests againstpage.evaluate()directly.tests/test_masters.pysuite: 728 passed.black/flake8clean.masters deleteis DANGEROUS/manual-only persmoke_matrix.py(no automated release gate, no--sandbox).Closes #793
Closes #791
🤖 Generated with Claude Code