@codex @claude review

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

Claude PR Review — Add feeds FileFeed upload support

• Read CLAUDE.md for repo-specific conventions
• Review direct_cli/commands/feeds.py changes
• Review test coverage (tests/test_dry_run.py, tests/test_wsdl_parity_gate.py)
• Review docs/README & WSDL audit changes
• Post review findings

Overall this is a clean, well‑scoped PR: a typed FileFeed surface, dry‑run fidelity, six new dry‑run tests covering both add and update, and matching WSDL‑parity bookkeeping (audit rows flipped to supported, follow‑up rows removed, new INTERNAL_VALIDATION + INTERNAL_VALIDATION_PROBES entries for the SourceType discriminator). Code matches the conventions in CLAUDE.md (typed flags, UsageError before request construction, no raw JSON for write payloads).

Below are observations worth considering — nothing here is blocking.

Notable observations

1. _FILE_FEED_MAX_BYTES = 50 * 1024 * 1024 is a magic constant with no cited source — direct_cli/commands/feeds.py:16

The 50 MiB cap is pre‑base64; the wire payload after b64 + JSON envelope is ~33% larger. The Yandex Direct docs publish a feed file size limit, but I couldn't find an explicit FileFeed.Data inline-request limit they cite. If 50 MiB came from a doc, a one‑line comment with the URL would future‑proof this; if it's a defensive guess, that's worth saying too so the next maintainer doesn't tighten/loosen it blindly. Either way the API will give an authoritative error — this is just guard‑rail clarity.

2. _FILE_FEED_MAX_FILENAME_LENGTH = 255 likewise has no source — direct_cli/commands/feeds.py:17

255 is a common filesystem NAME_MAX, but the API limit (if any) may be different. Same suggestion: cite or note "defensive".

3. add: the _has_url_feed_options(url, ...) check is redundant on url — direct_cli/commands/feeds.py:204-206

Lines 200–203 already short‑circuit when url and file_feed_path are both set or both unset. By the time line 204 runs, url is guaranteed None whenever file_feed_path is truthy, so passing url into _has_url_feed_options is dead. Harmless, but slightly misleading to a reader. Could drop the url argument from the call.

4. _file_feed_payload: empty‑filename guard is unreachable from the CLI — direct_cli/commands/feeds.py:49-51

click.Path(dir_okay=False) ensures path.name is non‑empty, and "" or path.name falls back to path.name when the user passes --file-feed-filename "". So filename cannot end up "". Fine to keep as belt‑and‑braces, just noting it's not exercised by any test path.

5. No CLI‑side guard against changing SourceType on update

The PR body cites the Yandex constraint that "changing source/business type is not allowed" on feeds.update. The CLI happily accepts --file-feed-path against a feed that was originally URL (or vice versa) and lets the API reject it. That's a reasonable tradeoff — checking would require a feeds.get round‑trip — but worth being explicit in the help text or a CLAUDE.md/PR note so users aren't surprised. Fix this →

6. Filename is forwarded verbatim, no path-traversal stripping — direct_cli/commands/feeds.py:49

If a user passes --file-feed-filename "../evil.csv", that string is sent to the API as‑is. Server‑side this is almost certainly sanitised, and the security boundary is the API, not the CLI — flagging only because the same pattern is sometimes treated more defensively elsewhere in the repo.

7. Dry‑run reads and base64‑encodes the file

