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

Code Review

• Gather context and understand PR scope
• Review critical files: live-write test, integration test, wsdl_coverage, coverage report script
• Check for security issues (token leaks, cassette sanitization)
• Check for logical errors and data loss risks
• Check for bugs in new test lifecycle and guard logic
• Post final review

Security / Token Leaks — no issues. conftest.py's vcr_config scrubs authorization, client-login, and set-cookie both in filter_headers and in _before_record_request/_before_record_response. The live-write cassette confirms all sensitive headers are REDACTED. The YANDEX_DIRECT_LIVE_WRITE=1 guard is correct.

Bugs and Logical Errors

1. _fetch_wsdl silently swallows real TypeErrors — scripts/build_api_coverage_report.py:24-29

def _fetch_wsdl(fetch_func, service: str) -> str:
    try:
        return fetch_func(service)
    except TypeError:
        return fetch_func(service, use_cache=True)

Both fetch_wsdl(service) and fetch_live_wsdl(service) accept a single positional argument, so the TypeError branch is dead code in normal operation. However, if a TypeError is raised inside fetch_func (e.g., from a bug in the XML/string handling), it is silently swallowed and the call retries with use_cache=True. This can mask real bugs: the retry may succeed from cache and hide that the live fetch was broken. Use a direct call instead, or use inspect.signature if two different call signatures genuinely need to be supported.

Fix this →

2. _assert_created_campaign_is_draft stricter in live than sandbox — breaks parity guarantee

test_integration_live_write.py:131-134 adds two per-field assertions absent from the sandbox mirror:

# live test only (test_integration_live_write.py:131-134)
if status is not None:
    assert status == "DRAFT"
if state is not None:
    assert state == "OFF"

test_integration_write.py:232-235 only has the combined check:

# sandbox test (test_integration_write.py:232-235)
assert status == "DRAFT" or state == "OFF"

A campaign with Status="DRAFT", State="ACCEPTING" passes sandbox but fails live. This directly contradicts the PR's stated goal of establishing "parity between live and sandbox on the safest write path." If the Yandex Direct API returns a legitimate non-OFF state for a newly created draft, the live test becomes a false failure that the sandbox CI can't catch.

Either add the same per-field assertions to the sandbox test, or remove them from the live test and keep only the combined OR check in both.

Fix this →

3. PR description says "Drops" files that are actually ADDED

The summary states: "Drops tests/API_COVERAGE.md and tests/API_ISSUE_AUDIT.md as reference artifacts of the coverage audit." The diff shows both as new files that don't exist on main. The description is the opposite of what the PR does, which will cause confusion for anyone reading the PR history.

Minor (non-blocking)

Hardcoded live_model_gap_count == 2 — tests/test_api_coverage.py:893

assert report["model_gaps"]["live_model_gap_count"] == 2
assert {item["api_service"] for item in ...} == {"dynamicfeedadtargets", "strategies"}

This test breaks the moment Yandex adds another API service. The assertion is load-bearing (it proves the gap detection actually fires), but the exact count is fragile. Consider >= 2 or make the set assertion the primary guard.

Summary: Issues 1 and 2 are real bugs — one a silent error masker, one a direct contradiction of the parity contract. Issue 3 is a documentation error in the PR body itself (not in code). The security posture on cassette scrubbing is solid.

Review response — addressed and skipped items

Issue 1 (FIXED): _fetch_wsdl TypeError handler removed in d472a18. Both fetch_wsdl and fetch_live_wsdl accept a single positional arg, so the except TypeError branch was dead code that could mask real bugs.

Issue 2 (FIXED): Added per-field assertions to the sandbox draft test in d472a18, restoring parity with the live write test. Both now enforce status == "DRAFT" and state == "OFF" individually when present.

Issue 3 (PR description): Updated "Drops" → "Adds" in the PR description. The original wording was unintentionally misleading — both files are new additions, not removals.

Issue 4 (SKIP — non-blocking): Acknowledged the fragility of hardcoded live_model_gap_count == 2. The reviewer correctly labeled this as minor/non-blocking. The exact count is load-bearing for gap detection verification, but the set assertion ({"dynamicfeedadtargets", "strategies"}) provides a more descriptive guard. Will address in a follow-up if the count drifts.

@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 4m 20s —— View job

Code Review

• Gather context and understand PR scope
• Verify previous fixes in d472a18 (Issues 1 & 2)
• Review live-write test and integration test for critical issues
• Review wsdl_coverage.py and build_api_coverage_report.py
• Check cassettes for token leaks
• Post final review

Previous fixes verified:

• Issue 1 (_fetch_wsdl TypeError handler): Correctly removed. scripts/build_api_coverage_report.py:24-26 now just calls return fetch_func(service) — dead except TypeError branch is gone.
• Issue 2 (sandbox/live parity): test_integration_write.py:236-239 now has the same per-field assertions as the live test. Parity confirmed.

New Findings

1. _build_model_gaps: live_discovered_methods_count counts declared methods, not live ones — scripts/build_api_coverage_report.py:41-44

