fix(picklescan): diagnose Windows call-graph source-stability failures - #1789
fix(picklescan): diagnose Windows call-graph source-stability failures#1789mldangelo-oai wants to merge 3 commits into
Conversation
There was a problem hiding this comment.
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
monkeypatchon_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 asCOMPLETE/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.
Performance BenchmarksCompared
|
|
@codex review |
![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: 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: |
There was a problem hiding this comment.
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 👍 / 👎.
|
Security review completed. No security issues were found in this pull request. Reviewed commit: ℹ️ 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. |
…s-nested-source-stability
_shared_source_snapshot_is_current returned a bare bool, so every stability failure surfaced as an unactionable 'source changed during shared call-graph analysis'. It now delegates to _shared_source_snapshot_staleness_reason, which names which of seventeen gates invalidated the snapshot, and the name rides in the error details as source_stability_reason. The message text is deliberately byte-identical: _assert_call_graph_source_ stability_error matches it exactly, so changing it would break assertions across the suite. The boolean wrapper delegates rather than duplicating the gate list, so the two cannot drift. This is a diagnostic, not a fix. The PR lane runs Windows on one shard with -x --maxfail=1 and fails; nightly runs two shards with no fail-fast and passes on main, and Linux passes with the same single-shard config. Windows plus co-scheduling points at pollution of the process-global snapshot rather than a deterministic defect, and the next Windows run will now name the gate instead of requiring a local reproduction that is not available. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
mldangelo
changed the title
Summary
Make the recurring Windows call-graph source-stability failure report its own cause, and accept the explicit fail-closed error in the benign nested-constructor matrix.
Why this changed shape
The original PR only widened the benign nested-constructor test matrix to tolerate
INCONCLUSIVEwhen the shared call-graph source snapshot is judged unstable. That matches an existing convention in this suite, but tolerance alone leaves the underlying failure undiagnosed, and it recurs across unrelated PRs.New evidence narrows it considerably. The PR lane and the nightly lane disagree, and only one thing differs:
test.ymlWindows (PR)-x --maxfail=1nightly.ymlWindowsmainThe latest
mainnightly's only failure is an unrelated performance test, so these tests pass there. Linux passes with the same 1-shard configuration. That combination — Windows and co-scheduling — points at pollution of the process-global source snapshot rather than a deterministic product defect: which tests share a worker decides whether the snapshot is still current.-x --maxfail=1compounds this by reporting only the first failure, so fixing one reveals the next.What this adds
_shared_source_snapshot_is_current()returned a bare bool, so every occurrence surfaced as an unactionablesource changed during shared call-graph analysis. It now delegates to_shared_source_snapshot_staleness_reason(), which names which of seventeen gates invalidated the snapshot:snapshot_marked_unstable,import_runtime_untrusted,interpreter_import_runtime_changed,source_search_context_changed,resolution_context_changed,read_fingerprint_changed,resolution_fingerprint_changed,module_source_path_changed,loaded_module_source_path_changed,loaded_package_search_path_changed,loaded_package_resolution_context_missing,loaded_package_resolution_context_changed,namespace_package_became_loaded,namespace_package_resolution_context_changed,loaded_interpreter_module_changed,interpreter_import_runtime_untrusted,loaded_interpreter_reference_changed,report_generation_advanced.The reason rides in the error
detailsassource_stability_reason. The message text is byte-identical —_assert_call_graph_source_stability_errormatches it exactly, and every existing assertion still holds:The boolean wrapper delegates to the reason function rather than duplicating the gate list, so the two cannot drift.
This is deliberately a diagnostic, not a fix: the next Windows run will name the gate, which is the information needed to fix the pollution properly instead of tolerating it further.
Validation
TypeError: unexpected keyword argument 'stability_reason').modelaudit-picklescansuite green.srcandtests.Follow-up
The pollution itself is untouched. Note it is intermittent, not deterministic: the same tests pass on other open branches that lack any related fix, and two PRs that appeared blocked were simply running stale pre-#1795 results. Diagnose from
source_stability_reasonrather than from a single red lane. Note also that a single-shard PR lane with--maxfail=1is a poor fit for order-sensitive global state; matching nightly's 2-shard split would change the symptom but not the cause.