This is intentional and correct (otherwise dry‑run wouldn't show a faithful payload), but for a 50 MiB feed it means --dry-run does ~67 MB of base64 work and prints it to stdout. Not a bug; just a UX note worth mentioning in the README example if anyone tries it with a real file.

Tests & WSDL bookkeeping

tests/test_dry_run.py adds six tests covering: FileFeed upload payload shape (add/update), filename override, mixed --url/--file-feed-path rejection, --file-feed-filename without path rejection, and the new field appearing in the update "no fields" error. Coverage looks complete for the new surface.

tests/test_wsdl_parity_gate.py correctly:

• adds --file-feed-path to the SourceType discriminator mapping;
• adds an INTERNAL_VALIDATION row + INTERNAL_VALIDATION_PROBES row so the new UsageError is exercised against the gate;
• removes the two feeds.add / feeds.update follow‑up entries from OPTIONAL_FIELD_DEFAULT_FOLLOWUPS and OPTIONAL_FIELD_AUDIT;
• adds the FileFeed.* rows to OPTIONAL_FIELD_CLI_OPTIONS.

tests/WSDL_OPTIONAL_FIELD_AUDIT.md shows the audit row flips (missing_followup → supported) and a +6/-6 swing in the totals, consistent with the six paths newly covered. Looks correct.

Verdict

Looks ready to merge once the magic‑constant sourcing is either documented or explicitly called out as defensive. The substantive concerns are all minor — the security boundary, the parity bookkeeping, and the test coverage are all in good shape.
· Branch: codex/issue-264-feeds-filefeed

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

Addressed actionable review feedback in ea61cf2:

• fix: check file size with path.stat().st_size before reading FileFeed data.
• fix: add dry-run rejection tests for oversized FileFeed input and overlong filename.
• fix: document FileFeed size/filename limits in code comments and clarify update help text around Yandex Direct SourceType switching.

Re-running review after fix: @codex @claude review

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

Claude PR Re-Review — Add feeds FileFeed upload support

• Pull latest changes and inspect commit ea61cf2
• Review the size-check fix in direct_cli/commands/feeds.py
• Review new dry-run rejection tests
• Review documentation/help-text updates around SourceType switching and size limits
• Post review findings

Verdict: LGTM — all review items addressed

The follow-up commit cleanly resolves every actionable item from the prior round. No new concerns.

Item-by-item verification

1. Copilot #1 — read entire file before size check (direct_cli/commands/feeds.py:70)
Fixed at direct_cli/commands/feeds.py:62-79. path.stat().st_size is now checked before path.read_bytes(), so oversized files are rejected before being loaded into memory. The double OSError handling (one for stat(), one for read_bytes()) is defensible — handles a race between stat and read.

2. Copilot #2 — missing tests for oversized file / overlong filename
Fixed at tests/test_dry_run.py:4099-4131:

• test_feeds_add_rejects_overlong_filefeed_filename — exercises the 255-char cap with "x" * 256.
• test_feeds_add_rejects_oversized_filefeed_before_reading — clever use of fh.truncate((50 * 1024 * 1024) + 1) to create a sparse 50MiB+1 file without consuming disk. This implicitly proves the size check happens before read_bytes() (otherwise the test would OOM/be slow). Nice.

3. Prior review #1 / #2 — magic constants without source
Both constants now carry comments at direct_cli/commands/feeds.py:16-21:

• _FILE_FEED_MAX_BYTES is explicitly described as a defensive pre-read guard with the API as the final validator. ✓
• _FILE_FEED_MAX_FILENAME_LENGTH = 255 is attributed to "feeds.add/update docs". ✓

4. Prior review #5 — no CLI warning on SourceType switching
Fixed at direct_cli/commands/feeds.py:264, 271. Both --url and --file-feed-path help text on feeds update now explicitly says "Yandex Direct rejects source switches." Users will see this in --help.

5. Prior review #3 — redundant url arg in _has_url_feed_options call on add
Fixed at direct_cli/commands/feeds.py:215-216 — now passes None for the url slot, accurately reflecting that the url short-circuits above have already excluded the combined case.

WSDL parity bookkeeping

Cross-checked the audit/gate changes:

• WSDL_FIELD_TO_CLI_OPTION["SourceType"] now includes --file-feed-path (tests/test_wsdl_parity_gate.py:553).
• New INTERNAL_VALIDATION row + matching INTERNAL_VALIDATION_PROBES entry for ("feeds","add","SourceType") is wired correctly (tests/test_wsdl_parity_gate.py:605, 1748-1755).
• OPTIONAL_FIELD_CLI_OPTIONS adds the six new FileFeed.* rows; OPTIONAL_FIELD_DEFAULT_FOLLOWUPS and OPTIONAL_FIELD_AUDIT entries for feeds.add/feeds.update follow-ups are removed.
• tests/WSDL_OPTIONAL_FIELD_AUDIT.md flips all six rows to supported and updates the totals (missing_followup -6, supported +6) — consistent with what was implemented.

Minor nit (non-blocking)

The new test_feeds_add_rejects_oversized_filefeed_before_reading test name is great — it advertises the invariant it proves. If you ever want a belt-and-braces version, you could add an explicit assertion that the test runs in <100 ms (e.g. by wrapping in a time.monotonic() check). Skip if you find that overkill — the sparse-file pattern already guarantees it implicitly.

Nothing else to flag — ready to merge.
· Branch: codex/issue-264-feeds-filefeed

Codex Review: Didn't find any major issues. Hooray!

Final review classification after fix and 5-minute window:

• Copilot: fix - file size is now checked via stat() before reading FileFeed data.
• Copilot: fix - oversized FileFeed and overlong filename branches now have focused dry-run rejection tests.
• Claude initial notes: fix for limit comments, redundant URL argument, and update help text around SourceType switching; remaining dry-run/filename/path notes are no-op because they are non-blocking UX/security-boundary observations and the API remains final validator.
• Claude re-review: LGTM, all review items addressed.
• Codex: no major issues.
• Independent subagent review: complete, no blockers.

No blocking/actionable comments remain. CI is green.