@claude review. 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 finished @axisrow's task in 2m 21s —— View job

Code Review — PR #113

• Gather context and read changed files
• Analyze for bugs, security issues, logic errors, data loss risks
• Post review findings

Findings

1. _build_notification — None vs False conflation (agencyclients.py:28, 32)

"Value": "YES" if send_account_news else "NO",
"Value": "YES" if send_warnings else "NO",

Both CLI options have default=None (agencyclients.py:105, 110), meaning the user may not pass either flag. When send_account_news=None (flag omitted) and notification_email is provided, the code still emits "Value": "NO" for RECEIVE_RECOMMENDATIONS — the same as if the user explicitly passed --no-send-account-news. The Yandex Direct API likely treats an explicit "NO" differently from an omitted field (explicit opt-out vs. system default).

Result: direct agencyclients add --notification-email [email protected] ... (without --send-account-news/--send-warnings) will silently opt the new client out of both notification types.

The old code used if send_account_news is not None: guards for exactly this reason. The fix should preserve that guard per option within EmailSubscriptions:

if notification_email:
    subs = []
    if send_account_news is not None:
        subs.append({"Option": "RECEIVE_RECOMMENDATIONS", "Value": "YES" if send_account_news else "NO"})
    if send_warnings is not None:
        subs.append({"Option": "TRACK_POSITION_CHANGES", "Value": "YES" if send_warnings else "NO"})
    if subs:
        notification["EmailSubscriptions"] = subs

Fix this →

2. get_first_leads_turbopage_id probe with ID 0 — vacuous test pass (test_integration.py:98–109)

result = invoke_get("leads", "get", "--turbo-page-ids", "0", "--limit", "1", "--format", "json")
if parse_json_output(result) is not None:
    return 0

If the API returns an empty JSON array [] (exit 0) for the non-existent ID 0 — which is standard REST behavior — parse_json_output returns [] (not None), so the function returns 0. The subsequent test then calls leads get --turbo-page-ids 0, gets [], and assert_success passes because [] is valid JSON. The test exercises the CLI wiring but verifies nothing about actual lead data retrieval.

get_first_business_id's probe with --ids 0 avoids this: isinstance(data, list) and data correctly rejects empty list, returning None → skip. The leads probe should follow the same pattern, or skip rather than return 0 on an empty response:

data = parse_json_output(result)
if isinstance(data, list) and data:
    return data[0].get("Id")
return None  # skip instead of vacuous pass with 0

3. is_agency_access_denied — substring "403" match too broad (test_integration.py:196)

"403" in result.output

This fires on any output containing the substring "403" — including error codes like 4030, 4031, references to "item 403 was processed", etc. The shell equivalent in test_safe_commands.sh uses a safer word-boundary regex (^|[^0-9])403([^0-9]|$). The Python version should at least check " 403 " or "403\n" / "code=403" / a list of known strings.

4. Breaking change: --ids removed from agencyclients get (agencyclients.py:44)

The --ids flag is replaced by --logins with no deprecation path. Any existing scripts calling direct agencyclients get --ids … will get Error: No such option: --ids with no helpful message. Worth at least documenting in the changelog or emitting a click.UsageError with a migration hint if users pass --ids.

Summary

Issue #1 is the one actionable bug that could affect production behavior (unintended opt-out of email notifications for new clients). The rest are test robustness or UX polish.

Triage по ревью bot'а:

#1 (None vs False conflation) — FIXED в af74344. Восстановлены per-option guards if send_account_news is not None / if send_warnings is not None в _build_notification.

#2 (vacuous probe pass for leads) — FIXED в af74344. get_first_leads_turbopage_id теперь требует non-empty list (isinstance(data, list) and data) вместо просто is not None.

#3 ("403" in result.output слишком широкий) — SKIP. Проверка защищена условием result.exit_code != 0, поэтому false-positive в реальной работе невозможен (CLI не печатает "403" в успешном выводе agencyclients get). Косметическое улучшение, не меняющее поведение тестов.

#4 (--ids -> --logins без deprecation) — SKIP. CLI pre-1.0, новый контракт намеренно соответствует Yandex Direct API (поле называется Logins). Старый --ids был неверным неймингом для этого ресурса. README в этом же PR обновлён.

@claude review. 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 finished @axisrow's task in 2m 14s —— View job

Code Review — PR #113

• Gather context
• Read changed files
• Analyze for critical issues
• Post findings

Findings (current branch, including af74344 fixes)

1. get_first_advideo_probe_id — vacuous pass, same pattern as the fixed leads probe (test_integration.py:197)

probe_result = invoke_get("advideos", "get", "--ids", str(candidate), ...)
if parse_json_output(probe_result) is not None:   # ← same bug as leads, unfixed
    return str(candidate)

Issue #2 from the previous review was fixed in af74344 for get_first_leads_turbopage_id — the guard was tightened to isinstance(data, list) and data to reject an empty []. That fix was not applied here. If the Yandex Direct API returns [] for a video creative ID (e.g., it exists in creatives but advideos doesn't index it), parse_json_output returns [], which is not None, so the probe returns str(candidate). The subsequent test calls advideos get --ids <candidate> and may get [], which passes assert_success because it is valid JSON. The test exercises the CLI wiring but asserts nothing about actual video ad retrieval.

