refactor: consolidate scan routing and result contracts - #1872
mldangelo-oai wants to merge 18 commits into
Conversation
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. |
Performance BenchmarksCompared
|
|
@codex review |
There was a problem hiding this comment.
Copilot review overview
🔵 Needs a closer look
It spans many security-sensitive routing and result contracts without a complete full-suite or cross-platform validation run.
Review effort: Balanced
Findings: None
What changed in this PR
Consolidates security scan routing, result handling, and duplicated scanner utilities while preserving existing detection and fail-closed contracts.
Changes:
- Centralizes routing, inconclusive-result, finding-identity, and scanner helper logic.
- Removes unused wrappers and duplicate implementations across scanners.
- Adds import-isolation, conversion-contract, and UTF-8 offset regressions.
| File | Description |
|---|---|
tests/test_models.py |
Tests imports and result conversion contracts. |
tests/scanners/test_catboost_scanner.py |
Reformats scanner imports. |
tests/integrations/test_sarif_formatter.py |
Updates finding-identity imports. |
tests/detectors/test_jit_script_detector.py |
Updates helper usage and UTF-8 coverage. |
modelaudit/utils/helpers/secure_hasher.py |
Removes unused hashing wrappers. |
modelaudit/utils/file/detection.py |
Removes obsolete routing and buffered parsers. |
modelaudit/utils/__init__.py |
Uses supported Path.is_relative_to. |
modelaudit/scanners/zip_scanner.py |
Uses shared symlink resolution. |
modelaudit/scanners/xgboost_scanner.py |
Uses shared inconclusive metadata handling. |
modelaudit/scanners/weight_distribution_scanner.py |
Removes unused tensor sizing logic. |
modelaudit/scanners/torchserve_mar_scanner.py |
Reuses configuration and normalization helpers. |
modelaudit/scanners/torch7_scanner.py |
Reuses statement-boundary parsing. |
modelaudit/scanners/tf_savedmodel_scanner.py |
Inlines obsolete filesystem wrappers. |
modelaudit/scanners/text_scanner.py |
Removes unused documentation analysis. |
modelaudit/scanners/tar_scanner.py |
Removes unused TAR validation wrapper. |
modelaudit/scanners/sevenzip_scanner.py |
Simplifies extraction-budget state. |
modelaudit/scanners/safetensors_scanner.py |
Uses shared inconclusive metadata handling. |
modelaudit/scanners/rknn_scanner.py |
Reuses public-IP classification. |
modelaudit/scanners/r_serialized_scanner.py |
Reuses public-IP classification. |
modelaudit/scanners/pytorch_zip_scanner.py |
Reuses memo and base-scanner helpers. |
modelaudit/scanners/openvino_scanner.py |
Reuses XML parsing and path APIs. |
modelaudit/scanners/onnx_scanner.py |
Removes thin wrappers and inlines parsing logic. |
modelaudit/scanners/oci_layer_scanner.py |
Removes unused model-prefix detection. |
modelaudit/scanners/nemo_scanner.py |
Reuses stat identity and bounded evidence logic. |
modelaudit/scanners/mxnet_scanner.py |
Reuses inconclusive-result handling. |
modelaudit/scanners/metadata_scanner.py |
Calls URL redaction directly. |
modelaudit/scanners/manifest_scanner.py |
Reuses integer configuration handling. |
modelaudit/scanners/llamafile_scanner.py |
Removes redundant signal wrappers. |
modelaudit/scanners/lightgbm_scanner.py |
Reuses public-IP classification. |
modelaudit/scanners/keras_zip_scanner.py |
Shares Keras policy and result helpers. |
modelaudit/scanners/keras_utils.py |
Defines shared exact-module policy. |
modelaudit/scanners/keras_h5_scanner.py |
Shares Keras policy and result helpers. |
modelaudit/scanners/joblib_scanner.py |
Reuses pickle and base-scanner helpers. |
modelaudit/scanners/jinja2_template_scanner.py |
Removes unused buffered analysis paths. |
modelaudit/scanners/jax_checkpoint_scanner.py |
Reuses configuration and memo helpers. |
modelaudit/scanners/gguf_scanner.py |
Removes redundant URL wrappers. |
modelaudit/scanners/executorch_scanner.py |
Reuses stat identity logic. |
modelaudit/scanners/coreml_scanner.py |
Inlines bounded decoding and containment checks. |
modelaudit/scanners/compressed_scanner.py |
Removes unused stream-copy helper. |
modelaudit/scanners/cntk_scanner.py |
Inlines single-use predicates. |
modelaudit/scanners/catboost_scanner.py |
Uses shared inconclusive metadata handling. |
modelaudit/scanners/base.py |
Adds shared scanner result and utility helpers. |
modelaudit/scanners/archive_member_security.py |
Consolidates executable-prefix and AST helpers. |
modelaudit/scanners/archive_dispatch.py |
Centralizes routing and failure-result construction. |
modelaudit/scanners/_pickle_memo.py |
Adds shared pickle memo coercion. |
modelaudit/scanners/_network_indicators.py |
Adds shared public-IP classification. |
modelaudit/scanners/_evidence_redaction.py |
Removes redundant redaction wrappers. |
modelaudit/scanner_selection.py |
Simplifies aliases and adds extension matching. |
modelaudit/scanner_results.py |
Inlines member-hash identity serialization. |
modelaudit/models.py |
Consolidates finding conversion and coverage checks. |
modelaudit/integrations/sbom_generator.py |
Updates finding-identity import. |
modelaudit/integrations/sarif_formatter.py |
Updates finding-identity import. |
modelaudit/integrations/mlflow.py |
Updates finding-identity import. |
modelaudit/integrations/_mlflow_identity.py |
Inlines URL redaction behavior. |
modelaudit/finding_identity.py |
Relocates finding-identity helpers. |
modelaudit/detectors/secrets.py |
Removes an unused authentication predicate. |
modelaudit/detectors/network_comm.py |
Removes redundant URL/context wrappers. |
modelaudit/detectors/cve_patterns.py |
Consolidates CVE attribution construction. |
modelaudit/core.py |
Delegates routing and result helpers. |
modelaudit/core_results.py |
Centralizes result classification and merging. |
modelaudit/cli.py |
Updates finding-identity imports. |
modelaudit/analysis/unified_context.py |
Removes unused layer counting. |
modelaudit/analysis/semantic_analyzer.py |
Removes unused visitor state. |
modelaudit/analysis/opcode_sequence_analyzer.py |
Simplifies severity aggregation. |
modelaudit/analysis/entropy_analyzer.py |
Derives float count from collected values. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
🛡️ Codex Security Review · Automatically triggeredSecurity 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. |
|
Codex Review: Didn't find any major issues. Hooray! 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". |
|
@codex review |
|
Codex Review: Didn't find any major issues. Nice work! 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". |
🛡️ Codex Security Review · Automatically triggeredSecurity 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. |
|
@codex review |
|
@codex review |
|
Codex Review: Didn't find any major issues. 👍 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". |
🛡️ Codex Security Review · Automatically triggeredSecurity 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. |
|
@codex review |
|
Codex Review: Didn't find any major issues. Nice work! 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". |
🛡️ Codex Security Review · Automatically triggeredSecurity 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. |
|
@codex 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". |
🛡️ Codex Security Review · Automatically triggeredSecurity 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. |
|
@codex review Please review the current head after integration with main |
|
Codex Review: Didn't find any major issues. Bravo. 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". |
There was a problem hiding this comment.
Copilot review overview
🔵 Needs a closer look
It spans security-critical routing and result contracts across many scanners without a complete full-suite or cross-platform validation run.
Review effort: Balanced
Findings: None
Resolved since last review (1)
🛡️ Codex Security Review · Automatically triggeredSecurity 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. |
Repeated scan-routing decisions, failure-result construction, and scanner helpers had separate implementations. Consolidate these paths while preserving detection ownership, finding order, fail-closed outcomes, archive budgets, public result models, and optional-dependency handling.
Part 5 of the six-PR simplification series, based on the standalone picklescan PR. This removes redundant result conversions/state, duplicated scanner policy/configuration helpers, unused Flax preanalysis and buffered SentencePiece paths, and JIT wrappers. The root dependency remains
modelaudit-picklescan>=0.1.11,<0.2.0.Measured scope: 2,360 fewer maintained lines across 70 changed paths: production −2,423; tests +63; tooling/generated code unchanged. This includes both new helpers and uses normal Ruff formatting (1,582 additions, 3,942 deletions; the identity helper move contributes zero net reduction).
Validation:
Current-main integration (
2a5185a): preserves the upstream SafeTensors FDICT fix and deterministic initial/retried cache interruption fixture. Focused cache/routing checks and five neighboring compression cases pass on this branch; real benign/malicious CLI scans preserve scanner selection, JSON findings, SHA-256 metadata, and exit codes 0/1. Ruff checks pass. Changed-file mypy passes; full local mypy retains the previously documented optional TensorFlow baseline error. Hosted CI validates the published head.Retained ZIP-routing regressions cover malicious Keras/PyTorch archives, benign lookalikes, and nested fallback/selection. All 17 focused cases pass on both the parent and current PR5, without skips (
tests/test_core.py,tests/scanners/test_zip_scanner.py, andtests/test_scanner_selection.py). Independent AST comparison confirms the extracted selector matches both former branch bodies.The integrated committed prefix passed 1,956 affected tests with three Windows-only and two opt-in integration skips. Four fresh-process regressions cover package import order; identity normalization retains the exact historical URL-normalization expression.
13,390 nonoverlapping cases passed across completed scanner/core/model/analysis/registry cohorts and bounded JIT/PyTorch ZIP checks. Subsequent integration corrections are covered by the affected-prefix run above.
On the actual stacked prefix, 383 focused model/result/registry/routing/JIT cases passed, plus 98 adapter cases with the local standalone package and 98 against published
modelaudit-picklescan==0.1.10.28 actual CLI executions (14 parent/candidate pairs) match for JSON/SARIF and SBOM output, terminal output, finding/detail order, and exit codes 0/1/2. Only measured run metadata, checkout/output locations, and generated archive extraction paths are normalized; native counter-map key order is excluded because it is already nondeterministic in the parent.
Independent source review found no actionable boundary issue; an extracted JIT UTF-8 decoder differential matched 66,876 inputs.
Ruff format/check pass. Mypy 2.3.1 with the installed Python 3.12 dependency profile reports only the existing unreachable TensorFlow test import; the devbox package index cannot currently install upstream-pinned mypy 2.4.0 because its required
librtis unavailable.Limits: this is not a complete-suite pass. Full JIT, PyTorch ZIP, and filetype runs exceeded local time bounds; their bounded changed-contract cohorts completed. An obsolete masking assertion found in the interrupted PyTorch ZIP run was corrected and revalidated in PR3. Framework/platform skips and network-dependent deselections remain; remote acquisition and cross-platform CI are not claimed as locally verified.
The
_matching_path_extensionshelper is introduced here for the acquisition consolidation in PR6, whereutils/sources/cloud_storage.pyandutils/sources/jfrog.pyconsume it.PRs 1: tooling and 2: tests have landed. Remaining review order: 3: raw evidence → 4: picklescan → 5: scanning/results → 6: acquisition/cache/progress. PR3 targets
main; PRs 4–6 target the preceding branch.