feat: extract standalone pickle scanner package with parity harness#832
Merged
mldangelo-oai merged 24 commits intomainfrom Apr 6, 2026
Merged
feat: extract standalone pickle scanner package with parity harness#832mldangelo-oai merged 24 commits intomainfrom
mldangelo-oai merged 24 commits intomainfrom
Conversation
Add the standalone modelaudit_picklescan package boundary, ModelAudit adapters, differential harness coverage, and package install smoke tests while preserving root package bundling until parity is ready.
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a standalone pickle-scanning package ( Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client (archive caller)
participant Scanner as In-repo PickleScanner
participant Engine as Standalone Engine (modelaudit_picklescan)
participant Adapter as picklescan_adapter
participant Fallback as Legacy Fallback Scanner
Client->>Scanner: scan_stream(file_obj, file_size, source)
activate Scanner
Scanner->>Engine: scan_pickle_stream(file_obj, source)
activate Engine
Engine-->>Engine: parse opcodes, collect findings/notices/coverage
Engine-->>Scanner: PickleReport
deactivate Engine
Scanner->>Adapter: pickle_report_to_scan_result(report)
activate Adapter
Adapter-->>Adapter: map verdicts/severity/rule codes, build ScanResult
Adapter-->>Scanner: ScanResult
deactivate Adapter
Scanner->>Fallback: run legacy fallback scan (seek or spooled copy)
activate Fallback
Fallback-->>Fallback: legacy checks/issues/metadata
Fallback-->>Scanner: legacy ScanResult
deactivate Fallback
Scanner->>Scanner: _merge_missing_pickle_checks(package_result, legacy_result)
Scanner-->>Client: merged ScanResult
deactivate Scanner
sequenceDiagram
participant CI as CI
participant Builder as Build Job
participant Dist as Distribution (wheel/sdist)
participant Venv as Isolated Test Env
participant Package as modelaudit_picklescan
CI->>Builder: build wheel + sdist (including picklescan)
activate Builder
Builder-->>Dist: modelaudit_picklescan wheel/sdist
deactivate Builder
Dist->>Venv: install wheel (uv pip install with venv python)
activate Venv
Venv->>Package: import modelaudit_picklescan
Package-->>Venv: module loaded
Venv->>Package: scan_bytes(payload)
activate Package
Package-->>Package: analyze opcodes -> PickleReport(status=COMPLETE)
Package-->>Venv: PickleReport
deactivate Package
Venv-->>CI: assert report.status == "complete" and report.verdict == "clean"
deactivate Venv
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
Contributor
Performance BenchmarksCompared
|
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
Add label-aware drift summaries, pin the no-safe-fixture-FP result in the current corpus, document the pure legacy baseline, and suppress scanner logs inside _build_report().
mldangelo-oai
changed the title
Route archive wrappers through the blended PickleScanner path, keep the harness baseline pure, and suppress parse-failure escalation for known benign trailing-tail cases without weakening fail-closed truncation handling.
mldangelo-oai
changed the title
Format AGENTS.md with Prettier, accept nt.system in standalone package tests, and serialize harness fixture paths with POSIX separators so Windows CI can match expected entries.
mldangelo-oai
changed the title
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5ef682612a
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1f882cc2c6
ℹ️ 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".
mldangelo
reviewed
Apr 2, 2026
Member
mldangelo
left a comment
mldangelo
left a comment
There was a problem hiding this comment.
Review: PR #832 — feat: extract standalone pickle scanner package with parity harness
Verdict: Request Changes
This is a significant architectural PR extracting a standalone modelaudit_picklescan package with dual-engine scanning and a differential parity harness. The approach is sound, but there are detection coverage gaps and a behavioral change in the PyTorch ZIP scanner that need attention.
Architecture
The dual-engine merge strategy (legacy primary + standalone fallback) is a good transitional approach. The import boundary test is clean, the typed API surface is well-designed, and the differential harness provides ongoing parity visibility.
1. CRITICAL: Standalone engine has dramatically smaller blocklist
The standalone engine's _DANGEROUS_WILDCARD_MODULES contains ~18 modules vs. 80+ in the legacy ALWAYS_DANGEROUS_MODULES. Missing coverage includes:
- Thread/process spawning:
multiprocessing,threading,_thread,signal - Network/exfil:
http,requests,aiohttp,smtplib,ftplib,telnetlib - Package install:
pip(all submodules),venv,ensurepip - Filesystem:
shutil,tempfile,tarfile,zipfile,glob - Code execution:
code,codeop,compileall,py_compile,pdb,cProfile
Why this matters: While the merge strategy covers this when used within ModelAudit, the standalone package exposes scan_file(), scan_stream(), scan_bytes() as public APIs. The README encourages standalone usage without warning about the reduced coverage. Anyone using modelaudit_picklescan independently gets significantly weaker detection.
Recommendation: Add a prominent warning to the standalone package README documenting the reduced blocklist, OR expand the standalone blocklist to match the legacy one before merging.
2. MEDIUM: PyTorch ZIP scanner file_size behavioral change
The old code explicitly passed original_file_size (the ZIP container's size) to _scan_pickle_bytes:
# IMPORTANT: Pass original ZIP file size, not pickle data size
# This enables proper density-based CVE detection
sub_result = self.pickle_scanner._scan_pickle_bytes(file_like, original_file_size)The new code passes len(data) (the pickle member's actual size) and stores the original file size only as metadata. If the legacy _scan_pickle_bytes uses file_size for density-based heuristics, memory limit thresholds, or CVE-specific detection boundaries, this change could introduce false negatives on embedded pickle members in large ZIP archives.
Recommendation: Verify no CVE detection regresses with this change. If the IMPORTANT comment was load-bearing, this needs investigation.
3. Performance: every pickle scanned twice
The dual-engine strategy reads every pickle stream twice (standalone engine, then legacy engine). For large pickles, this doubles I/O and CPU. Non-seekable streams additionally copy to a SpooledTemporaryFile first. This is acceptable as a transitional strategy but should be documented with a tracking issue for convergence.
4. Dead code in standalone engine
_is_dangerous_globalfunction is defined but never called_WARNING_GLOBALShas a deadif warning_names is None and module in _WARNING_GLOBALSbranch (only entry has a non-None frozenset value, making this unreachable)
5. Merge deduplication edge case
The _pickle_record_identity deduplication uses (location, rule_code) as the key. Two checks with the same location and rule_code but meaningfully different messages or severity would be treated as duplicates. This is likely intentional to avoid noise, but if the standalone and legacy engines produce findings with the same key but different diagnostic quality, the better finding could be silently suppressed.
6. Positive observations
- Import boundary test (
test_import_boundary.py) — clean structural verification that the standalone package doesn't import frommodelaudit _should_suppress_parse_failure_escalation— well-designed false-positive suppression for.binfiles, Unicode tails, and zero-byte padding_parse_positive_float— correctly handles bool, NaN, inf, zero, and negatives viamath.isfinite()apply_pickle_member_contextannotates both checks AND issues — improvement over legacy behavior- Differential harness — excellent observability for ongoing parity tracking
- Wrapper scanner coverage (S310, S104, S115 tests) — good regression coverage for embedded pickle analysis in joblib/numpy/pytorch/executorch paths
- Incremental stream parsing in
scan_stream()instead of materializing large streams — nice improvement
Test coverage
Comprehensive — the adapter tests, standalone API tests, harness oracle isolation tests, and wrapper parity tests are thorough. The benign malformed-tail false positive regression is a particularly important addition.
Summary
The architecture is sound and the implementation is thorough, but the standalone package's reduced blocklist creates a security gap if the package is used independently (which the API surface encourages). Either document this limitation prominently or close the gap before merging. The PyTorch ZIP file_size change also needs verification against CVE-specific tests.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b6f494b7cd
ℹ️ 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".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 15
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
modelaudit/utils/file/streaming.py (1)
89-100:⚠️ Potential issue | 🟡 MinorPass streaming source context into
scan_streamto preserve finding locations.At Line 99,
scan_streamis called withoutsource, so scanners that default to"<stream>"can emit non-actionable locations instead of the actual URL.💡 Proposed fix
partial_methods = [ ("scan_stream", True), ("scan_bytes", False), ("scan_fileobj", False), ("_scan_pickle_bytes", True), ] for method_name, needs_size in partial_methods: if hasattr(scanner, method_name): method = getattr(scanner, method_name) try: temp_file.seek(0) - scan_result = method(temp_file, bytes_to_read) if needs_size else method(temp_file) + if method_name == "scan_stream": + try: + scan_result = method(temp_file, bytes_to_read, source=url) + except TypeError: + scan_result = method(temp_file, bytes_to_read) + else: + scan_result = method(temp_file, bytes_to_read) if needs_size else method(temp_file) break except Exception: scan_result = None🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modelaudit/utils/file/streaming.py` around lines 89 - 100, The call to scanner.scan_stream is missing the streaming source context so findings get "<stream>" locations; when invoking the selected partial method in the loop (checking method_name from partial_methods and using method = getattr(scanner, method_name)), ensure you pass the original source variable into scan_stream to preserve locations—i.e., if method_name == "scan_stream", call method with the temp_file plus the source (and bytes_to_read when needs_size is True); leave other method invocations unchanged.tests/utils/file/test_streaming_analysis.py (1)
9-9: 🧹 Nitpick | 🔵 TrivialAdd type hints to test function signatures.
Per coding guidelines, test methods should have
-> Nonereturn type annotations and fixture parameters should be typed (e.g.,tmp_path: Path,monkeypatch: pytest.MonkeyPatch).Proposed fix
+from pathlib import Path + import fsspec from fsspec.implementations.local import LocalFileSystem from modelaudit.scanners.base import IssueSeverity, ScanResult from modelaudit.scanners.pickle_scanner import PickleScanner from modelaudit.utils.file import streaming -def test_stream_analyze_file_uses_scanner(tmp_path, monkeypatch): +def test_stream_analyze_file_uses_scanner(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> None:-def test_stream_analyze_file_falls_back_to_bytes_to_read(tmp_path, monkeypatch): +def test_stream_analyze_file_falls_back_to_bytes_to_read(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> None:As per coding guidelines: "Add type hints
-> Noneon all test methods, annotate fixtures liketmp_path: Path/monkeypatch: pytest.MonkeyPatch"Also applies to: 42-42
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/utils/file/test_streaming_analysis.py` at line 9, Update the test signature for test_stream_analyze_file_uses_scanner to include return type and fixture type annotations: change its definition to accept tmp_path: Path and monkeypatch: pytest.MonkeyPatch and add the -> None return annotation; locate the function named test_stream_analyze_file_uses_scanner in tests/utils/file/test_streaming_analysis.py and apply the same pattern to any other test functions flagged (e.g., at the other noted location).modelaudit/scanners/executorch_scanner.py (1)
123-126:⚠️ Potential issue | 🟠 MajorCap archive member reads to prevent memory spikes on large pickles.
The pickle member is read entirely into memory without size validation. Per project guidelines, archive member reads should be capped at 10 MB for metadata validation to prevent memory spikes on unexpectedly large pickles.
Proposed fix
+ MAX_PICKLE_MEMBER_SIZE = 10 * 1024 * 1024 # 10 MB cap + for name in pickle_files: - data = z.read(name) + info = z.getinfo(name) + if info.file_size > MAX_PICKLE_MEMBER_SIZE: + result.add_check( + name="Pickle Member Size Check", + passed=False, + message=f"Pickle member {name} exceeds 10 MB size limit ({info.file_size} bytes)", + severity=IssueSeverity.WARNING, + location=f"{path}:{name}", + details={"member": name, "size": info.file_size, "limit": MAX_PICKLE_MEMBER_SIZE}, + rule_code="S903", + ) + continue + data = z.read(name)Based on learnings: "Cap archive member reads for metadata validation (10 MB) to prevent memory spikes on large pickles"
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modelaudit/scanners/executorch_scanner.py` around lines 123 - 126, The loop that reads pickle archive members (iterating over pickle_files and updating bytes_scanned) currently does z.read(name) which loads the entire member into memory; change this to stream the member and cap reads at 10 MB (10 * 1024 * 1024) for metadata validation: open the member via z.open(name) or a streaming API, read at most the cap, increment bytes_scanned by the actual bytes read, and handle the case where the member exceeds the cap (e.g., log/warn and skip full processing or mark as too-large). Update the logic around pickle_files and bytes_scanned so no single z.read(name) can load >10MB into memory.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/test.yml:
- Around line 757-796: The smoke tests currently run the installed Python from
the repository root which allows the checkout to shadow the installed wheel;
update both "Smoke test wheel install" and "Smoke test standalone pickle package
wheel" steps so the test interpreter is executed with the working directory
outside the repo and without the repo on PYTHONPATH. Concretely, change the
lines that call "/tmp/.../bin/python - <<'PY'" to run from a clean cwd (for
example prefix with "cd /tmp &&" or run the interpreter from a subshell whose
cwd is /tmp) and unset PYTHONPATH (e.g. "PYTHONPATH= cd /tmp &&
/tmp/.../bin/python - <<'PY'") so imports like import modelaudit and import
modelaudit_picklescan resolve to the installed wheels rather than the local
checkout.
In `@docs/agents/picklescan-package-split.md`:
- Around line 9-25: The documented package layout is missing the top-level
package entry that exports the public API; add a
src/modelaudit_picklescan/__init__.py that re-exports the public symbols (e.g.,
import and expose api.py, options.py, report.py classes/functions) so users can
do from modelaudit_picklescan import ...; ensure __all__ or explicit imports in
__init__.py include the public surface (api, options, report) referenced by the
API contract.
In `@modelaudit/scanners/pickle_scanner.py`:
- Around line 5126-5138: The scan_stream public entrypoint currently skips the
size gate that scan() enforces; add the same file size check by calling the
existing _check_size_limit(file_size, source) (or extract that logic into a
small shared helper and call it from both scan() and scan_stream()) before
_prepare_scan_context and before any IO/opcode work; ensure you still call
_prepare_scan_context(source) and then return
_scan_pickle_stream_with_package_engine(file_obj, file_size, source=source) only
after the size guard passes so oversized embedded pickles can't bypass the
configured limit.
- Around line 4731-4740: The spooling loop in pickle_scanner.py (inside the with
tempfile.SpooledTemporaryFile block that writes chunks from file_obj into spool
using remaining_bytes and _NON_SEEKABLE_PICKLE_COPY_CHUNK_BYTES) can block long
operations; add periodic cancellation/timeout checks by calling the scanner's
check_interrupted() and/or _check_timeout() inside the while loop (e.g., before
each blocking read and right after writing each chunk) so the loop aborts
promptly on cancellation or timeout; keep chunk handling logic intact and ensure
these checks run each iteration.
- Around line 4699-4716: The legacy compatibility call to _scan_pickle_bytes
reads to EOF and can consume subsequent payload bytes; before calling
_scan_pickle_bytes(file_obj, file_size) seek back to stream_start and pass a
bounded view containing only file_size bytes (e.g., read file_size bytes from
file_obj into a temporary BytesIO and call _scan_pickle_bytes on that) so the
legacy pass is limited to the declared file_size; keep using
_merge_missing_pickle_checks(package_result, legacy_result) and ensure you
restore or leave the original file_obj cursor consistent with existing behavior.
In `@modelaudit/scanners/picklescan_adapter.py`:
- Around line 305-318: The function _apply_member_context_to_record currently
leaves record.location unchanged when it's non-empty, doesn't already include
member_location, and lacks a "(pos " marker; update
_apply_member_context_to_record so that after setting
record.details["pickle_filename"] it ensures member_location is applied in all
non-matching cases by prepending member_location (e.g., set record.location =
f"{member_location} {record.location}") when record.location is non-empty and
neither equals/starts with member_location nor contains "(pos "); keep the
existing early-return checks for empty location and matching member context.
In `@packages/modelaudit-picklescan/README.md`:
- Around line 9-24: Add a short "Installation" section at the top of the README
that shows how to install the package (e.g., "pip install
modelaudit-picklescan") and optionally notes if it's bundled with the main
`modelaudit` package; place it before or directly above the existing "Usage"
examples (which reference ScanOptions, scan_bytes, scan_file) so users see how
to install before running those snippets.
In `@packages/modelaudit-picklescan/src/modelaudit_picklescan/engine/scanner.py`:
- Around line 987-1016: The function _decode_possible_encoded_pickle is decoding
full base64/hex candidates before _looks_like_pickle_payload rejects oversized
payloads; to prevent large allocations, cap the input fed to base64.b64decode
and binascii.unhexlify using _NESTED_PICKLE_SCAN_LIMIT_BYTES: for base64,
compute max_base64_chars = ceil((_NESTED_PICKLE_SCAN_LIMIT_BYTES * 4) / 3) and
only decode padded[:max_base64_chars] (adding "=" padding as needed); for hex,
compute max_hex_chars = _NESTED_PICKLE_SCAN_LIMIT_BYTES * 2 and only call
binascii.unhexlify on hex_candidate[:max_hex_chars]; keep the same
_looks_like_pickle_payload checks and append ("base64"/"hex", decoded) only when
the truncated decode passes the predicate.
- Around line 147-201: _bounded_size currently returns negative requested_size
unchanged when self._byte_limit is None, causing read()/readline() to treat -1
as a real bound; normalize negative sizes first by treating requested_size of
None or any negative value as "unbounded" (i.e., set requested_size = None) at
the top of _bounded_size, then apply the existing logic that enforces
_byte_limit and computes remaining; this ensures read() and readline()
semantics: -1 means read all unless _byte_limit restricts it. Reference:
_bounded_size(), read(), readline(), and attribute _byte_limit.
In `@packages/modelaudit-picklescan/tests/test_api.py`:
- Around line 394-408: The test
test_scan_stream_preserves_absolute_offsets_from_current_stream_position
currently asserts that the substring f"pos {len(prefix)}" is not in
finding.location which is ambiguous; change it to explicitly verify absolute
offsets by parsing the numeric "pos N" values from each finding.location (e.g.,
with a regex) and asserting each parsed position is >= len(prefix) (or assert at
least one parsed position meets that condition if that matches intent), or
alternatively add a clear comment in the test explaining that the negative
assertion is intended to ensure positions are reported as absolute stream
offsets; update the loop over report.findings in the test to perform the numeric
comparison using the parsed positions and keep the existing metadata assertion
on report.metadata["first_pickle_end_pos"] unchanged.
In `@packages/modelaudit-picklescan/tests/test_import_boundary.py`:
- Around line 13-14: The current substring check (using "from modelaudit" /
"import modelaudit" on variable source) is over-broad and matches substrings and
comments; replace that logic with AST-based import detection: parse source with
ast.parse(source) inside a try/except, walk AST nodes looking for ast.Import and
ast.ImportFrom and only flag when a top-level imported module name equals
"modelaudit" (for Import, check alias.name; for ImportFrom, check node.module ==
"modelaudit" or node.module startswith "modelaudit." if you intend to catch
submodules), then append path.relative_to(PACKAGE_SRC.parents[1]) to
forbidden_imports when a real import is found; keep fallback behavior (ignore
SyntaxError) as needed.
In `@packages/modelaudit-picklescan/tests/test_report.py`:
- Around line 30-68: Add two focused tests to cover the two independent guards
in PickleReport.is_clean: one test that creates a PickleReport with
status=ScanStatus.COMPLETE, verdict=SafetyVerdict.UNKNOWN and no findings and
asserts is_clean is False (e.g., name it
test_complete_unknown_no_findings_is_not_clean), and another that creates a
PickleReport with status=ScanStatus.COMPLETE, verdict=SafetyVerdict.CLEAN but
with a non-empty findings tuple and asserts is_clean is False (e.g., name it
test_complete_clean_with_findings_not_clean); ensure each test only flips the
single condition it targets so the verdict and findings branches in
PickleReport.is_clean are covered independently.
In `@scripts/compare_pickle_scanners.py`:
- Around line 208-233: The try/finally in _build_report can mask setup errors
because fixtures and comparisons are only defined inside the try; initialize
fixtures and comparisons before entering the try (or move their creation out of
the logging-suppression block) so an UnboundLocalError cannot occur if
_discover_pickle_fixtures() or LegacyBaselinePickleScanner() fails; keep the
logging.disable(previous_logging_disable_level) in the finally to restore state,
but ensure fixtures and comparisons exist (e.g., set fixtures = [] and
comparisons = [] before the try) so subsequent code always has those variables
defined.
In `@tests/scanners/test_pickle_scanner.py`:
- Around line 132-136: The test
test_scan_stream_preserves_legacy_findings_for_non_seekable_stream currently
reads payload bytes directly from a repository asset; change it to create the
payload under the tmp_path fixture so the test is self-contained: accept
tmp_path in the test signature, write the known malicious bytes (or copy the
asset bytes into tmp_path within the test) to a file inside tmp_path, then read
from that tmp_path file into the payload variable (or open a non-seekable stream
from that tmp_path file) and proceed unchanged; update any variable names
(payload) and stream creation to use the tmp_path file so the test no longer
depends on repository paths.
---
Outside diff comments:
In `@modelaudit/scanners/executorch_scanner.py`:
- Around line 123-126: The loop that reads pickle archive members (iterating
over pickle_files and updating bytes_scanned) currently does z.read(name) which
loads the entire member into memory; change this to stream the member and cap
reads at 10 MB (10 * 1024 * 1024) for metadata validation: open the member via
z.open(name) or a streaming API, read at most the cap, increment bytes_scanned
by the actual bytes read, and handle the case where the member exceeds the cap
(e.g., log/warn and skip full processing or mark as too-large). Update the logic
around pickle_files and bytes_scanned so no single z.read(name) can load >10MB
into memory.
In `@modelaudit/utils/file/streaming.py`:
- Around line 89-100: The call to scanner.scan_stream is missing the streaming
source context so findings get "<stream>" locations; when invoking the selected
partial method in the loop (checking method_name from partial_methods and using
method = getattr(scanner, method_name)), ensure you pass the original source
variable into scan_stream to preserve locations—i.e., if method_name ==
"scan_stream", call method with the temp_file plus the source (and bytes_to_read
when needs_size is True); leave other method invocations unchanged.
In `@tests/utils/file/test_streaming_analysis.py`:
- Line 9: Update the test signature for test_stream_analyze_file_uses_scanner to
include return type and fixture type annotations: change its definition to
accept tmp_path: Path and monkeypatch: pytest.MonkeyPatch and add the -> None
return annotation; locate the function named
test_stream_analyze_file_uses_scanner in
tests/utils/file/test_streaming_analysis.py and apply the same pattern to any
other test functions flagged (e.g., at the other noted location).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 3ba70df9-b75d-4949-81fe-906281c7a148
📒 Files selected for processing (38)
.github/PULL_REQUEST_TEMPLATE.md.github/workflows/release-please.yml.github/workflows/test.ymlAGENTS.mdCHANGELOG.mddocs/agents/picklescan-package-split.mdmodelaudit/scanners/executorch_scanner.pymodelaudit/scanners/joblib_scanner.pymodelaudit/scanners/numpy_scanner.pymodelaudit/scanners/pickle_scanner.pymodelaudit/scanners/picklescan_adapter.pymodelaudit/scanners/pytorch_zip_scanner.pymodelaudit/utils/file/streaming.pypackages/modelaudit-picklescan/README.mdpackages/modelaudit-picklescan/pyproject.tomlpackages/modelaudit-picklescan/src/modelaudit_picklescan/__init__.pypackages/modelaudit-picklescan/src/modelaudit_picklescan/api.pypackages/modelaudit-picklescan/src/modelaudit_picklescan/engine/__init__.pypackages/modelaudit-picklescan/src/modelaudit_picklescan/engine/scanner.pypackages/modelaudit-picklescan/src/modelaudit_picklescan/options.pypackages/modelaudit-picklescan/src/modelaudit_picklescan/report.pypackages/modelaudit-picklescan/tests/conftest.pypackages/modelaudit-picklescan/tests/test_api.pypackages/modelaudit-picklescan/tests/test_import_boundary.pypackages/modelaudit-picklescan/tests/test_options.pypackages/modelaudit-picklescan/tests/test_report.pypyproject.tomlscripts/README.mdscripts/compare_pickle_scanners.pytests/conftest.pytests/scanners/test_executorch_scanner.pytests/scanners/test_joblib_scanner.pytests/scanners/test_numpy_scanner.pytests/scanners/test_pickle_scanner.pytests/scanners/test_picklescan_adapter.pytests/scanners/test_pytorch_zip_scanner.pytests/scripts/test_compare_pickle_scanners.pytests/utils/file/test_streaming_analysis.py
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fd4da06b9b
ℹ️ 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".
…scan-package-split # Conflicts: # modelaudit/scanners/pickle_scanner.py
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9750e1000b
ℹ️ 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".
Contributor
There was a problem hiding this comment.
Actionable comments posted: 9
♻️ Duplicate comments (2)
packages/modelaudit-picklescan/tests/test_import_boundary.py (1)
13-13: 🧹 Nitpick | 🔵 TrivialConsider specifying encoding for
read_text().While
read_text()defaults to the locale encoding, explicitly specifyingencoding="utf-8"ensures consistent behavior across platforms and aligns with Python best practices.- source = path.read_text() + source = path.read_text(encoding="utf-8")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/modelaudit-picklescan/tests/test_import_boundary.py` at line 13, The call reading the test fixture uses path.read_text() without an explicit encoding; update the code where the variable source is assigned (the line with source = path.read_text()) to call read_text with encoding="utf-8" so the file is read consistently across platforms and avoids locale-dependent behavior.tests/scanners/test_pickle_scanner.py (1)
134-140:⚠️ Potential issue | 🟠 MajorKeep this payload fixture fully tmp_path-scoped; avoid repository asset reads.
The test still depends on
tests/assets/...as an input source. This breaks the fixture-isolation rule for tests.Suggested direction
- payload_path.write_bytes( - (Path(__file__).resolve().parents[1] / "assets" / "exploits" / "exploit4_supply_chain_attack.pkl").read_bytes() - ) + payload_path.write_bytes(b"\x80\x02cos\nsystem\n(S'echo pwned'\ntR.")As per coding guidelines "Keep fixtures deterministic and self-contained under
tmp_path; never rely on host paths or global temp filenames".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/scanners/test_pickle_scanner.py` around lines 134 - 140, The test function test_scan_stream_preserves_legacy_findings_for_non_seekable_stream currently reads the payload from tests/assets on disk; instead, keep the fixture fully tmp_path-scoped by providing the payload bytes directly into payload_path without reading repository files—e.g., obtain payload bytes from a test helper or inline test data, write those bytes to payload_path and use payload = payload_path.read_bytes() (update references to payload_path/payload in the test accordingly) so there are no reads from tests/assets or other host paths.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/agents/picklescan-package-split.md`:
- Around line 9-18: The package layout is missing the py.typed marker; add an
empty file at src/modelaudit_picklescan/py.typed to mark the package as typed
and update the build config so it’s included in the wheel (e.g., add the file to
package data in your hatch/pyproject.toml or hatch.toml under the
modelaudit-picklescan package entry). Ensure the marker is committed alongside
__init__.py and that the packaging section (the hatch build or
[tool.hatch.build] include) references src/modelaudit_picklescan/py.typed so
downstream type checkers (mypy/pyright) will recognize the package as typed.
In `@modelaudit/scanners/pickle_scanner.py`:
- Around line 4772-4791: _scan_spooled_non_seekable_pickle_stream currently
copies a non-seekable stream into a SpooledTemporaryFile then calls
_scan_pickle_stream_with_package_engine which duplicates that spool into another
temp file; change the flow to reuse the first spool to avoid double temp I/O by
updating _scan_pickle_stream_with_package_engine to accept a flag or parameter
(e.g., reuse_spool or already_spooled) or an overload that, when True or when
passed a SpooledTemporaryFile, will perform the legacy package-engine pass
in-place without creating a second temp file; adjust
_scan_spooled_non_seekable_pickle_stream to pass that flag and rely on the
existing _copy_pickle_stream_to_spool behavior so large archive members use only
the single spool.
- Around line 4742-4747: The code is starting the scan timeout twice
(prepare_scan_context() already begins the timer) so the standalone scan gets a
full timeout and the legacy compatibility pass gets another, allowing scans to
exceed the configured limit; remove the redundant call to _start_scan_timer() in
the block that creates the SpooledTemporaryFile and instead rely on the timer
started by _prepare_scan_context() so that _copy_pickle_stream_to_spool(),
_scan_pickle_bytes(), and the legacy_result path share a single timeout budget.
- Around line 5210-5225: scan_stream currently forwards the original file_size
and any exceptions from _scan_pickle_stream_with_package_engine escape instead
of returning a failed ScanResult; change it to mirror scan(): call
_check_scan_stream_size_limit and interpret its result as either an early
ScanResult failure or a clamped_size value, return the ScanResult if it's a
failure, otherwise pass clamped_size (not the original file_size) into
_scan_pickle_stream_with_package_engine, and wrap that call in a try/except
catching Exception to return a failed ScanResult with the exception details and
source on error.
In `@modelaudit/utils/file/streaming.py`:
- Around line 99-106: The TypeError catch in the scan invocation (inside the
block handling method_name == "scan_stream") can mask runtime TypeErrors;
instead of try/except TypeError around method(temp_file, bytes_to_read,
source=url), use inspect.signature to check whether the callable (method)
accepts a 'source' parameter (or **kwargs) before calling with source, and only
call with source=url when the signature supports it; otherwise call the two-arg
form method(temp_file, bytes_to_read). Update the logic around method invocation
in the scan_stream branch (the code that currently tries method(..., source=url)
and falls back on TypeError) to perform this pre-call signature check and remove
the broad TypeError handler. Ensure you reference the existing variables/methods
(method, method_name, temp_file, bytes_to_read, url) when implementing the
check.
In `@packages/modelaudit-picklescan/src/modelaudit_picklescan/engine/scanner.py`:
- Around line 196-205: In _bounded_size, remove the unreachable check "or
requested_size < 0" from the conditional that returns remaining; negative
requested_size is already normalized to None at the top, so change the branch
"if requested_size is None or requested_size < 0:" to simply "if requested_size
is None:" in the _bounded_size method to eliminate the dead code while
preserving behavior.
- Around line 1012-1019: The hex candidate slice uses an incorrect multiplier
causing twice-the-intended length: change the slice that builds hex_candidate in
scanner.py (the expression using _MAX_HEX_NESTED_PICKLE_CHARS * 2) to use
_MAX_HEX_NESTED_PICKLE_CHARS directly so hex_candidate =
stripped[:_MAX_HEX_NESTED_PICKLE_CHARS].replace("\\x", ""); keep the subsequent
checks (len >= 16, even length, _HEX_CANDIDATE_RE.fullmatch, and the repetition
check) and the bounding assignment to bounded_hex_candidate =
hex_candidate[:_MAX_HEX_NESTED_PICKLE_CHARS] unchanged. Ensure you reference the
existing constants _MAX_HEX_NESTED_PICKLE_CHARS and
_NESTED_PICKLE_SCAN_LIMIT_BYTES when verifying the fix.
In `@scripts/compare_pickle_scanners.py`:
- Around line 111-124: The current _fixture_label function derives labels from
filenames (using SAFE_FIXTURE_NAMES, MALICIOUS_FIXTURE_NAMES and string checks),
which can silently change label outcomes for potential_fp/potential_fn when
files are renamed; replace this by loading a checked-in manifest (e.g., a JSON
or YAML mapping keyed by fixture relative path) and use that manifest to return
the canonical label for a Path in _fixture_label; update callers (potential_fp,
potential_fn, any code reading label) to rely on the manifest-backed label, and
add a clear error/warning if a path is missing from the manifest so missing
entries are explicit rather than defaulting to "unknown".
- Around line 214-219: The three post-budget global flags
(_post_budget_global_memo_limit_exceeded,
_post_budget_global_reference_limit_exceeded,
_post_budget_global_scan_deadline_exceeded) may leak state between fixtures
because they are only reset in _scan_global_references_unbounded(); modify
_prepare_scan_context() to reset those three flags along with
_path_validation_result and opcode_sequence_analyzer so each fixture starts with
a clean post-budget state, ensuring they cannot persist when
should_run_post_budget_scan is false for subsequent fixtures.
---
Duplicate comments:
In `@packages/modelaudit-picklescan/tests/test_import_boundary.py`:
- Line 13: The call reading the test fixture uses path.read_text() without an
explicit encoding; update the code where the variable source is assigned (the
line with source = path.read_text()) to call read_text with encoding="utf-8" so
the file is read consistently across platforms and avoids locale-dependent
behavior.
In `@tests/scanners/test_pickle_scanner.py`:
- Around line 134-140: The test function
test_scan_stream_preserves_legacy_findings_for_non_seekable_stream currently
reads the payload from tests/assets on disk; instead, keep the fixture fully
tmp_path-scoped by providing the payload bytes directly into payload_path
without reading repository files—e.g., obtain payload bytes from a test helper
or inline test data, write those bytes to payload_path and use payload =
payload_path.read_bytes() (update references to payload_path/payload in the test
accordingly) so there are no reads from tests/assets or other host paths.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0c612f40-5b14-45c6-ad1d-83495dc510f2
📒 Files selected for processing (18)
.github/workflows/test.ymlAGENTS.mdCHANGELOG.mddocs/agents/picklescan-package-split.mdmodelaudit/scanners/executorch_scanner.pymodelaudit/scanners/pickle_scanner.pymodelaudit/scanners/picklescan_adapter.pymodelaudit/utils/file/streaming.pypackages/modelaudit-picklescan/README.mdpackages/modelaudit-picklescan/src/modelaudit_picklescan/engine/scanner.pypackages/modelaudit-picklescan/tests/test_api.pypackages/modelaudit-picklescan/tests/test_import_boundary.pypackages/modelaudit-picklescan/tests/test_report.pyscripts/compare_pickle_scanners.pytests/scanners/test_executorch_scanner.pytests/scanners/test_pickle_scanner.pytests/scanners/test_picklescan_adapter.pytests/utils/file/test_streaming_analysis.py
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ce3fc13144
ℹ️ 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".
…scan-package-split # Conflicts: # modelaudit/scanners/joblib_scanner.py # tests/conftest.py
…scan-package-split # Conflicts: # modelaudit/scanners/pickle_scanner.py # tests/scanners/test_pickle_scanner.py
Contributor
There was a problem hiding this comment.
Actionable comments posted: 9
♻️ Duplicate comments (1)
modelaudit/utils/file/streaming.py (1)
117-126:⚠️ Potential issue | 🟠 MajorDo not fail-open when
scan_streamraises at runtime.
This block swallows scanner execution errors, and downstream code can still return a successful complete result, which risks silent false negatives.Based on learnings: "Applies to modelaudit/scanners/**/*.py : Preserve or strengthen security detections; test both benign and malicious samples when modifying scanners"Proposed fix
- if scan_result is None: + partial_scan_error: Exception | None = None + if scan_result is None: partial_methods = [ ("scan_stream", True), ("scan_bytes", False), ("scan_fileobj", False), ("_scan_pickle_bytes", True), ] for method_name, needs_size in partial_methods: if hasattr(scanner, method_name): method = getattr(scanner, method_name) try: temp_file.seek(0) if method_name == "scan_stream" and needs_size: if _scan_stream_accepts_source_keyword(method): scan_result = method(temp_file, bytes_to_read, source=url) else: scan_result = method(temp_file, bytes_to_read) else: scan_result = method(temp_file, bytes_to_read) if needs_size else method(temp_file) break - except Exception: + except Exception as exc: scan_result = None + partial_scan_error = exc + + if scan_result is None and partial_scan_error is not None: + result = ScanResult(scanner_name="streaming") + result.metadata = { + "streaming_analysis": True, + "bytes_analyzed": bytes_to_read, + "analysis_complete": False, + "file_size": file_size, + "operational_error": True, + "operational_error_reason": type(partial_scan_error).__name__, + } + result.finish(success=False) + return result, False🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modelaudit/utils/file/streaming.py` around lines 117 - 126, The current try/except around invoking scanner methods (when method_name == "scan_stream" and using _scan_stream_accepts_source_keyword, calling method(temp_file, bytes_to_read, source=url) or method(temp_file, bytes_to_read) / method(temp_file)) swallows all exceptions and sets scan_result = None, which can cause a fail-open. Change this so scanner runtime errors are not silently ignored: remove the bare except Exception that masks failures and either re-raise the exception after optional contextual logging or replace it with a narrow, documented exception handling path that logs the error and signals failure to callers (e.g., by raising a ScannerRuntimeError or setting an explicit failure flag rather than silently returning a successful None). Ensure references: method_name, scan_stream, _scan_stream_accepts_source_keyword, method, temp_file, bytes_to_read, and scan_result are updated accordingly so downstream logic cannot treat a swallowed exception as a benign scan_result.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@modelaudit/scanners/pickle_scanner.py`:
- Around line 5423-5424: Replace the os.path.splitext usage when computing
file_ext by using pathlib.Path to get the suffix: compute suffix =
Path(source).suffix.lower() (instead of file_ext =
os.path.splitext(source)[1].lower()) and compare suffix to ".bin"; ensure
pathlib.Path is imported (from pathlib import Path) at the top of the module and
update any references to file_ext to use the new suffix variable (in the same
scope where source is used).
- Around line 5415-5419: scan_stream() currently returns immediately for .bin
streams from the _scan_pickle_stream_with_package_engine() success path and thus
skips calling _scan_remaining_bin_tail_if_needed(), and on parse-failure it
rewinds to 0 then calls _scan_binary_payload() without honoring the original
file_size, allowing reads outside the declared slice; update scan_stream() so
that after successful _scan_pickle_stream_with_package_engine() it still invokes
_scan_remaining_bin_tail_if_needed() for .bin source streams (same parity as
scan()), and on parse-failure avoid rewinding to 0 or ensure
_scan_binary_payload() is called with the original file_size/stream window so
positioned/shared streams cannot read past the slice; adjust calls in
_scan_pickle_stream_with_package_engine, _scan_remaining_bin_tail_if_needed, and
_scan_binary_payload accordingly and run tests for the affected callers
(pytorch_zip_scanner and executorch_scanner) to preserve security detections.
- Around line 4931-4945: The legacy compatibility path
(reuse_seekable_stream_for_legacy, _copy_pickle_stream_to_spool,
_scan_pickle_bytes) can raise and currently end up replacing or returning a new
runtime-error legacy_result which can erase detections already present in
package_result; change the error-handling so that exceptions from the
legacy/spool path do not replace package_result but are merged into it as an
operational/inconclusive/legacy-failure state (e.g., set an error flag/message
in package_result.metadata and preserve existing findings), then run
_merge_missing_pickle_checks into package_result and return package_result
(apply the same preservation/merge pattern to the other error-handling block
around lines 4974-4987). Ensure you still propagate successful legacy_result
merges via _merge_missing_pickle_checks when no error occurs.
In `@modelaudit/scanners/picklescan_adapter.py`:
- Around line 405-425: In _finding_reference_keys, currently findings without a
string "import_reference" are skipped; instead, if import_reference is missing
or not a str, build a fallback import_reference from finding.details["module"]
and finding.details["name"] (e.g., join them with a dot only when both present)
and use that as the key before falling back to (import_reference, None); keep
the existing positions logic using positions and _location_position, and ensure
you fall back to None only when neither import_reference nor module+name can be
formed; also add a regression test in tests/scanners/test_picklescan_adapter.py
covering the case where a dangerous-global finding uses module/name but no
import_reference so both detections correlate.
- Around line 54-61: _pars e_min_int currently uses int(value) which can
truncate floats and raise OverflowError for infinities; update _parse_min_int to
explicitly reject non-integral and infinite floats and malformed strings before
converting: keep the bool check, if isinstance(value, float) use
math.isfinite(value) and value.is_integer() and otherwise return default; if
isinstance(value, str) ensure it contains only digit characters (use
str.isdigit()) before converting; wrap int(value) to also catch OverflowError
(in the same except block as TypeError/ValueError) and return default on error;
finally keep the minimum check (parsed >= minimum) and return default if it
fails.
In `@packages/modelaudit-picklescan/src/modelaudit_picklescan/engine/scanner.py`:
- Around line 755-774: The loop in _scan_string_literal repeatedly calls
sorted(_SUSPICIOUS_STRING_PATTERNS) which wastes work since
_SUSPICIOUS_STRING_PATTERNS is constant; define a module-level pre-sorted tuple
(e.g. _SUSPICIOUS_STRING_PATTERNS_SORTED =
tuple(sorted(_SUSPICIOUS_STRING_PATTERNS))) near the constant declaration, then
change the loop in _scan_string_literal to iterate over
_SUSPICIOUS_STRING_PATTERNS_SORTED (leaving the body that calls
self._add_finding/Finding unchanged) so sorting is done once at import time.
In `@scripts/compare_pickle_scanners.py`:
- Line 85: The module currently calls _load_fixture_labels_manifest() at import
time into _FIXTURE_LABELS_BY_PATH which causes import failures if the manifest
is missing or malformed; change this to lazy initialization by setting
_FIXTURE_LABELS_BY_PATH = None and add a helper _get_fixture_labels() that on
first call invokes _load_fixture_labels_manifest(), caches the result in
_FIXTURE_LABELS_BY_PATH, and returns it (handle and re-raise or wrap manifest
errors at call time as needed). Update _fixture_label to call
_get_fixture_labels() instead of reading _FIXTURE_LABELS_BY_PATH directly so the
manifest is only loaded when needed.
- Around line 141-142: The counting is comparing the enum's string value; change
the comparisons to use the Severity enum constants directly (e.g., replace
finding.severity.value == "warning" with finding.severity == Severity.WARNING
and do the same for critical using Severity.CRITICAL), and ensure Severity is
imported or referenced where warning_count and critical_count are computed
(i.e., the expressions that build counts from report.findings).
In `@tests/scanners/test_pickle_scanner.py`:
- Line 642: Replace the hard-coded string "inconclusive" with the shared
constant INCONCLUSIVE_SCAN_OUTCOME wherever used (e.g., in the assignment to
fallback.metadata["scan_outcome"] and the similar occurrence around line 650) so
the test uses the canonical symbol; update the two occurrences to reference
INCONCLUSIVE_SCAN_OUTCOME instead of the raw literal to prevent drift if the
constant changes.
---
Duplicate comments:
In `@modelaudit/utils/file/streaming.py`:
- Around line 117-126: The current try/except around invoking scanner methods
(when method_name == "scan_stream" and using
_scan_stream_accepts_source_keyword, calling method(temp_file, bytes_to_read,
source=url) or method(temp_file, bytes_to_read) / method(temp_file)) swallows
all exceptions and sets scan_result = None, which can cause a fail-open. Change
this so scanner runtime errors are not silently ignored: remove the bare except
Exception that masks failures and either re-raise the exception after optional
contextual logging or replace it with a narrow, documented exception handling
path that logs the error and signals failure to callers (e.g., by raising a
ScannerRuntimeError or setting an explicit failure flag rather than silently
returning a successful None). Ensure references: method_name, scan_stream,
_scan_stream_accepts_source_keyword, method, temp_file, bytes_to_read, and
scan_result are updated accordingly so downstream logic cannot treat a swallowed
exception as a benign scan_result.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6d7ead12-6e8e-4b7f-a577-c9675fd03cea
📒 Files selected for processing (19)
CHANGELOG.mddocs/agents/picklescan-package-split.mdmodelaudit/scanners/joblib_scanner.pymodelaudit/scanners/pickle_scanner.pymodelaudit/scanners/picklescan_adapter.pymodelaudit/utils/file/streaming.pypackages/modelaudit-picklescan/pyproject.tomlpackages/modelaudit-picklescan/src/modelaudit_picklescan/engine/scanner.pypackages/modelaudit-picklescan/src/modelaudit_picklescan/py.typedpackages/modelaudit-picklescan/tests/test_import_boundary.pyscripts/compare_pickle_scanners.pyscripts/compare_pickle_scanners_fixture_labels.jsontests/conftest.pytests/scanners/test_joblib_scanner.pytests/scanners/test_pickle_scanner.pytests/scanners/test_picklescan_adapter.pytests/scripts/test_compare_pickle_scanners.pytests/test_real_world_dill_joblib.pytests/utils/file/test_streaming_analysis.py
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 21926a86c9
ℹ️ 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".
…scan-package-split # Conflicts: # tests/conftest.py
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 31d589d807
ℹ️ 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".
Contributor
There was a problem hiding this comment.
Actionable comments posted: 11
♻️ Duplicate comments (2)
modelaudit/scanners/pickle_scanner.py (2)
5450-5470:⚠️ Potential issue | 🟠 MajorKeep
.binrescans inside the caller's slice.
stream_startis discarded,first_pickle_end_posis slice-relative, the parse-failure path seeks to absolute0, and_scan_binary_content()reads to EOF. Positioned/shared streams can therefore rescan bytes outside the declared[stream_start, stream_start + file_size)window.Based on learnings: Preserve or strengthen security detections; test both benign and malicious samples when modifying scanners.
Also applies to: 5481-5509
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modelaudit/scanners/pickle_scanner.py` around lines 5450 - 5470, The code allows rescanning bytes outside the caller's declared slice because stream_start is discarded and parse-failure paths seek to absolute 0 while _scan_binary_content reads to EOF; fix by keeping and using the original stream_start (don't discard stream_start), make all downstream operations respect the slice window by treating positions as offsets from stream_start (e.g., use file_obj.seek(stream_start + first_pickle_end_pos) instead of absolute seeks), and constrain _scan_remaining_bin_tail_if_needed and _scan_binary_content to read at most file_size bytes (or accept a base_offset parameter). Update _scan_pickle_stream_with_package_engine, _scan_remaining_bin_tail_if_needed, and _scan_binary_content to accept and use stream_start/base_offset and ensure any error/parse-failure path restores the stream to stream_start and never seeks to absolute 0; also ensure can_scan_tail_from_input logic preserves stream position.
4944-4958:⚠️ Potential issue | 🟠 MajorDon't let the legacy compatibility pass erase standalone findings.
package_resultalready exists here. Re-raising legacy parse/recursion failures sends control back to the outer handlers, which rebuild a fresh result and drop the standalone detections. On.binsources that can even fall back to the binary-only branch.Based on learnings: Preserve or strengthen security detections; test both benign and malicious samples when modifying scanners.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modelaudit/scanners/pickle_scanner.py` around lines 4944 - 4958, The try/except around reuse_seekable_stream_for_legacy/_scan_pickle_bytes is re-raising RecursionError or parse failures which causes the outer handler to rebuild a fresh package_result and drop any standalone detections; before re-raising those exceptions, merge or persist the legacy scan findings into the existing package_result (e.g. transfer detections from legacy_result into package_result or call package_result.merge/append with legacy_result) and ensure you still call _record_pickle_runtime_error(package_result, error, location=source) and package_result.finish(success=False) only if appropriate so legacy detections are not lost when raising from _scan_pickle_bytes or when _is_pickle_parse_failure returns True.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/test.yml:
- Around line 786-789: The root package "build" job currently skips
workflow-only PRs while the new "picklescan-package" job includes them; update
the root "build" job's if condition to mirror the new job by adding the same
check for needs.changes.outputs.workflows (i.e., include "||
needs.changes.outputs.workflows == 'true'") so workflow-only changes also
trigger the root build job; locate the top-level "build" job definition and
adjust its if expression to match the logic used by the "picklescan-package"
job.
In `@modelaudit/scanners/pickle_scanner.py`:
- Around line 264-275: _has_trusted_incomplete_tail_context currently treats any
presence of first_pickle_end_pos or all-benign import_references as proof the
unscanned remainder is safe; instead require an explicit benign-incomplete-tail
signal before granting the exemption. Change the first branch so it only returns
True when first_pickle_end_pos is an int >= 0 AND a metadata flag like
"incomplete_tail_is_benign" (or "trusted_incomplete_tail") is truthy; likewise
only evaluate import_references (and return True when all
reference.get("is_dangerous") are false) if metadata indicates an incomplete
tail (e.g., "has_incomplete_tail") and that tail is marked benign. Ensure you
update checks in _has_trusted_incomplete_tail_context to use these metadata
flags (first_pickle_end_pos, import_references, and the new
"incomplete_tail_is_benign"/"has_incomplete_tail" flag names) so only benign
malformed-tail cases are exempted.
In `@modelaudit/scanners/picklescan_adapter.py`:
- Around line 263-288: The suppression logic currently allows skipping
"Standalone Pickle Parse Failure" based on extension or benign-tail heuristics
even when first_pickle_end_pos is missing; change the flow in the loop over
report.notices so that every suppression path (both the UnicodeDecodeError
branch that calls _has_only_non_dangerous_import_references and the ValueError
branch that checks source_ext and _has_only_benign_serialization_tail_imports)
first requires has_trusted_pickle_boundary (i.e., first_pickle_end_pos is an int
>= 0) before returning True; keep the early return for
has_trusted_pickle_boundary intact but otherwise do not return True from those
helper-based checks unless has_trusted_pickle_boundary is true, and add tests:
one benign malformed-tail fixture and one malicious regression fixture to ensure
suppression only occurs when the trusted boundary is present.
- Around line 45-52: The _parse_positive_float function can raise OverflowError
for extremely large numeric inputs; update its exception handling to also catch
OverflowError so that values like 10**10000 return the provided default instead
of crashing. Locate _parse_positive_float and modify the except clause
(currently catching TypeError and ValueError) to include OverflowError, ensuring
the function returns default on any of those exceptions while preserving the
existing bool-check and final positive/finite validation.
In `@packages/modelaudit-picklescan/README.md`:
- Around line 49-52: Remove the maintainer-only parity-harness note from the
public README: delete or relocate the sentence referencing
scripts/compare_pickle_scanners.py and the guidance about changing detection
logic, and instead add a short user-facing line that focuses on PickleScanner
usage (modelaudit.scanners.pickle_scanner.PickleScanner) or point readers to
contributor docs; place the parity-harness workflow and the
compare_pickle_scanners.py instructions into the repository's
contributor/developer documentation (e.g., CONTRIBUTING.md or docs/developer/)
so PyPI-visible README.md contains only installation, usage, API, and
troubleshooting info.
In `@packages/modelaudit-picklescan/src/modelaudit_picklescan/engine/scanner.py`:
- Around line 746-752: The _resolve_global_operand function currently treats
_GlobalRef instances as valid operands which bypasses malformed detection;
update the implementation in _resolve_global_operand so it only returns a value
when the input is a plain str and returns None for every other type (remove the
branch that checks for _GlobalRef and its .symbol), ensuring malformed operands
propagate and trigger MALFORMED_STACK_GLOBAL handling.
- Around line 344-346: The loop using pickletools.genops consumes the opcode
before calling self._check_limits(), so when self._check_limits() raises
_OpcodeBudgetExceeded the stream has already advanced and _handle_opcode() and
security checks are skipped; fix this by capturing the stream position
immediately when entering the genops iteration (e.g., before calling
self._check_limits()) and include that captured boundary offset in the
_OpcodeBudgetExceeded exception or pass it to _scan_post_budget_tail(); update
_check_limits/_OpcodeBudgetExceeded handling to propagate the boundary offset,
and modify _scan_post_budget_tail(self, boundary_offset) to seek self.stream to
that boundary_offset before tail scanning so GLOBAL/STACK_GLOBAL at the cutoff
are inspected.
In `@scripts/compare_pickle_scanners.py`:
- Around line 60-66: The override for
PickleScanner._scan_pickle_stream_with_package_engine narrows the base/helper
signature by omitting the reuse_seekable_stream_for_legacy: bool = False
parameter; update the method signature to include
reuse_seekable_stream_for_legacy: bool = False (matching the base) and ensure
the parameter is accepted (and passed through to any calls that expect it or
ignored safely) so future callers forwarding that kwarg won't raise TypeError.
- Around line 33-40: VERDICT_RANK currently treats "unknown" as lower than
"clean" causing an "unknown"->"clean" change to be classified as potential_fp
and EXIT_FAILURE_DELTAS omits status_drift; update by removing "unknown" from
the numeric ranking (or set it to a neutral/None value) and special-case
"unknown" in _classify_delta so any transition from or to "unknown" is handled
explicitly (e.g., treat unknown->clean as a completeness improvement, not a
potential_fp, and clean->unknown as a status_drift); also add "status_drift" to
EXIT_FAILURE_DELTAS so completeness regressions fail the CI as intended.
In `@tests/scanners/test_picklescan_adapter.py`:
- Line 205: Replace the list-comprehension-and-count assertion with an
any()-based check to avoid creating an intermediate list: for the assertion that
no check has name "Standalone Pickle Import" (iterating over result.checks) use
a generator with any() and negate it (e.g., assert not any(check.name ==
"Standalone Pickle Import" for check in result.checks)); apply the same refactor
to the other similar assertion mentioned around the other occurrence (line with
the same "Standalone Pickle Import" check).
---
Duplicate comments:
In `@modelaudit/scanners/pickle_scanner.py`:
- Around line 5450-5470: The code allows rescanning bytes outside the caller's
declared slice because stream_start is discarded and parse-failure paths seek to
absolute 0 while _scan_binary_content reads to EOF; fix by keeping and using the
original stream_start (don't discard stream_start), make all downstream
operations respect the slice window by treating positions as offsets from
stream_start (e.g., use file_obj.seek(stream_start + first_pickle_end_pos)
instead of absolute seeks), and constrain _scan_remaining_bin_tail_if_needed and
_scan_binary_content to read at most file_size bytes (or accept a base_offset
parameter). Update _scan_pickle_stream_with_package_engine,
_scan_remaining_bin_tail_if_needed, and _scan_binary_content to accept and use
stream_start/base_offset and ensure any error/parse-failure path restores the
stream to stream_start and never seeks to absolute 0; also ensure
can_scan_tail_from_input logic preserves stream position.
- Around line 4944-4958: The try/except around
reuse_seekable_stream_for_legacy/_scan_pickle_bytes is re-raising RecursionError
or parse failures which causes the outer handler to rebuild a fresh
package_result and drop any standalone detections; before re-raising those
exceptions, merge or persist the legacy scan findings into the existing
package_result (e.g. transfer detections from legacy_result into package_result
or call package_result.merge/append with legacy_result) and ensure you still
call _record_pickle_runtime_error(package_result, error, location=source) and
package_result.finish(success=False) only if appropriate so legacy detections
are not lost when raising from _scan_pickle_bytes or when
_is_pickle_parse_failure returns True.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 88134196-7453-45cd-8867-2222560719cc
⛔ Files ignored due to path filters (1)
packages/modelaudit-picklescan/uv.lockis excluded by!**/*.lock
📒 Files selected for processing (19)
.github/workflows/release-please.yml.github/workflows/test.ymlCHANGELOG.mddocs/agents/picklescan-package-split.mdmodelaudit/scanners/pickle_scanner.pymodelaudit/scanners/picklescan_adapter.pymodelaudit/scanners/pytorch_zip_scanner.pypackages/modelaudit-picklescan/README.mdpackages/modelaudit-picklescan/pyproject.tomlpackages/modelaudit-picklescan/src/modelaudit_picklescan/engine/scanner.pypackages/modelaudit-picklescan/tests/test_api.pypackages/modelaudit-picklescan/tests/test_options.pypackages/modelaudit-picklescan/tests/test_report.pyscripts/compare_pickle_scanners.pytests/conftest.pytests/scanners/test_pickle_scanner.pytests/scanners/test_picklescan_adapter.pytests/scanners/test_pytorch_zip_scanner.pytests/scripts/test_compare_pickle_scanners.py
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ff4d529f68
ℹ️ 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".
Comment on lines
+4750
to
+4759
| with contextlib.suppress(AttributeError, OSError, ValueError): | ||
| file_obj.seek(0) | ||
| self._scan_binary_payload( | ||
| file_obj, | ||
| result, | ||
| start_pos=0, | ||
| file_size=file_size, | ||
| full_file=True, | ||
| ) | ||
| result.finish(success=True) |
There was a problem hiding this comment.
In scan_stream(), the .bin parse-failure branch wraps seek(0) + _scan_binary_payload() in contextlib.suppress. For non-seekable streams (which were already copied to a spool in _scan_spooled_non_seekable_pickle_stream), seek(0) fails, binary scanning is skipped, and the method still returns success=True. This can miss executable/signature findings in malformed .bin streams.
Useful? React with 👍 / 👎.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What changed
modelaudit_picklescanpackage underpackages/modelaudit-picklescanwith typed reports/options and stream/bytes/file scan APIs.joblib, NumPy object arrays, PyTorch ZIP, ExecuTorch) on the blended rootPickleScannerpath so legacy-only rule coverage is preserved while the package boundary is introduced.scan_stream()to parse incrementally instead of materializing large streams, preserving spooled ZIP-member behavior.S310,S104,S115), benign malformed-tail false positives, harness oracle isolation, and standalone-package API behavior.Review comments addressed
picklescan_adapter.pyby usingmath.isfinite().global_severitylocal in the standalone opcode engine by returning early when no callable reference exists.False-positive / false-negative audit
potential_fpdrift:safe: match=6for both summaries.S310,S104, andS115are covered by tests).unknown-labeled rule-drift case. The harness now keeps that drift observable instead of absorbing it into the baseline.Validation
uv run ruff format modelaudit/ packages/modelaudit-picklescan/src packages/modelaudit-picklescan/tests scripts/ tests/uv run ruff check --fix modelaudit/ packages/modelaudit-picklescan/src packages/modelaudit-picklescan/tests scripts/ tests/uv run mypy modelaudit/ tests/uv run mypy packages/modelaudit-picklescan/src packages/modelaudit-picklescan/testsuv run mypy scripts/compare_pickle_scanners.pyuv run pytest -n auto -m "not slow and not integration" --maxfail=1 -q→ 3068 passed, 20 skipped, 14 subtests passeduv run pytest tests/scanners/test_joblib_scanner.py tests/scanners/test_numpy_scanner.py tests/scanners/test_pytorch_zip_scanner.py tests/scanners/test_executorch_scanner.py tests/scanners/test_picklescan_adapter.py tests/scripts/test_compare_pickle_scanners.py -q→ 100 passeduv run python scripts/compare_pickle_scanners.py→ safe fixturesmatch=6for package and adapter; malicious/unknown drift summarized by labelSummary by CodeRabbit
New Features
Documentation
Improvements
Tests