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

Code Review — PR #117

Tasks

• Gather context and read changed files
• Review direct_cli/utils.py (core logic change)
• Review scripts/build_api_coverage_report.py (schema gate)
• Review tests/test_api_coverage.py (regression tests)
• Review command file changes
• Post findings

Findings

1. changes.check behavioral regression (production risk)

direct_cli/commands/changes.py:36-40

Before this PR, changes.check sent no FieldNames in the request payload:

# before
params = {"CampaignIds": parse_ids(campaign_ids)}

After this PR, it always sends FieldNames: ["CampaignIds", "AdGroupIds", "AdIds", "CampaignsStat"]:

# after
field_names = fields.split(",") if fields else get_default_fields("changes")
params = {"CampaignIds": parse_ids(campaign_ids), "FieldNames": field_names}

If the Yandex API returns all change categories by default when FieldNames is absent, users relying on that default behavior would now receive only the four categories declared in COMMON_FIELDS. The PR's own smoke-test list covers 7 migrated commands — changes is not among them. This is the only migrated command where FieldNames was absent before and is now unconditionally injected. Worth a targeted live smoke test before merge.

Fix this →

2. Double-reporting in field_name_mismatches (logical error)

scripts/build_api_coverage_report.py

build_schema_gate first runs _validate_common_field_defaults which validates COMMON_FIELDS directly against WSDL enums and appends mismatches with "source": "COMMON_FIELDS". The main loop then captures the wire payload — which sends those same COMMON_FIELDS values via get_default_fields — and validates them, appending a second mismatch with "source": "wire_payload".

For any invalid value in COMMON_FIELDS that flows through to the wire payload, field_name_mismatches will contain two entries for the same invalid field. The gate correctly fails in both cases, but the duplicates make CI output ambiguous and would break any assertion like assert len(mismatches) == 1 for a single bad value. The unit tests avoid this only because they use controlled wire payloads (without the bad value from COMMON_FIELDS).

Fix this →

3. _capture_operation_body swallows internal TypeErrors (fragile test logic)

scripts/build_api_coverage_report.py:121-128

def _capture_operation_body(capture_func, cli_group: str, operation: str) -> dict:
    try:
        return capture_func(cli_group, operation)
    except TypeError:
        if operation != "get":
            raise
        return capture_func(cli_group)

This catches TypeError to support old single-argument capture lambdas. But if capture_func(cli_group, operation) raises TypeError for a reason other than argument count (e.g., a type error inside the function body), the exception is silently swallowed for get operations and the function is retried with one argument, masking the root cause. This is only test infrastructure, but it could make regressions harder to diagnose.

Fix this →

4. _CapturedResponse.data = {} mutable class attribute (low risk)

scripts/build_api_coverage_report.py:323

data = {} is a class-level attribute shared across all _CapturedResponse instances. If any command does result.data["key"] = value on the captured response, that mutation persists across all subsequent invocations in the same process (e.g., across multiple CliRunner.invoke calls in a test run). Currently safe because commands only pass result.data to format_output which reads but does not mutate it — but fragile for future command additions.

The fix is to initialize per-instance: def __init__(self): self.data = {}.

5. _common_field_resource_targets silently skips unrecognized COMMON_FIELDS keys

scripts/build_api_coverage_report.py:131-143

COMMON_FIELDS entries whose key is neither a CLI group name in CLI_TO_API_SERVICE nor an API service name in the reverse map are silently dropped and never validated by _validate_common_field_defaults. A typo in a COMMON_FIELDS key (e.g., "dynamicfeedadtarget" instead of "dynamicfeedadtargets") would pass the gate undetected. The coverage report doesn't flag these orphan entries.

Summary

