@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 5m 27s —— View job

PR Review

• Read changed files
• Analyze for bugs, security issues, logical errors, data loss risks, performance problems
• Post findings

Summary: The PR is well-structured and closes the stated ACs. Three issues worth fixing before merge — one is a clear functional bug, one is a design fragility that will bite the next person who adds a deprecated method, and one is a minor hardcoded-URL risk.

🐛 Bug: find_nested_schema_violations reports unknown nested keys twice

File: direct_cli/wsdl_coverage.py, lines 548–579

For both the list-item branch and the dict branch, unknown keys at the first nesting level are appended explicitly and then also emitted again by the recursive call to the same function — which independently checks the same level.

Dict branch (lines 571–579):

elif isinstance(value, dict):
    for nested_key in value:
        if nested_key not in nested_expected:
            violations.append(f"{path}.{key}.{nested_key}")   # ← added here
    violations.extend(
        find_nested_schema_violations(
            {"params": value}, nested_schema, path=f"{path}.{key}"  # ← also added here
        )
    )

The recursive call with path="{path}.{key}" re-checks every key in value against the same nested_expected and appends the same path string for any unknown key. This is confirmed by the test test_payload_cases_validate_nested_imported_fields — BogusNestedKey ends up in violations twice. The test only asserts any("BogusNestedKey" in v ...), so it passes despite the duplication.

The gate still correctly fails when violations exist (the list is non-empty), but the report produced by scripts/build_api_coverage_report.py will contain duplicate entries, making it harder to diagnose real problems.

Fix: Remove the explicit inner loop for direct unknown keys and let the recursive call handle them. Fix this →

⚠️ Design fragility: deprecation guard only runs after Click processes required options

File: direct_cli/commands/agencyclients.py, line 138 / scripts/build_api_coverage_report.py, lines 597–600

assert_not_runtime_deprecated is placed inside the Click command function body:

def add(ctx, login, first_name, ...):
    try:
        assert_not_runtime_deprecated("agencyclients", "add")
        ...

Click processes and validates required options before calling the function body. This means if required options are missing, Click raises UsageError("Missing option '--login'.") before the deprecation guard ever runs.

The gate test in _validate_runtime_deprecated_methods therefore relies entirely on RUNTIME_DEPRECATED_CAPTURE_FIXTURES providing every required option — otherwise Click's missing-option error fires first, producing a UsageError that lacks the replacement hint, and the gate flags it as a violation (reason: "UsageError did not include replacement hint"). The gate would appear to catch an "unguarded" method when the method IS guarded but the fixture is incomplete.

The current fixture for agencyclients.add is complete and correct. But there is no automated check that keeps RUNTIME_DEPRECATED_CAPTURE_FIXTURES in sync with RUNTIME_DEPRECATED_METHODS. The next person who adds a deprecated method with required options and forgets to update the fixture will get a misleading gate failure and no guidance on why.

Suggested mitigations (pick one or both):

1. Add a docstring/comment to RUNTIME_DEPRECATED_CAPTURE_FIXTURES explicitly stating it must include all required options for the function body to be reached.
2. Consider moving the guard to a @click.pass_context decorator or eager=True callback so it fires before option processing — though that's a larger refactor.

🔍 Minor: hardcoded XSD refresh URL on a different domain with no constant

File: direct_cli/wsdl_coverage.py, line 346

url = f"https://soap.direct.yandex.ru/v5/{filename}"

The WSDL base URL uses api.direct.yandex.com; the imported XSD refresh URL uses soap.direct.yandex.ru. The two domains serve different content and presumably this is intentional, but:

• There is no named constant for the XSD base URL (unlike WSDL_BASE_URL)
• There is no comment explaining why it's a different domain
• If the URL changes, refresh_all_caches() and check_wsdl_drift.py silently break for XSDs; this would only be discovered when a developer tries to refresh the cache

Suggested fix: extract a XSD_BASE_URL constant beside WSDL_BASE_URL with a brief comment.

✅ Everything else looks good

• The _load_schema_contexts recursion guard (import_ns not in contexts) is correct and handles mutual namespace imports without infinite recursion.
• The _CapturedClient / captured: dict = {} inside the for mode, argv in modes: loop is correct; the # noqa: B023 is justified — the lambda is only invoked within the same iteration.
• advideo_probe_id correctly catches create_client() failures and returns None.
• The assert_not_runtime_deprecated in agencyclients.add is correctly placed before dry_run — the PR description specifically calls out that both real-call and --dry-run paths are rejected, which is confirmed by the tests.
• PAYLOAD_COVERED_COMMANDS keying (using argv[0].argv[1]) is consistent with DRY_RUN_PAYLOAD_EXCLUSIONS keying.
• The test_all_canonical_dry_run_commands_have_payload_coverage_or_exclusion test correctly keeps the registry in sync with the actual CLI.

Addressed all three review findings in db4d475.

