Skip to content

fix: add standalone-primary pickle migration mode#902

Merged
mldangelo-oai merged 3 commits intomdangelo/codex/picklescan-audit-hardeningfrom
mdangelo/codex/pickle-migration-boundary-audit
Apr 10, 2026
Merged

fix: add standalone-primary pickle migration mode#902
mldangelo-oai merged 3 commits intomdangelo/codex/picklescan-audit-hardeningfrom
mdangelo/codex/pickle-migration-boundary-audit

Conversation

@mldangelo-oai
Copy link
Copy Markdown
Contributor

Summary

This PR continues the picklescan audit by tightening the root PickleScanner migration boundary.

The split-package docs said the root scanner should treat the standalone modelaudit-picklescan engine as primary and merge legacy-only compatibility checks into it. A direct default flip is not safe yet: the focused scanner suite produced 58 compatibility failures because callers and tests still depend on legacy-primary result shape. This change makes that future path explicit and testable without breaking current behavior.

Changes:

  • Keep the default root pickle scan path legacy-primary for compatibility.
  • Add opt-in use_standalone_pickle_primary=True scanner config to exercise the standalone-primary merge path.
  • Add pickle_primary_engine metadata so results show whether the root scanner returned the legacy or standalone-primary result.
  • Extend scripts/compare_pickle_scanners.py with --include-root and --root-standalone-primary to compare the actual root scanner alongside the pure legacy/package/adapter baselines.
  • Add regression tests for default legacy-primary behavior, opt-in standalone-primary merging, and root parity harness reporting.
  • Update migration docs, script docs, and the changelog.

Validation

  • uv run ruff format --check modelaudit/ packages/modelaudit-picklescan/src packages/modelaudit-picklescan/tests tests/
  • uv run ruff check modelaudit/ packages/modelaudit-picklescan/src packages/modelaudit-picklescan/tests tests/
  • uv run mypy modelaudit/ packages/modelaudit-picklescan/src packages/modelaudit-picklescan/tests tests/
  • uv run pytest tests/scanners/test_pickle_scanner.py tests/scanners/test_picklescan_adapter.py tests/scripts/test_compare_pickle_scanners.py -q --tb=short
    • 410 passed, 14 subtests passed
  • uv run pytest -n auto -m "not slow and not integration" --maxfail=1
    • 3336 passed, 75 skipped
  • env PYTHONPATH=packages/modelaudit-picklescan/src uv run python scripts/compare_pickle_scanners.py --root-standalone-primary
    • compared 34 fixtures
    • package: 13 match, 21 rule_drift
    • adapter: 23 match, 11 rule_drift
    • root: 26 match, 8 rule_drift
    • safe fixtures: root 6 match, 0 drift

Notes

This is intentionally stacked on top of #901 / mdangelo/codex/picklescan-audit-hardening; it should be reviewed as the migration-boundary layer after the standalone picklescan hardening work.

The remaining root drift is rule-shape/accounting drift, not verdict or exit-status drift. The next audit should close those remaining standalone-primary rule drifts so the default can eventually flip safely.

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai bot commented Apr 10, 2026

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Repository UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: 4f870e99-e5c2-4926-9e4c-983361af1b0b

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch mdangelo/codex/pickle-migration-boundary-audit

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Apr 10, 2026

Workflow run and artifacts

Performance Benchmarks

Compared 6 shared benchmarks with a regression threshold of 15%.
Status: 0 regressions, 0 improved, 6 stable, 0 new, 0 missing.
Aggregate shared-benchmark median: 611.05ms -> 609.91ms (-0.2%).