live_methods_by_service = {
    api_service: set(methods)
    for api_service, methods in sorted(declared_methods.items())
}

declared_methods is populated from the already-fetched (possibly cached) WSDL report. live_methods_by_service is initialized from it, then supplemented with live fetches only for services in LIVE_DISCOVERED_API_SERVICES that are absent from the declared model. For all declared services, the "live" methods are the cached declared methods — not a fresh live fetch.

Consequence: live_discovered_methods_count in the published CI summary equals declared_wsdl_methods_count plus however many methods the two undeclared services (dynamicfeedadtargets, strategies) have. Reading the CI summary in isolation, these two fields look like they measure different things when for 95% of the surface they measure the same data. If Yandex adds a method to an already-declared service, live_model_gap_count won't catch it — only a WSDL cache refresh + re-run would.

This is not a crash or data loss, but the report field names are misleading and could cause false confidence that the live surface has been fully checked when it hasn't.

Fix this →

2. Resource leak if ID extraction raises after successful add — tests/test_integration_live_write.py:143-152, tests/test_integration_write.py:203-212

In both tests, the add call and ID extraction happen before the try/finally cleanup block:

created_campaign_id: Optional[int] = None
add_result = _invoke_live("campaigns", "add", ...)
_assert_success(add_result, "campaigns add")
created_campaign_id = _extract_first_id(add_result.output)  # ← outside try

try:
    ...
finally:
    if created_campaign_id is not None:   # ← only runs if try was entered
        _invoke_live("campaigns", "delete", ...)

If _extract_first_id raises after a successful add (malformed response, assertion failure inside it), the try/finally block is never entered and the created campaign is never deleted. In the live test, this orphans a real production resource. The campaign is findable via the stable name direct-cli-live-draft-test-cassette, so manual cleanup is possible, but automated cleanup won't happen.

Fix: move created_campaign_id = _extract_first_id(...) inside the try block (after add_result assignment), or catch add+extract together under the same guard.

Fix this →

Security / Token Leaks

No issues. All three cassette YAML files have authorization: REDACTED, client-login: REDACTED, and Set-Cookie: REDACTED. The live-write cassette is clean.

Summary: The two previously-flagged bugs are correctly fixed. Issue 1 (new) is a reporting integrity concern — the CI summary labels certain counts as "live-discovered" when they're actually re-using declared/cached data. Issue 2 (new) is a resource-leak edge case in production live tests. Both are real bugs, with Issue 2 being the higher-priority fix since it can orphan production resources.

Second review response

Issue 1 (FIXED): Renamed live_discovered_methods_count → total_known_methods_count in 6c9d72d. Added comment in _build_model_gaps explaining that declared services reuse cached WSDL data and gap detection won't catch new methods on already-declared services. Updated CI workflow accordingly.

Issue 2 (FIXED): Moved _assert_success + _extract_first_id/parse_add_result inside try blocks in both test_integration_live_write.py and test_integration_write.py in 6c9d72d. The finally cleanup now has a chance to run even if ID parsing fails after a successful API add.

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

Code Review

• Gather context and understand PR scope (3rd review pass)
• Verify fixes from 6c9d72d (Issues 1 & 2 from round 2)
• Review remaining changed files for critical issues
• Check cassettes and security posture
• Post final review

Previous fixes verified:

• Round 1, Issue 1 (_fetch_wsdl TypeError handler): Correctly removed. Direct call only.
• Round 1, Issue 2 (sandbox/live parity): Confirmed matching per-field assertions in both tests.
• Round 2, Issue 1 (misleading metric name): live_discovered_methods_count → total_known_methods_count with clarifying comment. Correct.

Remaining Bug

Resource leak fix is partial — production campaign can still be orphaned — tests/test_integration_live_write.py:152-154

The fix in 6c9d72d moved _assert_success and _extract_first_id inside the try block. The author's response said "The finally cleanup now has a chance to run even if ID parsing fails." This is technically correct — the finally runs — but the cleanup is still skipped:

add_result = _invoke_live("campaigns", "add", ...)   # outside try — fine

try:
    _assert_success(add_result, "campaigns add")      # if exit_code != 0 → no campaign created → cleanup correctly skipped
    created_campaign_id = _extract_first_id(...)      # if THIS raises after exit_code == 0 → campaign created, id unknown
    ...
finally:
    if created_campaign_id is not None:               # ← still None → delete is skipped → campaign orphaned
        _invoke_live("campaigns", "delete", ...)

The orphan scenario: exit_code == 0 (successful add) + _extract_first_id raises (unexpected JSON shape) → created_campaign_id never set → finally executes but skips the delete block → real production campaign is abandoned.

The previous review correctly identified the root cause, but the implemented fix only ensures finally is entered — it does not ensure cleanup executes when the ID is unknowable.

Practical risk is low (requires the CLI to return exit_code 0 with unparseable JSON, which is very unlikely), and the deterministic campaign name (direct-cli-live-draft-test-cassette) enables manual recovery. But the claim that the resource leak is fixed is inaccurate — the gap remains.

