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

PR Review: sandbox integration tests for write commands

Tasks:

• Read request and classify
• Read changed files (conftest.py, test_integration_write.py, ads.py, feeds.py)
• Analyze for bugs, security issues, logical errors, data loss risks
• Post review findings

Production code changes

ads.py — create_client moved after dry-run check ✅
Correct fix. Previously ads get --dry-run would fail if no token was set, even though dry-run never calls the API. No issues.

feeds.py — hardcoded SourceType: "URL" ✅
This is a required field fix. Since feed_data.update(extra) runs after the default is set (line 79), passing --json '{"SourceType": "YML"}' correctly overrides it. No issue.

Critical issues in test code

1. _fixture_invoke silently skips on any error — masks real CLI regressions

conftest.py:103-107:

def _fixture_invoke(*args, label="fixture"):
    result = _invoke(*args)
    if result.exit_code != 0:
        pytest.skip(f"{label} failed in sandbox: {result.output[:200]}")
    return result

This converts any fixture failure (including CLI bugs) into a skip. If adgroups add or ads add regresses due to a real code bug, the tests that depend on those fixtures (TestWriteAds, TestWriteKeywords, TestWriteDynamicAds, TestWriteSmartAdTargets) silently skip rather than fail. The regression goes undetected — you'd only notice by monitoring that the skip count increased.

The campaigns lifecycle test (TestWriteCampaigns) does assert correctly, so campaigns add regressions are caught. But the adgroup/ad/keyword chain has no such protection.

Fix: distinguish between known sandbox limitation errors (specific error codes/messages) and unexpected failures. Skip only for known sandbox errors; fail on unexpected ones. Fix this →

2. turbopages test — xfail + pytest.skip() is logically broken

test_integration_write.py:554-568:

@pytest.mark.xfail(reason="BUG: sandbox loops on turbopages add (HTTP 202 → timeout)", strict=False)
def test_add(self, unique_suffix):
    r = _invoke(...)
    if r.exit_code == 0:
        ...
    else:
        pytest.skip(f"sandbox may not support turbo pages: {r.output[:200]}")

pytest.skip() inside an xfail-marked test causes the test to be reported as skipped, not xfailed. The xfail marker never actually triggers — every run either passes or skips. If the bug is ever fixed (sandbox stops returning HTTP 202), the xfail mark just becomes dead weight with no signal. Either remove the xfail and just skip unconditionally, or replace pytest.skip() with an actual assertion that raises so xfail can do its job. Fix this →

Minor (not blocking)

• Duplicate conftest import (test_integration_write.py:31-41): sys.path.insert + manual from conftest import ... is unnecessary — pytest auto-discovers conftest. It works in practice, but if conftest is ever patched/mocked in a test session, the two module copies could diverge. Low risk but worth cleaning up.
• TOKEN imported but unused in test_integration_write.py (flake8 F401 — would fail flake8 tests if run in CI).

Re: Duplicate conftest import (minor)

Verdict: HALLUCINATION — the claim is factually incorrect.

pytest auto-discovers fixtures from conftest.py, but does NOT auto-import regular functions, module-level variables, or decorators. Of the 7 symbols imported:

• TOKEN — module-level variable
• _invoke — plain function
• assert_success — plain function
• parse_add_result — plain function
• parse_first_result — plain function
• skip_if_no_token — pytest.mark.skipif marker
• tomorrow — plain function

None are decorated with @pytest.fixture. The sys.path.insert + manual import is the only way to access them. Removing it would break the test file.

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

Tasks:

• Read request and classify
• Read changed files (conftest.py, test_integration_write.py, ads.py, feeds.py)
• Analyze for bugs, security issues, logical errors, data loss risks
• Post review findings

PR Review: sandbox integration tests for write commands

Note on previous review's minor finding: Confirmed the sys.path.insert + manual from conftest import ... is necessary — pytest auto-discovers fixtures but not plain functions/variables. The previous review's claim that this was unnecessary was incorrect.

Previously identified issues: now fixed ✅

Both critical issues from the prior review cycle are resolved in the current branch:

• _fixture_invoke now uses pytest.fail for non-sandbox errors — the fix at conftest.py:119-127 correctly distinguishes sandbox limitations from real CLI regressions. Good.
• turbopages test — now uses pytest.skip() directly at the top of test_add instead of the broken xfail + pytest.skip() combination. Good.

Remaining critical issue

1. "error_code" in _SANDBOX_ERROR_PATTERNS is dangerously broad — partially reverts the _fixture_invoke fix (conftest.py:108)

