@codex @claude review. Focus on critical issues: bugs, security vulnerabilities, logical errors, data loss risks, performance problems, CLI contract violations, WSDL/API mismatch, and issue-scope gaps. Do NOT nitpick style, naming conventions, minor formatting, or subjective preferences; only flag issues that could break functionality, violate the public CLI contract, or make issue #253 incomplete.

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

Code Review — PR #265 (URL feed detail flags)

Scoped to critical issues only (bugs, security, logical errors, CLI contract, WSDL/API mismatch, issue-scope gaps), per the request.

Verdict

No blocking issues. The change correctly implements UrlFeed.RemoveUtmTags, UrlFeed.Login, UrlFeed.Password for feeds add/feeds update, preserves the empty-subtype UsageError gate, and properly defers FileFeed to follow-up #264. WSDL parity audit counts and per-field statuses are coherent.

Checks performed

• PR diff against origin/feat/issue-239-optional-fields-gate
• direct_cli/commands/feeds.py — add / update semantics
• tests/test_dry_run.py — add/update payload tests + UsageError tightening
• tests/test_wsdl_parity_gate.py — OPTIONAL_FIELD_CLI_OPTIONS + audit follow-ups
• tests/WSDL_OPTIONAL_FIELD_AUDIT.md — status flips and counts
• Issue-scope cross-check (UrlFeed in scope; FileFeed deferred)

Findings

1. No critical bugs. The helper _url_feed_payload at direct_cli/commands/feeds.py:17-32 correctly conditions each subfield on truthiness, and the update handler at feeds.py:164-171 still raises click.UsageError when no field besides Id is provided — so the parity gate's "empty subtype no-op" invariant (CLAUDE.md, gate #1) is preserved.

2. WSDL parity is internally consistent. Audit counts shift missing_followup: 2823 → 2817 and supported: 418 → 424 (net +6 supported), matching exactly the six rows flipped (3 UrlFeed.* under feeds.add, 3 under feeds.update). The WSDL_FIELD_TO_CLI_OPTION mapping additions at tests/test_wsdl_parity_gate.py:800-808 align with the audit, and the FileFeed follow-up rows correctly cite #264.

3. feeds add empty-UrlFeed impossible by construction. Because --url is required=True on add, the UrlFeed object always contains at least Url, so no silent empty subtype can leak. ✅

4. feeds update allows UrlFeed updates without --url. This is intentional and matches the WSDL (UrlFeedUpdate.Url is minOccurs=0). Tested by test_feeds_update_payload_accepts_urlfeed_details. No mismatch.

5. Minor (non-blocking) observations — flagged only because the request asks for any CLI-contract / API-mismatch surfaces, not as required changes:

• No way to clear Login/Password. if login: / if password: skip both None and empty strings, so users cannot send an empty Login/Password to unset previously-stored credentials. Yandex Direct's update model is "omit to leave unchanged", so this is almost certainly correct — but if the WSDL allows clearing via empty string, it would be the only way to remove auth from a feed that previously had it. Worth confirming against the Yandex docs you linked; not necessary to fix in this PR.
• remove_utm_tags.upper() is redundant. click.Choice(["YES", "NO"], case_sensitive=False) already normalizes the returned value to the canonical case in choices on Click ≥ 7.0. Harmless and matches the existing business_type.upper() style — keep for consistency.
• --password echoed in dry-run JSON. Same surface as other credential-bearing dry-run payloads in the repo; consistent with current behavior, no regression.

6. Issue #253 scope. PR body and the audit both indicate FileFeed is split to #264. I was unable to verify #264 exists from the runner (no gh permissions here) — if it doesn't, the audit will silently link a dead reference and test_optional_field_audit_links_resolve (if present) could fail. Quick sanity check on your end: gh issue view 264 -R axisrow/direct-cli.

Conclusion

PR is ready w.r.t. the stated scope. No bugs, no security regressions, no data-loss risks, no WSDL mismatch, no CLI contract violations identified. Issue #253 (UrlFeed details) is fully addressed; FileFeed deferral to #264 is consistent across audit + parity gate.
· branch: codex/issue-253-feeds-urlfeed-fields

