fix(joblib): fail closed on inconclusive warning scans - #1796
fix(joblib): fail closed on inconclusive warning scans#1796mldangelo-oai wants to merge 3 commits into
Conversation
|
@codex review |
There was a problem hiding this comment.
Pull request overview
This PR fixes a fail-open behavior in the Joblib scanner: when Joblib NumPy-wrapper validation is inconclusive, the scan now fails closed even if the embedded pickle analysis reports warning-level findings (e.g., origin verification warnings). This aligns the scanner’s success semantics with the project’s fail-closed expectations for incomplete analysis and adds a deterministic regression for a Windows-observed failure mode.
Changes:
- Update
JoblibScanner.scan()to always returnsuccess=Falseforscan_outcome=inconclusiveresults unless a trusted incomplete tail is explicitly present. - Add a regression test covering an invalid NumPy wrapper combined with an embedded warning-level finding, ensuring the scan fails closed and is not cacheable.
- Document the fix in the root changelog under Bug Fixes.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
modelaudit/scanners/joblib_scanner.py |
Makes inconclusive Joblib scans fail closed regardless of embedded warning findings (unless a trusted incomplete tail is present). |
tests/scanners/test_joblib_scanner_codecs.py |
Adds a deterministic regression reproducing the prior fail-open path (inconclusive wrapper + embedded WARNING finding). |
CHANGELOG.md |
Notes the Joblib fail-closed behavior change under Bug Fixes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Performance BenchmarksCompared
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 713940829d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
Security review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
…ickle scanner The existing regression stubs out the embedded pickle scanner entirely, so it proves the guard but not the lane that actually failed. This adds a companion regression that keeps real payload parsing and metadata composition and forces only the embedded verdict to success=True - the state Windows reaches when the embedded result retains a trusted incomplete tail. Both regressions fail on main and pass with the scan() guard. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Suppressed comments (1)
tests/scanners/test_joblib_scanner_codecs.py:1426
- The test forces an embedded pickle scan to report
success=Trueby directly mutatingScanResult.successand the private_merged_children_successfield afterfinish(). This bypassesScanResult.finish()invariants (e.g., restored-critical handling) and couples the test to private implementation details. Prefer the publictrust_merged_child_failures()API and re-runfinish(success=True)to recomputesuccessconsistently.
if not result.success:
result.success = True
result._merged_children_success = True
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
@codex review |
|
@codex security review |
|
Codex Review: Didn't find any major issues. More of your lovely PRs please. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
Security review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
Summary
Keep Joblib scans failed closed whenever NumPy-wrapper validation is inconclusive, regardless of what the embedded pickle scanner reports.
The defect
JoblibScanner.scan()failed an inconclusive scan only when there were no WARNING/CRITICAL findings:That conflates "we found something" with "we finished the analysis". When findings exist the scanner takes the
elsebranch and reportssuccess=Trueas long as no finding is CRITICAL — even thoughscan_outcome=inconclusiveandanalysis_incomplete=true.Joblib was the outlier. Roughly a dozen scanners already use the unconditional form this PR adopts —
lightgbm,catboost,coreml,rknn,r_serialized,paddle,xgboost,skops,flax_msgpack,llamafile,pytorch_zip,torchserve_marall finish withscan_outcome != INCONCLUSIVE and not has_errors. This change brings joblib in line, keeping only the joblib-specifictrusted_incomplete_tailexemption.Correction to the original justification
This PR was originally filed as the fix for a recurring Windows CI failure of
test_scan_fails_closed_on_protocol1_pickle_in_numpy_wrapper_tail. That claim was wrong and has been removed.The Windows lane passes on #1780, #1783, #1788, #1790 and #1791 — every one of which contains
mainwithout this fix. The test is therefore intermittent, not deterministically failing, and this change is not what makes it pass. The Windows failures on #1792/#1793 were separately traced to stale July-30 runs that predated #1795.The defect above is real and reproducible on POSIX independently of any platform flake, which is the basis for merging this. It is a correctness fix, not a CI fix.
Why it stays hidden on POSIX in normal runs:
ScanResult.finish()computessuccess and self._merged_children_success, and the embedded pickle scanner usually returnssuccess=False, which pins the parent toFalse. The joblib-level guard only becomes observable when the embedded result reports success.Coverage
Two regressions, both failing on
mainand passing here:test_scan_fails_closed_on_invalid_numpy_wrapper_with_origin_warning— stubs the embedded scanner to isolate the guard.test_scan_fails_closed_when_embedded_pickle_reports_success— keeps the real pickle scanner, real payload parsing, and real metadata composition, forcing only the embedded verdict.Demonstrated behaviour change with the embedded verdict forced to success:
successscan_outcometrusted_incomplete_tailTrueinconclusiveFalseinconclusiveValidation
tests/scanners/test_joblib_scanner_codecs.py,test_joblib_scanner.py,test_pickle_scanner.py: 472 passed.