fix: reject diffs containing binary files instead of silently dropping them

[vitali87]

Summary

Coverage validation is computed over hunks. A binary PatchedFile has zero hunks, so a plan that covered only the text files passed validate_plan with no warnings and the binary change (e.g. an updated image) was never materialised into any branch — it silently vanished from the split.

• New validate_no_binary_files(parsed_diff) raises PlanValidationError(BINARY_FILES_UNSUPPORTED) listing the files
• Called first in validate_plan, and in split right after parsing so the failure is reported before an LLM call is spent

Test plan

• New tests: binary diff raises; text-only passes; validate_plan rejects even when all text hunks are covered
• uv run ruff check / ruff format --check clean
• uv run pytest -q — 447 passed

[coderabbitai]

Warning

Review limit reached

Next included review available in 50 minutes.

View limit details

Limit details: You’ve used the included review currently available.

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: d0340e7f-4840-46e0-8533-e2c7a1200114

📥 Commits

Reviewing files that changed from the base of the PR and between fc6ad35 and 4a5e22a.

📒 Files selected for processing (6)

• pr_split/cli.py
• pr_split/exceptions.py
• pr_split/planner/__init__.py
• pr_split/planner/validator.py
• tests/test_cli_coverage.py
• tests/test_validator.py

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[greptile-apps]

Greptile Summary

Binary-diff rejection now protects newly generated split plans, but executing an existing dry-run plan still skips that check. A saved plan containing a binary file can continue toward branch creation and omit that file from the generated split branches.

Confidence Score: 4/5

Not ready to merge: saved plans with binary changes can be executed without rejecting the unsupported binary file.

One verified non-security correctness issue remains in the saved-plan execution path.

Files Needing Attention: pr_split/cli.py

T-Rex Logs

What T-Rex did

• Reproduced the focused execute binary-preflight reproduction source and ran the binary validation control output and preflight acceptance outputs to validate the P1 finding.
• Noted a second finding-comment proof for a P1 finding and linked it to the corresponding review discussion.
• Validated the general-contract-validation-proof showing the script constructed a saved plan with binary img.png and an assigned a.py text hunk, mocked Git/GitHub prerequisites, and stopped before branch creation, confirming the acceptance path is taken.
• Inspected the reproduction source and the binary-validation and preflight-acceptance logs to verify the workflow steps and the evidence supporting the P1 findings.

_{Ran code and verified through T-Rex}

Comments Outside Diff (2)

1. pr_split/cli.py, line 1031 (link)

   Saved-plan binary guard is bypassed

   execute() parses a saved dry-run plan and validates only text-hunk coverage. Binary patches have no hunks, so a plan containing a binary file plus fully assigned text changes passes this check and continues toward branch creation, where the binary change can be omitted. Call validate_no_binary_files(parsed_diff) here, or perform equivalent full plan validation, before accepting the saved plan.

   Artifacts

   
   

   Focused execute binary-preflight reproduction source

   • This script constructs the saved binary-plus-text plan, runs the direct validator control, invokes `execute()` with external prerequisites mocked, and stops after preflight; takeaway: it isolates the missing execute binary-validation call.

   Binary validation control output

   • This command output records the direct control run rejecting binary img.png in the shared fixture; takeaway: the available binary validator correctly blocks this diff.

   Execute preflight acceptance output

   • This command output records `execute()` reaching post-coverage presentation with zero binary-validator calls and one coverage-validator call; takeaway: execute accepts the unsafe saved plan through preflight.

   

   _{Ran code and verified through T-Rex}

2. General comment

   execute omits binary-file validation for saved dry-run plans

   • Bug
     • execute() accepts the binary-plus-text saved plan through coverage because binary files expose no hunks for validate_coverage(), allowing the binary change to proceed toward omission during later materialization.
   • Cause
     • pr_split/cli.py:1028-1035 parses the saved diff and calls only validate_coverage, rather than invoking validate_no_binary_files or an equivalent full plan validation.
   • Fix
     • Call validate_no_binary_files(parsed_diff) before validate_coverage in execute() and handle its PlanValidationError through the existing preflight error path.

   _{Ran code and verified through T-Rex}

_{Reviews (2): Last reviewed commit: "fix: reject diffs containing binary file..." | Re-trigger Greptile}

[vitali87]

Good catch — execute skipped the new check, so a dry-run plan saved with a binary file could still proceed to branch creation and silently drop it. Pushed a commit that runs validate_no_binary_files on the saved diff before validate_coverage in execute (same red-message/exit-1 handling), plus test_execute_rejects_saved_plan_with_binary_files, which fails without the fix and asserts _create_branches_and_commits is never called.