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

Claude PR Review — Done

• Read the diff vs origin/main
• Inspect direct_cli/commands/feeds.py changes
• Inspect test changes (tests/test_dry_run.py, tests/test_api_coverage.py)
• Compare with Wave 1 sitelinks pattern (PR feat(api): expose separate *FieldNames params for sitelinks and keywordbids #403) for consistency
• Post review findings (critical issues only)

Verdict

No blocking issues found. This is a clean, narrow change that faithfully mirrors the already-merged Wave 1 sitelinks pattern (direct_cli/commands/sitelinks.py:105-184). Recommend merge.

Critical-issue checks (per trigger filter)

• Bugs / logical errors — None. The empty-CSV guard if file_feed_field_names is not None and not parsed_file_feed_field_names (direct_cli/commands/feeds.py:157, 163) correctly distinguishes "flag not passed" (None → silently omit, matches WSDL minOccurs="0" at tests/wsdl_cache/feeds.xml:179-182) from "flag passed with empty/whitespace value" (e.g. , → reject). parse_csv_strings returns None in both the "input is None" and "input parses to empty list" cases, so the dual-check (raw arg is not None and parsed result falsy) is necessary and correct.
• UsageError swallowing — The except click.UsageError: raise re-raise at direct_cli/commands/feeds.py:203 is required because the broader except Exception below would otherwise turn a UsageError into print_error(str(e)) + click.Abort() and lose Click's standard usage-error exit semantics. Correctly handled, matches sitelinks.
• Payload injection — Keys are only added to params when non-empty (direct_cli/commands/feeds.py:178-181), so defaults aren't perturbed for callers who don't opt in. Backward-compatible.
• Security — Pure string-list passthrough to the typed API client; no shell, no eval, no path/URL handling. No injection surface introduced.
• Performance — One small CSV split on a short flag value per invocation. Negligible.
• Data loss / production safety — feeds.get is read-only; no mutating risk. Not in WRITE_SANDBOX / DANGEROUS paths.
• WSDL parity — Both rows correctly removed from NESTED_FIELDNAMES_EXCLUSIONS (tests/test_api_coverage.py:2537-2538 deletion) so test_every_nested_fieldnames_param_has_cli_option now enforces them unconditionally. WSDL declares both fields with minOccurs="0", matching the optional-flag treatment in code.
• Test coverage — 5 new tests in tests/test_dry_run.py:22013-22078: payload shape, default omission, --help visibility, and both empty-CSV rejections. The empty-CSV tests use "," (single comma) to force parse_csv_strings to return None while the raw arg is non-None — exercising the exact branch in the new guard.

Observations (non-blocking, not requested)

• No client-side validation of enum membership (Filename / Login,Url,RemoveUtmTags) — values are forwarded to the API as-is. This is consistent with how sitelinks handles SitelinkFieldNames and other *FieldNames flags across the codebase, so it's not a deviation. Server returns the appropriate error if an invalid enum value is sent.
  · branch feat/412-feeds-nested-field-names