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

Review: PR #315 — clients ERIR organization flags

• Read the diff (git diff origin/main...HEAD)
• Verify dry-run payload shape vs Yandex Direct docs
• Verify WSDL audit changes match implementation
• Check parity gate / WSDL coverage map updates
• Post review findings

Focused on critical issues per the trigger. Style/naming intentionally not nitpicked.

1. Kpp field is not in the cached WSDL — runtime risk, no compensating test (critical)

tests/wsdl_cache/imports/generalclients.xsd:166-174 defines OrgInfo as:

<xsd:complexType name="OrgInfo">
    <xsd:element name="Name" .../>
    <xsd:element name="EpayNumber" .../>
    <xsd:element name="RegNumber" .../>
    <xsd:element name="OksmNumber" .../>
    <xsd:element name="OkvedCode" .../>
</xsd:complexType>

No Kpp. The PR body discloses this — "implemented from the official docs but has no audit row in the current cache" — but the only test exercising Kpp is the dry-run at tests/test_dry_run.py:4781, which never reaches the API. So:

• If Yandex's live endpoint accepts Kpp (per https://yandex.ru/dev/direct/doc/en/clients/update), the flag works and the cached WSDL is simply stale.
• If the live endpoint validates against the same schema as the cached WSDL, every invocation of --erir-organization-kpp will fail at runtime — and because Kpp is sandwiched between other valid fields, the failure mode could be the whole clients.update request being rejected (not a graceful partial accept).

The PR audit row (tests/WSDL_OPTIONAL_FIELD_AUDIT.md:5529-5530) lists --erir-organization-kpp under the parent ErirAttributes / ErirAttributes.Organization cells as a side-effect of being in OPTIONAL_FIELD_CLI_OPTIONS, even though there is no child ErirAttributes.Organization.Kpp row to match it against. That hides the discrepancy in the audit doc.

Recommendation: either (a) run the command against the live sandbox to confirm Kpp is accepted, refresh the WSDL cache from upstream if it now includes Kpp, and add the audit row; or (b) drop --erir-organization-kpp from this PR and gate it on a separate verification ticket. As-is, this ships a flag the test suite can't validate.

Verify with sandbox →

2. No way to clear nillable ERIR Organization fields (scope gap, low severity)

All five WSDL OrgInfo fields are nillable="true" (lines 168-172). Yandex therefore supports an explicit-null write to clear a previously-set value. The CLI helpers use truthy guards (if name: etc. at direct_cli/utils.py:360-371), so passing an empty string is silently dropped and there is no --clear-erir-organization-* flag pair. Adjacent typed updates (e.g. feeds update --clear-feed-login) do expose clear flags.

This is consistent with the existing Notification/TinInfo helpers in this file, so it's not a regression — but it leaves the typed surface for #306 strictly write-only-with-new-value. Worth tracking as a follow-up rather than blocking, since issue #306 only asks for the flags themselves.

3. Test only exercises the all-fields-set path (minor)

test_clients_update_erir_organization_payload (tests/test_dry_run.py:4760-4787) sets all six flags at once. There is no explicit assertion that:

• partial payloads (e.g. only --erir-organization-name) emit only the supplied keys,
• ErirAttributes is absent from the item when no ERIR flag is passed (regression protection for the existing clients update tests).

The helper structure makes these implicit, but explicit cases would harden the contract — particularly the negative case, which is what protects against ErirAttributes: {} leaking into a future refactor.

What looks correct

• Payload shape matches the WSDL: ErirAttributes is placed at the top of ClientUpdateItem alongside ClientInfo/Phone/Notification/Settings/TinInfo, exactly as tests/wsdl_cache/imports/generalclients.xsd:243-249 (ErirAttributesUpdate) requires.
• build_erir_organization / build_erir_attributes correctly return None when empty, so build_client_update_item won't emit empty wrappers — preserves the existing "Provide at least one field to update" UsageError invariant for the no-op call.
• OPTIONAL_FIELD_CLI_OPTIONS updates and OPTIONAL_FIELD_CHILD_PREFIX_FOLLOWUPS removals are internally consistent with the audit doc transition from missing_followup to supported.
• The WSDL_FIELD_TO_CLI_OPTION map is untouched, which is correct: all OrgInfo subfields are minOccurs="0", so none of the new fields needs a required-field mapping.
• README adds the example in both EN and RU sections.

Summary

One real critical concern (the Kpp field), one out-of-scope feature gap (no clear flags), one minor test-coverage suggestion. The rest of the change is structurally sound and consistent with the existing typed-helper pattern for clients update.
· branch codex/issue-306-clients-erir-organization

Codex Review: Didn't find any major issues. More of your lovely PRs please.

Addressed the actionable Copilot comment by passing the new ErirAttributes payload as the explicit keyword argument erir_attributes=... instead of a positional argument.

Re-verified:

• python3 -m black --check direct_cli/commands/clients.py
• python3 -m pytest tests/test_dry_run.py -k clients_update_erir_organization_payload
• python3 -m pytest tests/test_cli.py -k clients_update_help_documents_erir_organization_flags

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

Review: PR #315 follow-up (keyword-arg fix)

• Read the latest diff (git diff origin/main...HEAD)
• Confirm build_client_update_item(..., erir_attributes=...) is invoked as keyword
• Re-check dry-run payload shape vs Yandex docs
• Re-check WSDL audit + parity gate updates
• Post review findings

Verdict: LGTM — Copilot follow-up addressed correctly, no critical issues.

Copilot keyword-arg follow-up — fixed ✅

direct_cli/commands/clients.py:140-156 now calls:

client_data = build_client_update_item(
    client_info, phone, notification,
    parse_client_setting_specs(list(settings)),
    parse_tin_info(tin_type, tin),
    erir_attributes=build_erir_attributes(...),
)

The new optional argument is passed by keyword exactly as suggested, and the underlying signature in direct_cli/utils.py:399-409 defines erir_attributes with a default (= None), so the keyword call is self-documenting and refactor-safe.

Critical-issue checks

Non-blocking observations

• --erir-organization-kpp is intentionally not listed under a dedicated ErirAttributes.Organization.Kpp parity row because the cached WSDL has no such field; the flag exists for docs parity. This is the right call but worth a one-liner in the PR body (already noted) so future audit refreshes don't surprise anyone.
• All ERIR strings are forwarded verbatim — no client-side format validation for Kpp, RegNumber, OksmNumber, OkvedCode. Yandex will validate at the API edge; matches the pattern used for --tin.

Nothing else to flag — the follow-up is good to merge from a correctness / data-safety standpoint.
· branch codex/issue-306-clients-erir-organization

Codex Review: Didn't find any major issues. Already looking forward to the next diff.

Review triage after latest commit 5cb6099:

• Copilot positional-argument comment: FIXED. The call now uses erir_attributes=...; latest Claude follow-up confirmed this.
• Earlier Claude Kpp concern: resolved as no-op for this PR after follow-up review. --erir-organization-kpp is intentionally included for official docs parity; the PR body notes that current cached WSDL has no dedicated Kpp audit row.
• Clear ERIR fields: follow-up/out of clients update: ErirAttributes.Organization typed flags #306 scope. This issue covers typed value flags for ErirAttributes.Organization, not explicit null/clear semantics.
• Extra partial-payload tests: no-op for this PR. Existing no-op guard tests plus the new exact payload dry-run cover the accepted scope.

No blocking/actionable review comments remain.