_SANDBOX_ERROR_PATTERNS = (
    "Object not found",
    "not found",          # also overly broad — see below
    "not supported",
    "Campaign not found",
    "Ad group not found",
    "not accessible",
    "error_code",         # <-- matches virtually any API error
)

The tapi-yandex-direct library raises exceptions that include the raw API error response in their string representation. API error responses contain "error_code" as a JSON field name. As a result, _is_sandbox_error() returns True for almost any API error — meaning _fixture_invoke skips instead of fails even when the underlying cause is a real CLI regression (e.g., wrong field name in the request body, missing required parameter).

Concrete example: if adgroups add is broken and sends a request the API rejects with {"error": {"error_code": 8800, "error_string": "..."}}, the exception would contain "error_code" → _is_sandbox_error returns True → pytest.skip. The regression is invisible.

Fix: Remove "error_code" from _SANDBOX_ERROR_PATTERNS. Also remove "not found" — it's a substring of the three specific patterns already in the list and adds no coverage while also matching non-sandbox rejections. Fix this →

Medium issue

2. _fixture_parse skips on API-level Errors in the response body (conftest.py:140-141)

def _fixture_parse(result):
    ...
    if "Errors" in first and first["Errors"]:
        pytest.skip(f"API rejected fixture resource: {first['Errors']}")  # should be pytest.fail

_fixture_invoke ensures the CLI exits 0. But if the CLI exits 0 while the API returned an error within the response body (e.g. {"AddResults": [{"Errors": [...]}]}), _fixture_parse silently skips. This masks bugs where the CLI sends malformed request data that the API rejects at the application level — exactly the category of bugs the Type-field regression tests are meant to catch.

pytest.skip here prevents the test from running, but the fixture succeeded from the CLI's perspective — the request was made. An API rejection in the response body means the CLI sent bad data, which is a CLI bug, not a sandbox limitation. Fix this →

Minor (not blocking)

• _skip_on_error is dead code (test_integration_write.py:43-48): defined but never called. The tests use inline if r.exit_code == 0 ... else pytest.skip(...) instead. Safe to remove.
• Optional imported but unused (conftest.py:13): from typing import Optional — flake8 F401.

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

Tasks:

• Read request and classify
• Read changed files (conftest.py, test_integration_write.py, ads.py, feeds.py)
• Analyze for critical issues
• Post review findings

PR Review: sandbox integration tests for write commands (round 3)

Production code (ads.py, feeds.py): No issues. Both changes are correct.

Previous critical issues: Both confirmed fixed — _fixture_parse now calls pytest.fail for non-sandbox API errors, and the turbopages test uses pytest.skip directly.

Critical issue

1. The two primary regression tests skip rather than fail on API rejection (test_integration_write.py:147-149, 491-493)

TestWriteAds and TestWriteSmartAdTargets are explicitly labeled "Critical regression guards for Type-field fixes from PR #12". But both tests call pytest.skip when the API returns errors in the response body — which is exactly what happens if the Type-field bug is reintroduced:

# TestWriteAds.test_add_text_ad_update_delete (line 147-149)
if "Errors" in first and first["Errors"]:
    # Sandbox didn't persist adgroup — not a CLI bug
    pytest.skip(f"adgroup not persisted in sandbox: {first['Errors']}")

# TestWriteSmartAdTargets.test_add_update_delete (line 491-493)
if "Errors" in first and first["Errors"]:
    pytest.skip(f"smartadtargets add rejected (sandbox): {first['Errors']}")

If ads add regresses and sends an invalid Type field, the API returns {"AddResults": [{"Errors": [...]}]}. The CLI exits 0 (it just formats output), assert_success passes, and then the test skips instead of fails. The regression guard is silent.

The fix already exists in _fixture_parse (line 139-143) — use _is_sandbox_error to distinguish sandbox limitations from CLI regressions:

if "Errors" in first and first["Errors"]:
    err_text = str(first["Errors"])
    if _is_sandbox_error(err_text):
        pytest.skip(f"adgroup not persisted in sandbox: {first['Errors']}")
    pytest.fail(f"API rejected ads add (potential Type-field regression): {first['Errors']}")

The same applies at test_integration_write.py:457-458 (TestWriteDynamicAds) and TestWriteKeywords line 183-185. Fix this →

Medium issue

2. "Invalid request" and "is omitted" are overly broad sandbox patterns (conftest.py:101-109)

These two patterns were added while "error_code" and "not found" were removed (previous cycle). They carry the same risk — both describe API errors that could result from CLI bugs:

