Skip to content

fix(fitstats): seal S-X² scalar control admission - #1029

Merged
seonghobae merged 17 commits into
mainfrom
fix/fitstats-sx2-callback-boundary-1028
Aug 25, 2026
Merged

seonghobae merged 17 commits into
mainfrom
fix/fitstats-sx2-callback-boundary-1028

Conversation

@seonghobae

@seonghobae seonghobae commented Aug 19, 2026 •

Copy link
Copy Markdown
Contributor

Closes #1028.
Closes #1308.
Closes #1365.

Defects

The public S-X² control boundary had three compatible trust-boundary defects:

  1. caller-defined int/float subclasses could execute conversion callbacks before Rust fit-statistic dispatch;
  2. exact built-in/concrete NumPy integers accepted for real-valued controls could be silently rounded by binary64 normalization (for example 2**53 + 1); and
  3. on platforms where np.longdouble is wider than binary64, an exact finite long-double control could likewise be silently rounded before the Rust f64 boundary.

RED → GREEN

Callback-free scalar admission — #1028

  • RED f981d5561bc97d2357bef4ce48483546efeef651: hostile integer/floating subclasses fail with package-owned ValueError and zero caller callbacks across q_theta, q_xi, min_expected, fdr_q, and min_effect.
  • GREEN 0ded85f7156dc6cb0f1fe84d625bf380331747c5: exact built-in/concrete-NumPy scalar admission with the existing quadrature and numeric domains.
  • Install/evidence 0b078963e70673f459a9aac0cf26552c2cc745c0 / 52763f55fa0b767b355ac3fe236156e4605ea310; NumPy-integer compatibility 7a36b0d76dc84f825931c145ef56188d979006d2.

Lossless integer-valued real controls — #1308

  • RED 50c9d185e7dcf003692f131992b041d8e487bea2: built-in 2**53 + 1 and concrete np.uint64(2**53 + 1) cannot survive a value-changing binary64 conversion.
  • GREEN 1baa218a7ea7e7c5295d3f7f2efe093db31aaadd: integer-valued real controls convert only when binary64 round-trips to the exact admitted integer; overflow retains the stable finite-number failure.

Lossless extended-precision real controls — #1365

  • RED 8f59a28919dc13432f80ce09c575ea9cd0945fa0: where the platform exposes wider np.longdouble, a finite value immediately adjacent to 1.0 must not be silently rounded to binary64; an exactly representable long-double remains supported.
  • GREEN 6720e6ffbacf60c3d29bcffbbe149a39033307d3: every trusted NumPy floating control converts to built-in float only when converting back to its exact NumPy scalar type preserves the admitted value; non-finite/overflow behavior remains fail-closed.
  • Governed evidence/current source head 2f4a4e0306b01a6de0eeca030e15a510b06012e1 records integer and floating-point losslessness at the Rust f64 boundary.

Ownership boundary

Python validation/marshalling only. S-X²/G² statistics, Orlando–Thissen/Lord–Wingersky quadrature/fit-statistic arithmetic, BH/FDR decisions, effect-size calculations, likelihoods, and every production numerical result remain unchanged and Rust-owned.

Verification boundary

Protected base at the immediately preceding fresh refetch is main@9c12eab15fb8a187b135f9be1961f0693a431c23. Exact current source/evidence head immediately before this lifecycle write is 2f4a4e0306b01a6de0eeca030e15a510b06012e1, open, non-Draft and unmerged. PR #1029 remains the only active S-X² control writer; predecessor-head checks/reviews do not transfer.

Every applicable exact-current-head CI/security/coverage/package/fuzz/SBOM/provenance/dependency context plus the active ruleset's independent approvals, last-push approval, and resolved required conversations must be complete before merge. No self-approval, gate weakening, force update, or cross-repository mutation is used.

Summary by CodeRabbit

  • Bug Fixes

    • Hardened S-X² controls to reject unsafe numeric subclasses before processing.
    • Added validation for supported ranges, finite values, and exact float64 representation.
    • Prevented lossy integer and extended-precision conversions.
    • Preserved support for exact built-in and compatible NumPy scalar values.
  • Tests

    • Added regression coverage for rejected conversions and accepted lossless values.
  • Documentation

    • Documented the updated S-X² control validation behavior.

@coderabbitai

coderabbitai Bot commented Aug 19, 2026 •

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 90cc0316-aca3-4026-843c-10fde65cd3e8

📥 Commits

Reviewing files that changed from the base of the PR and between 3695142 and 2f4a4e0.

📒 Files selected for processing (3)
  • docs/changelog.d/1028-fitstats-sx2-control-callback-safety.md
  • python/fast_mlsirm/_fitstats_control_safety.py
  • tests/test_fitstats_sx2_control_callback_safety.py

Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review.


📝 Walkthrough

Walkthrough

The change adds callback-free S-X² control validation. It accepts exact built-in and concrete NumPy scalars, rejects caller-defined numeric subclasses, preserves supported domains, and rejects values that cannot be represented exactly as float64.

Changes

S-X² control safety

