@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 1m 53s —— View job

Review: PR #462 — v4 deferred methods

• Read the diff vs origin/main
• Review v4adimage.py (get/set, association parsing)
• Review v4keywords.py (get-suggestion)
• Review v4finance.py (pay-campaigns-by-card, dry-run-only)
• Check registries/gates (smoke_matrix, api_coverage, EXPECTED_COMMANDS)
• Post review findings

No critical bugs, security vulnerabilities, or data-loss risks found. The implementation is consistent with the existing v4 Live patterns and well guarded. Summary below.

What holds up well

• pay-campaigns-by-card is correctly fenced off. _require_dry_run(dry_run) blocks any live send, the command is in DANGEROUS in smoke_matrix, excluded in api_coverage_payloads, and the contract marks it PARAM_UNDOCUMENTED / live_probe_allowed=False. The financial token is masked in the dry-run body via _masked_finance_body. This matches the stated "live path stays closed" intent.
• Money parsing is safe. parse_v4_money_sum rejects non-positive, non-finite, and malformed amounts before the body is built (direct_cli/v4/money.py:16).
• v4adimage set validation is solid — non-integer / non-positive / duplicate AdID all raise UsageError, the 10000 cap is enforced, and attach/detach semantics (omit AdImageHash → detach) match the documented contract (v4adimage.py:62).
• Field names match the contracts exactly (AdIDS, CampaignIDS, StatusAdImageModerate, etc. vs. v4_contracts.py:526), and all registries (cli.py, EXPECTED_COMMANDS, smoke_matrix, api_coverage_payloads) are updated consistently. Tests cover dry-run shapes, error paths, and contract wiring.

Minor, non-blocking notes (not critical — flagging for awareness only)

1. scripts/probe_drift_urls.sh:39 hard-codes https://yandex.ru/dev/direct/doc/... URL string literals. This sits adjacent to the CLAUDE.md "No URL literals outside the registry" convention (reports drift checker: hard-coded URLs retired by Yandex (type / fields-list / period / spec.html) #426). It's a manual-only diagnostic (not wired into CI) probing candidate paths that by definition aren't in the registry, so it doesn't break anything — but it's the same pattern that caused the captcha-cache poisoning, so worth a deliberate exception comment or keeping an eye on.
2. Stale contract note: the AdImageAssociation contract notes still say "No CLI command is exposed yet; this row only records the contract." (v4_contracts.py:558) — now inaccurate since this PR exposes v4adimage get/set. Cosmetic only.

Neither note affects functionality. From a correctness/security/data-loss standpoint this PR is in good shape.
· feat/v4-deferred-methods

@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 31s —— View job

Review: PR #462 — v4 deferred methods (focused: bugs / security / data-loss / perf)

• Read the diff vs origin/main
• Review v4adimage.py (get/set, association parsing)
• Review v4keywords.py (get-suggestion)
• Review v4finance.py (pay-campaigns-by-card)
• Check registries/gates & contracts
• Post review findings

No critical bugs, security vulnerabilities, or data-loss risks found. This is a clean, well-fenced implementation. Details below.

Correctness / safety — holds up

• pay-campaigns-by-card cannot send live. _require_dry_run is the first statement (v4finance.py:711); it's in DANGEROUS (smoke_matrix.py), excluded from coverage payloads, and the contract is live_probe_allowed=False. The token is masked via _masked_finance_body (v4finance.py:119). Live path is genuinely closed.
• No token leakage. All finance dry-runs route through _masked_finance_body, which overwrites finance_token with <redacted> after build_v4_body. v4adimage/v4keywords carry no token at all.
• v4adimage set validation is robust (v4adimage.py:62): non-integer / non-positive / duplicate AdID all raise UsageError, the 10000 cap is enforced, and AD_ID=HASH splits with split("=", 1) so base64 hashes containing = padding are preserved correctly. Omitting the hash → detach matches the documented contract.
• Field names match the contract exactly (AdIDS, CampaignIDS, StatusAdImageModerate; v4_contracts.py:526), and the param shape is documented (PARAM_OBJECT), so call_v4's fail-closed shape guard (v4/__init__.py:42) is satisfied.
• Per-subcommand safety split is right: v4adimage.get (read) → SAFE, v4adimage.set (mutation) → DANGEROUS. command_category keys on the subcommand, so the mutating half stays out of any auto-runner.
• Registries are consistent — cli.py, EXPECTED_COMMANDS, smoke_matrix, api_coverage_payloads, and the contract notes are all updated together.

Minor, non-blocking (awareness only — not critical)

1. Broad live read via SAFE smoke. v4adimage.get is in the SAFE list and, with no filters, sends {"Action":"Get","SelectionCriteria":{}} — i.e. up to 10000 associations per the docs. It's strictly read-only so there's no harm, just a larger-than-typical production read when the safe-smoke script runs. Worth knowing; not a defect.
2. Empty-token CSV edge. In _get_param, a degenerate value like --logins "," passes the if logins: truthiness check but parse_csv_strings(",") returns None, so SelectionCriteria["Logins"] is set to null. Read-only and harmless; only flagging for completeness.
3. scripts/probe_drift_urls.sh URL literals (carried from the prior review) — manual-only diagnostic, not wired into CI, so it doesn't break the "no URL literals outside the registry" invariant, but it's the same pattern behind reports drift checker: hard-coded URLs retired by Yandex (type / fields-list / period / spec.html) #426. A one-line exception comment would make the intent explicit.

