fix(detectors): stop FileDetector scoring an unexamined file as a pass - #2080
fix(detectors): stop FileDetector scoring an unexamined file as a pass#2080CTWalk wants to merge 1 commit into
Conversation
FileDetector.detect() coerced a None from _test_file to 0.0, the no-hit end of the scale, while _test_file is declared -> Union[None, float]. FileIsExecutable returns that None when libmagic is missing, so every file it was handed came back as not an executable without being examined. Yield the None instead, so the evaluator counts it in nones and reports SKIP. This is the call NVIDIA#2029 made for the format-mismatch branch of the same function. test_fileisexectuable_nonexec passed without the detector running; it now expects [0.0] where libmagic is present and [None] where it is not, so it runs in both environments. Co-authored-by: Claude Signed-off-by: CTWalk <100585900+CTWalk@users.noreply.github.com>
34c6acc to
0a93ee1
Compare
cmcnosky
left a comment
There was a problem hiding this comment.
At head 0a93ee1c, the change preserves the None returned by FileIsExecutable._test_file() when the magic dependency is unavailable. At base 7ff1f277, FileDetector.detect() coerces that result to 0.0, causing ZeroToleranceEvaluator to report PASS; at the head it remains unscored, producing SKIP with zero passes, zero failures, one unscored result, and zero evaluated results.
The base-class edit is in the right place because that is where the distinction between “not examined” and “examined, no hit” was lost. FileIsExecutable is the only current in-tree _test_file() implementation that returns None; external subclasses using the documented Union[None, float] contract will also preserve their unscored result.
Verification at the exact head on Python 3.12.13 and macOS arm64:
-
python -m pytest tests/detectors/ -q— 757 passed, 34 skipped -
python -m pytest tests/detectors/test_detectors_base.py tests/detectors/test_detectors_fileformats.py tests/evaluators/test_base.py -q— 51 passed, 6 skipped -
python -m pytest tests/evaluators/ -q— 58 passed -
git diff --check 7ff1f277..0a93ee1c— clean
No code-review blockers found.
Summary
FileDetector.detect()ends withwhile
_test_fileis declared-> Union[None, float](detectors/base.py:316)and
0.0is the no-hit end of the scale. ANonemeaning "I could not examinethis file" is reported as "this file is not a hit".
FileIsExecutable._test_filereturns thatNonewhenself.magic is None,which happens whenever
python-magicimports but the systemlibmagicismissing:
_load_depscatches theImportErrorand logs it. On such a machinethe detector never reads a byte and every file it is handed comes back as not an
executable.
Yielding the
Noneinstead lets the evaluator count it innonesand printSKIPrather thanPASS. That is the call #2029 already made for the otherbranch of this same function, and it matches
StringDetector's[None] * len(all_outputs)andHFDetector'sgraceful_fail. The edit is indetectors/base.pybecause the coercion is there; a subclass cannot undo it.FileDetector's docstring already documented the format-mismatchNone, so itgains one sentence for this one.
Two things to flag rather than leave to review.
analyze/analyze_log.py:60andanalyze/qual_review.py:82compare scores with>=/>and raiseTypeErroron a
None; that is pre-existing, since 20 detector modules already emitNoneand this makes
fileformatsthe 21st, so it wants its own fix. And_test_filecould raise instead, but the base signature already permits
Noneand the wholedetector layer uses that channel.
Reproduction
On a machine without
libmagic, the condition your suite already guards for withskipif(magic is None):test_fileisexectuable_nonexecwas one of the cases that passed without thedetector running: it asserts
[0.0]on a text file,_test_filereturnedNone, andelse 0.0supplied the expected answer. Rather than guard it withskipifand lose the case, it now expects[0.0]where libmagic is present and[None]where it is not, so it runs everywhere and asserts something true inboth. Nothing that ran before is skipped now.
The new test monkeypatches
magic = None, so it too runs either way. Revertingthe one-line fix and keeping both tests gives
2 failed.Why this is not a duplicate
continueskips into
yield Nonefor output/result alignment. The finalelse 0.0appears in its diff only as unchanged context, and its test double's
_test_filenever returnsNone.Nonescores shrinking the ASRdenominator. This one is upstream of it: the
Nonenever arrives.detectors/base.pyand applies the same idiom, oneNoneper output instead of a score, to
TriggerListDetector. Different class,different region.
fixed in
fileformats.py. Different input, different line.Verification
python -m pytest tests/detectors/ -q: 744 passed, 34 skipped against743 passed, 34 skipped on an unpatched tree. Same 13 errors either way,
all in
test_detectors_judge.pyand unrelated to this changepython -m pytest tests/detectors/test_detectors_fileformats.py -q:18 passed, 6 skipped, up from 17 passed, 6 skipped
black25.1.0 clean on the lines touchedpython -m pytest tests/in full, on the submitter's machineAI assistance
AI assistance was used in preparing this change, per
AGENTS.md. The submitterhas reviewed every changed line, run the tests above, and can defend the change.