Skip to content

fix: detect operator archive Python execution paths - #1538

Merged
mldangelo-oai merged 5 commits into
mainfrom
mdangelo/codex/fix-archive-python-operator-detector
Jun 9, 2026
Merged

mldangelo-oai merged 5 commits into
mainfrom
mdangelo/codex/fix-archive-python-operator-detector

Conversation

@mldangelo-oai

@mldangelo-oai mldangelo-oai commented Jun 8, 2026 •

Copy link
Copy Markdown
Contributor

Summary

  • Detect operator accessor execution paths through assigned multi-result accessors, namespace mappings, and static containers.
  • Preserve bounded container mutation state across aliases, augmented assignment, methodcaller writers, and literal/tracked mapping expansion.
  • Support bounded hashable itemgetter keys while rejecting invalid runtime shapes that previously caused false positives.

Validation

  • Published commit: 227e8566038cc1408dc4ac8bceca65aa9674f3e3
  • Exact remote tree verified byte-for-byte against the reviewed worktree
  • PROMPTFOO_DISABLE_TELEMETRY=1 uv run --frozen pytest -q tests/scanners/test_zip_scanner.py: 829 passed, 4 optional ONNX skips
  • Focused operator/namespace regressions: 257 passed
  • Scoped Ruff lint/format, mypy, and git diff --check: clean
  • All review threads replied to and resolved

@github-actions

github-actions Bot commented Jun 8, 2026 •

Copy link
Copy Markdown
Contributor

Workflow run and artifacts

Performance Benchmarks

Compared 12 shared benchmarks with a regression threshold of 15%.
Status: 0 regressions, 0 improved, 12 stable, 0 new, 0 missing.
Aggregate shared-benchmark median: 1.383s -> 1.386s (+0.2%).

Workload Benchmark Target Size Files Baseline Current Change Status
nested-payload-review tests/benchmarks/test_picklescan_benchmarks.py::test_picklescan_nested_payload_review[nested_hex] nested_hex 130 B 1 451.8us 468.6us +3.7% stable
chunked-upload-stream tests/benchmarks/test_picklescan_benchmarks.py::test_picklescan_chunked_upload_stream chunked_stream 278.2 KiB 1 114.09ms 118.17ms +3.6% stable
clean-training-checkpoint tests/benchmarks/test_picklescan_benchmarks.py::test_picklescan_clean_training_checkpoint safe_large 278.2 KiB 1 110.93ms 114.31ms +3.1% stable
single-checkpoint-preflight tests/benchmarks/test_scan_benchmarks.py::test_scan_single_checkpoint_before_load single_checkpoint.pkl 183.0 KiB 1 71.41ms 73.53ms +3.0% stable
nested-payload-review tests/benchmarks/test_picklescan_benchmarks.py::test_picklescan_nested_payload_review[nested_base64] nested_base64 98 B 1 449.9us 461.4us +2.6% stable
mixed-model-repository tests/benchmarks/test_scan_benchmarks.py::test_scan_release_candidate_repository release-candidate 547.3 KiB 32 480.92ms 469.82ms -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 516.6us 522.4us +1.1% stable
suspicious-pickle-intake tests/benchmarks/test_scan_benchmarks.py::test_scan_suspicious_pickle_intake suspicious-intake 183.8 KiB 4 120.78ms 121.98ms +1.0% stable
duplicate-heavy-registry tests/benchmarks/test_scan_benchmarks.py::test_scan_duplicate_registry_snapshot registry-snapshot 915.2 KiB 13 389.57ms 393.19ms +0.9% stable
direct-malicious-upload tests/benchmarks/test_picklescan_benchmarks.py::test_picklescan_direct_malicious_upload malicious_reduce 52 B 1 434.2us 432.0us -0.5% stable
nested-payload-review tests/benchmarks/test_picklescan_benchmarks.py::test_picklescan_nested_payload_review[nested_raw] nested_raw 78 B 1 448.4us 449.7us +0.3% stable
warm-cache-rescan tests/benchmarks/test_scan_benchmarks.py::test_scan_warm_cached_repository_rescan release-candidate 547.3 KiB 32 92.57ms 92.58ms +0.0% stable

Detect itemgetter, multi-accessor selection, mapping defaults, literal and assigned containers, and preserve benign accessor behavior.
@mldangelo-oai
mldangelo-oai marked this pull request as ready for review June 8, 2026 19:20

Copy link
Copy Markdown
Contributor Author

Critical review fixes pushed in 06852a75e35fbc8802e810269015159a79c9baf3.