From a correctness, security, and data-loss standpoint this PR is in good shape to merge. I was unable to execute the test suite in this environment (pytest invocation not permitted by the sandbox), so I relied on static review — the PR reports the full suite green (2033 passed, 47 skipped).
· feat/v4-deferred-methods

@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 1m 25s —— View job

Review: PR #462 — v4 deferred methods (focused: bugs / security / data-loss / perf)

• Read the diff vs origin/main
• Review v4adimage.py (get/set, association parsing)
• Review v4keywords.py (get-suggestion)
• Review v4finance.py (pay-campaigns-by-card)
• Check registries/gates & contracts
• Post review findings

No critical bugs, security vulnerabilities, or data-loss risks found. This is the third pass and the implementation has only tightened since the prior reviews. Details below.

Correctness / safety — verified

• pay-campaigns-by-card is genuinely closed to live. _require_dry_run(dry_run) is the first statement (v4finance.py:702); the contract is PARAM_UNDOCUMENTED + live_probe_allowed=False (v4_contracts.py:127). Even if the dry-run guard were bypassed, call_v4's fail-closed guard would still refuse it — an undocumented param shape at SAFETY_DANGEROUS raises UsageError rather than posting (v4/__init__.py:43-49). Belt and suspenders. It's also in DANGEROUS (smoke_matrix.py:172) and excluded from coverage payloads.
• No finance-token leakage. The dry-run routes through _masked_finance_body, which overwrites finance_token with <redacted> after build_v4_body (v4finance.py:119-124). v4adimage/v4keywords carry no token.
• v4adimage set validation is robust (v4adimage.py:70-108): non-integer / non-positive / duplicate AdID all raise UsageError; the 10000 cap is enforced; AD_ID=HASH uses split("=", 1) so base64 = padding in the hash is preserved; omitting the hash → detach matches the contract.
• Prior CSV-null edge is fixed. The earlier review noted --logins "," could write "Logins": null. Commit bdb5a7e now gates every SelectionCriteria key on a truthiness check (v4adimage.py:41-62), so degenerate CSV omits the key entirely; malformed ID lists still raise UsageError via parse_ids. Two edge-case tests added.
• get-suggestion input is clean — empty/whitespace seeds are stripped and an all-empty set raises UsageError (v4keywords.py:20-23).
• Money parsing stays safe — parse_v4_money_sum rejects non-positive / non-finite / malformed amounts before the body is built; _campaign_ids_param rejects non-positive IDs.
• Registries consistent — cli.py, EXPECTED_COMMANDS, smoke_matrix, api_coverage_payloads, and the now-corrected AdImageAssociation contract note are all updated together. Read/write safety split is right (v4adimage.get→SAFE, .set→DANGEROUS).

Minor, non-blocking (awareness only)

1. pay-campaigns-by-card dry-run omits ContractID/PayMethod that the documented PayCampaigns shape carries (v4finance.py:734-742). The docstring acknowledges this is a best-effort preview of an undocumented method, and the live path is closed, so it's harmless — just noting the dry-run body is not a complete mirror.
2. scripts/probe_drift_urls.sh URL literals (carried from prior reviews) — manual-only diagnostic, not wired into CI, so it doesn't violate the "no URL literals outside the registry" invariant, but it's the same pattern behind reports drift checker: hard-coded URLs retired by Yandex (type / fields-list / period / spec.html) #426. A one-line exception comment would make intent explicit.

From a correctness, security, and data-loss standpoint this PR is in good shape to merge. I relied on static review — the PR reports the full suite green (2033 passed, 47 skipped); I was unable to execute pytest in this environment.
· feat/v4-deferred-methods

Спасибо за третий проход. Триаж двух non-blocking замечаний:

1. pay-campaigns-by-card dry-run опускает ContractID/PayMethod — SKIP (намеренно). Метод PARAM_UNDOCUMENTED; по contract-source rule мы не достраиваем недокументированные поля. Dry-run явно помечен в docstring как best-effort превью, а live-путь закрыт (_require_dry_run + fail-closed guard). Добавлять поля «по аналогии с PayCampaigns» было бы выдумыванием wire-shape без источника.

2. probe_drift_urls.sh URL-литералы — ALREADY_FIXED в коммите e5c42ca. Explicit-exception комментарий уже в файле (строки 10–14): литералы допустимы, потому что скрипт пробирует кандидаты путей, которых по определению нет в реестре, выполняет read-only HTTP-пробы и никогда не пишет в кэш доков, поэтому отравить его не может. Diff vs origin/main показывает литералы, но комментарий рядом с ними уже объясняет исключение.

Ни одного FIX-вердикта — перехожу к проверке CI и мержу.