No data loss or security issues found. The primary production risk is the behavioral change in changes.check adding FieldNames to a request that previously omitted it. The double-reporting issue (#2) and silent-skip issue (#5) are logical gaps in the schema gate itself, which is the core mechanism this PR is hardening — worth addressing to preserve the gate's trustworthiness. Issues #3 and #4 are test infrastructure concerns.

Addressed review findings in 78da917.

#1 changes.check FieldNames behavioral change — verified the WSDL claim and the picture is the opposite of a regression:

• WSDL tests/wsdl_cache/changes.xml: FieldNames is minOccurs="1" maxOccurs="unbounded" and Timestamp is minOccurs="1" maxOccurs="1" — both required by spec.
• Pre-PR-fix: close #108 with single source of truth for default FieldNames #117 code sent no FieldNames at all (and Timestamp only if timestamp:), violating the WSDL on two fields. There are no integration tests for changes.check, so the live behavior was never validated either way.
• Post-PR-fix: close #108 with single source of truth for default FieldNames #117 default ["CampaignIds", "AdGroupIds", "AdIds", "CampaignsStat"] matches all four values of CheckFieldEnum, i.e. the broadest possible response — equivalent to "give me everything", which is what an unfiltered call would conceptually mean.
• Conclusion: the change brings the request into compliance, not out. While here, also flipped --timestamp to required=True to match the WSDL. Same WSDL-spec correctness reasoning.

#2 double-reporting in field_name_mismatches — fixed. After _validate_common_field_defaults flags (cli_group, operation, request_field, value) from COMMON_FIELDS, the wire-payload pass now skips that exact tuple. Added test_schema_gate_dedupes_common_fields_and_wire_payload_mismatches.

#3 _capture_operation_body swallowing internal TypeError — fixed. Replaced the try/except TypeError arity-fallback with an explicit inspect.signature arity check (_capture_func_accepts_operation). Internal TypeErrors now surface from both 1-arg and 2-arg capture functions. Added test_capture_operation_body_does_not_swallow_internal_typeerror.

#4 _CapturedResponse.data mutable class attribute — fixed. Moved self.data = {} into __init__. No behavior change today (no command mutates result.data), but eliminates the cross-test bleed risk you flagged.

#5 silent skip of orphan COMMON_FIELDS keys — fixed. Added a new gate bucket orphan_common_fields populated by _orphan_common_fields_keys() (returns COMMON_FIELDS keys matching neither a CLI group nor an API service). Any non-empty bucket flips schema_parity_ok to false. Added test_schema_gate_flags_orphan_common_fields_keys.

Verification: pytest -q → 239 passed, python3 scripts/build_api_coverage_report.py → schema_parity_ok: True with all 7 buckets empty.

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

Code Review — PR #117 (Post-Fix Pass)

Tasks

• Read prior review and author's fix summary (78da917)
• Read scripts/build_api_coverage_report.py (core schema gate)
• Read direct_cli/utils.py (COMMON_FIELDS changes)
• Read tests/test_api_coverage.py (new regression tests)
• Read direct_cli/commands/changes.py (behavioral change)
• Spot-check other changed command files
• Post findings

All five findings from the prior review were correctly addressed in 78da917. The gate tests pass, the dedup logic is correct, _capture_operation_body no longer silences internal TypeErrors, _CapturedResponse.data is now per-instance, and orphan COMMON_FIELDS keys fail the build. Two residual observations below — neither is a production blocker.

Findings

1. Non-get operations: wire payload is never captured by the gate (design gap)

scripts/build_api_coverage_report.py:498

The main loop in build_schema_gate skips entire CLI groups if they have no get method:

if "get" not in get_cli_methods_for_service(cli_name):
    continue

changes (which only has check) and keywordsresearch (which only has hasSearchVolume) are both skipped. For these two groups, only _validate_common_field_defaults runs — COMMON_FIELDS values are validated against the WSDL enum, but the actual wire payload sent by the command is never captured or compared.

Today this is fine because both commands call get_default_fields(), making COMMON_FIELDS the single source of truth. But if someone later hardcodes FieldNames inline in either command (bypassing get_default_fields()), the gate stays green even if the hardcoded values are invalid. The PR's own headline — "there is no longer a way to add a new get resource that bypasses validation" — is accurate; the gap is limited to non-get-only groups.

This is a design limitation inherited from the gate architecture, not introduced by this PR. Worth documenting so the next contributor doesn't assume these commands are fully covered.

2. Inconsistent capture_errors entry shape

scripts/build_api_coverage_report.py:191-200 and 505-513

Errors appended by _validate_common_field_defaults carry "source": "COMMON_FIELDS":

capture_errors.append({..., "source": "COMMON_FIELDS"})

Errors appended in the main loop do not:

capture_errors.append({"cli_group": ..., "api_service": ..., "operation": "*", "error": ...})

Both end up in the same capture_errors list returned by build_schema_gate. Any consumer iterating that list expecting uniform keys will hit a KeyError on "source". Currently only the presence/absence of the list is checked (schema gate fails on any non-empty list), so this doesn't cause a bug today — but it's fragile.

Summary

No data loss, no security issues, no correctness bugs in the core logic. The schema gate and its regression tests are solid. The two observations are a design gap in gate coverage for non-get operations (pre-existing, both affected commands are currently correct) and an inconsistent diagnostic dict shape (cosmetic risk only). Ready to merge.

Thanks for the post-fix pass.

#1 non-get groups: wire payload not captured — accurate observation; this is a known design gap, not a regression. Tracked separately as #118 ("Extend WSDL schema gate to mutating operations"), which I opened earlier today specifically for this. The issue includes the rationale for keeping #117 scoped to get (default-FieldNames invisible regressions are a get-shaped problem; mutating operations need different validation primitives — typed-object schemas vs enum lists). Won't expand the gate in this PR.

#2 inconsistent capture_errors shape — fixed in ff5a907. All three capture_errors.append sites now carry a uniform "source" field with one of three values: "COMMON_FIELDS", "wsdl_fetch", "wire_payload". Consumers iterating the list can now rely on the key always being present.

Verification: pytest -q → 239 passed, python3 scripts/build_api_coverage_report.py → schema_parity_ok: True.