Benchmark Target Size Files Baseline Current Change Status
tests/benchmarks/test_scan_benchmarks.py::test_validate_file_type_pytorch_zip state_dict.pt 1.5 MiB 1 33.8us 35.5us +5.1% stable
tests/benchmarks/test_scan_benchmarks.py::test_scan_duplicate_directory duplicate-corpus 840.0 KiB 81 433.16ms 432.02ms -0.3% stable
tests/benchmarks/test_scan_benchmarks.py::test_scan_pytorch_zip state_dict.pt 1.5 MiB 1 31.04ms 30.96ms -0.3% stable
tests/benchmarks/test_scan_benchmarks.py::test_detect_file_format_safe_pickle safe_model.pkl 49.4 KiB 1 118.1us 118.0us -0.1% stable
tests/benchmarks/test_scan_benchmarks.py::test_scan_mixed_directory mixed-corpus 1.7 MiB 54 121.21ms 121.29ms +0.1% stable
tests/benchmarks/test_scan_benchmarks.py::test_scan_safe_pickle safe_model.pkl 49.4 KiB 1 25.49ms 25.49ms +0.0% stable

@mldangelo-oai mldangelo-oai force-pushed the mdangelo/codex/pickle-migration-boundary-audit branch 2 times, most recently from 1221963 to f26b178 Compare April 10, 2026 03:25
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: f26b1781aa

ℹ️ 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-oai mldangelo-oai force-pushed the mdangelo/codex/pickle-migration-boundary-audit branch 2 times, most recently from aa60a2d to 8ac54be Compare April 10, 2026 03:48
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

except Exception as error:
if isinstance(error, RecursionError) or self._is_pickle_parse_failure(error):
raise
self._record_pickle_runtime_error(package_result, error, location=source)
package_result.finish(success=False)
return package_result

P1 Badge Ignore legacy fallback crashes in standalone-primary mode

Guard the legacy-exception path behind use_standalone_pickle_primary. In _scan_pickle_stream_with_package_engine, any non-parse exception from legacy fallback always records pickle_scan_runtime_failed and returns success=False before the standalone-primary branch runs. This lets a clean standalone scan fail solely due to legacy compatibility code, breaking standalone-primary ownership semantics.

ℹ️ 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-oai mldangelo-oai force-pushed the mdangelo/codex/pickle-migration-boundary-audit branch from 8ac54be to bb028cd Compare April 10, 2026 04:54
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

except Exception as error:
if isinstance(error, RecursionError) or self._is_pickle_parse_failure(error):
raise
self._record_pickle_runtime_error(package_result, error, location=source)
package_result.finish(success=False)
return package_result

P1 Badge Ignore legacy fallback crashes in standalone-primary mode

In _scan_pickle_stream_with_package_engine, any non-parse exception from the legacy fallback unconditionally records an operational error on package_result and returns failure. That makes use_standalone_pickle_primary=True still fail because of legacy fallback runtime errors, contradicting standalone-primary ownership semantics and potentially forcing exit code 2 on otherwise complete package scans. Fresh evidence: this branch executes before _merge_missing_pickle_checks(..., propagate_fallback_state=False).

ℹ️ 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".

* fix: narrow legacy pickle stdlib policy

Remove broad CRITICAL treatment for noisy stdlib modules in the root pickle scanner while preserving exact dangerous helper coverage and warning-level suspicious refs.

* fix: preserve exact stdlib helper detections

Keep logging configuration loaders and uuid subprocess getnode helpers as exact dangerous callables in both root and standalone pickle policies.

* fix: flag uuid getnode pickle calls

Keep uuid.getnode in the exact dangerous-call policy after narrowing broad uuid module severity, since it can dispatch to platform helper probes that invoke subprocess-backed paths.

Cover both root PickleScanner and standalone picklescan policy regressions.

