Repository navigation
fix: deduplicate onnx custom domain findings - #1656
mldangelo-oai merged 7 commits into
Conversation
|
@codex review |
Performance BenchmarksCompared
|
|
Codex Review: Didn't find any major issues. More of your lovely PRs please. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
PR #1656 independent reviewExact head: P2: preserve domain and overload identity in explicit custom-operator findings
An exact-head reproduction containing |
|
@codex address that feedback on exact head |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cb4afc8df3
ℹ️ 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".
|
Addressed the exact-head feedback in |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0555e6962d
ℹ️ 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".
|
Pinned dedup QA input: |
|
Addressed the follow-up review feedback in |
|
Final test-only follow-up is in |
|
Additional bounded QA for |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c81728e828
ℹ️ 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".
|
Addressed the long-name collision feedback in |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2ad045ab25
ℹ️ 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".
|
@codex address these exact-head independent-review blockers before merge:
Use raw identities or collision-resistant digests for dedup, length-frame every hashed field, and truncate only display values. Add public JSON/check/SARIF regressions for all three boundaries. Independent report: |
|
Summary
Commit / PR
Testing
|
|
Addressed the exact-head collision blockers in |
|
Codex Review: Didn't find any major issues. Breezy! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Independent merge-gate review: promptfoo/modelaudit PR #1656Reviewed live state at FindingsNo actionable changed-code finding survived adversarial source review, security diff discovery, or runtime validation on the exact current head. I found no P0, P1, P2, or P3 defect introduced by this PR. The previously reported identity-loss and collision defects are fixed at the reviewed head and covered through final JSON, check consolidation, and SARIF output. Changed-code versus pre-existing issues
Merge dispositionCode disposition: approve after live gates clear. The exact head is technically merge-ready based on this review, but GitHub currently reports This is a conditional approval, not an unconditional merge authorization while GitHub’s gates remain blocked. Exact target
The review used a detached clone at Change assessmentThe PR changes reporting after ONNX operator classification:
The generic Prior feedback reconciliationGitHub GraphQL returned seven review threads: 7 resolved, 0 unresolved. Four resolved threads remain attached to current lines; three are outdated. All formal bot reviews are
No prior blocker remains actionable on Security reviewThe security-sensitive surface is attacker-controlled ONNX protobuf metadata flowing through scanner classification, aggregation, final issue/check deduplication, and SARIF serialization. No security candidate survived discovery. Specifically:
Security diff coverage closed 2/2 source-file worklist rows with no candidate findings. The validated security scan artifacts are:
Independent validationAll commands used the detached exact-head clone with
Pinned Hugging Face ONNX modelPinned artifact:
Independent base/head comparison:
The exact-head opt-in integration test passed: Malformed and fail-closed controlsA separate adversarial model contained 25 distinct custom operators plus a
A serialized ONNX The changed regression suite also covers missing opset imports, external-data findings, mixed domains, multiple files, long names/domains, NUL-containing identities, final JSON, check consolidation, and SARIF fingerprints. CI statusSnapshot for exact head
Workflow run: 27324929590. Remaining validation gaps
|
…t31-onnx-custom-domain-dedup-20260610 # Conflicts: # tests/scanners/test_onnx_scanner.py
Summary
occurrence_count, boundedoperator_samples, boundedoperator_identities, and boundedrepresentative_nodes.(domain, op_type, overload)identity through final JSON and SARIF serialization.com.microsoftallowlist, and no change to schema/local-function/Python-operator classification.Root Cause
OnnxScanner._check_custom_ops()deduplicatedmetadata["custom_domains"], but emitted a failedCustom Operator Domain Checkinside the per-node loop. Optimized ONNX exports with repeated runtime kernels therefore produced one S1111 issue/check per node even when the domain evidence was identical.Review follow-up found final-output identity risks after aggregation: explicit standard-domain
custom_opchecks needed domain/overload/hash identity in emitted results, core check consolidation needed per-domain/per-identity keys, long custom operator/domain display truncation could collide, and NUL-delimited hashes were ambiguous for protobuf strings. This PR now keeps raw identities for dedup/counting, emits bounded display values plus SHA-256 identity evidence, and length-frames every hashed field.Security Tradeoff
The change only aggregates reporting after existing classification has decided a node is an external custom operator or explicit custom operator. It does not weaken custom-domain detection, suppress distinct untrusted domains, or change CRITICAL Python operator detection. Malicious/malformed controls cover repeated custom domains plus
PyOp, missing custom-domain opset imports, external-data findings, multi-file evidence, mixed domains, final check/issue consolidation, explicitcustom_opoverload/domain identities, long display-prefix collisions, and NUL-containing identity hash boundaries.Pinned Real-Model QA
Baseline reproduction on starting SHA
8d6c4864fe2ea833ceaef1b9803d225afb1e8d69used the exact pinned revision:Baseline outcome:
onnx/model_O4.onnxis 80,602 bytes withdomain_node_counts {'com.microsoft': 96}andcom_microsoft_ops {'Attention': 24, 'SkipLayerNormalization': 48, 'FastGelu': 24}. Current main emittedfailed_custom_count 96,failed_custom_domains {'com.microsoft': 96}, andmetadata_custom_domains ['com.microsoft']. The same revision'sonnx/model.onnxhas nocom.microsoftgraph nodes;onnx/model_qint8_avx512_vnni.onnxis 561,845,741 bytes and was not downloaded for bounded local QA.Post-fix exact-head QA on
ea8c7cc115fdb59c048ec9dddc4f7ace469b22bb:Post-fix outcome:
failed_custom_check_count=1,failed_custom_domains=['com.microsoft'],com_microsoft_node_count=96,com_microsoft_occurrence_count=96,com_microsoft_ops={'Attention': 24, 'FastGelu': 24, 'SkipLayerNormalization': 48},com_microsoft_distinct_operator_identity_count=3, andcom_microsoft_domain_hash_present=true.Additional bounded QA:
sentence-transformers/all-MiniLM-L12-v2@a50ef00143b4d5391434df20ae11632588ac25be, filesonnx/model_O2.onnx,onnx/model_O3.onnx, andonnx/model_O4.onnx, each now emits exactly onecom.microsoftS1111 check withoccurrence_count=36and two distinct operator identities.Validation
uv sync --extra all-ciPROMPTFOO_DISABLE_TELEMETRY=1 uv run pytest tests/scanners/test_onnx_scanner.py -k "long_custom_domain_operator_identities or long_custom_domains_survive or identity_hash_length_frames or custom_domain_aggregate_reports or custom_domain_custom_op_overloads or custom_domains_survive_core or long_explicit_custom_op" -m "not slow and not integration" --maxfail=1-> 7 passedPROMPTFOO_DISABLE_TELEMETRY=1 uv run pytest tests/scanners/test_onnx_scanner.py -m "not slow and not integration" --maxfail=1-> 258 passed, 1 deselected, 1 warningMODELAUDIT_RUN_HF_REAL_MODEL_TESTS=1 PROMPTFOO_DISABLE_TELEMETRY=1 HF_HUB_DISABLE_TELEMETRY=1 HF_HOME=/tmp/modelaudit-hf-t31-cache uv run pytest tests/scanners/test_onnx_scanner.py -k "pinned_hf_multilingual" -m "slow and integration" -s --maxfail=1-> 1 passed, 258 deselecteduv run ruff format --check modelaudit/ packages/modelaudit-picklescan/src packages/modelaudit-picklescan/tests tests/-> 419 files already formatteduv run ruff check modelaudit/ packages/modelaudit-picklescan/src packages/modelaudit-picklescan/tests tests/-> all checks passeduv run mypy modelaudit/ packages/modelaudit-picklescan/src packages/modelaudit-picklescan/tests tests/-> success, 474 source filesPROMPTFOO_DISABLE_TELEMETRY=1 uv run pytest -n auto -m "not slow and not integration" --maxfail=1-> 18,551 passed, 793 skipped, 40 warningsPROMPTFOO_DISABLE_TELEMETRY=1 uv run pytest tests/scanners/test_onnx_scanner.py -k "long_custom_domain_operator_identities or long_custom_domains_survive or identity_hash_length_frames" -m "not slow and not integration" --maxfail=1-> 3 passedgit diff --check-> cleanFinal fetch before validation:
origin/mainremained8d6c4864fe2ea833ceaef1b9803d225afb1e8d69, so no merge was needed.