test: live-write test matrix — phases 1-5 + manual coverage docs (#59)

[axisrow]

Summary

Consolidates PRs #60–#64 (closed) into a single change to avoid merge conflicts from overlapping shared helpers.

Phase 1 — Read-only integration tests (test_integration.py):

• TestReadOnlyLeads, TestReadOnlyTurbopages, TestReadOnlyBusinesses, TestReadOnlyAdVideos, TestReadOnlyAgencyClients

Phase 2 — Standalone draft asset tests (sitelinks, adimages, advideos, creatives)

Phase 3 — Nested draft campaign tests (adgroups, ads, keywords, bids, keywordbids, audiencetargets)

Phase 4 — Non-standard campaign types (dynamicads, smartadtargets — sandbox category A/B)

Phase 5 — Suspend/resume/archive smoke tests (keywords, audiencetargets, dynamicads, smartadtargets, ads)

Phase 7 — Manual coverage docs (tests/MANUAL_COVERAGE.md):
Documents commands that cannot be automated (ads moderate, financial operations, agencyclients, advideos/creatives requiring real video URL).

Key design decisions

• All helpers defined once (no duplicates across phases)
• _PNG_B64_450X450 — verified valid 450×450 PNG (1487 bytes)
• _assert_draft_or_success — graceful skip when API rejects suspend/resume on DRAFT objects
• _safe_delete_campaign — pytest.fail on cleanup errors (no silent leaks)
• advideos test documented as partial coverage (example.com URL rejected by Yandex — see MANUAL_COVERAGE.md)

Test plan

• pytest tests/test_comprehensive.py tests/test_cli.py tests/test_dry_run.py — 125 passed
• black + flake8 — clean
• git diff main — no duplicate helpers
• pytest -m integration -v — manual (requires token)
• YANDEX_DIRECT_LIVE_WRITE=1 pytest -m integration_live_write --record-mode=rewrite -v — manual

Closes #59
Supersedes #60, #61, #62, #63, #64

🤖 Generated with Claude Code

[axisrow]

@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]

Claude finished @axisrow's task in 3m 9s —— View job

---

Code Review — PR #65

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

---

Critical Issues

---

1. _is_draft_state_error masks real API failures — test reliability bug

tests/test_integration_live_write.py:94-276

_DRAFT_STATE_PATTERNS = (
    "Invalid object status",
    "is draft",
    "has not been saved",
    "DRAFT",
    "cannot be suspended",
    "cannot be resumed",
    "Operation is not available",   # ← dangerously generic
)

def _is_draft_state_error(output: str) -> bool:
    return any(p.lower() in output.lower() for p in _DRAFT_STATE_PATTERNS)

Two entries are over-broad:

• "Operation is not available" — this exact phrase appears in Yandex Direct quota errors, permission errors (ACCESS_DENIED), and maintenance windows. Any suspend/resume test on an underpowered account or during planned API downtime will silently pytest.skip() instead of failing. You won't know the test is broken.

• "DRAFT" → "draft" after .lower() — the campaign name is "direct-cli-live-draft-test-cassette". If a suspend/resume error message echoes back the targeted object's campaign name (common in nested-resource errors), the word "draft" in the campaign name will trigger a false skip.

_assert_draft_or_success calls pytest.skip() on either match, so both patterns actively hide real failures in every Phase 5 test.

Fix: Replace "Operation is not available" with the actual Yandex Direct error codes for draft-state rejections (e.g. "8800", "4006"), or use full phrases that can't appear in object names. Fix this →

---

2. adimages get --limit 1 doesn't verify the just-uploaded image — logic bug

tests/test_integration_live_write.py:406-424

r = _invoke_live(
    "adimages", "get",
    "--fields", "AdImageHash,Name",
    "--limit", "1",       # ← returns ANY 1 image in the account
    "--format", "json",
)
hashes_in_response = {img.get("AdImageHash") for img in ...}
assert img_hash in hashes_in_response   # ← will fail if account has other images

--limit 1 with no filter returns any single image. If the account has pre-existing images, the response won't contain the just-uploaded image, and the assertion fails even though the upload succeeded. The test would fail spuriously on any non-empty account.

Fix: Pass --ids (or add a hash filter) to retrieve the specific uploaded image, or remove the --limit 1 to retrieve all images. Fix this →

---

