Skip to content

test(picklescan): stabilize benign nested constructor checks - #1789

Open
mldangelo-oai wants to merge 1 commit into
mainfrom
fix/picklescan-windows-nested-source-stability
Open

test(picklescan): stabilize benign nested constructor checks#1789
mldangelo-oai wants to merge 1 commit into
mainfrom
fix/picklescan-windows-nested-source-stability

Conversation

@mldangelo-oai

Copy link
Copy Markdown
Contributor

Summary

  • reproduce the July 29 Windows Python 3.11 Nightly failure by forcing call-graph source instability during benign nested constructor scans
  • keep constructor notices and clean-result assertions, while accepting only the explicit fail-closed source-stability error when a source changes
  • cover all ten benign constructor shapes under both ordinary encoded scans and forced hex-source instability

Validation

  • confirmed the new hex/slice source-change regression fails before the assertion fix and passes afterward
  • 40 benign nested constructor/source-instability cases passed
  • 3 adjacent nested-constructor and fail-closed source-stability regressions passed
  • repository-wide Ruff lint and format checks passed
  • changed picklescan test passes mypy; full local mypy reports pre-existing macOS/environment-only errors in six untouched files, while the exact main commit GitHub Type Check is green

Nightly failure: https://github.com/promptfoo/modelaudit/actions/runs/30423861843

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

This PR stabilizes modelaudit-picklescan’s benign nested-constructor scan tests by explicitly modeling and asserting the “fail-closed” behavior when the shared call-graph source snapshot changes mid-analysis (an observed flaky condition in Windows/Python 3.11 nightly runs).

Changes:

  • Expands the benign nested-constructor test matrix to include a forced “hex + source changed” scenario via monkeypatch on _ensure_shared_source_snapshot_stable.
  • Updates assertions to accept only the explicit call-graph source-stability failure mode as INCONCLUSIVE/UNKNOWN, while keeping the clean path as COMPLETE/CLEAN.
  • Keeps the original expectations around “no findings” and presence of the correct nested-payload notice across all benign constructor shapes and encodings.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@github-actions

Copy link
Copy Markdown
Contributor

Workflow run and artifacts

Performance Benchmarks

Compared 13 shared benchmarks with a regression threshold of 15%.
Status: 0 regressions, 0 improved, 13 stable, 0 new, 0 missing.
Aggregate shared-benchmark median: 4.423s -> 4.342s (-1.8%).

Workload Benchmark Target Size Files Baseline Current Change Status
direct-malicious-upload tests/benchmarks/test_picklescan_benchmarks.py::test_picklescan_direct_malicious_upload malicious_reduce 52 B 1 239.5us 221.5us -7.5% stable
rejected-basic-auth-candidates tests/benchmarks/test_scan_benchmarks.py::test_rejected_basic_auth_candidates_scan_linearly - 371.1 KiB 1 2.579s 2.516s -2.4% stable
warm-cache-rescan tests/benchmarks/test_scan_benchmarks.py::test_scan_warm_cached_repository_rescan release-candidate 547.3 KiB 32 152.90ms 149.39ms -2.3% stable
padded-multi-stream-upload tests/benchmarks/test_picklescan_benchmarks.py::test_picklescan_padded_multi_stream_upload multi_stream_padded 4.1 KiB 1 332.9us 340.3us +2.2% stable
chunked-upload-stream tests/benchmarks/test_picklescan_benchmarks.py::test_picklescan_chunked_upload_stream chunked_stream 278.2 KiB 1 114.44ms 112.87ms -1.4% stable
nested-payload-review tests/benchmarks/test_picklescan_benchmarks.py::test_picklescan_nested_payload_review[nested_base64] nested_base64 98 B 1 290.0us 293.7us +1.3% stable
nested-payload-review tests/benchmarks/test_picklescan_benchmarks.py::test_picklescan_nested_payload_review[nested_hex] nested_hex 130 B 1 317.7us 314.3us -1.1% stable
duplicate-heavy-registry tests/benchmarks/test_scan_benchmarks.py::test_scan_duplicate_registry_snapshot registry-snapshot 915.2 KiB 13 571.70ms 566.38ms -0.9% stable
suspicious-pickle-intake tests/benchmarks/test_scan_benchmarks.py::test_scan_suspicious_pickle_intake suspicious-intake 183.8 KiB 4 150.03ms 148.76ms -0.8% stable
mixed-model-repository tests/benchmarks/test_scan_benchmarks.py::test_scan_release_candidate_repository release-candidate 547.3 KiB 32 639.04ms 633.77ms -0.8% stable
clean-training-checkpoint tests/benchmarks/test_picklescan_benchmarks.py::test_picklescan_clean_training_checkpoint safe_large 278.2 KiB 1 110.54ms 109.77ms -0.7% stable
single-checkpoint-preflight tests/benchmarks/test_scan_benchmarks.py::test_scan_single_checkpoint_before_load single_checkpoint.pkl 183.0 KiB 1 104.09ms 103.57ms -0.5% stable
nested-payload-review tests/benchmarks/test_picklescan_benchmarks.py::test_picklescan_nested_payload_review[nested_raw] nested_raw 78 B 1 276.2us 275.3us -0.3% stable

@mldangelo-oai

Copy link
Copy Markdown
Contributor Author

@codex review

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

Copy link
Copy Markdown

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: 9c18a9594b

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

assert report.verdict == SafetyVerdict.CLEAN
if source_changes:
assert report.status == ScanStatus.INCONCLUSIVE
if report.status == ScanStatus.INCONCLUSIVE:

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Preserve clean assertions for normal constructor cases

When source_changes is false, this branch allows every ordinary raw/base64/hex case to return INCONCLUSIVE/UNKNOWN; therefore, a regression that makes nested-constructor source tracking consistently report instability would pass instead of detecting that benign inputs no longer scan cleanly. The explicit source_changes=True cases already cover the fail-closed outcome, so keep the normal cases deterministic—mock a stable snapshot if necessary—and require COMPLETE/CLEAN.

AGENTS.md reference: AGENTS.md:L124-L124

Useful? React with 👍 / 👎.

@chatgpt-codex-connector

Copy link
Copy Markdown

Security review completed. No security issues were found in this pull request.

Reviewed commit: 9c18a9594b

View security finding report

ℹ️ About Codex security reviews in GitHub

This is an experimental Codex feature. Security reviews are triggered when:

  • You comment "@codex security review"
  • A regular code review gets triggered (for example, "@codex review" or when a PR is opened), and you’re opted in so security review runs alongside code review

Once complete, Codex will leave suggestions, or a comment if no findings are found.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants