fix(masters): harden archive_master/copy_master — TOCTOU race, transient verify errors - #799
Merged
Merged
Conversation
…ent 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
Owner
Author
🔍 Local review (cycle 1) — round a3f0968e-829b-450b-8500-779adc3df7d6Reviewed locally (
Totals: 2 FIX (both resolved in this round), 2 SKIP (left as-is, see rationale below). Resolution notes
|
…ive/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.
Owner
Author
🔍 Local review (cycle 1) — round 5274df8a-e511-4092-abfb-36299b8acf14Reviewed locally (
Fixed in ada744b: the TOCTOU re-check now reads status from the overview page itself (_read_status_text) instead of navigating to the grid, and archive_master re-raises BrowserAuthError from its post-click poll for _with_session's auto-heal. |
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
…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).
Owner
Author
🔍 Local review (cycle 2) — round 65224f1c-5172-427a-b208-19e371766163Reviewed locally (
Fixed in ca44c41: the TOCTOU re-check now polls for a recognised status (same helper suspend_master/resume_master already use) instead of reading once, tolerating a momentary hydration lag. Merged with a concurrent not_found_hint follow-up commit from this PR (8ff33b2) → 4f202ae. This is cycle 2/3 — one more FIX-bearing round would hit the cap. |
…iew 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.
Owner
Author
🔍 Local review (cycle 3/3 — final) — round c1ace792-b95e-44c0-882a-30ea4650e2e3Reviewed locally (
Fixed in c6a6189: copy_master now reads its own starting status from the overview page (via _wait_for_recognised_status) and compares the re-check against that, instead of the up-front grid read. This was cycle 3 of the 3-cycle cap. Per cycle-review policy, the cap is a signal, not just a limit: three consecutive rounds each found a real defect in the same new TOCTOU re-check helper (navigation-breaks-click → hydration-race → cross-source-status-mismatch). All three trace back to the same invariant: the re-check must read its status from the exact same source, at the exact same page, with the exact same tolerance for rendering lag, as whatever it is being compared against — no navigation, no polling gap, no cross-source comparison. The current implementation (ca44c41 + c6a6189) satisfies that invariant for both archive_master and copy_master. Two remaining SKIP items (redundant polling on a dead session; resume_master lacks the same TOCTOU guard) are real but out of this PR's scope (issue #797 covers only archive/copy) and are proposed as follow-up, not blocking. |
Owner
Author
🔍 Local review (cycle 2) — round 8abf5a79-ef8a-4048-8c87-78a1cc3b6f1cReviewed locally (
Totals: 2 FIX (both resolved in this round), 3 SKIP (left as-is, see rationale below). Resolution notes
|
axisrow
added a commit
that referenced
this pull request
Aug 7, 2026
…menu Cycle-review round 1 on this PR found a follow-on issue in the original #805 fix, independently flagged by Codex, a teammate reviewer, and /review: opening the row menu, then running the TOCTOU re-check (whose own fetch_masters_list call unconditionally navigates via page.goto(GRID_URL), destroying the just-opened menu), then re-opening the menu a second time, wastes the entire first open on every single call -- not just a rare retry path -- and reintroduces a real delay between the re-check and the click that the "immediately before the click" guarantee (issue #793's own stated intent) is meant to close. Fix: the TOCTOU re-check now runs BEFORE _open_grid_row_menu (logically right after the up-front DRAFT guard), so the menu is opened exactly once, right before the click, with nothing able to navigate the page in between. This differs from archive_master/copy_master's own re-check ordering (PR #799, where the menu lives on a DIFFERENT page than the one the re-check reloads) -- delete_master's row menu and its re-check both live on the same grid page, so there is no reason to open the menu before re-checking here. Rewrote the tests whose scroll/click-count assertions assumed the old double-open: two pre-existing tests reverted to single-open expectations, and the old call-count-only regression test replaced with two tests that assert BOTH the call order (re-check before open) and the abort-before-open path when status changed -- mutation-checked against the pre-reorder code (all four fail without the fix). Live-verified end to end: created a fresh test DRAFT campaign (713364586), deleted it successfully on the first attempt with the reordered fix, confirmed gone via a grid diff (80 rows -> 79). All 3379 tests pass; black/flake8 clean. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HstdqaLSqTV1uip4DvArjG
axisrow
added a commit
that referenced
this pull request
Aug 7, 2026
…re-check (#807) Смерджено в ручную * fix(masters): re-open the grid row menu after delete_master's TOCTOU re-check The TOCTOU re-check added in #793 (_find_master_row, right before the irreversible DeleteCampaignAction click) unconditionally navigates the page: it calls fetch_masters_list -> _capture_grid_campaigns_request, which does page.goto(GRID_URL) to capture the grid's own data request. That navigation destroys the row menu popup already opened earlier in delete_master -- delete_item's locator then targets an element that no longer exists on the freshly-reloaded grid page, so the click times out every time with "found but not clickable". Same root cause PR #799 fixed for archive_master/copy_master's overview-page "⋮" menu, just not caught for delete_master at the time since it was outside that PR's scope (issue #797 was archive/copy only). Live-confirmed via issue #805: a stuck test DRAFT campaign (713359607, created during #796's live verification) failed to delete 4/4 times with this exact error before the fix, and deleted successfully on the first attempt after it -- confirmed gone via a grid diff (80 rows -> 79). Fix: extracted the row-menu-open logic (scroll + trigger-click retry loop) into a new _open_grid_row_menu helper, called once to open the menu initially and a second time -- after the TOCTOU re-check -- right before the delete click, mirroring PR #799's own fix pattern for the overview page's menu. Updated two pre-existing tests whose scroll/click-count assertions implicitly assumed a single menu-open; added a new regression test (test_reopens_row_menu_after_toctou_recheck_before_clicking) asserting _open_grid_row_menu's call count -- mutation-checked against the pre-fix code (all three fail without the fix, one because _open_grid_row_menu doesn't exist yet). All 3378 tests pass; black/flake8 clean. Closes #805 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HstdqaLSqTV1uip4DvArjG * fix(masters): run delete_master's TOCTOU re-check before opening the menu Cycle-review round 1 on this PR found a follow-on issue in the original #805 fix, independently flagged by Codex, a teammate reviewer, and /review: opening the row menu, then running the TOCTOU re-check (whose own fetch_masters_list call unconditionally navigates via page.goto(GRID_URL), destroying the just-opened menu), then re-opening the menu a second time, wastes the entire first open on every single call -- not just a rare retry path -- and reintroduces a real delay between the re-check and the click that the "immediately before the click" guarantee (issue #793's own stated intent) is meant to close. Fix: the TOCTOU re-check now runs BEFORE _open_grid_row_menu (logically right after the up-front DRAFT guard), so the menu is opened exactly once, right before the click, with nothing able to navigate the page in between. This differs from archive_master/copy_master's own re-check ordering (PR #799, where the menu lives on a DIFFERENT page than the one the re-check reloads) -- delete_master's row menu and its re-check both live on the same grid page, so there is no reason to open the menu before re-checking here. Rewrote the tests whose scroll/click-count assertions assumed the old double-open: two pre-existing tests reverted to single-open expectations, and the old call-count-only regression test replaced with two tests that assert BOTH the call order (re-check before open) and the abort-before-open path when status changed -- mutation-checked against the pre-reorder code (all four fail without the fix). Live-verified end to end: created a fresh test DRAFT campaign (713364586), deleted it successfully on the first attempt with the reordered fix, confirmed gone via a grid diff (80 rows -> 79). All 3379 tests pass; black/flake8 clean. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HstdqaLSqTV1uip4DvArjG --------- Co-authored-by: axisrow <axisrow@users.noreply.github.com> Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
This was referenced Aug 7, 2026
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
Follow-up to PR #794 (delete_master hardening, closing #793/#791): local code review of that PR found
archive_masterandcopy_master(direct_cli/browser/masters.py) share the exact same "read row → click → poll_find_master_row" shape but were left unhardened, 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 and opening the "⋮" menu. Both functions now re-verify status immediately before their click via a new shared
_reverify_status_or_raisehelper, aborting with an explicit message if the row vanished or its status changed.archive_master's re-check runs via_click_menu_item's newpre_click_checkhook — fires right after the menu is confirmed open, right before the item click, the narrowest window achievable without splitting that function.copy_masterrequires the status to still match what it first read (expected_status=existing["Status"]) — cloning has no single required status the way archiving requiresSUSPENDED, so "changed at all" is the unsafe signal here.Finding 2 — non-tolerant post-click verify loop (both functions,
copy_masterworse). Both post-click verify loops let a transientBrowserSessionErrorfrom_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 narrowerBrowserAuthError. Both now use a new shared_poll_master_row_toleranthelper (same tolerant-poll shape asdelete_master's own PR #794 fix) — a timeout while every poll errored reports that the click already landed, rather than reading like the action itself failed. This matters more forcopy_master: it's explicitly non-idempotent, so a false failure risks a caller retrying into a duplicate campaign.Test changes
Fixed 5 pre-existing test fixtures that used the raw grid status string
"STOPPED"instead offetch_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.Added 8 new tests (4 each to
TestArchiveMaster/TestCopyMaster) covering both findings, mirroring PR #794's own test additions fordelete_master.Testing
pytest tests/test_masters.py— 736 passedpytest(full offline suite) — 3385 passed, 23 skippedblack/flake8cleanNo live browser verification performed — this changes only error-handling/re-verification logic around existing click paths, same class of change as PR #794 which also shipped without a live archive/copy mutation run (
masters archive/masters copyare DANGEROUS/manual-only persmoke_matrix.py).Closes #797
🤖 Generated with Claude Code