3. Missing cleanup in test_live_draft_advideos_add_get and test_live_draft_creatives_chain_advideo_to_creative — resource leak

tests/test_integration_live_write.py:435-483

if r.exit_code != 0:
    pytest.skip(...)          # expected path — no resource created
video_id = _extract_first_id(r.output)
# NO try/finally below — if advideos get fails, video_id is never deleted
r = _invoke_live("advideos", "get", "--ids", str(video_id), "--format", "json")
_assert_success(r, "advideos get")

The PR correctly notes example.com will be rejected, so in practice no resource is created. But the advideos resource has no CLI delete subcommand, so if the add ever succeeds (changed test account, API policy change), there is no recovery path in the code or in MANUAL_COVERAGE.md.

This is acceptable as a documented limitation given the CLI doesn't expose delete for videos, but MANUAL_COVERAGE.md should explicitly state "no cleanup possible via CLI" for this case.

---

4. keywords update is a breaking CLI change with no changelog notice

direct_cli/commands/keywords.py:135-171

The update command drops --bid, --context-bid, and --status options and replaces them with --keyword, --user-param-1, --user-param-2. The API semantics are correct (those fields don't belong in keywords/update), but:

• Any user or script calling direct keywords update --bid 15.0 --id 123 will get no such option: --bid with no actionable hint.
• The fix direction (bids set) is documented in the new docstring, but only for users who read help text.

Fix: At minimum, note this in the PR description. Consider adding a UsageError shim for --bid that says "--bid is no longer accepted on keywords update; use: direct bids set --keyword-id ID --bid VALUE" to avoid silent breakage for existing users. Fix this →

---

Non-blocking Observations

• _assert_success (line 145) vs _assert_success (test_integration.py line 54): The integration test version validates JSON output; the live-write version does not. Both are intentional for their context but the naming inconsistency could mislead future contributors.

• test_live_draft_smartadtargets_* feed cleanup on pytest.skip(): Initially looks like a leak, but finally blocks DO run on pytest.skip.Exception (a BaseException), so fid cleanup is correct.

• _assert_created_campaign_is_draft double assertion (line 218): The assert status == "DRAFT" or state == "OFF" followed by the individual if ... assert checks is redundant but harmless.

[axisrow]

@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]

Claude encountered an error —— View job

---

I'll analyze this and get back to you.

[axisrow]

@claude please review this PR again. Previous attempt errored out.

[claude]

Claude encountered an error —— View job

---

I'll analyze this and get back to you.

[claude]

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

---

Code Review — PR #65

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

---

Status: Previous review issues addressed

Compared to the previous review, three of four flagged issues are resolved in the current branch:

• "DRAFT" removed from _DRAFT_STATE_PATTERNS; "Operation is not available" narrowed to "Operation is not available for object" ✓
• adimages get --limit 1 removed — the get now fetches all images and scans for the uploaded hash ✓
• keywords update deprecation shim added with UsageError + migration hint ✓

---

Critical Issues

---

1. _deprecated_bid_option generates a misleading migration message for --status

direct_cli/commands/keywords.py:135-159

The callback uses param.name for both the flag shown in the error AND in the suggested replacement command:

raise click.UsageError(
    f"--{param.name} is no longer accepted on 'keywords update'; "
    f"use: direct bids set --keyword-id ID --{param.name} VALUE"
)

Two problems:

a) Wrong flag name for --context-bid: Click normalizes option names — param.name for --context-bid is context_bid (underscore, not hyphen). The error message tells users --context_bid is no longer accepted, which is not the flag they typed.

b) Non-existent redirect for --status: The message directs users to direct bids set --keyword-id ID --status VALUE, but bids set has no --status option. Users will fix one error and hit another with no actionable hint.

Fix: use param.opts[0] (returns "--context-bid") and write separate messages per removed option rather than a generic template that misfires on --status. Fix this →

---

2. _DRAFT_STATE_PATTERNS — remaining broad patterns (partially addressed)

tests/test_integration_live_write.py:94-101

Previous review flagged "DRAFT" (false-match on campaign name) and "Operation is not available" (too generic). Both were addressed. The remaining concern:

"cannot be suspended" and "cannot be resumed" are still generic enough that a Yandex Direct access-control or quota error containing one of these phrases would cause pytest.skip() instead of a test failure. These are low-risk in practice (Yandex Direct tends to use error codes like 8800 / 4006 for permission failures), but "cannot be suspended" in particular appears in generic permission-denied messages in some Yandex API error bodies.

Not a blocker if the test account is stable, but worth a note that these patterns were intentionally left broader than error codes to avoid brittleness to API error message changes. Fix this →

---

Non-blocking Observations

3. Redundant compound assertion in _assert_created_campaign_is_draft (test_integration_live_write.py:228):
The assert status == "DRAFT" or state == "OFF" is fully redundant — the two if blocks below it already check each field individually and produce clearer error output. The compound form could also fire confusingly if neither Status nor State comes back in the API response (both None → assertion fails before the individual checks run). Harmless in practice since the get call explicitly requests both fields.

4. Silent cleanup failures for nested resource deletes (e.g., test_integration_live_write.py:519, 568):
adgroups delete and feeds delete in finally blocks don't check return codes. Campaign cleanup uses pytest.fail (correct), but adgroup/feed leaks will be silent. Low risk since cascade-deleting the campaign likely removes nested objects, but inconsistent with the stated "cleanup fails loudly" safety contract in the module docstring.

---

Summary

One bug to fix before merge: the _deprecated_bid_option callback produces wrong option names (underscores vs hyphens) and a broken redirect message for --status. Everything else is either already fixed from the previous review, documented as acceptable, or non-blocking.

[axisrow]

@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]

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

---

Code Review — PR #65

• Gather context and read changed files
• Check status of previously flagged issues
• Analyze for new critical issues
• Post findings

---

Status: All previous critical issues resolved

All four issues from Review 1 and the one critical issue from Review 2 are now fixed:

• "DRAFT" removed from _DRAFT_STATE_PATTERNS ✓
• "Operation is not available" → "Operation is not available for object" ✓
• adimages get --limit 1 removed ✓
• keywords update deprecation shim added ✓
• _deprecated_bid_option: "context_bid" dict key maps to param.name, message string uses "--context-bid" (correct hyphen) ✓
• --status redirect no longer points to bids set — says "status is not mutable via the keywords API" ✓

---

One remaining critical issue

adimages get hash check is still broken for dict-shaped responses

tests/test_integration_live_write.py:415-421

data = json.loads(r.output)
hashes_in_response = {
    img.get("AdImageHash") for img in (data if isinstance(data, list) else [])
}
assert img_hash in hashes_in_response

The --limit 1 fix was applied, but the response parsing was not updated to handle the tapi-yandex-direct dict shape. When adimages get returns {"result": {"AdImages": [...]}} (the same dict shape _extract_campaigns explicitly handles), data if isinstance(data, list) else [] produces [], making hashes_in_response an empty set. The assertion then fails with "Uploaded image hash X not found in get response" even though the upload succeeded.

In the cassette, the test always skips (5004 error), so this only surfaces on accounts where image upload succeeds during a live run. Fix mirrors _extract_campaigns:

data = json.loads(r.output)
if isinstance(data, list):
    images = data
else:
    result_data = data.get("result", data)
    images = result_data.get("AdImages", [])
hashes_in_response = {img.get("AdImageHash") for img in images}
assert img_hash in hashes_in_response, ...

Fix this →

---

Non-blocking observations

Silent cleanup failures for nested resources — finally blocks for adgroups delete and feeds delete don't check exit codes (tests/test_integration_live_write.py:519, 568, 966, 1201). Campaign cleanup uses pytest.fail (correct), but adgroup/feed leaks will be silent. Inconsistent with the module-level "cleanup fails loudly" contract in the docstring. Low practical risk since campaign deletion likely cascades.

Redundant compound assertion (test_integration_live_write.py:226) — assert status == "DRAFT" or state == "OFF" is weaker than the two individual if ... assert checks below it and never triggers independently. Harmless.

---

Summary

One bug to fix before merge: the adimages get hash verification uses incomplete response parsing that makes the assertion always fail on accounts where image upload succeeds. Everything else from previous reviews is resolved.

[axisrow]

@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]

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

---

Code Review — PR #65

• Gather context and read changed files
• Check status of previously flagged issues
• Analyze for new critical issues
• Post findings

---

Status: All previous critical issues resolved

Every issue flagged in Reviews 1–3 is now fixed:

• "DRAFT" removed from _DRAFT_STATE_PATTERNS ✓
• "Operation is not available" → "Operation is not available for object" ✓
• adimages get --limit 1 removed; response parsing handles dict shape ✓
• keywords update deprecation shim added ✓
• _deprecated_bid_option dict keyed on param.name ("context_bid"), message uses "--context-bid" with hyphen ✓
• --status message does not redirect to bids set ✓

---

One remaining critical issue

dynamicads get response parsing — same dict-shape bug as the just-fixed adimages

tests/test_integration_live_write.py:863–867

data = json.loads(r.output)
targets = data if isinstance(data, list) else data.get("result", [])
assert any(
    t.get("Id") == did for t in targets
), f"Dynamic ad target {did} not found in get response"

When the API returns {"result": {"DynamicTextAdTargets": [...]}} (the standard tapi-yandex-direct dict shape, same as adimages), data.get("result", []) returns the inner dict {"DynamicTextAdTargets": [...]}. Iterating over that dict yields its string keys, so t.get("Id") raises AttributeError on the string "DynamicTextAdTargets".

This is the same class of bug that was fixed for adimages at lines 415–423. The cassette never exercises this path (campaign creation returns 3500 and skips), so it only surfaces in live runs on accounts that support DYNAMIC_TEXT_CAMPAIGN.

Fix mirrors the adimages fix:

data = json.loads(r.output)
if isinstance(data, list):
    targets = data
else:
    result_data = data.get("result", data)
    targets = result_data.get("DynamicTextAdTargets", [])
assert any(
    t.get("Id") == did for t in targets
), f"Dynamic ad target {did} not found in get response"

Fix this →

---

Non-blocking observations

Silent cleanup failures for adgroups delete and feeds delete — finally blocks on lines 524, 573–575, 627–629, 670–672, 720–722, 784–788, 870–872, 966–968, 1002–1003, 1050–1053, 1120–1123, 1200–1205 don't check exit codes. Campaign cleanup uses pytest.fail (correct), but adgroup/feed leaks are silent. Low practical risk since campaign deletion likely cascades, but inconsistent with the docstring's "cleanup fails loudly" contract.

Redundant compound assertion (test_integration_live_write.py:226) — assert status == "DRAFT" or state == "OFF" is weaker than the two individual if ... assert checks below it. Harmless in practice.

---

Summary

One bug to fix before merge: dynamicads get parsing breaks with AttributeError on live accounts that support DYNAMIC_TEXT_CAMPAIGN. The fix is a one-liner mirror of the adimages fix. Everything else from all three previous reviews is resolved.

[axisrow]

@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]

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

---

Code Review — PR #65

• Gather context and read changed files
• Check status of previously flagged issues
• Analyze for new critical issues
• Post findings

---

Status: All critical issues resolved — PR is clean

Every issue flagged across all four previous reviews is now fixed in the current branch:

Issue	Status
"DRAFT" in _DRAFT_STATE_PATTERNS matching campaign name	✓ Removed
"Operation is not available" too broad → swallowed quota/permission errors	✓ Narrowed to "Operation is not available for object"
adimages get --limit 1 — spurious failure on non-empty accounts	✓ Removed; full list fetch now scans for uploaded hash
adimages get dict-shape response not unwrapped → empty hashes_in_response	✓ Fixed with result_data.get("AdImages", [])
dynamicads get data.get("result", []) returns inner dict → AttributeError on string keys	✓ Fixed in latest commit (3986917) with result_data.get("DynamicTextAdTargets", [])
keywords update dropped --bid/--context-bid/--status silently	✓ Deprecation shim added with migration messages
_deprecated_bid_option used param.name (underscored) in error string	✓ Dict keyed on param.name; message strings hardcode correct hyphenated flag names
--status error message redirected to bids set --status (non-existent option)	✓ Message says "status is not mutable via the keywords API"

---

No new critical issues found

The codebase is in good shape. The two non-blocking observations carried over from previous reviews remain unfixed but are genuinely harmless:

• Redundant compound assertion (test_integration_live_write.py:226): assert status == "DRAFT" or state == "OFF" is weaker than the individual if ... assert checks below it. A new draft campaign always returns both fields, so this has no practical impact.
• Silent cleanup failures for adgroups delete / feeds delete / retargeting delete in finally blocks: exit codes not checked (unlike _safe_delete_campaign). Low risk since campaign cascade-delete covers most of these; inconsistent with the module docstring's "cleanup fails loudly" contract but not a correctness bug.

---

Ready to merge.