feat(masters): add --add-metrika-counter/--remove-metrika-counter to masters update

[axisrow]

Summary

Implements Yandex Metrika counter linking/unlinking in direct masters update — part of issue #648's Этап C (structural sections: sitelinks, audience, Metrika counters/goals). Browser-driven (Мастер кампаний has no API), mirroring the --add-audience-tag/--remove-audience-tag (#681) add/remove-by-position pattern almost field-for-field, since the "Счетчики Яндекс Метрики" section's DOM shape (MetrikaCountersTagGroup.*) turned out to be structurally identical to the audience-tags widget.

• --add-metrika-counter TEXT — types into the section's search input, clicks the matching autocomplete suggestion.
• --remove-metrika-counter POSITION — removes a counter by its 0-based on-page position.
• _read_metrika_counters/_add_metrika_counter/_remove_metrika_counter in direct_cli/browser/masters.py, wired into update_master and _verify_saved (multiset check), same as audience tags.

⚠️ NOT LIVE-VERIFIED (with one exception — see below)

Testid structure was confirmed via live read-only recon against campaign 713277109 (Playwright + mcp claude-in-chrome, no mutations saved). Actual add/remove/save behavior has not been exercised end to end.

One thing IS live-confirmed (via mcp claude-in-chrome against the real Chrome session, 2026-08-06): the exact text format --add-metrika-counter needs. Typing a partial query (e.g. just a counter's label, "Ксамата") surfaces the same suggestion as typing more, but the suggestion's accessible text is a single line shaped "{label} • {domain/path} • {numeric counter id}" (confirmed: "Ксамата • yandex.ru/maps • 88834924"). Since the exact-match lookup compares against the WHOLE string, callers must pass that full string, not just the label — this is now documented in the CLI help text and the _add_metrika_counter docstring.

Still open:

1. Whether match.click() actually commits the counter and whether a save persists it — the recon session closed the suggestion popup with Escape and reloaded the edit page without saving, specifically to avoid mutating this production campaign's counter set.
2. A possible "leftover text after Escape" gap — the same recon session that confirmed the text format also observed Escape closing the suggestion dropdown but leaving the typed text sitting in the input (the field had to be explicitly cleared before navigating away). This mirrors the class of bug found in issue masters add --draft: клик 'Сохранить как черновик' не создаёт кампанию, redirect timeout (5/5 live-попыток) #796 (_set_target_action_price's price popup not being closed before the terminal click) — worth revisiting whether _add_metrika_counter's error path needs the same explicit-clear treatment once this is exercised end to end, not just an Escape press.
3. Hydration/settle timing for this section — currently reuses _AUDIENCE_TAG_SUGGEST_TIMEOUT_MS's 5s budget with no independent calibration.
4. MetrikaCountersTagGroup.Expander's purpose — left unimplemented/ignored, its function was not determined from recon.

None of the add/remove/save behavior is claimed as working in the docstrings/CLI help. A live mutation-verification pass is planned as a follow-up (browser session currently queued for other in-flight work).

Test plan

• pytest tests/test_masters.py -k "Metrika or metrika" — 15 passed
• Full offline suite pytest -n auto — 3392 passed, 23 skipped, no regressions
• black/flake8 clean on all changed files
• Live verification (add, remove, save-persists) — pending, tracked as a follow-up before this is relied upon

Relates to #648 (no dedicated sub-issue for this specific section).

🤖 Generated with Claude Code

https://claude.ai/code/session_01FF5MKQjSX89UpwHfFG3uBe

[axisrow]

Закрываю и переоткрою для форсирования нового CI-прогона — тот же рассинхрон, что и на #800.

[axisrow]

🔍 Local review (cycle 1) — round 0e565e97-94a1-45bf-8a96-dc881c99ac9e

Reviewed locally (/review + Codex companion), no bots pinged.

Verdict	Reviewer	Finding	Location
FIX	codex	The post-save verification for Metrika counters compared the user-supplied add text against a differently-formatted read-back of the same counter, so every successful add was reported as a save failure that could prompt a retry and duplicate the link.	direct_cli/browser/masters.py

Fixed in 3915f45: post-save verification now compares Metrika counters by their stable numeric id (extracted from either text format) instead of raw display text.

Note: the built-in /review did not return a parseable review this round (it repeatedly reported waiting on its own internal monitor with no findings) — this is being retried per cycle-review policy before finalizing.

[axisrow]

@claude review PR #801 at exact head 3915f45 (round 808c98c1-c3de-4a83-9a47-993b803a2592). Focus on critical issues: bugs, security vulnerabilities, logical errors, data loss risks, performance problems. Do NOT nitpick style, naming conventions, minor formatting, or subjective preferences — only flag issues that could break functionality or cause real harm in production.

[claude]

Claude finished @axisrow's task in 2m 19s —— View job

---

Review — PR #801 (Metrika counters in masters update)

Reviewed at head 3915f45, focusing on correctness / data-loss / logic (not style). Files: direct_cli/browser/masters.py, direct_cli/commands/masters.py, tests, CHANGELOG.

Verdict: no blocking bugs found. The cycle-1 fix is correct and the browser plumbing mirrors the audience-tags pattern faithfully. Details below.

---

✅ The cycle-1 identity fix is correct

_metrika_counter_identity (masters.py) reduces both text shapes to the counter's numeric id:

• suggestion "Ксамата • yandex.ru/maps • 88834924" → first line → rsplit(" • ", 1)[1] = 88834924
• read-back "gc.ksamata.ru • 72112213\n30 целей" → first line "gc.ksamata.ru • 72112213" → 72112213

And the _verify_saved multiset math is sound. Worth calling out because it's subtle and easy to get wrong: the block does expected_counters[id] -= 1 (item assignment leaves 0/negative entries in the dict) and then expected_counters += Counter(adds). That += invokes Counter.__iadd__, which ends with _keep_positive() — so the zeroed-out removed counters are stripped even when add_metrika_counters is empty (empty += still calls _keep_positive). The comparison against a freshly-built Counter of the actual read-back therefore matches. Good.

✅ Duplicate/ordering safety

• _parse_remove_metrika_counter_options rejects duplicate positions up front — this is what prevents the "remove index 2 twice → shifts and removes the wrong counter, and _verify_saved double-decrements" class of bug.
• Removals run high-to-low against the pre-mutation metrika_counters_before snapshot, so a higher removal never shifts a lower pending position. Out-of-range is checked against len(_counters_before) (the before-snapshot), which is the right frame of reference.

⚠️ The one thing most likely to fail in the live pass (already flagged NOT LIVE-VERIFIED, calling out the specific mechanism)

_add_metrika_counter does field.type(text) with text = the entire formatted suggestion string ("{label} • {domain/path} • {numeric id}"), then requires an option whose first line == text. The live recon only confirmed that typing a partial query (the bare label, "Ксамата") surfaces the suggestion. It's plausible that typing the full string — bullets + numeric id included — filters Yandex's autocomplete to zero results (the search likely matches on label/domain, not on the rendered display string), in which case _add_metrika_counter would always raise "No suggestion exactly matching…". This is the highest-risk assumption to validate first when the live pass happens; if it fails, the fix is to type only the label/query portion while still matching the full first line against text. Not a code defect given the honest NOT-VERIFIED labeling — just naming the concrete failure mode.

Minor (non-blocking, matches existing audience-tags behavior — noting for parity, not asking for a change)

• --remove-metrika-counter is type=int with no lower bound. A negative position (e.g. -1) passes the index >= len check and then fails later (close-button testid ...tag.-1.close won't resolve → BrowserSessionError), so it's caught, just with a less-specific message than an up-front validation would give. Identical to --remove-audience-tag, so this is consistent by design.
• Message base mismatch: input positions are documented 0-based, but user-facing messages render position {index + 1} / positions 1-N. Intentional (0-based in, 1-based in prose) and consistent with the audience-tag sibling — flagging only so it isn't mistaken for a bug.

None of these are data-loss or security issues. The count-based post-action verification (raise if the list doesn't grow/shrink as expected) is a good guard against a silent no-op commit.

---

• Read the diff (browser + command + tests + CHANGELOG)
• Analyzed for bugs / logic / data-loss / perf
• Posted findings
  • branch ao/direct-cli-84/masters-648-metrika-counters

[axisrow]

📋 Review summary — all cycles

Cycle	Reviewer	Finding	Verdict	Resolution
1	codex	The post-save verification for Metrika counters compared two structurally different text representations of the same counter, so every successful add was reported as a save failure.	FIX	Fixed in 3915f45
2	codex	Independently re-verified the fix's Counter arithmetic and dedup guards; no remaining issue	(approve)	n/a
2 (cloud fallback)	claude	Confirmed the fix is correct; local /review had stayed silent across both local rounds	(approve)	n/a

Totals: 1 FIX (resolved), 0 SKIP, 0 UNVERIFIED.

Note: local mode's built-in /review did not return a parseable verdict in either round (repeatedly reported waiting on its own internal monitor). Per cycle-review policy, a user-authorized targeted cloud fallback ping to @claude was used instead for final confirmation.

[axisrow]

📋 Review summary — all cycles (re-review after cleanup commit)

Cycle	Reviewer	Finding	Verdict	Resolution
1	codex	The post-save verification for Metrika counters compared two structurally different text representations of the same counter, so every successful add was reported as a save failure.	FIX	Fixed in 3915f45
2	codex + claude (cloud fallback)	Independently confirmed the fix is correct	(approve)	n/a
re-review	codex (delayed /review report)	A metrika-only save skipped the same pre-reload settle wait already applied to audience-touching saves for a confirmed server-commit race, risking the same false-negative failure mode.	FIX	Fixed in f954f9d
re-review	codex	The Metrika verify read still uses a shorter polling budget than the audience-tags equivalent and has no re-navigate-on-mismatch retry — a residual, already-documented design tradeoff, not addressed further.	SKIP	Pre-existing, already-documented tradeoff (short read budget over an unconfirmed multiplier); left as-is per the PRs own stated rationale

Totals: 2 FIX (both resolved), 1 SKIP, 0 UNVERIFIED.

Both fixes were independently re-verified by Codex against the final head. The PR remains explicitly NOT LIVE-VERIFIED end-to-end (self-disclosed by the author); the fixes in this cycle address code-level correctness of the verification logic itself, not the live browser-interaction assumptions.

[axisrow]

📋 Review summary — all cycles (final, after 2 re-reviews)

Cycle	Reviewer	Finding	Verdict	Resolution
1	codex	The post-save verification for Metrika counters compared two structurally different text representations of the same counter, so every successful add was reported as a save failure.	FIX	Fixed in 3915f45
2	codex + claude (cloud fallback)	Independently confirmed the fix is correct	(approve)	n/a
re-review 1	codex (delayed /review report)	A metrika-only save skipped the same pre-reload settle wait already applied to audience-touching saves for a confirmed server-commit race.	FIX	Fixed in f954f9d
re-review 1	codex	Read-budget asymmetry (pre-existing, documented tradeoff)	SKIP	Left as-is
re-review 2	claude (delayed /review report)	The identity-extraction helper collapsed certain malformed inputs to an empty string, letting two different malformed counters falsely match each other instead of failing to match anything.	FIX	Fixed in 12c3bdc
re-review 2	claude	The Metrika verify read uses a shorter polling budget than the audience-tags equivalent; a pre-existing, already-documented design tradeoff.	SKIP	Duplicate of re-review 1's finding
re-review 2	claude	Adding an already-linked counter or removing at a negative position may surface a confusing generic error instead of a specific one; pre-existing pattern shared with the audience-tags sibling commands.	SKIP	Pre-existing pattern, shared with audience-tags siblings

Totals: 3 FIX (all resolved), 3 SKIP, 0 UNVERIFIED.

Re-review cap reached (2/2). Both /review rounds arrived significantly delayed (up to ~1 hour after being launched, well past the cycle's normal window) — each carried one additional real, code-level finding beyond what Codex had already found in the same round, both now fixed. Per cycle-review policy, this is the last re-review before handing back to the user rather than looping again. The PR remains explicitly NOT LIVE-VERIFIED end-to-end by its own author's disclosure; all fixes in this cycle address code-level correctness of the verification/identity logic, not the live browser-interaction assumptions.