* fix: route misnamed compressed wrappers by header (#904)

* fix: route misnamed compressed wrappers by header

Detect gzip, bzip2, xz, lz4, and zlib wrappers from magic bytes even when the filename uses a misleading extension. Preserve existing joblib and R serialized scanner precedence, and add positive/negative routing regressions.

* fix: reject compressed wrapper raw trailers (#905)

* fix: reject compressed wrapper raw trailers

Use bounded low-level bzip2 and xz decompression so valid first members cannot hide raw unscanned trailing bytes. Preserve concatenated valid-member support and add fail-closed regressions.

* fix: bound compressed decompression probes

Cap bzip2/xz per-call decompression max_length to a chunk-sized probe so a single compressed chunk cannot allocate up to the full remaining scan budget before limit checks run.

* fix: bound zlib decompression probes

Use the same chunk-sized decompression probe for zlib streams so per-call output is bounded before size and ratio limit checks run.

* fix: harden lz4 wrapper decoding (#906)

* fix: harden lz4 wrapper decoding

Use the lz4 frame decompressor API through the bounded concatenated-stream loop so raw trailers fail closed and per-call output probes stay chunk-sized.

Add fake-frame regressions for optional lz4 coverage.

* fix: support legacy lz4 chunk decoding

Fall back to lz4.frame decompression contexts when LZ4FrameDecompressor is unavailable while keeping bounded output probes and raw-trailer rejection.

Add fallback-specific lz4 regressions for bounded reads, raw trailers, and concatenated frames.
@mldangelo-oai mldangelo-oai merged commit 630bdcb into mdangelo/codex/picklescan-audit-hardening Apr 10, 2026
@mldangelo-oai mldangelo-oai deleted the mdangelo/codex/pickle-migration-boundary-audit branch April 10, 2026 08:04
mldangelo-oai added a commit that referenced this pull request Apr 11, 2026
* fix: harden standalone pickle scanner

* fix: fail closed on truncated pickle literal scans

* fix: fail closed on oversized encoded pickle literals

* fix: deep-freeze standalone pickle reports

* fix: mark incomplete nested pickle scans inconclusive

* fix: narrow standalone pickle wildcard globals

* fix: preserve deep nested pickle findings

* fix: bound nested pickle recursion resources

* fix: add standalone-primary pickle migration mode (#902)

* fix: add standalone-primary pickle migration mode

* fix: isolate standalone pickle primary fallback state

* fix: narrow legacy pickle stdlib policy (#903)

* fix: narrow legacy pickle stdlib policy

Remove broad CRITICAL treatment for noisy stdlib modules in the root pickle scanner while preserving exact dangerous helper coverage and warning-level suspicious refs.

* fix: preserve exact stdlib helper detections

Keep logging configuration loaders and uuid subprocess getnode helpers as exact dangerous callables in both root and standalone pickle policies.

* fix: flag uuid getnode pickle calls

Keep uuid.getnode in the exact dangerous-call policy after narrowing broad uuid module severity, since it can dispatch to platform helper probes that invoke subprocess-backed paths.

Cover both root PickleScanner and standalone picklescan policy regressions.

* fix: route misnamed compressed wrappers by header (#904)

* fix: route misnamed compressed wrappers by header

Detect gzip, bzip2, xz, lz4, and zlib wrappers from magic bytes even when the filename uses a misleading extension. Preserve existing joblib and R serialized scanner precedence, and add positive/negative routing regressions.

* fix: reject compressed wrapper raw trailers (#905)

* fix: reject compressed wrapper raw trailers

Use bounded low-level bzip2 and xz decompression so valid first members cannot hide raw unscanned trailing bytes. Preserve concatenated valid-member support and add fail-closed regressions.

* fix: bound compressed decompression probes

Cap bzip2/xz per-call decompression max_length to a chunk-sized probe so a single compressed chunk cannot allocate up to the full remaining scan budget before limit checks run.

* fix: bound zlib decompression probes

Use the same chunk-sized decompression probe for zlib streams so per-call output is bounded before size and ratio limit checks run.

* fix: harden lz4 wrapper decoding (#906)

* fix: harden lz4 wrapper decoding

Use the lz4 frame decompressor API through the bounded concatenated-stream loop so raw trailers fail closed and per-call output probes stay chunk-sized.

Add fake-frame regressions for optional lz4 coverage.

* fix: support legacy lz4 chunk decoding

Fall back to lz4.frame decompression contexts when LZ4FrameDecompressor is unavailable while keeping bounded output probes and raw-trailer rejection.

Add fallback-specific lz4 regressions for bounded reads, raw trailers, and concatenated frames.

* docs: normalize unreleased changelog

* fix: address pickle scanner review feedback

* fix: surface nested pickle incomplete notices
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant