fix(model-comparison): harden casewise numeric trust boundary - #990
seonghobae wants to merge 6 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
Included review availability: Your plan includes up to 1 review per rolling hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe change hardens casewise model-comparison input validation. Trusted built-in and NumPy real scalars remain supported. Arbitrary float-protocol objects and caller-defined numeric subclasses are rejected without invoking conversion callbacks. Regression tests cover rejection and compatibility behavior. ChangesCasewise numeric admission
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to The change is localized to numeric input validation and its associated tests and changelog; no actionable merge-blocking risk remains after normal checks and review. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
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 |
![opencode-agent[bot]](https://avatars.githubusercontent.com/in/1549082?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
OpenCode cannot approve yet because required coverage evidence did not pass.
Review outcome
1. HIGH .github/workflows/opencode-review.yml:1 - Coverage evidence did not prove required test/docstring evidence
-
Problem: The required coverage-evidence job result was
failure, so OpenCode cannot establish approval sufficiency for this head. -
Root cause: Automated approval is only valid when the same-head coverage-evidence job proves supported repository test suites passed and configured docstring gates passed or were advisory, or reports not applicable because no supported source files or package manifests exist. Missing, failed, skipped, unavailable, or unsupported-tooling test evidence is a blocker.
-
Fix: Install or configure the repository test/docstring evidence tooling when source files or package manifests exist, rerun the current-head coverage-evidence job, and approve only after it reports
successwith required evidence or explicit no-source not-applicable evidence. -
Regression test: Keep the approval branch checking
needs.coverage-evidence.result == successbefore posting APPROVE, and publish REQUEST_CHANGES when coverage-evidence blocker states such as cancelled, skipped, failed, unsupported-tooling, or below-100 evidence are present. -
Result: REQUEST_CHANGES
-
Reason: coverage-evidence result was
failure, so required test/docstring evidence was not proven for current headf59c74f79cd5a37e8c1bd74e1f83a1924a11b508. -
Head SHA:
f59c74f79cd5a37e8c1bd74e1f83a1924a11b508 -
Workflow run: 32167081903
-
Workflow attempt: 1
Coverage evidence
Coverage evidence job did not run or did not publish coverage evidence.
Changed-File Evidence Map
flowchart LR
PR["PR changed files"] --> Evidence["OpenCode bounded evidence"]
Evidence --> S1["Docs: 608-model-comparison-casewise-trust-boundary.md"]
S1 --> I1["operator or user guidance"]
I1 --> R1["Review risk: Docs: 608-model-comparison-casewise-trust-boundary.md"]
R1 --> V1["docs review"]
Evidence --> S2["Changed file: model_comparison.py"]
S2 --> I2["repository behavior"]
I2 --> R2["Review risk: Changed file: model_comparison.py"]
R2 --> V2["required checks"]
Evidence --> S3["Test (2 files)"]
S3 --> I3["regression suite"]
I3 --> R3["Review risk: Test (2 files)"]
R3 --> V3["targeted test run"]
OpenCode Review Overview
Pull request overviewOpenCode cannot approve yet because required coverage evidence did not pass. Review outcome1. HIGH .github/workflows/opencode-review.yml:1 - Coverage evidence did not prove required test/docstring evidence
Coverage evidenceCoverage evidence job did not run or did not publish coverage evidence. Changed-File Evidence Mapflowchart LR
PR["PR changed files"] --> Evidence["OpenCode bounded evidence"]
Evidence --> S1["Docs: 608-model-comparison-casewise-trust-boundary.md"]
S1 --> I1["operator or user guidance"]
I1 --> R1["Review risk: Docs: 608-model-comparison-casewise-trust-boundary.md"]
R1 --> V1["docs review"]
Evidence --> S2["Changed file: model_comparison.py"]
S2 --> I2["repository behavior"]
I2 --> R2["Review risk: Changed file: model_comparison.py"]
R2 --> V2["required checks"]
Evidence --> S3["Test (2 files)"]
S3 --> I3["regression suite"]
I3 --> R3["Review risk: Test (2 files)"]
R3 --> V3["targeted test run"]
|
|
@opencode-agent Please re-review the unchanged current exact head |
|
@opencode-agent Please re-review unchanged exact head |
|
@opencode-agent Please re-review unchanged exact head |
|
Superseded by #1009 at identical source SHA |
1 participant
Advances #608.
Defect
The public
compare_nonnested_models()boundary bounded the casewise log-likelihood vectors but still called arbitraryfloat(item)on each non-boolean element. A caller-controlled__float__provider or numeric subclass could therefore execute code during Python validation/marshalling before the Rust Vuong kernel.Test-first lineage
22130c2be0a2300d3fd31a588fcf67705596eb52— RED public regressions for hostile float providers/subclasses on bothloglik_aandloglik_b, plus accepted NumPy scalar compatibility.b0aaeb25a0b89a06aaf131984bc82bfe97f16663— GREEN: route casewise scalar admission through the existing exact trusted Python/NumPy real-scalar boundary before conversion.92f2fcacc212cb236b558e88c1c83b37eea20507— remove an unused test import before hosted validation.89d1bf01c3cfc7ecb2510a6439934482b9d763a2— initial changelog trace.89d1bf01…exposed two branch-owned integration defects: the new changelog fragment did not use the governed fragment schema, and an older resource-exhaustion test still expected an arbitrary__float__object to be an accepted conversion path.eab1f43312fec35f677e5cc9087ec0a22ee921b9— format the fragment with the required level-one title andFixedsection.f59c74f79cd5a37e8c1bd74e1f83a1924a11b508— preserve explicitMemoryErroronly for still-accepted iterator creation/iteration callbacks; arbitrary numeric protocols now remain fail-closed without dispatch.Preserved scientific ownership
Python still only validates, marshals, and orchestrates. The casewise log-likelihood-ratio mean,
omega, BIC correction, Vuong z statistic, and two-sided p value remain owned by the compiled Rust path. Formal distinguishability remains a separate fail-closed requirement and this change does not manufacture a model preference.Exact built-in
int/floatand the package-supported concrete NumPy integer/float scalar classes remain admitted. Booleans, caller subclasses, and arbitrary conversion providers fail closed without invoking conversion callbacks.Current integration state
Exact source head
f59c74f79cd5a37e8c1bd74e1f83a1924a11b508is Ready after terminal-success repository CI, Security Scan, CodeQL, and Semgrep on that same SHA, with no formal reviews or unresolved review threads at the Ready transition. Central protected-head review/coverage workflows must evaluate this unchanged head before merge; predecessor-head evidence does not transfer.Summary by CodeRabbit