1. Double-reporting in find_nested_schema_violations — fixed. The inline loop that appended unknown direct children duplicated the recursive call's work. Removed both inline loops; the recursive call now handles direct and deeply nested unknown keys uniformly. Verified manually: clients.update (with bogus payload) used to emit ['params.Clients[0].ClientId', 'params.Clients[0].Email', 'params.Clients[0].ClientId', 'params.Clients[0].Email']; now emits each path exactly once.

2. Deprecation guard fixture brittleness — addressed by test_runtime_deprecated_methods_have_capture_fixture. The test walks RUNTIME_DEPRECATED_METHODS, looks up each command's required options via the live Click app, and asserts that RUNTIME_DEPRECATED_CAPTURE_FIXTURES has a matching entry. Future contributors who add a deprecated method with required options will get a clear failure pointing at the missing fixture. Also added a docstring on RUNTIME_DEPRECATED_CAPTURE_FIXTURES explicitly stating the invariant.

3. Hardcoded XSD URL — fixed. Extracted XSD_BASE_URL = "https://soap.direct.yandex.ru/v5/{filename}" next to WSDL_BASE_URL, with an inline comment explaining the cross-domain split (api.direct.yandex.com for WSDL vs. soap.direct.yandex.ru for XSDs) so future failures are grep-able.

Verification: 250 passed, schema_parity_ok=True with all 9 buckets empty.

Note on the third Codex/external finding (clients.update / agencyclients.update payload contract divergence) — that was tracked separately as #120; not in scope for this 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 4m 11s —— View job

PR Review

• Read changed files
• Analyze for bugs, security issues, logical errors, data loss risks, performance problems
• Post findings

Summary: All three findings from the previous review pass are correctly fixed. This pass found no new critical bugs, security issues, or data loss risks. One minor performance note worth awareness below.

✅ Previous findings — all addressed correctly

1. Double-reporting in find_nested_schema_violations
Fixed cleanly in direct_cli/wsdl_coverage.py:536-581. Both inline loops that duplicated the recursive call were removed. The comment explaining the delegation is correct and the logic is clean.

2. Deprecation guard fixture brittleness
test_runtime_deprecated_methods_have_capture_fixture (tests/test_api_coverage.py:1238) correctly walks RUNTIME_DEPRECATED_METHODS, introspects required options via the live Click app, and asserts a fixture entry exists. The RUNTIME_DEPRECATED_CAPTURE_FIXTURES docstring in scripts/build_api_coverage_report.py:457-462 makes the invariant explicit. The gate test test_runtime_deprecated_gate_rejects_parser_error_without_replacement further verifies that a Click parser error without the replacement hint is correctly flagged as a violation.

3. Hardcoded XSD URL
XSD_BASE_URL = "https://soap.direct.yandex.ru/v5/{filename}" at direct_cli/wsdl_coverage.py:20 with an inline comment explaining the cross-domain split. Clean.

⚠️ Minor: _field_name_operations calls get_operation_field_name_enums twice per operation

File: scripts/build_api_coverage_report.py, lines 83–90

def _field_name_operations(fetch_wsdl_func, api_service: str) -> dict[str, dict]:
    wsdl_xml = _fetch_wsdl(fetch_wsdl_func, api_service)
    return {
        operation: get_operation_field_name_enums(wsdl_xml, operation)
        for operation in parse_wsdl_operations(wsdl_xml)
        if get_operation_field_name_enums(wsdl_xml, operation)  # ← called again
    }

Each get_operation_field_name_enums call invokes both get_operation_request_schema and parse_wsdl_field_enums, both of which call _load_schema_contexts (which parses XML and resolves imported XSDs). For a service with N operations this is called 2×N times; across ~27 services with ~5 ops each, _load_schema_contexts runs ~270 times per report build. The XSD reads are cached (_cached_imported_xsd has lru_cache), but WSDL XML is parsed from string each time.

This is a pre-existing pattern (the double-call was there before this PR; what's new is _load_schema_contexts being called by both get_operation_request_schema and parse_wsdl_field_enums). No correctness issue — only CI build-time impact. Fix this →

✅ Everything else looks correct

• _load_schema_contexts recursion guard is correct: target_ns in contexts is checked first inside add_schema, and import_ns not in contexts is checked before recursing. Handles mutual namespace imports without infinite recursion.
• _collect_complex_type_fields seen: frozenset correctly prevents cycles in xsd:extension inheritance chains.
• _advideo_get_accepts passes string IDs to Ids — this is consistent with how advideos.get CLI command itself constructs the body (line 38: [x.strip() for x in ids.split(",")]), so no type mismatch vs. the API.
• assert_not_runtime_deprecated → except click.UsageError: raise in agencyclients.add correctly propagates the guard's error past the generic except Exception handler.
• _CapturedClient lambda + # noqa: B023 is justified: captured is a fresh dict = {} per loop iteration and the lambda is only invoked within that iteration's try block.
• XSD_BASE_URL.format(filename=filename) in fetch_imported_xsd is safe: filename comes from IMPORTED_XSD_REGISTRY.values(), a hardcoded dict of three known filenames — no user-controlled input reaches this format call.