• "Invalid request" — returned when the request doesn't conform to the API spec; a CLI bug sending malformed data would trigger this
• "is omitted" — matches "Required field X is omitted"; a CLI bug failing to send a required field would trigger this

In both cases, _is_sandbox_error returns True and _fixture_invoke skips instead of failing on what is actually a CLI regression. Fix this →

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

Tasks:

• Read request and classify
• Read changed files (conftest.py, test_integration_write.py)
• Analyze for critical issues (round 4)
• Post review findings

PR Review: sandbox integration tests for write commands (round 4)

Production code (ads.py, feeds.py): No issues.

Previous critical issues from rounds 1–3: All confirmed fixed — _fixture_invoke/_fixture_parse correctly distinguish sandbox errors from regressions; _SANDBOX_ERROR_PATTERNS is now specific; turbopages uses unconditional pytest.skip.

Critical issue

1. parse_first_result + manual error check = dead code in TestWriteSmartAdTargets and TestWriteDynamicAds

conftest.py:78-80 — parse_first_result already asserts no errors:

assert "Errors" not in first or not first["Errors"], (
    f"API rejected: {first.get('Errors')}"
)

In TestWriteSmartAdTargets (test_integration_write.py:501-507) and TestWriteDynamicAds (test_integration_write.py:465-470), parse_first_result(r) is called first, then an if "Errors" in first check follows — but that check is unreachable. If the API returns application-level errors, parse_first_result raises AssertionError before first is ever assigned:

# TestWriteSmartAdTargets
if r.exit_code == 0:
    first = parse_first_result(r)           # raises AssertionError if errors exist
    if "Errors" in first and first["Errors"]:  # ← dead code, never reached
        err_text = str(first["Errors"])
        if _is_sandbox_error(err_text):
            pytest.skip(...)                # ← never reached
        pytest.fail(...)                    # ← never reached

Consequence: any API-level error — whether a sandbox limitation or a real CLI regression — surfaces as a bare AssertionError (test FAIL) rather than going through the sandbox-discrimination logic. Sandbox limitations that should skip instead always fail, causing persistent CI noise. If sandbox reliably rejects smartadtargets, this critical regression guard will become a permanently failing test that gets ignored.

The fix pattern already exists in TestWriteAds (lines 142–152), which manually parses the JSON instead of calling parse_first_result. Apply the same approach to TestWriteSmartAdTargets and TestWriteDynamicAds. Fix this →

Medium issue

2. TestWriteFeeds unconditionally skips on non-zero exit — implicit regression guard is broken

test_integration_write.py:294-295:

if r.exit_code != 0:
    pytest.skip(f"feeds add failed (CLI may need SourceType): {r.output[:200]}")

The skip message literally describes the feeds.py SourceType regression scenario. If the SourceType: "URL" default is removed from feeds.py, feeds add would fail with exit_code != 0 (the API rejects the request) — and this test silently skips instead of failing. This is the same class of bug fixed in previous rounds for _fixture_invoke.

Fix: apply _is_sandbox_error discrimination here:

if r.exit_code != 0:
    if _is_sandbox_error(r.output):
        pytest.skip(f"feeds add not supported in sandbox: {r.output[:200]}")
    pytest.fail(f"feeds add failed (SourceType regression?): {r.output[:500]}")

Fix this →

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

Tasks:

• Read request and classify
• Read changed files (conftest.py, test_integration_write.py)
• Analyze for critical issues (round 5)
• Post review findings

PR Review: sandbox integration tests for write commands (round 5)

Production code (ads.py, feeds.py): No issues.

Previous critical issues (rounds 1–4): All confirmed fixed — _fixture_invoke/_fixture_parse use _is_sandbox_error discrimination; _SANDBOX_ERROR_PATTERNS is specific; turbopages uses unconditional pytest.skip; TestWriteDynamicAds and TestWriteSmartAdTargets use manual JSON parsing with sandbox discrimination; TestWriteFeeds applies _is_sandbox_error on non-zero exit.

Critical issue

1. except BaseException: in sandbox_retargeting_list swallows pytest.fail() (conftest.py:243-244)

@pytest.fixture
def sandbox_retargeting_list(unique_suffix):
    try:
        result = _fixture_invoke(...)   # may call pytest.fail() for regressions
        rtg_id = _fixture_parse(result) # may call pytest.fail() for API errors
    except BaseException:               # ← catches pytest.fail() too
        pytest.skip("sandbox cannot create retargeting lists (requires Metrica goals)")

