feat(masters): add --add-sitelink/--remove-sitelink to masters update - #803
Merged
Conversation
4 tasks
axisrow
force-pushed
the
ao/direct-cli-84/masters-648-sitelinks-v2
branch
from
August 7, 2026 09:46
c2c9ed7 to
06aea15
Compare
…e` (#648) Implements the "Быстрые ссылки" (sitelinks) section for Мастер кампаний, following the point-position pattern already used for audience tags: - Browser layer (direct_cli/browser/masters.py): _read_sitelinks, _add_sitelink, _remove_sitelink, plus testid constants and a Counter-based multiset verification in _verify_saved mirroring the audience-tags check. - CLI layer (direct_cli/commands/masters.py): --add-sitelink "Title|Href|Description" and --remove-sitelink <position> on `masters update`, with _parse_add_sitelink_options/ _parse_remove_sitelink_options helpers. - Tests (tests/test_masters.py): unit coverage for the readers/mutators, verification, CLI parsers, and CLI wiring. Based on two live, read-only recon passes (2026-08-06, campaign 713277109) that confirmed the section/card/row testids but NOT how the inline edit form closes/commits, whether its fields are plain textareas or contenteditable divs, or the real max sitelink count -- all documented inline as NOT LIVE-VERIFIED, with the contenteditable path assumed conservatively (reusing _clear_text_field). Restores the full implementation that was accidentally dropped to two unused parser stubs during a prior rebase onto main (the rebase's conflict resolution kept only HEAD's side for interleaved hunks in browser/masters.py, discarding the sitelinks browser layer, CLI wiring, and all 28 tests). Re-applied cleanly against current main; all previously passing tests plus the 28 sitelink tests pass, black/flake8 clean. Closes #648 (Этап C sitelinks slice). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FF5MKQjSX89UpwHfFG3uBe
axisrow
force-pushed
the
ao/direct-cli-84/masters-648-sitelinks-v2
branch
from
August 7, 2026 11:31
06aea15 to
327f6e6
Compare
Two cycle-review findings (Codex + code-review agent, PR #803): 1. The sitelink add/remove loop in `update_master` didn't poll `_read_sitelinks` after each click to confirm it committed to the DOM before proceeding to the next removal or the save click — unlike the audience-tag and Metrika-counter loops in the same function, which do this exact check citing a live-confirmed bug (issue #681) where a save right after a non-committed click reloaded with the change missing. Without it, removing multiple sitelinks high-to-low could target a stale DOM and remove the wrong card if an earlier click hadn't committed yet. 2. `_verify_saved`'s pre-reload settle-wait guard (the same issue #681 race) was extended to Metrika counters but not to sitelinks, even though this same PR wired up `sitelinks_before`/`add_sitelinks`/ `remove_sitelink_indices` as `_verify_saved` params right next to it. Both fixes mirror the existing audience-tag/Metrika-counter pattern exactly. New tests cover the commit-confirmation loop (raises before the save click on a non-committing add/remove, tolerates a delayed commit) and the settle-wait guard (sitelinks-only save still gets the 5s wait). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FF5MKQjSX89UpwHfFG3uBe
Owner
Author
Local review (cycle 1) - round 91542a6d-30ff-4714-a759-303242410139Reviewed locally (built-in /review + Codex companion), no bots pinged.
Totals: 2 FIX (both resolved in a follow-up commit, with new regression tests and a verified mutation-check), 2 SKIP (documented/non-blocking), 5 IRRELEVANT (out of this PR's diff, confirmed via git diff hunk boundaries). |
…w input Cycle-review finding (Codex, cycle 2 of PR #803): `_verify_saved`'s post-save sitelink check built its expected multiset from the raw CLI-supplied Title/Href, but whether Yandex's displayed value equals the raw input byte-for-byte (protocol, trailing slash, host casing) was never live-verified. A display-format mismatch would make an already-successful add report a false "did not save as requested" AFTER the irreversible save — and because `update_master`'s retry path re-reads the (now updated) baseline and re-applies the same `add_sitelinks`, a naive retry would create a duplicate sitelink. `update_master`'s add loop already re-reads `_read_sitelinks` right after each add's own commit-confirmation poll succeeds — this now captures that freshly-observed card (`_read_sitelinks(page)[-1]`) into `add_sitelinks_observed` and passes it to `_verify_saved` instead of the raw CLI input. Mirrors `_metrika_counter_identity`'s identical "compare what actually got read back, not the raw request" fix (commit 3915f45) for the same failure shape. The CLI-facing result dict still reports the raw requested values (`AddedSitelinks`), only the internal verification comparison changed. New test confirms `_verify_saved` accepts an add whose displayed Href differs cosmetically from the raw CLI input; mutation-tested (reverting the fix makes the test fail with exactly the false-mismatch error this fix eliminates). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FF5MKQjSX89UpwHfFG3uBe
Owner
Author
Review summary — all cycles
Totals: 3 FIX (all resolved, with regression tests and verified mutation-checks), 4 SKIP (documented/non-blocking, explicitly scoped to the stated live-verification follow-up), 6 IRRELEVANT (out of this PRs diff) / HALLUCINATION (factually incorrect claims, verified against the actual code). |
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
Implements быстрые ссылки (sitelinks) support in
direct masters update— part of issue #648's Этап C (structural sections: sitelinks, audience, Metrika counters/goals). This is a browser-driven feature (Мастер кампаний has no API), following the same add/remove-by-position pattern already established for--add-audience-tag/--remove-audience-tag(#681), adapted for sitelinks' 3-field card structure (Title/Href/Description) instead of a plain text tag.--add-sitelink "Title|Href|Description"— pipe-delimited, adds a new card via the "Добавить" button.--remove-sitelink POSITION— removes a card by its 0-based on-page position._read_sitelinks/_add_sitelink/_remove_sitelinkindirect_cli/browser/masters.py, wired intoupdate_master(snapshot-before + high-to-low removal, mirroring the audience-tags convention) and_verify_saved(multiset check on(Title, Href)—Descriptionis not bulk-readable, see below).Testid structure was confirmed via two live read-only recon passes against campaign 713277109 (Playwright +
mcp claude-in-chrome, no mutations saved) — but the actual add/remove/save behavior has not been exercised end to end against a real Yandex session. Specifically open questions:SitelinkRow.*edit form closes/commits — no dedicated "Готово"/save testid appeared in recon. The code pressesEscapethen clicks the section container as a best-effort dismiss, relying onupdate_master's own re-read (_verify_saved) as the real proof of success rather than trusting the close action itself.contenteditablevs plain<textarea>forSitelinkRow.name/href/description.textarea— conservatively assumedcontenteditable(same class as headline/text slots), reusing_clear_text_field+.type(). If it turns out to be a plain textarea, this degrades to a harmless no-op equivalent, not a functional bug.SitelinkRow.*triple immediately available — assumed by analogy with every other "click Добавить, fill, commit" flow in this module (target actions, audience tags), never itself clicked in recon._SITELINKS_SLOT_COUNT = 5comes from UI copy ("добавить до 5 штук"), not from actually adding a 5th sitelink past the 4 the recon campaign already had.None of this is claimed as working in the docstrings/CLI help — see the module comment above
_SITELINKS_EDITOR_TESTIDindirect_cli/browser/masters.pyfor the full recon writeup. A live mutation-verification pass is planned as a follow-up (browser session currently queued for other in-flight work) before this is considered safe to rely on in production.Test plan
pytest tests/test_masters.py -k "sitelink or Sitelink"— 28 passedpytest -n auto— 3405 passed, 23 skipped, no regressionsblack/flake8clean on all changed filesRelates to #648 (no dedicated sub-issue for this specific section).
Supersedes #800 and #802 (both closed — CI got permanently stuck queued on the original branch, even after an empty-commit push and a close/reopen cycle on #800→#802; recreating on a fresh branch name to rule out a branch-scoped webhook/Actions issue).
🤖 Generated with Claude Code
https://claude.ai/code/session_01FF5MKQjSX89UpwHfFG3uBe