Fix mirrors the leads fix:

data = parse_json_output(probe_result)
if isinstance(data, list) and data:
    return str(candidate)

Fix this →

2. _build_notification — empty Notification: {} always sent (agencyclients.py:136–148)

When no notification flags are given, _build_notification(None, None, None, None) returns {}. The add command always includes "Notification": {} in the body:

body = {
    "method": "add",
    "params": {
        ...
        "Notification": _build_notification(...),  # → {} when all flags are None
    },
}

test_agencyclients_add_payload_uses_top_level_fields (test_dry_run.py:1563) asserts "Notification" in body["params"], implying an empty dict is expected and intentional. Whether the Yandex Direct API accepts {"Notification": {}} or rejects it with a validation error is not tested here — if the API is strict about this field, silent add failures could occur for users who don't supply notification options. This is pre-existing behavior (not a regression introduced in this PR) and low severity, but worth an API-contract check.

3. agencyclients get — SelectionCriteria now unconditional (agencyclients.py:76)

criteria = {"Archived": archived}   # always set, default "NO"
params = {"SelectionCriteria": criteria, "FieldNames": field_names}

Old code only sent SelectionCriteria when ids was non-empty. New code always sends it with at least {"Archived": "NO"}. This is intentional and correct for the WSDL-backed agencyclients/get — the API requires SelectionCriteria — but it's a breaking behavioral change: callers who previously relied on omitting criteria to get all clients (including archived) now need to pass --archived YES explicitly. The README update in this PR documents --archived with a default of NO, so this is acceptable, but might silently hide archived clients from existing scripts. Not a production data-loss risk, just a visibility regression for users who don't re-read the docs.

Summary

Issue #1 is the only actionable gap from this review. The None vs False bug (#1 from the last review) is correctly fixed, and the leads probe fix (#2 from the last review) is correctly applied — it just wasn't replicated to the advideo probe.

Triage второго ревью bot'а:

#1 (advideo probe — same vacuous-pass pattern) — FIXED в 74f7972. Гард ужесточён до isinstance(probe_data, list) and probe_data, как в leads-фиксе.

#2 (empty Notification: {} в add) — SKIP. Это pre-existing поведение, явно покрыто тестом test_agencyclients_add_payload_uses_top_level_fields. PR не вводит регрессию; рефакторинг payload вне скоупа.

#3 (SelectionCriteria теперь всегда отправляется) — SKIP. Сам бот пометил это как Informational и не настаивает; это намеренное приведение в соответствие с WSDL-контрактом, README обновлён, дефолт --archived NO явно задокументирован.

@claude review. 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 finished @axisrow's task in 3m 6s —— View job

Code Review — PR #113

• Gather context from prior reviews and triage notes
• Read current state of changed files
• Analyze for bugs, security issues, logic errors, data loss risks
• Post findings

Findings (current branch, post 74f7972 + af74344)

All medium-severity issues from the two prior rounds are correctly fixed. The one remaining item worth flagging:

get_first_leads_turbopage_id — primary discovery path skips the non-empty guard (test_integration.py:94–96)

turbo_page_id = get_first_turbopage_id()
if turbo_page_id:
    return turbo_page_id          # ← returned without probing leads

The ID-0 fallback probe is now correctly guarded (isinstance(data, list) and data), but the primary path — using the first turbo page from turbopages.get — returns that ID unconditionally without checking whether leads.get --turbo-page-ids <id> yields any data. If the account has turbo pages but no leads, test_get_leads calls leads get --turbo-page-ids <id>, gets [], and assert_success passes because [] is valid JSON. The test exercises the CLI wiring but asserts nothing about actual lead data.

This is an inherent test-environment limitation (you can't guard against zero leads without live data), so it doesn't block the PR — but it's the same vacuous-pass pattern that was fixed for the probe path and the advideo probe in the prior two rounds, just not fully closed on the primary path.

Fix if desired:

turbo_page_id = get_first_turbopage_id()
if turbo_page_id:
    probe = invoke_get("leads", "get", "--turbo-page-ids", str(turbo_page_id), "--limit", "1", "--format", "json")
    probe_data = parse_json_output(probe)
    if isinstance(probe_data, list) and probe_data:
        return turbo_page_id

Fix this →

Summary

No new medium or high severity issues. All previous critical findings are correctly resolved.

Triage третьего ревью bot'а:

#1 (primary turbo-page discovery without leads probe) — SKIP. Бот сам отмечает это как Low с явной оговоркой "doesn't block the PR". Принципиальное отличие от предыдущих: основной путь возвращает ID реально существующей turbo page, и API-запрос идёт корректно. Отсутствие leads у этой страницы — валидное состояние аккаунта; делать pre-flight probe тут означает превратить "тест на сетевую совместимость с реальным аккаунтом" в "тест зависит от состояния продакшен-данных", что хуже текущего поведения. Vacuous-pass паттерн в probe-fallback'ах был критичен, потому что они подавали заведомо несуществующий ID 0; primary path этим не страдает.