@codex @claude review. Focus on critical issues: bugs, security vulnerabilities, logical errors, data loss risks, performance problems, API contract mismatches, and missing tests. 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 59s —— View job

Claude Code Review

Verdict: No critical issues. The PR implements InstantMessenger and PointOnMap nested vcard flags correctly and the all-or-none validation is the right approach.

Critical checks (all pass)

• WSDL contract — vcards.xml:42-57 declares InstantMessenger.{MessengerClient, MessengerLogin} and MapPoint.{X, Y, X1, Y1, X2, Y2} all as minOccurs=1. Both helpers reject any partial submission (vcards.py:25-29, vcards.py:55-60), so the API never sees a malformed nested object. The required-field WSDL gate (wsdl_coverage.py:575) only walks one level into the container, so the nested-within-optional required fields don't need an INTERNAL_VALIDATION entry; the all-or-none helpers are sufficient.
• Zero-coordinate handling — _build_point_on_map filters with if value is not None, not truthiness, so 0.0 is correctly treated as "provided" (a map point at (0,0) won't be silently dropped). vcards.py:46, vcards.py:50.
• Empty/partial dict guard at consumer — if instant_messenger: / if point_on_map: (vcards.py:228-231) are safe because the helpers always return either None or a fully populated dict. No empty-dict path.
• Tests — happy-path dry-runs verify exact payload structure; partial-rejection tests verify the UsageError message includes the offending flag names. Coverage is adequate.
• WSDL audit reclassification — both fields move from missing_followup to supported with the new option lists; counts go 401→411 supported, 2840→2830 missing.

Minor observations (non-blocking, listed for awareness only)

1. Asymmetric partial-rejection coverage — test_vcards_add_instant_messenger_partial_rejected only covers the client-without-login case. A login-without-client test would mirror it, but the underlying check is symmetric so the existing test is enough in practice.
2. decimal vs Python float — WSDL types coordinates as xsd:decimal; the CLI uses Click type=float. For lat/lon at typical Yandex precision (~5 decimals) this is fine, and JSON wire format converts cleanly. Not an issue here, but worth keeping in mind for any future decimal-typed money fields (where float drift would matter).
3. Empty-string edge case — if not messenger_client and not messenger_login treats "" like None, so --instant-messenger-client "" (no login) silently drops InstantMessenger rather than erroring. Click users essentially never pass explicit "", so this is theoretical. Switching to is None checks would be one-line tighter but isn't a real-world hazard.

Conclusion

Mergeable as-is. No bugs, no security/data-loss risk, no API-contract mismatch, no missing test that would mask a real failure mode.

• Read PR diff against origin/feat/issue-239-optional-fields-gate
• Inspect direct_cli/commands/vcards.py changes
• Inspect new dry-run tests
• Inspect WSDL parity gate changes
• Verify WSDL schema for InstantMessenger / MapPoint and the required-field gate
• Post final review
  · Branch: codex/issue-246-vcards-nested-fields

Codex Review: Didn't find any major issues. Keep them coming!

Addressed the actionable UsageError propagation feedback in 2b4790b:

• vcards add now re-raises click.UsageError before the broad exception handler;
• partial InstantMessenger and PointOnMap tests now assert normal Click usage exit_code=2.

Verification:

• python3 -m pytest tests/test_dry_run.py -k 'vcards and (instant_messenger or point_on_map)'
• python3 -m pytest tests/test_wsdl_parity_gate.py
• python3 -m pytest tests/test_cli.py tests/test_dry_run.py tests/test_wsdl_parity_gate.py

@codex @claude review. Please re-check only blocking issues: bugs, contract mismatches, and missing tests.

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

Claude Re-Review (commit 2b4790b)

Verdict: Fix is correct. No blocking issues, mergeable.

Blocking-issue checks (all pass)

• UsageError propagation — direct_cli/commands/vcards.py:248-249 adds except click.UsageError: raise immediately before the broad except Exception in the add command. Ordering is correct (specific exception class first), so UsageError raised by _build_instant_messenger / _build_point_on_map bypasses print_error + click.Abort() and propagates with Click's standard formatting (Usage: ...\nError: ...) and exit code 2.
• Test strengthening — both partial-input tests now assert result.exit_code == 2 (tests/test_dry_run.py:3912, tests/test_dry_run.py:3984), not just != 0. This pins the Click usage-error contract; if the broad except Exception ever swallows UsageError again, exit code would drop to 1 and the test would fail. Good regression guard.
• No collateral changes — fix is two new lines in vcards.py and two new assertions in tests. Helper logic, payload shape, and happy-path tests are untouched.
• Scope correctness — vcards add is the only mutating subcommand that calls the grouped-flag helpers (get and delete don't); update does not exist for vcards. The fix is applied at the only site that needs it.
• Existing message assertions preserved — --instant-messenger-client and --instant-messenger-login and PointOnMap requires all coordinate flags substring checks still run, so message regressions remain caught.

Contract / coverage

• No WSDL contract change; all-or-none semantics for InstantMessenger.{MessengerClient, MessengerLogin} and MapPoint.{X, Y, X1, Y1, X2, Y2} remain enforced before the API call.
• No new tests are missing for the fix itself — the two strengthened assertions cover exactly the regression that prompted the change.

Conclusion

Ready to merge. The Copilot-flagged usage-error UX issue is resolved at the right layer, and the test assertions lock in the fix.

• Pull latest commit (2b4790b)
• Verify UsageError now propagates outside try/except Exception in vcards add
• Verify new tests assert Click usage exit code (exit_code=2) for partial nested objects
• Confirm no regression in happy-path payload shape
• Post verdict
  · Branch: codex/issue-246-vcards-nested-fields

Codex Review: Didn't find any major issues. Bravo.