Layer / File(s) Summary
Control admission and package wiring
python/fast_mlsirm/_fitstats_control_safety.py, python/fast_mlsirm/__init__.py
Adds trusted scalar conversion, extended-precision checks, quadrature and domain validation, lossless normalization, and package initialization wiring.
Regression coverage and changelog
tests/test_fitstats_sx2_control_callback_safety.py, docs/changelog.d/1028-fitstats-sx2-control-callback-safety.md, CHANGELOG.md
Tests subclass rejection, NumPy scalar compatibility, lossy integer rejection, and lossless or lossy longdouble conversion. Documents the validation behavior.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Merge Risk: ⚪ Minimal · up to 2f4a4

The change tightens S-X² scalar validation to prevent callback execution and lossy numeric conversion while leaving statistical calculations unchanged; no actionable merge-blocking risk remains beyond normal checks and review.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly identifies the fitstats change and the hardened S-X² scalar-control admission.
Linked Issues check ✅ Passed The changes satisfy the coding objectives in issues #1028, #1308, and #1365: callback-free scalar admission, exact integer-to-float64 validation, lossless long-double handling, preserved domain behavi…
Out of Scope Changes check ✅ Passed The validation module, package integration, regression tests, and changelog updates directly support the linked issue objectives. No unrelated code changes are identified.
Docstring Coverage ✅ Passed Docstring coverage is 94.44% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 18 functions across 3 files. (1 skipped: 1 …
Full details: Linked Issues check

Explanation

The changes satisfy the coding objectives in issues #1028, #1308, and #1365: callback-free scalar admission, exact integer-to-float64 validation, lossless long-double handling, preserved domain behavior, and Rust-owned numerical computation.

Full details: Docstring Coverage

Explanation

Docstring coverage is 94.44% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 18 functions across 3 files. (1 skipped: 1 unsupported.)

✨ Finishing Touches 💡 1
🛠️ Fix failing CI checks 💡
  • Create stacked PR
  • Commit on current branch
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/fitstats-sx2-callback-boundary-1028

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@seonghobae
seonghobae marked this pull request as ready for review August 19, 2026 14:47

Copy link
Copy Markdown
Contributor Author

@opencode-agent review
@cwl-noema-review review

Re-dispatch unchanged exact head 7a36b0d76dc84f825931c145ef56188d979006d2 against protected main@04d0bc21a2a20693bcf16108cd76d394fe844d23 under central .github@bbedc1a51ec1a2421f129955c629b3cd0507a4ec (includes trusted-uv fix #1136 and scheduler OIDC fix #1140). Repository CodeQL, Semgrep, and Security Scan are already exact-head success; CI may finish in parallel. Please bind any verdict to this exact head and current central workflow evidence; no predecessor-head evidence transfers.

@seonghobae seonghobae closed this Aug 19, 2026
@seonghobae seonghobae reopened this Aug 19, 2026
@seonghobae
seonghobae enabled auto-merge (squash) August 20, 2026 01:11
@seonghobae

Copy link
Copy Markdown
Contributor Author

@opencode-agent review
@cwl-noema-review review

Please review exact current head 7a36b0d76dc84f825931c145ef56188d979006d2 against protected main@04d0bc21a2a20693bcf16108cd76d394fe844d23. Focused S-X² callback-safety tests pass locally (6 passed). Exact-head CI, coverage, CodeQL, Semgrep, package, fuzz, and GPU-smoke evidence is successful; the current hosted Strix result is the only failed required check and its repository rerun endpoint is unavailable. Preserve fail-closed security policy, reacquire same-head provider evidence when available, and do not modify fit-statistic arithmetic for an infrastructure failure.

@seonghobae

Copy link
Copy Markdown
Contributor Author

Current required-check review for head 7a36b0d76dc84f825931c145ef56188d979006d2: strix is the only reported failure; the hosted job/log route does not expose a usable report for this run, and the provider rerun route remains unavailable. No source defect is evidenced, so keep the fit-statistics patch and required gate unchanged; rerun through the hosted workflow when available.

@seonghobae

Copy link
Copy Markdown
Contributor Author

Direct failed-job rerun was attempted for run 32300267347 and returned HTTP 404 because central workflow 303795245 is unavailable through the repository API. No source change or gate weakening is justified.

@seonghobae

Copy link
Copy Markdown
Contributor Author

Current-head Checks review for 7a36b0d: Strix is the only terminal failure. The hosted failed run 32300267347 returns HTTP 404 because central workflow 303795245 is unavailable through the repository API; source-facing checks remain green. Retry requires the hosted workflow route to return, so no source change or gate weakening is justified. Please reacquire review evidence against this exact head. @opencode-agent review @cwl-noema-review review

@seonghobae

Copy link
Copy Markdown
Contributor Author

Strix RCA correction: exact current HEAD is 7a36b0d. The failed attempt stopped during trusted target visibility lookup because the GitHub App installation hit API rate limit HTTP 403. No source finding was emitted. This is GitHub provider infrastructure; failed jobs were rerun against this exact HEAD.

Copy link
Copy Markdown
Contributor Author

@opencode-agent @cwl-noema-review Re-review the exact current head 7a36b0d76dc84f825931c145ef56188d979006d2.

Current CI, Security Scan, SAST Semgrep, and CodeQL evidence for this SHA is terminal-success. Review only this SHA and publish fresh independent approval evidence; do not reuse prior-head reviews. Protected merge policy remains unchanged.

Copy link
Copy Markdown
Contributor Author

Please re-review this exact current head only: 7a36b0d76dc84f825931c145ef56188d979006d2, based on main@04d0bc21a2a20693bcf16108cd76d394fe844d23. Prior review records are predecessor-head evidence and must not transfer. Review-only dispatch requested; do not update the branch or merge via bypass.

Copy link
Copy Markdown
Contributor Author

@opencode-agent Please perform a review-only formal review of exact current PR head 7a36b0d76dc84f825931c145ef56188d979006d2. Re-check changed-file scope, current-head findings, unresolved threads, mergeability, and every required Check. Do not reuse a stale review, mutate the branch, self-approve, or merge.

@opencode-agent
opencode-agent Bot disabled auto-merge August 21, 2026 00:18
@seonghobae

Copy link
Copy Markdown
Contributor Author

Current-head review fix pushed at 91b9d3d. The shared S-X2 control admission now maps an overflowing exact built-in integer to the documented ValueError contract instead of leaking OverflowError. Focused validation passed: 7 S-X2 callback/control tests, interrogate 100 percent, compileall, and diff check. The only local Ruff findings are the existing dynamic-import E402 baseline in package init.py; the changed helper and regression test pass Ruff. Please rerun current-head OpenCode/Noema and protected checks. No further source change is actionable; normal merge remains subject to independent formal approval.

@seonghobae
seonghobae enabled auto-merge (squash) August 21, 2026 00:25

@devin-ai-integration devin-ai-integration 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.

✅ Devin Review: No Issues Found

Devin Review analyzed this PR and found no bugs or issues to report.

Open in Devin Review

@opencode-agent opencode-agent Bot added area: performance Performance, resource use, scalability, or benchmarking priority: medium Normal-priority or P2 work status: blocked Blocked by conflict, dependency, or required prerequisite type: bug Defect or incorrect behavior labels Aug 22, 2026
@coderabbitai

coderabbitai Bot commented Aug 22, 2026

Copy link
Copy Markdown
Contributor

Caution

Review failed

An error occurred during the review process. Please try again later.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

1 similar comment
@coderabbitai

coderabbitai Bot commented Aug 22, 2026

Copy link
Copy Markdown
Contributor

Caution

Review failed

An error occurred during the review process. Please try again later.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@seonghobae
seonghobae enabled auto-merge (squash) August 24, 2026 12:45
devin-ai-integration[bot]

This comment was marked as resolved.

@opencode-agent
opencode-agent Bot disabled auto-merge August 24, 2026 13:55
devin-ai-integration[bot]

This comment was marked as resolved.

@seonghobae seonghobae closed this Aug 25, 2026
@seonghobae seonghobae reopened this Aug 25, 2026
@seonghobae seonghobae closed this Aug 25, 2026
@seonghobae seonghobae reopened this Aug 25, 2026
@opencode-agent

Copy link
Copy Markdown
Contributor

Scheduled review-feedback autofix for this PR head.

  • Head SHA: 716ed36fb4c4a32c2a2e1c76f844a013053cf6d6

# Conflicts:
#	python/fast_mlsirm/__init__.py

@devin-ai-integration devin-ai-integration 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.

Devin Review found 2 new potential issues.

Open in Devin Review

Comment thread python/fast_mlsirm/_fitstats_control_safety.py
Comment thread docs/changelog.d/1028-fitstats-sx2-control-callback-safety.md
@seonghobae

Copy link
Copy Markdown
Contributor Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Aug 25, 2026 •

Copy link
Copy Markdown
Contributor
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@seonghobae
seonghobae merged commit 4d41dba into main Aug 25, 2026
39 of 41 checks passed
@seonghobae
seonghobae deleted the fix/fitstats-sx2-callback-boundary-1028 branch August 25, 2026 05:41
seonghobae added a commit that referenced this pull request Aug 25, 2026
Pin the baseline to protected main@9c12eab1 (v0.9.0). The open-PR queue
collapsed from 74 to 6 since the previous snapshot; replace the stale PR
table with the current six lanes (#1363, #1345, #1279, #1029, #1005, #998)
including their merge-forward state. Update gap-matrix evidence columns for
landed work: automatic Rust backend (#951/#1070), crossed multiple-membership
(#1014), the Figma design-boundary ADR now at docs/adr/0016 via merged
#1130, and cross-engine conformance (#1082) after #1077 closed as completed.
No production code, formula, estimator, or release surface changed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area: performance Performance, resource use, scalability, or benchmarking priority: medium Normal-priority or P2 work status: blocked Blocked by conflict, dependency, or required prerequisite type: bug Defect or incorrect behavior

Projects

None yet

2 participants