@codex @claude review. Focus on critical issues: bugs, security vulnerabilities, logical errors, data loss risks, documentation or test contract regressions, and issue-scope gaps. Do not nitpick style, naming, minor formatting, or subjective preferences.

Claude finished @axisrow's task in 4m 6s —— View job

Claude Code Review

• Load review tools and gather PR diff
• Review direct_cli/commands/clients.py changes
• Review direct_cli/utils.py changes
• Review test contract changes
• Review tests/WSDL_OPTIONAL_FIELD_AUDIT.md and README.md
• Cross-check WSDL parity gate / silent-data-loss & no-op invariants
• Post final review

Verdict

No blocking bugs found. Implementation mirrors the existing --erir-organization-* / --erir-contract-* pattern cleanly, the no-op invariant still holds (build_client_update_item returns {} ⇒ UsageError when nothing is provided), and parse_tin_info is correctly reused to validate --erir-contragent-tin-type. One thing the maintainer should be aware of, plus one small contract-test polish.

Notable

1. --erir-contragent-kpp is not backed by the cached WSDL (medium / known)

The PR description already calls this out, but it's the only real correctness risk worth flagging. tests/wsdl_cache/imports/generalclients.xsd defines ContragentBaseInfo with exactly five fields — Name, Phone, EpayNumber, RegNumber, OksmNumber — and ContragentInfoUpdate adds only TinInfo. There is no Kpp element on Contragent* anywhere in the cached schema (and consequently no audit row, no parity-gate row, no fixture coverage).

Effects:

• Sending --erir-contragent-kpp X produces {"ErirAttributes":{"Contragent":{"Kpp":"X"}}} in the request. If the live API validates against the same schema cache reflected in the docs page, it will either ignore Kpp (silent data loss) or reject the request — neither path is covered by a dry-run fixture, since the dry-run test omits --erir-contragent-kpp from the no-Kpp expected payload assertion only if the user doesn't pass the flag; with the flag it would have to be in the asserted dict.
• This exactly mirrors the precedent set for --erir-organization-kpp, which also has no WSDL element on OrgInfo. So this is not a regression introduced by Add clients ERIR contragent flags #317 — it's an accepted pattern. Worth keeping on the radar for a future WSDL refresh; if Yandex never adds Kpp to ContragentBaseInfo in the WSDL, the two Kpp flags should either be removed or pinned behind a --allow-doc-only style guard.

2. Test contract: test_clients_update_erir_contragent_payload doesn't assert flag ordering, but does fix dict ordering

tests/test_dry_run.py:4888-4920 compares against a dict literal that lists keys in a specific order (Name, Kpp, Phone, EpayNumber, RegNumber, OksmNumber, TinInfo). Python dict equality ignores order, so this is fine — just noting the test will continue to pass even if build_erir_contragent reorders keys in the future. Not a defect.

3. parse_tin_info is called twice per invocation (top-level + contragent)

direct_cli/commands/clients.py:227 and :254. Each call validates its own tin_type independently and a partial Contragent TinInfo (e.g. only --erir-contragent-tin-type LEGAL) yields {"TinInfo": {"TinType": "LEGAL"}} with no Tin. The WSDL marks both TinType and Tin as minOccurs=0, so this is schema-legal, and the explicit dry-run test (test_clients_update_erir_contragent_tin_info_partial_payload) locks the behavior in. Consistent with the existing top-level --tin-type/--tin pair. No issue, just confirming the choice is intentional.

Non-issues verified

