[BUG]: Binary Files Check Throws False Positives

[Rd4dev]

Describe the bug

The current binary file detection logic in [pre-commit.sh](# Pre-commit hook to check for binary files.) (introduced in #5525) uses the file --mime command to identify binary files, but this approach leads to false positives for non-binary text-based files like: .textproto, .json, .xml, .txt, .proto, .md.

if file --mime "$file" | grep -q 'binary'; then

Steps To Reproduce

1. Introduce a new .textproto or .md file locally.
2. Run the scripts/pre-commit.sh script.
3. The script will fail with:

Please remove the 1 detected binary file(s).
BINARY FILES CHECK FAILED

4. Even though the files are valid text files.

This issue was recently observed in a PR #5836 where new .textproto files were introduced but mistakenly identified as binary files.

Expected Behavior

Text files such as .txt, .md, .json, .xml, .textproto should NOT be classified as binary. These files are human-readable and should be allowed in commits without triggering binary file check failures.

Binary files include formats such as:

• Images: .png, .jpeg, .jpg, .gif, .bmp, .webp
• Videos: .mp4, .mov, .avi, .mkv
• Archives and executables: .zip, .tar, .gz, .exe, .bin
• Compiled objects and libraries: .class, .so, .dll
• Other binary data files

The check should only flag and fail on actual binary files to prevent unwanted binary files in the repository. Files that are text-based or human-readable should pass the binary check without warnings or failures.

Proposed / Alternate Approach:

An alternative is to use Git’s own diff statistics:

base_commit=$(git merge-base origin/develop HEAD)
binary_files=$(git diff --numstat "$base_commit"...HEAD | awk '$1 == "-" && $2 == "-" { print $3 }')

This approach was tested in the fork PR and appeared to correctly detect only genuine binary files (e.g., images), without false positives on text files.

It’s important to test different edge cases —or to check with team and see maybe introducing a Kotlin script with proper testing could be a more sustainable solution to avoid future regressions.

Additional Context

Additional Findings / Suggestions:

1. Extending Pre-commit Hook: Currently, the pre-commit script was only setup to check for binary files. It could be refactored to allow running multiple checks by move the existing binary file check into a separate script (e.g., binary-files-check.sh) and have the main pre-commit hook call this script..
2. Executable Permissions: Some of these scripts may require executable permissions to run properly. It could be useful to clarify whether executable permissions should be automatically set or whether explicit documentation should be provided to enable them.
3. Formatting colors in CI: The sourcing of the formatting.sh did not embed color codes in the CI, while utilizing color codes right from the file, does work.

[BenHenning]

Ah. I didn't realize this check only verified against new files.

FWIW I'm not sure that checking against file extensions is going to be the most reliable approach here. Perhaps we could do the following:

• Check extensions first as an early failure (so that we have to essentially allowlist all valid extensions in the codebase--that's probably fine).
• Verify all files only contain text. This part is trickier, but I don't think it would be unreasonable. We just need to validate that all files only contain ASCII, plus maybe a few additional allowlisted Unicode characters (I know we use some for non-English languages). In fact, we could probably just use isLetter as part of the check here per https://kotlinlang.org/api/core/kotlin-stdlib/kotlin.text/is-letter.html. That follows the Unicode specification for letters so it will work across languages.
• Update the check to operate against all files, not just new files.

This will likely mean migrating this check over to Kotlin.

As for the actual validity check, perhaps we could start with exclusively allowing:

• ASCII
• Digits and letters (per Kotlin's checks)
• Additional characters on a case-by-case basis (since I think these will be rarer)

[BenHenning]

Also sent #5849 to disable the check until it can be fully fixed since we can't let something like this stay blocking.

[Sandesh282]

@BenHenning I'd like to take this on. Here's my proposed approach based on your comment above:

Approach

Migrate the check from shell (file --mime) to a Kotlin script following existing patterns (like TestFileCheck), with a two-layer detection:

Layer 1 — Extension check (fast path):

• Maintain an allowlist of known text extensions (.kt, .java, .xml, .json, .textproto, .proto, .md, .sh, .bazel, .yaml, .yml, etc.)
• Maintain a denylist of known binary extensions (.png, .jpg, .gif, .webp, .jar, .aar, .zip, .gz, .class, .so, etc.)
• Unknown extensions → treated as binary (fail-safe)

Layer 2 — Content validation (for allowed extensions):

• Verify file contents only contain valid text characters:
  • ASCII printable characters (0x20–0x7E)
  • Common whitespace (tab, newline, carriage return)
  • Unicode letters (Char.isLetter() per Kotlin's Unicode spec)
  • Unicode digits (Char.isDigit())
• Any character outside this set → flag as binary

Deliverables:

• BinaryFileCheck.kt in scripts/src/java/org/oppia/android/scripts/binary/
• BinaryFileCheckTest.kt with tests for: text files passing, binary extensions failing, unknown extensions failing, Unicode content passing, null bytes failing, .textproto passing (the original false positive case)
• CI integration in the appropriate workflow
• Update/remove the disabled pre-commit.sh check

Questions before starting

1. Should .pb files (e.g., test_file_exemptions.pb) be exempted or flagged as binary?
2. Should this operate on all files or only changed files per PR?
3. Is Char.isLetter() + Char.isDigit() sufficient for the Unicode scope, or would you prefer a more restrictive starting set?

Happy to adjust the approach based on your feedback. Thanks!

[BenHenning]

That plan seems really reasonable @Sandesh282. The only feedback I have is:

• Just using an allow list is probably sufficient. If we add a new type of text file then we can update the allow list (the failure should make this clear to developers who run into it).
• The character check seems reasonable (letter, digit, or printable ASCII).
• This should operate on all files since once it's checked in everything should be valid in the codebase.
• We will need an exemption list for valid checked-in binary files. We ought to exempt the whole filepath (rather than the type of file since binary files are exempted on a case-by-case basis), and fail if any of the exemption paths don't point to a real file (to ensure that moved/removed files are cleaned up from the exemption list).

Assigning this over to you as well.