Addressed review feedback in bb84815: added explicit update-only --clear-login / --clear-password flags that serialize UrlFeed.Login/Password as null, reject conflicting set+clear flags, and removed password usage from README examples. Verification: python3 -m pytest tests/test_dry_run.py -k "feeds and (urlfeed or clear or without_fields)"; 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.\n\n@codex @claude review. Focus on critical issues: bugs, security vulnerabilities, logical errors, data loss risks, performance problems, CLI contract violations, WSDL/API mismatch, and issue-scope gaps. Do NOT nitpick style, naming conventions, minor formatting, or subjective preferences; only flag issues that could break functionality, violate the public CLI contract, or make issue #253 incomplete.

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

Code Review — PR #265 (URL feed detail flags, follow-up commit bb84815)

Scoped to critical issues only.

Verdict

No blocking issues. The follow-up correctly implements the requested null-clearing path, the conflict guard, and the README scrub. Issue #253 scope (UrlFeed) is fully addressed; FileFeed is deferred to #264 consistently across audit + parity gate.

Todo

• Fetch diff vs origin/feat/issue-239-optional-fields-gate
• Read direct_cli/commands/feeds.py (clear-flag logic, null serialization, conflict guards)
• Read tests/test_dry_run.py (new clear/conflict cases)
• Read tests/test_wsdl_parity_gate.py and tests/WSDL_OPTIONAL_FIELD_AUDIT.md deltas
• Read README.md to confirm password example was removed
• Issue-scope cross-check (feeds add/update: complete UrlFeed and FileFeed WSDL parity #253 UrlFeed; FileFeed deferred to feeds add/update: add FileFeed typed upload support #264)
• Post final review

Findings

1. Clear-flag semantics correct. _url_feed_payload at direct_cli/commands/feeds.py:17-32 emits "Login": None / "Password": None only when the clear flag is true, falling back to the set value otherwise (if clear_login: … elif login: …). Dry-run output in test_feeds_update_payload_can_clear_urlfeed_credentials confirms the wire payload contains {"Login": None, "Password": None}, which tapi_yandex_direct serializes as JSON null — the only documented way to drop credentials on UrlFeedUpdate. ✅

2. Conflict guard correctly placed before payload build. feeds.py:179-184 raises click.UsageError for --login + --clear-login (and the password pair) before _url_feed_payload is called, so the contradictory combination cannot leak into the request. The guard uses login is not None (not truthiness) so even --login "" triggers the conflict path. ✅ Covered by test_feeds_update_rejects_setting_and_clearing_login. Note: the UsageError is then caught by the surrounding except Exception and re-raised as click.Abort() (exit code 1 instead of 2) — this matches the existing pattern for --name/--url empty validation in the same handler, so no regression vs. prior behavior.

3. Clear flags scoped to update only. add (feeds.py:99-120) does not expose --clear-login/--clear-password, and _url_feed_payload defaults clear_login=False, clear_password=False, so the add path can never emit a null for these fields. Correct — clearing makes no semantic sense on create.

4. Empty-subtype no-op gate preserved. The "provide at least one of …" message at feeds.py:201-204 now lists --clear-login and --clear-password, and test_feeds_update_without_fields_errors asserts both flag names appear in the error. The len(feed_data) == 1 guard still fires when nothing meaningful was passed. CLAUDE.md WSDL parity gate #1 (empty-subtype no-op) holds. ✅

5. WSDL parity audit deltas consistent. Counts shift missing_followup 2823→2817 (-6) and supported 418→424 (+6), matching the 6 row flips (3 UrlFeed.* × 2 ops). The mapping ("feeds","update","UrlFeed.Login"): {"--login", "--clear-login"} at tests/test_wsdl_parity_gate.py:806-807 (and the password equivalent) is correct — both flags resolve the same WSDL optional path, and set[str] membership is the audit's declared satisfaction model.

6. README scrub. Both EN (line 528) and RU (line 1235) one-liner examples no longer contain --password FEED_PASSWORD. They retain --login feedbot (non-secret), and the update example demonstrates --clear-login --clear-password — which is actually a better doc pattern than passing real credentials. ✅

7. Issue #253 scope. UrlFeed details (RemoveUtmTags, Login, Password — set and clear) are fully covered; FileFeed.* is consistently labeled as missing_followup linking #264 in both the follow-up table and the per-field audit. Cross-check passes. ⚠️ Sanity note (unchanged from prior review): I cannot verify #264 exists from this runner. Worth a quick gh issue view 264 -R axisrow/direct-cli if you have not already opened it — otherwise the audit links will be dead.

Non-blocking observations (no action required)

• add still passes --password on the command line. Same surface as the rest of the repo (and now unused in the EN/RU docs), no regression. If a future hardening pass adds prompt=True, hide_input=True and an envvar fallback, doing it for clients update, etc. at the same time would be more consistent than one-off here.
• _url_feed_payload still uses truthiness (if login:) for the set path. Empty string --login "" is silently dropped. This is fine because the clear flags now provide the explicit null path the API actually wants — empty-string ≠ null in Yandex's serializer, so dropping is the safer default. Not a regression.

Conclusion

PR fully addresses the Codex P2 feedback (explicit null path for credentials) and the Copilot security/UX feedback (README password scrub, conflict detection). No bugs, security regressions, data-loss risks, WSDL mismatch, or CLI contract violations identified. Issue #253 (UrlFeed details) is complete; FileFeed remains scoped to #264.

— branch: codex/issue-253-feeds-urlfeed-fields ·

Addressed the P1 CLI-contract review in 7401bb3: feed credentials now use --feed-login / --feed-password plus --clear-feed-login / --clear-feed-password, so they no longer collide with global direct --login. Also fixed the quality failure by annotating the UrlFeed payload dict for mypy. Verification: python3 -m pytest tests/test_dry_run.py -k "feeds and (urlfeed or clear or without_fields)"; python3 -m pytest tests/test_wsdl_parity_gate.py; mypy .; python3 -m pytest tests/test_cli.py tests/test_dry_run.py tests/test_wsdl_parity_gate.py.\n\n@codex @claude review. Focus on critical issues: bugs, security vulnerabilities, logical errors, data loss risks, performance problems, CLI contract violations, WSDL/API mismatch, and issue-scope gaps. Do NOT nitpick style, naming conventions, minor formatting, or subjective preferences; only flag issues that could break functionality, violate the public CLI contract, or make issue #253 incomplete.

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

Code Review — PR #265 (follow-up commit 7401bb3)

Scoped to critical issues only (bugs, security, logical errors, CLI contract, WSDL/API mismatch, issue-scope gaps).

Verdict

No blocking issues. The rename to --feed-login / --feed-password (+ --clear-feed-*) fully resolves the P1 global---login collision flagged by Codex. CLI contract, audit, parity gate, and README are internally consistent. Issue #253 (UrlFeed) is complete; FileFeed remains deferred to #264.

Todo

• Fetch diff vs origin/feat/issue-239-optional-fields-gate
• Read direct_cli/commands/feeds.py (renamed flags, conflict guard, mypy annotation)
• Read tests/test_dry_run.py deltas
• Read tests/test_wsdl_parity_gate.py mapping deltas
• Read tests/WSDL_OPTIONAL_FIELD_AUDIT.md deltas
• Read README.md example deltas
• Cross-check P1 collision fully resolved (no remaining --login/--password on feeds)
• Post review

Findings

1. P1 root-login collision fully resolved. direct_cli/cli.py:175 defines the root-level @click.option("--login", envvar="YANDEX_DIRECT_LOGIN") for Client-Login. The PR now uses --feed-login / --feed-password on feeds add (feeds.py:106-107) and feeds update (feeds.py:162-163), and --clear-feed-login / --clear-feed-password for the null path (feeds.py:164-167). grep confirms zero remaining --login / --password occurrences inside feeds.py. Click parses parent options before subcommand options, but having the same name on both still risks confusion in scripted invocations; the distinct prefix removes that ambiguity entirely. ✅

2. Conflict guard correct and exit code preserved. feeds.py:184-191 raises click.UsageError for --feed-login + --clear-feed-login (and password equivalent) before _url_feed_payload is built. The check is feed_login is not None and clear_feed_login, so even --feed-login "" triggers the conflict — empty-string cannot smuggle past the guard. The new except click.UsageError: raise clause at feeds.py:229-230 ensures the UsageError keeps Click's standard exit code 2 instead of being swallowed into click.Abort (exit code 1). That's an improvement over the v2 review's noted exit-code regression. ✅

3. Null-clearing semantics correct. _url_feed_payload (feeds.py:14-32) emits "Login": None / "Password": None only when the corresponding clear flag is true, falling back to the set value otherwise (if clear_login: … elif login: …). Dry-run test test_feeds_update_payload_can_clear_urlfeed_credentials (tests/test_dry_run.py:3478-3488) confirms {"Login": None, "Password": None} is emitted — the only path the Yandex API documents for dropping UrlFeedUpdate credentials. ✅

4. Clear flags restricted to update. add exposes only --feed-login / --feed-password (no clear variants), and _url_feed_payload's clear_login=False, clear_password=False defaults are unreachable from the add path. No way to send a null on create. Correct. ✅

5. Empty-subtype no-op gate preserved. feeds.py:207-212 error message now lists every flag combination that would satisfy the gate (--name, --url, --remove-utm-tags, --feed-login, --feed-password, --clear-feed-login, --clear-feed-password). Test test_feeds_update_without_fields_errors asserts presence of --name, --url, --remove-utm-tags, and --clear-feed-login in the error. CLAUDE.md WSDL parity gate invariant #1 holds. ✅

6. WSDL parity gate / audit math coherent. OPTIONAL_FIELD_CLI_OPTIONS updates at tests/test_wsdl_parity_gate.py:800-811 correctly reference the renamed flag names:

• ("feeds","update","UrlFeed.Login"): {"--feed-login","--clear-feed-login"}
• ("feeds","update","UrlFeed.Password"): {"--feed-password","--clear-feed-password"}

Audit counts shift missing_followup 2823→2817 (-6) and supported 418→424 (+6), matching exactly the 6 row flips (3 UrlFeed × 2 ops). The per-field rows for UrlFeed.Login/Password on update now list both flags in the satisfaction note. FileFeed.* (6 rows) consistently re-tagged with #264. ✅

7. mypy annotation correct. payload: Dict[str, object] (feeds.py:21) lets the dict accept both str values (Url, RemoveUtmTags, Login, Password) and the explicit None for clear paths. Dict[str, str] would have failed under --strict. ✅

8. Issue #253 scope. UrlFeed.* (RemoveUtmTags, Login, Password — set and clear) is complete. FileFeed.* (FileFeed, FileFeed.Data, FileFeed.Filename — both ops) is consistently labeled missing_followup → #264 in both audit table and parity-gate OPTIONAL_FIELD_AUDIT. ✅

Non-blocking observations (no action required)

• add handler still lacks except click.UsageError: raise. add (feeds.py:148-150) has only the catch-all except Exception. In practice add doesn't raise UsageError from inside the try block (all required=True validation runs before the try), so there's nothing to misroute today. If a future change adds a body-side UsageError to add, mirror the except click.UsageError: raise pattern used in update. Not a regression.
• --feed-password still echoed in dry-run JSON. Same surface as other credential-bearing dry-run payloads in this repo (e.g., agencyclients add-passport-organization); consistent and explicitly accepted in earlier reviews. Hardening (envvar-backed + hide_input=True + dry-run redaction) is worth a dedicated cross-cutting follow-up rather than one-off here. Not a regression introduced by this PR.
• UsageError is now raised, not abort-wrapped. This changes the exit code on validation failure for feeds update from 1 → 2. If any external script greps for the previous Abort exit code, it would need adjustment — but standard CLI conventions (and the rest of the codebase's typed-flag handlers, see clients update) use 2 for usage errors, so this aligns rather than diverges.

Conclusion

PR is ready. The P1 collision and all prior P2 nits (null path, conflict detection, README password scrub) are resolved. No bugs, no security regressions, no WSDL/API mismatch, no data-loss risks, no CLI contract violations identified. Issue #253 fully addressed; FileFeed follow-up #264 consistently linked across audit + gate.

— · branch: codex/issue-253-feeds-urlfeed-fields

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