• Empty-update no-op invariant (parity gate check Add Claude Code GitHub Workflow #1): with all eight new flags omitted, every builder returns None, build_client_update_item returns {}, falsy check raises UsageError("Provide at least one field to update"). Holds.
• Silent-data-loss invariant (check Add 1Password auth and simplify build config #2): not applicable — clients update has no --type discriminator.
• minOccurs=1 not validated (check feat: add Bitwarden CLI integration #3): ContragentInfoUpdate has no minOccurs=1 children.
• Audit migration (tests/WSDL_OPTIONAL_FIELD_AUDIT.md): 9 rows correctly moved from missing_followup to supported, totals (2783→2774, 458→467) reconcile, and the OPTIONAL_FIELD_CHILD_PREFIX_FOLLOWUPS entry for ErirAttributes.Contragent is correctly removed.
• Help-flag test (tests/test_cli.py:126-136) asserts presence of all eight new flags (including --erir-contragent-kpp).
• WSDL parity gate map (tests/test_wsdl_parity_gate.py:809-905) includes both parent-level and per-field entries for the seven WSDL-backed fields plus Kpp in the parent set.
• README dry-run snippets in both EN and RU sections.

Recommendation

LGTM to merge from a correctness/contract perspective. Consider opening a small follow-up issue to track the Kpp-without-WSDL question for both Organization and Contragent so it isn't forgotten on the next WSDL refresh.
• Branch: codex/issue-308-clients-erir-contragent

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

@codex @claude review. Re-run after addressing Copilot feedback on ambiguous tin type errors. Focus on critical issues: bugs, security vulnerabilities, logical errors, data loss risks, documentation or test contract regressions, and issue-scope gaps. Do not nitpick style, naming, minor formatting, or subjective preferences.

Claude finished @axisrow's task in 2m 23s —— View job

Claude Code Review (re-run)

• Load review tools and pull latest branch state
• Diff origin/main...HEAD and inspect changes since last review
• Verify Copilot feedback (ambiguous parse_tin_info error) is addressed
• Re-check no-op / silent-data-loss invariants and dry-run contracts
• Post final review

Verdict

Copilot's parse_tin_info ambiguity feedback is correctly addressed in commit 729abdd, and the fix doesn't regress the other two call sites. No new blocking issues. The follow-up risk I called out previously (--erir-contragent-kpp / --erir-organization-kpp not backed by the cached WSDL) still stands — unchanged by this commit, still worth a follow-up issue but not a blocker.

What the fix does

direct_cli/utils.py:358-375 — parse_tin_info gains an option_name: str = "--tin-type" kwarg and now raises:

Invalid tin type for {option_name}: '{tin_type}'. Expected one of: ...

direct_cli/commands/clients.py:254-258 — the contragent call passes "--erir-contragent-tin-type" explicitly.

tests/test_dry_run.py:4949 — adds assert "--erir-contragent-tin-type" in result.output to lock in the disambiguated message.

Critical checks (all pass)

1. No regression for other callers. The other two callers — direct_cli/commands/clients.py:227 (top-level --tin-type) and direct_cli/commands/agencyclients.py:353 (also --tin-type) — use the default kwarg, which correctly resolves to --tin-type since both flags are literally named that. So "Invalid tin type for --tin-type: ..." is accurate, not misleading.
2. No existing test breakage. The pre-existing assertion at tests/test_dry_run.py:4948 is a substring match for "Invalid tin type", which still appears in the new wording — safe.
3. Only one organization/contragent distinction needed. I verified there is no --erir-organization-tin-type flag (grep returned empty), so no second disambiguation site was missed.
4. No-op invariant still holds: with no flags, build_erir_contragent → None, build_erir_attributes → None, build_client_update_item → {}, → UsageError("Provide at least one field to update").
5. Partial TinInfo fixture (test_clients_update_erir_contragent_tin_info_partial_payload) still asserts {"TinInfo": {"TinType": "LEGAL"}} with no Tin — minOccurs=0 on both children, so the behavior is schema-legal and intentional.

Standing follow-up (from prior review, not introduced here)

--erir-contragent-kpp and --erir-organization-kpp have no element in the cached WSDL (tests/wsdl_cache/imports/generalclients.xsd defines ContragentBaseInfo with only Name/Phone/EpayNumber/RegNumber/OksmNumber, and ContragentInfoUpdate adds only TinInfo). The flags are documented on the Yandex docs page but not in the WSDL, so sending them will either silently drop server-side or be rejected — neither path is fixture-covered. Mirrors the accepted precedent of --erir-organization-kpp. Worth a tracking issue for the next WSDL refresh; not a blocker for this PR.

Recommendation

LGTM — Copilot's ambiguity concern is resolved cleanly. Merge when CI is green.

--- • Branch: codex/issue-308-clients-erir-contragent

Codex Review: Didn't find any major issues. Keep it up!

Review triage: FIX - Copilot ambiguous tin type error was addressed in 729abdd; Claude rerun verified the fix. FOLLOW-UP - Kpp docs/WSDL mismatch is non-blocking and can be tracked outside this PR. No blocking/actionable comments remain; CI is green.