Repository navigation
refactor(picklescan): simplify native reports and source analysis - #1871
Conversation
…/codex/modelaudit-03-raw-evidence
…o/codex/modelaudit-04-picklescan
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Codex Review: Didn't find any major issues. Keep it up! Reviewed commit: ℹ️ 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". |
🛡️ Codex Security Review · Automatically triggeredSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
|
@codex review |
|
Codex Review: Didn't find any major issues. Bravo. Reviewed commit: ℹ️ 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". |
🛡️ Codex Security Review · Automatically triggeredSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
|
@codex review Please review the current head after integration with main |
🛡️ Codex Security Review · Automatically triggeredSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
![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: bdf327f664
ℹ️ 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".
| } | ||
| Ok(dict.into_any().unbind()) | ||
| } | ||
| DetailValue::Dict(values) => Ok(detail_dict_to_py(py, values)?.into_any()), |
There was a problem hiding this comment.
Building the PyO3 extension fails here because detail_dict_to_py(...).into_any() returns a Bound<'_, PyAny>, while to_py_object must return Py<PyAny>; unlike the adjacent list branch, this expression never calls .unbind(). Add .unbind() after .into_any() so the independently published native package can compile.
AGENTS.md reference: packages/modelaudit-picklescan/AGENTS.md:L7-L10
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
detail_dict_to_py already returns PyResult<Py<PyDict>> and calls dict.unbind() in report.rs:203–209. The caller therefore invokes into_any() on an owned Py<PyDict>, yielding the required owned Py<PyAny>; the adjacent list starts as Bound and needs its own unbind. Verified the exact reviewed head with cargo check --locked --offline --manifest-path packages/modelaudit-picklescan/Cargo.toml --features extension-module,abi3: it passes. No additional unbind is needed.
The standalone scanner repeats report construction, integer-option validation, and source-analysis wrappers. Consolidate those paths, reuse the existing Rust detail conversion and PyO3 map conversion, and remove private branches whose callers always select the same behavior.
Public exports, option defaults and errors, scan bounds, fail-closed verdicts, finding order, and the zero-Python-dependency wheel boundary remain unchanged. The six affected assignment-alias test setups now call the same lower-level composition directly; their assertions are retained. This is PR 4 of the six-part simplification series; PRs 1–3 have landed. Net reduction: 214 maintained lines (216 production lines removed, two test lines added).
Validation:
Current-main integration (
2a5185a): preserves the upstream SafeTensors FDICT fix and deterministic initial/retried cache interruption fixture. Focused cache/routing checks and five neighboring compression cases pass on this branch; real benign/malicious CLI scans preserve scanner selection, JSON findings, SHA-256 metadata, and exit codes 0/1. Ruff checks pass. Changed-file mypy passes; full local mypy retains the previously documented optional TensorFlow baseline error. Hosted CI validates the published head.Before the latest base integration, the committed prefix passed 1,948 affected result, CLI, streaming, acquisition and SARIF/SBOM tests, with three Windows-only and two opt-in integration skips. Every file and mode matches the independently reconstructed PR4 prefix; this PR’s six-file standalone change is preserved.
Rust formatting, check, Clippy with warnings denied, and 187 Rust tests passed.
All 19 standalone test modules ran: 3,334 passed, three failed and one skipped. One failure came from the validation harness disabling bytecode creation; its exact case passed on parent and candidate with that setting removed. Both remaining Click 8.5 failures reproduce on the unchanged parent: a
LazyFile/_LazyFilepath expectation and a fragmented startup-hook verdict. No assertions were weakened.Rebuilt native extension and isolated wheel match this checkout. Clean, malicious and truncated wheel scans passed without Python runtime dependencies.
49 original/candidate option, report and stream-callback comparisons match. Only durations and originally nondeterministic opcode-counter mapping order are normalized.
On the actual PR4 prefix, root adapter tests passed 98 cases with the rebuilt candidate and 98 with the published 0.1.10 floor; one remote-model acquisition test was deselected in each run. Published Python files match release tag
modelaudit-picklescan-v0.1.10exactly.Full repository Ruff formatting/lint passed; installed mypy 2.3.1 passed all 28 standalone source/test files. The full repository check reports only the existing unreachable TensorFlow import in
tests/scanners/test_weight_distribution_scanner.py. Upstream-required mypy 2.4 could not be installed through this devbox's package index; its pin is preserved.No package version or dependency constraint is changed. Rust 1.83 and other supported platform/Python combinations await the hosted CI matrix.
Latest integration: merged main
86c40e4after #1870 landed. The original six-file picklescan patch is byte-identical against main; only two upstream dependency-test guards were added to the branch. All 22 dependency-lock tests, full Ruff lint/format checks, and focused mypy passed. Two fresh native reviews and an independent verifier found no blockers. Hosted CI is being refreshed forf279969. A Windows run exposed a pre-existing filesystem-sensitive cache test: unrelated changes above its temporary directory can make the cache return early. Paired parent/candidate experiments reproduced the failure; one existing fixture helper now scopes identity checks to the actual test tree. Both hit/miss and source-revalidation assertions remain unchanged. All seven importer-neighbor tests and eight fresh-process normal/churn probes pass. No production code changed for this CI fix.PRs 1: tooling, 2: tests, and 3: raw evidence have landed. Remaining merge order: 4: picklescan → 5: scanning/results → 6: acquisition/cache/progress. PR4 targets
main; PRs 5–6 target the preceding branch.