Addressed false negatives for literal .__call__ attrgetter fields, itemgetter, multi-accessor selection/unpacking, mapping defaults, attrgetter('__dict__'), numeric-equivalent keys, literal/assigned containers, and conditional container branches. Addressed false positives for dotted methodcaller names, nonexistent __getattr__, selected safe tuple members, safe existing-key defaults, and mutated tracked containers.

Focused validation only, per review policy:

  • pytest tests/scanners/test_zip_scanner.py -k operator_accessor -q (52 passed)
  • ruff check on the changed scanner/test files
  • mypy on the changed scanner/test files
  • direct malicious/benign probes for mutation and conditional-branch behavior

@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: 06852a75e3

ℹ️ 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 thread modelaudit/scanners/archive_member_security.py Outdated
Comment thread modelaudit/scanners/archive_member_security.py Outdated
Comment thread modelaudit/scanners/archive_member_security.py Outdated
Comment thread modelaudit/scanners/archive_member_security.py

Copy link
Copy Markdown
Contributor Author

I completed a critical review and prepared local commit 4f023fcc (fix: close operator accessor dataflow gaps), but the push is currently blocked because the configured HTTPS credential is rejected by GitHub (Invalid username or token). I have not resolved the review threads because the code is not published yet.

The local fix addresses all four open review findings:

  • precise container writes before itemgetter, including subscript assignment, update, descriptor methods, setdefault, |=, deletes, and sequence mutations;
  • static None keys for itemgetter/methodcaller;
  • methodcaller('__getattribute__', '__dict__') namespace maps;
  • assigned multi-result attrgetter/itemgetter sequences.

