fix(rotation): seal candidate container before iteration - #1323
Merged
Conversation
Contributor
|
Warning Review limit reachedNext included review available in 44 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
seonghobae
marked this pull request as ready for review
August 24, 2026 22:02
seonghobae
enabled auto-merge (squash)
August 25, 2026 01:34
auto-merge was automatically disabled
August 25, 2026 01:41
Pull request was closed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Defect
Rotation scalar/text controls on protected main are callback-safe after #1013, but
rotation_selection._candidate_names()still accepted an arbitrarySequence[str]and immediately iterated it. A caller-defined list/tuple/Sequence subclass could therefore execute__iter__while the package was still establishing which rotation criteria would be compared, before loading-evidence validation or compiled Rust-core discovery.Candidate criteria define the scientific comparison set passed to the Rust-owned selector, so the container itself is part of the Python trust boundary.
RED → GREEN
02583da92c45c1b174d7ae0d0132a62f25b4e9bc: public regression supplies a hostile list subclass whose__iter__must remain at zero and whose invalid admission must occur beforerotation_core()discovery; exact built-in list/tuple normalization compatibility is also pinned.db34721d87919673aade828f7ead6624f830abc4:_candidate_names()now establishes exact built-inlist/tupleidentity before iteration, then reuses the existing exact-string_method_name()boundary plus the existing minimum-count and uniqueness contracts.fac2b259624ad6e0dd9f7da8353c1dc28b5cec07records the fail-closed candidate-container contract.489eecde0a811fbddd563a17e2efde015660919b: public/internal annotations advertise the actually supported inert container contract (list[str] | tuple[str, ...]) instead of callback-capableSequence[str].Concurrent lineage reconciliation and CI RCA
The branch later advanced compatibly from
489eecde...to680689ebb20291719dc40b048f664669f7a2ff60. The intervening commits brought unrelated main-line statistical-study/recovery/CI-contract changes into the branch; comparing that head to current protectedmain@66bec7e551eb82dd41338deb5c8374ff4557ceefstill left only the rotation source/regression/changelog as the effective product delta.Exact-head CI run
32760522484on680689ebb...failed only in Python: package/Rust/GPU/fuzz succeeded, while both Python legs reached pytest and the 3.12 log reported1 failed, 5859 passed, 2 skipped. The sole failure wastests/test_rotation_selection.py::test_selection_validation_is_fail_closed: the predecessor regression expected the old vague diagnosticsequence, while the intentional public narrowing now correctly emitscandidates must be an exact list or tuple of criterion names. The new callback-safety regression itself passed.90020e7596e1e205348aca22efdfc248145feb1e: update only that stale predecessor expectation to the canonical exact-list/tuple diagnostic. Production behavior is unchanged; the test now asserts the intended fail-closed contract instead of an obsolete message fragment.Against current protected main the effective diff is exactly four rotation-related files:
python/fast_mlsirm/rotation_selection.py,tests/test_rotation_candidate_container_callback_safety.py,tests/test_rotation_selection.py, anddocs/changelog.d/rotation-candidate-container-callback-safety.md(6 commits ahead / 2 behind, merge basec26dac4bcdb7d600a2688c3e74eb4a68c0b45620). The two later main commits are release-cut/accessibility changes and do not overlap this rotation surface.Ownership boundary
Python validation/marshalling and regression-contract repair only. Rotation objectives, analytic gradients, GPA multi-start optimization, bootstrap congruence, policy scoring, Pareto selection, convergence, and every result-affecting psychometric/statistical operation remain unchanged and Rust-owned.
Exact integration boundary
This PR is now Ready for review at exact head
90020e7596e1e205348aca22efdfc248145feb1e. Repository-local CI, Security Scan, CodeQL, and SAST Semgrep are all terminalsuccess; formal reviews and review threads were empty immediately before the Ready transition. Making the PR Ready allows the protected central review/workflow ruleset to generate its required current evidence; it is not merge authority.Current repository rules require two approving reviews, last-push approval, resolved review threads, and the protected central workflows. The branch remains 2 unrelated commits behind protected
main@66bec7e551eb82dd41338deb5c8374ff4557ceef, so base-sensitive/merge-result evidence must also be refreshed before landing. No gate weakening, force update, review dismissal, self-approval, or ruleset bypass is used.