pytest.fail() raises Failed, which is a subclass of Exception, which is a subclass of BaseException. The except BaseException: catches it and re-issues pytest.skip(). This completely defeats the regression detection added in rounds 1–3: any CLI regression in retargeting add that causes _fixture_invoke to call pytest.fail() is silently converted into a skip in TestWriteAudienceTargets.

Note that pytest.skip() also raises a BaseException subclass (Skipped). The outer except BaseException: does not re-catch it because the pytest.skip() call is inside the except block itself, not inside a new try.

The simplest fix is to remove the try/except entirely — _fixture_invoke already calls pytest.skip() for known sandbox limitations and pytest.fail() for regressions, so the outer guard is redundant and harmful:

@pytest.fixture
def sandbox_retargeting_list(unique_suffix):
    result = _fixture_invoke(
        "retargeting", "add",
        "--name", f"test-rtg-{unique_suffix}",
        "--type", "RETARGETING",
        "--json", json.dumps({"Rules": [{"Operator": "ALL", "Arguments": [{"ExternalId": 12345}]}]}),
        label="retargeting add",
    )
    rtg_id = _fixture_parse(result)
    yield rtg_id
    _safe_delete("retargeting", "delete", "--id", str(rtg_id))

If sandbox rejects retargeting add with a known-sandbox-limitation error (e.g. "not supported"), _fixture_invoke will call pytest.skip() which propagates correctly. If it's a CLI regression, _fixture_invoke calls pytest.fail() which also propagates correctly. Fix this →

Medium issue

2. TestWriteSmartAdTargets and TestWriteDynamicAds — skip (not fail) on CLI-level failure (test_integration_write.py:528-529, 487-488)