The review also found and fixed additional false negatives and false positives around append/extend/insert, shifting pop/reverse, unresolved-key writes, +=/*=, safe overwrites/fallbacks, and oversized static containers. Analyzer state is now bounded to 32 precise items plus one overflow representative per rule code; a 2,000-write probe completes in about 0.98s for assignments and 5.56s for setdefault, while dangerous evicted values remain detectable.

Validation on the exact local commit:

  • focused operator/container matrix: 110 passed
  • associated module: 821 passed, 1 warning
  • full configured Ruff check: clean
  • full configured Ruff format check: 409 files clean
  • full configured mypy: 464 files clean
  • git diff --check: clean

Only modelaudit/scanners/archive_member_security.py and tests/scanners/test_zip_scanner.py are in the commit.

@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: e70b3870b2

ℹ️ 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 thread modelaudit/scanners/archive_member_security.py Outdated
Comment thread modelaudit/scanners/archive_member_security.py
Comment thread modelaudit/scanners/archive_member_security.py Outdated
Comment thread modelaudit/scanners/archive_member_security.py Outdated
Comment thread modelaudit/scanners/archive_member_security.py Outdated

Copy link
Copy Markdown
Contributor Author

@codex Please apply the completed review fix from local commit bbe41831 to this PR branch, then merge current main (067dcba7). The checkout cannot push because its HTTPS credential is invalid.

Files:

  • modelaudit/scanners/archive_member_security.py
  • tests/scanners/test_zip_scanner.py

Required changes:

  1. Gate namespace methodcaller defaults on actual key presence; treat untouched os.getcwd as present, but track assignment/deletion so the fallback becomes reachable after del os.getcwd.
  2. Model tracked sequence +=/*= via bounded __iadd__/__imul__ mutations.
  3. Preserve assigned methodcaller('update', ...) positional, keyword, literal **, and complete tracked-dict **updates writes with last-write semantics.
  4. Flatten bounded literal dict unpacking in evaluation order for direct and assigned static containers.
  5. Support bounded hashable literal itemgetter keys: bytes, finite complex, Ellipsis, and recursive tuples.
  6. Include all paired malicious/benign direct and ZIP-backed regressions from the local diff.

Local commits: bbe41831 (fix), 584dcdc7 (merge current main).

Validation after merging main:

  • PROMPTFOO_DISABLE_TELEMETRY=1 .venv/bin/pytest -q tests/scanners/test_zip_scanner.py -k 'operator_accessor or namespace_mapping' -> 256 passed, 575 deselected
  • scoped Ruff check/format -> clean
  • scoped mypy -> clean
  • git diff --check origin/main...HEAD -> clean

All five unresolved review threads have commit-specific replies. Please publish the implementation before resolving them.

@chatgpt-codex-connector

Copy link
Copy Markdown

Summary

Committed changes as 61ad7f7 and created the PR via make_pr.

Testing

  • ✅ PROMPTFOO_DISABLE_TELEMETRY=1 .venv/bin/pytest -q tests/scanners/test_zip_scanner.py -k 'operator_accessor or namespace_mapping'
  • ✅ uv run ruff check modelaudit/scanners/archive_member_security.py tests/scanners/test_zip_scanner.py
  • ✅ uv run ruff format --check modelaudit/scanners/archive_member_security.py tests/scanners/test_zip_scanner.py
  • ✅ uv run mypy modelaudit/scanners/archive_member_security.py tests/scanners/test_zip_scanner.py
  • ✅ git diff --check

View task →

Copy link
Copy Markdown
Contributor Author

@codex address that feedback

Local repair handoff for the five unresolved review threads is commit 0150269e on top of current main merge a886af98 (published PR head was e70b3870). HTTPS push is blocked in this checkout by Invalid username or token.

The patch:

  • gates namespace defaults on actual key presence and tracks os.getcwd assignment/deletion;
  • preserves tracked list += / *= and shared list/dict mutations through aliases without conflating separate containers or tuple rebinding;
  • carries assigned methodcaller('update', ...) positional, keyword, literal **, and tracked-dict ** writes with last-write semantics;
  • flattens bounded literal dict unpacking in evaluation order;
  • supports bounded bytes, finite complex, Ellipsis, and recursive tuple itemgetter keys.

Validation: 255 passed, 575 deselected for test_zip_scanner.py -k 'operator_accessor or namespace_mapping'; full Ruff lint/format clean (409 files); full mypy clean (464 files). User-owned SECURITY.md and security_best_practices_report.md were excluded and unchanged.

@chatgpt-codex-connector

Copy link
Copy Markdown

Summary

Committed the changes as 7b971d9 and created the PR record with make_pr.

Testing

  • ✅ PROMPTFOO_DISABLE_TELEMETRY=1 uv run pytest -q tests/scanners/test_zip_scanner.py -k 'operator_accessor or namespace_mapping'
  • ✅ uv run ruff format modelaudit/ packages/modelaudit-picklescan/src packages/modelaudit-picklescan/tests tests/
  • ✅ uv run ruff check --fix modelaudit/ packages/modelaudit-picklescan/src packages/modelaudit-picklescan/tests tests/
  • ✅ uv run mypy modelaudit/ packages/modelaudit-picklescan/src packages/modelaudit-picklescan/tests tests/
  • ✅ git diff --check
  • ⚠️ PROMPTFOO_DISABLE_TELEMETRY=1 uv run pytest -n auto -m "not slow and not integration" --maxfail=1 was started but did not complete in this environment after several minutes and was terminated to avoid a hung handoff.

View task →

Copy link
Copy Markdown
Contributor Author

Published the fully reviewed repair as 227e8566. In addition to the five review-thread fixes, this closes shared-container alias false negatives/stale false positives, validates methodcaller keyword and positional arity, accepts semantically empty **{}, rejects invalid dict-display unpacking, and bounds cumulative update expansion. Associated validation: 829 passed, 4 optional ONNX skips; focused operator/namespace slice 257 passed; scoped Ruff, format, mypy, and diff checks clean. The fetched remote tree exactly matches the reviewed worktree.

@mldangelo-oai
mldangelo-oai merged commit ae81048 into main Jun 9, 2026
23 of 27 checks passed
@mldangelo-oai
mldangelo-oai deleted the mdangelo/codex/fix-archive-python-operator-detector branch June 9, 2026 05:45

@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: 227e856603

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

"append": {2},
"get": {2, 3},
"insert": {3},
"pop": {1, 2, 3},

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 Handle no-arg methodcaller pop

For methodcaller('pop') with no pop argument, this leaves lookup_name empty, so list pop() (last item) is never resolved; payload=[print, os.system]; operator.methodcaller('pop')(payload)('id') runs without S101. Keep this malicious path covered per guidance.

Useful? React with 👍 / 👎.

Comment on lines +1093 to +1094
if not isinstance(value_node, ast.Dict):
return None

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 Expand tracked dicts in literal unpacking

For {**updates} where updates is a tracked static dict, this returns None rather than expanding the alias, so operator.itemgetter('run')({**updates})('id') misses os.system even though update(**updates) is modeled. Fresh evidence: this direct unpack path. guidance.

Useful? React with 👍 / 👎.

Comment on lines +3983 to +3986
if isinstance(target_element, ast.Name):
self._bind_name(target_element.id, resolved_names)
else:
self._shadow_binding_target(target_element)

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 starred accessor unpacking

When a multi-result accessor is unpacked into a starred target, this shadows the star capture instead of binding the slice, so _, *rest = operator.attrgetter('getcwd','system')(os); rest[0]('id') executes os.system without S101. Fresh evidence: starred destructuring. guidance.

Useful? React with 👍 / 👎.

@github-actions github-actions Bot mentioned this pull request Jun 24, 2026
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