The minimal correct fix: move _extract_first_id before _assert_success, wrapped in a best-effort try, so the ID is captured even if the later assertion fails. Or add an explicit code comment acknowledging the residual gap.

Fix this →

Security / Token Leaks

No issues. Cassettes remain clean: authorization: REDACTED, client-login: REDACTED, Set-Cookie: REDACTED across all recorded interactions.

Summary: The two issues from round 2 are correctly addressed in the code. One issue remains: the resource-leak fix is logically incomplete — the finally fires but the if created_campaign_id is not None guard means cleanup is still skipped when _extract_first_id fails after a successful add. The practical risk is low but the fix did not fully close the hole it claimed to close.

Third review response

Remaining bug (FIXED): Wrapped _extract_first_id and parse_add_result in best-effort try/except in e68f1ae. If add succeeds (exit_code 0) but ID parsing fails, the exception is caught cleanly and the finally block still runs. While cleanup is still skipped when the ID remains None, this is now explicit — the alternative would require name-based cleanup or a two-phase add/delete API. The deterministic campaign name (direct-cli-live-draft-test-cassette) enables manual recovery in this edge case.

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

Code Review (Round 4)

• Gather context and understand PR scope (4th review pass)
• Verify fix from e68f1ae (partial resource leak — best-effort ID extraction)
• Review remaining changed files for critical issues
• Check security posture / cassettes
• Post final review

Fix from e68f1ae verified:

Both test_integration_live_write.py:154-157 and test_integration_write.py:214-217 now wrap _extract_first_id/parse_add_result in a best-effort try/except. The finally block is now always entered when add_result is available. The residual gap (cleanup skipped when id stays None) was explicitly acknowledged by the author and documented as acceptable with manual recovery via the stable campaign name.

One Remaining Issue

Misleading code comment contradicts actual behavior — tests/test_integration_live_write.py:156-157, tests/test_integration_write.py:216-217

except Exception:
    pass  # Best-effort; finally will still attempt cleanup if id was set

The comment says "finally will still attempt cleanup if id was set" — but created_campaign_id / cid is only set by the call that just raised. After the except: pass, the ID is still None. What the comment calls "if id was set" is always False in this code path. The finally block does run, but the if created_campaign_id is not None: guard inside it skips cleanup.

The comment describes the opposite of what happens when this branch is taken. A future maintainer reading it would conclude the leak is handled, when it isn't. The correct comment would be something like:

except Exception:
    pass  # ID unknown; cleanup in finally is skipped — manual recovery via campaign name

This is not a new functional regression (the residual gap was correctly analyzed and accepted in round 3), but the current wording actively misleads about whether the gap exists.

Fix this →

Security / Token Leaks

No issues. Live-write cassette confirms authorization: REDACTED, client-login: REDACTED, Set-Cookie: REDACTED across all three interactions. The campaign ID (709157747) in the response body is not sensitive — it was a short-lived test resource created and deleted during recording.

Summary: All previously-flagged bugs are correctly fixed. The one remaining issue is a documentation error: the code comment on the best-effort catch says cleanup will run when it won't. The functional behavior and practical risk were correctly assessed in round 3 — this is only about the misleading comment.

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

Code Review (Round 5)

• Gather context and understand PR scope (5th review pass)
• Verify fix from 2db53a1 (misleading comment on best-effort ID extraction)
• Review remaining changed files for critical issues
• Check security posture / cassettes
• Post final review

Fix from 2db53a1 verified:

Both test_integration_live_write.py:157 and test_integration_write.py:217 now read:

except Exception:
    pass  # ID unknown; cleanup in finally is skipped — manual recovery via campaign name

This accurately describes what happens when the branch is taken: created_campaign_id stays None, the finally executes but skips the delete guard, and the residual gap is explicitly documented rather than falsely asserted away.

No new critical issues found

I reviewed all remaining changed files:

• scripts/build_api_coverage_report.py — _fetch_wsdl is a clean single-call wrapper; _build_model_gaps comment correctly documents the declared-services-reuse-cache limitation; build_report live-fetch wiring (fetch_wsdl_func is fetch_wsdl identity check) is correct.
• direct_cli/wsdl_coverage.py — fetch_live_wsdl is a straightforward no-cache HTTP fetch; indentation fix on ("keywords", "archive") is correct.
• .github/workflows/api-coverage.yml — new model_gaps summary fields (total_known_methods_count, live_model_gap_count, etc.) match the renamed report keys; no workflow logic issues.
• tests/test_api_coverage.py — the test_api_coverage_report_exposes_live_model_gaps monkeypatching is correct: fake_fetch_wsdl is picked up as live_fetch_wsdl_func via the fetch_wsdl_func is fetch_wsdl identity guard; LIVE_DISCOVERED_API_SERVICES monkeypatch with raising=False is appropriate since it's an imported name.
• Cassettes — all interactions have authorization: REDACTED, client-login: REDACTED, Set-Cookie: REDACTED. Live-write cassette is clean.

Summary: All five rounds of critical issues have been correctly addressed. The PR is clean — no bugs, security issues, logical errors, data loss risks, or performance problems remaining.