Both tests use the if r.exit_code == 0: ... else: pytest.skip(...) pattern. TestWriteAds (the other critical regression guard for PR #12) uses assert_success first — which calls pytest.fail on non-zero exit. The asymmetry means:

• If ads add fails at CLI level (e.g. argument parsing broken) → FAIL ✓
• If smartadtargets add or dynamicads add fail at CLI level → SKIP ✗

For TestWriteSmartAdTargets specifically, the --type VIEWED_PRODUCT argument is the regression being guarded. If that argument is removed or broken in the CLI, the invocation would exit non-zero — and the test would skip rather than fail.

Fix: apply _is_sandbox_error discrimination on the else branch of both tests, mirroring the approach used in TestWriteFeeds (lines 294-297):

else:
    if _is_sandbox_error(r.output):
        pytest.skip(f"smartadtargets add not supported (sandbox): {r.output[:200]}")
    pytest.fail(f"smartadtargets add failed (CLI regression?): {r.output[:500]}")

Fix this →

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

Tasks:

• Read request and classify
• Read current state of changed files (conftest.py, test_integration_write.py, ads.py, feeds.py)
• Verify round 5 fixes applied
• Analyze for new critical issues
• Post review findings

PR Review: sandbox integration tests for write commands (round 6)

Production code (ads.py, feeds.py): No issues. Both changes remain correct.

Previous critical issues (rounds 1–5): All confirmed fixed.

Specifically, round 5's two issues are resolved:

• sandbox_retargeting_list no longer has except BaseException: — _fixture_invoke and _fixture_parse propagate correctly now
• TestWriteDynamicAds and TestWriteSmartAdTargets both apply _is_sandbox_error discrimination in the else branch ✅

No new critical issues

The stated primary goals of this PR are met:

• Type-field regression guards (TestWriteAds, TestWriteSmartAdTargets): correctly use manual JSON parsing + _is_sandbox_error discrimination, and pytest.fail for genuine API rejections
• SourceType regression guard (TestWriteFeeds): correctly applies _is_sandbox_error discrimination on non-zero exit
• _fixture_invoke/_fixture_parse: correctly distinguish sandbox limitations from regressions across the fixture chain

Medium issues (not blocking on critical regression guards)

1. _SANDBOX_ERROR_PATTERNS still contains two potentially broad patterns (conftest.py:107-108)

_SANDBOX_ERROR_PATTERNS = (
    "Object not found",
    "not supported",
    "Campaign not found",
    "Ad group not found",
    "not accessible",
    "unknown parameter",   # ← same class as "error_code" from round 2
    "required field",      # ← same class as "is omitted" from round 3
)

These were added to handle the adextensions sandbox rejection ("unknown parameter") but apply to all commands via _fixture_invoke. If campaigns add or adgroups add regresses by adding a stray parameter or dropping a required field, the fixture chain skips rather than fails. This is the same pattern flagged and fixed multiple times in rounds 2–3 for "error_code", "not found", "Invalid request", and "is omitted". Fix this →

2. Eight secondary tests skip unconditionally on non-zero exit (test_integration_write.py:220, 239, 328, 348, 368, 391, 416, 447)

TestWriteRetargeting, TestWriteBids, TestWriteKeywordBids, TestWriteAudienceTargets, TestWriteSitelinks, TestWriteVCards, TestWriteAdExtensions, TestWriteAdImages — all use else: pytest.skip(...) unconditionally when exit_code != 0. CLI-level regressions in any of these commands would be silently hidden. The pattern used in TestWriteFeeds (lines 294–297) is the correct model.

These are not the PR's critical regression guards, so they're medium — but worth noting since the same class of issue has been fixed 6+ times across this review cycle.

Summary

The two critical regression guards for PR #12 (ads add Type-field, smartadtargets add Type-field) and the SourceType guard (feeds add) are all correctly implemented. All previously identified critical issues are resolved. The remaining concerns are medium severity and don't affect the stated goals of the 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 5s —— View job

Tasks:

• Read request and classify
• Read current state of changed files (conftest.py, test_integration_write.py, ads.py, feeds.py)
• Analyze for critical issues (round 7)
• Post review findings

PR Review: sandbox integration tests for write commands (round 7)

Production code (ads.py, feeds.py): No issues. Both changes remain correct.

Previous critical issues (rounds 1–6): All confirmed fixed. Specifically:

• _fixture_invoke/_fixture_parse correctly distinguish sandbox errors from regressions ✅
• _SANDBOX_ERROR_PATTERNS is now specific — only resource-not-found patterns, nothing that matches generic API errors ✅
• Previously broad patterns ("unknown parameter", "required field", "Invalid request", "error_code", "not found", "is omitted") are gone from the global set and only appear as extra_patterns in the specific fixtures where they're justified (e.g. sandbox_retargeting_list) ✅
• TestWriteAds and TestWriteSmartAdTargets (critical Type-field regression guards) use manual JSON parsing + _is_sandbox_error discrimination + pytest.fail for genuine API rejections ✅
• TestWriteDynamicAds and TestWriteSmartAdTargets apply _is_sandbox_error discrimination in the else branch ✅
• TestWriteFeeds applies _is_sandbox_error on non-zero exit ✅
• sandbox_retargeting_list no longer has except BaseException: that swallowed pytest.fail() ✅
• TestWriteTurboPages uses pytest.skip directly ✅

No new critical issues

The stated primary goals of this PR are met:

• Type-field regression guards (TestWriteAds, TestWriteSmartAdTargets): correctly fail on API rejections caused by bad request fields
• SourceType regression guard (TestWriteFeeds): correctly fails vs. skips based on error content
• Fixture chain (sandbox_campaign → sandbox_adgroup → sandbox_ad/sandbox_keyword): correctly propagates pytest.fail for regressions

One new medium issue

TestWriteBidModifiers.test_toggle_existing passes silently if toggle fails (test_integration_write.py:265–279)

r = _invoke("bidmodifiers", "toggle", "--id", str(modifier_id), "--disabled")
if r.exit_code == 0:
    assert_success(r, "bidmodifiers toggle off")  # redundant — always True here
    r = _invoke("bidmodifiers", "toggle", "--id", str(modifier_id), "--enabled")
    assert_success(r, "bidmodifiers toggle on")
# ← no else branch

If bidmodifiers toggle --disabled fails (exit_code != 0), there is no else branch — the test exits the if block and passes silently. A complete regression in the toggle command would produce a green test. The assert_success on line 271 is also redundant (it re-checks exit_code == 0 after the if r.exit_code == 0: guard already established that).

Not a blocker for the PR's stated goals (bidmodifiers toggle is not a Type-field regression guard), but worth fixing: Fix this →

Medium issues still present from round 6 (not blocking)

Eight secondary tests still skip unconditionally on any non-zero exit — TestWriteBids (line 220), TestWriteKeywordBids (line 239), TestWriteRetargeting (line 328), TestWriteAudienceTargets (line 348), TestWriteSitelinks (line 368), TestWriteVCards (line 391), TestWriteAdExtensions (line 416), TestWriteAdImages (line 447). CLI-level regressions in any of these commands would be silently hidden. The TestWriteFeeds pattern (lines 294–297) is the correct model, but these are not the PR's critical regression guards.