@claude @codex 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 #536 (live-write coverage + cassette refresh)

• Read tests/test_v5_live_write.py
• Inspect new cassettes (feeds, retargeting, strategies)
• Review refreshed cassettes for drift correctness
• Check tests/MANUAL_COVERAGE.md docs change
• Scan for secrets / real account IDs
• Verify error-handling & VCR replay semantics

Scoped to critical issues only (bugs, data-loss, logic, security), per the request. No blocking issues found. This is a clean, well-reasoned test-only PR. Details below.

✅ Verified correct

• Retargeting skip path is sound. The cassette returns HTTP 200 with an embedded {"Code":8800} error. output.format_output() → raise_for_api_result_errors() (direct_cli/output.py:42,74) raises DirectAPIResultError, which handle_api_errors converts to print_error + click.Abort() (exit code ≠ 0). So the guard r.exit_code != 0 and ("8800" in r.output or "not found" ...) (test_v5_live_write.py:1348) correctly triggers the skip rather than falling through to _extract_first_id (which would otherwise raise on the embedded Errors). Good — the exit_code != 0 check is meaningful here.
• Strategies cassette replay with duplicate requests works. The lifecycle issues two byte-identical archive requests (the in-try archive and the finally archive). With match_on=[..., "body"] and replay-only record_mode (conftest.py:295), VCR resolves each call to the next unplayed matching interaction — and the cassette records exactly 5 interactions (add, update, archive, unarchive, archive) in call order, so both archives replay. Correct.
• Refreshed cassettes match the new required-flag surface. ads update sends --type TEXT_AD (:572), feeds add sends --business-type RETAIL, campaigns add --type SMART_CAMPAIGN sends synthetic --counter-id 12345678. Consistent with the Аудит дублирования кода: системные дубли (~6000+ строк), незавершённая централизация хелперов #491 WSDL-parity gate.
• No secrets / no real account IDs. All cassettes redact authorization/client-login/cookie/Set-Cookie. The baked IDs (feed 3436439, strategy 708915404) are resource IDs of test-created drafts — same convention as existing campaign-ID cassettes — not credentials or account/login identifiers.
• retargeting rule shape is correct — ALL:12345:30 serializes to {"Operator":"ALL","Arguments":[{"ExternalId":12345,"MembershipLifeSpan":30}]}, and the MembershipLifeSpan-required note (else error 5000) is documented.

🟡 Non-blocking observations

1. Strategies cleanup doesn't fail loudly — inconsistent with the file's own safety contract. The module header (:8) states "cleanup fails loudly with the created ID if Yandex Direct refuses deletion", and _safe_delete_campaign honors that (pytest.fail on non-zero exit). But the strategies finally (:1424-1425) fires the cleanup archive and ignores its result:

   finally:
       # No strategies delete method exists; leave the strategy archived.
       _invoke_live("strategies", "archive", "--id", str(sid))

   On a live run (account with a shared wallet → add succeeds), if that final archive fails, the test leaves a non-archived, potentially serving public strategy behind with no signal. Since strategies have no delete API, this is the one resource that can leak in an active state. Consider checking the exit code and pytest.fail-ing with the strategy ID (matching the campaign-cleanup pattern) so a failed cleanup is loud rather than silent. Replay/CI is unaffected (cassette archive succeeds); this only matters on live re-recording. Fix this →

2. Documented, unavoidable side effect (FYI, not actionable). Because the API has no Strategies.delete, a live recording on a wallet-enabled account permanently creates a strategy that can only be archived — the recorded 708915404 is exactly this. This is correctly disclosed in MANUAL_COVERAGE.md and gated behind YANDEX_DIRECT_LIVE_WRITE=1. No spend risk since the strategy is never attached to a campaign. Flagging only so it's a conscious tradeoff.

Verdict

Solid. The skip guards, VCR matching, and cassette/CLI alignment all check out, and the docs accurately capture the API limitations. Only suggestion is tightening the strategies cleanup to fail loudly (#1) to match the module's stated contract — minor, and live-only.
· branch test/live-write-feeds-retargeting-strategies

@claude @codex review. Fixed the strategies cleanup in aa60653 — the finally block now checks the archive exit code and fails loudly with the strategy id, matching the feeds/retargeting cleanups and the module's safety contract. Please re-review. Focus on critical issues only: bugs, security, logical errors, data loss, performance. Do NOT nitpick style, naming, or formatting.

Claude finished @axisrow's task in 1m 21s —— View job

Re-review — PR #536 (strategies cleanup fix aa60653)

• Review the aa60653 cleanup fix
• Verify the finally block fails loudly with the strategy id
• Confirm replay/CI is unaffected
• Final verdict

The fix resolves the one non-blocking issue from the prior review. No remaining issues.

✅ Fix verified

The strategies finally now captures the cleanup-archive result and pytest.fails with the strategy id on a non-zero exit (test_v5_live_write.py:1423-1430):

finally:
    # No strategies delete method exists; archive is the only cleanup.
    r = _invoke_live("strategies", "archive", "--id", str(sid))
    if r.exit_code != 0:
        pytest.fail(
            f"Failed to archive strategy {sid}. "
            f"Manual cleanup required.\noutput: {r.output}"
        )

This is byte-for-byte the _safe_delete_campaign contract (:280-287) and matches the feeds (:1318-1323) and retargeting (:1369-1374) cleanups. The module's "cleanup fails loudly with the created ID" safety contract is now honored on every mutating resource — and since strategies is the one resource with no delete API, this is exactly where a silent leak would have been worst (a non-archived, potentially serving public shared strategy).

• sid is always bound in finally — it's assigned at :1405 before the try, after _assert_success confirms the add, so the cleanup can never NameError.
• Replay/CI unaffected. The fix adds an exit-code check on the already-existing cleanup archive call — no new request, so the cassette's interaction count is unchanged and still resolves both the in-try archive and the finally archive. CI confirms: Quality ✅ and API Coverage ✅ on this commit.
• Live-only behavior change. The new branch only fires on a genuine non-zero exit during live re-recording; the cassette replays a 0-exit archive, so existing replay runs are identical.

Verdict

Looks good — ship it. The fix is correct, minimal, consistent with the existing cleanup helpers, and changes nothing on the replay/CI path. Everything else from the first review still stands (sound skip guards, correct VCR matching, no secrets/real account IDs, accurate MANUAL_COVERAGE.md notes).
· branch test/live-write-feeds-retargeting-strategies

Codex Review: Didn't find any major issues. 🎉