fix(config): harden integer callback boundaries - #873
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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)
📝 WalkthroughWalkthroughConfiguration validation now rejects caller-controlled integer subclasses and arbitrary ChangesConfiguration integer safety
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to The change hardens integer validation at public simulation and fit boundaries, and the current head is merge-ready after normal checks and review with no actionable merge-blocking risk remaining. Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 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 heade2375ce88ee0b456f483a0374db57aea894df4fa. -
Head SHA:
e2375ce88ee0b456f483a0374db57aea894df4fa -
Workflow run: 31939283649
-
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 (2 files)"]
S1 --> I1["operator or user guidance"]
I1 --> R1["Review risk: Docs (2 files)"]
R1 --> V1["docs review"]
Evidence --> S2["Changed file: config.py"]
S2 --> I2["repository behavior"]
I2 --> R2["Review risk: Changed file: config.py"]
R2 --> V2["required checks"]
Evidence --> S3["Test: test_config_integer_callback_safety.py"]
S3 --> I3["regression suite"]
I3 --> R3["Review risk: Test: test_config_integer_callback_safety.py"]
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 (2 files)"]
S1 --> I1["operator or user guidance"]
I1 --> R1["Review risk: Docs (2 files)"]
R1 --> V1["docs review"]
Evidence --> S2["Changed file: config.py"]
S2 --> I2["repository behavior"]
I2 --> R2["Review risk: Changed file: config.py"]
R2 --> V2["required checks"]
Evidence --> S3["Test: test_config_integer_callback_safety.py"]
S3 --> I3["regression suite"]
I3 --> R3["Review risk: Test: test_config_integer_callback_safety.py"]
R3 --> V3["targeted test run"]
|
|
@opencode-agent Please re-review exact unchanged head |
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
Strix VULN-0001 failed the current head because MLS2PLMConfig and FitConfig could be constructed with values that bypass memory-safety bounds until a later explicit validate() call. Run the existing validators from __post_init__ so invalid objects cannot exist, and keep validate() public and idempotent. Co-authored-by: Seongho Bae <seonghobae@users.noreply.github.com>
e2375ce to
805df46
Compare
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Head 805df46 is not merge-ready. Construction-time validate() is the right fail-closed hook, but two buyer-facing holes remain on this exact surface.
seedandverbosenever pass_trusted_integer.MLS2PLMConfig(seed=HostileIndex())andFitConfig(seed=HostileIndex())construct. Save-timeint(config.seed)inio.pydispatches__index__.config.seed + restartinfit.pyTypeErrors on a hostile object and wrapsnp.uint8(250) + 10to4.- Admitted narrow NumPy integers stay on the frozen fields.
MLS2PLMConfig(n_dims=np.uint8(16), items_per_dim=np.uint8(16))validates, thenn_itemsbecomes0andsimulate()cannot match the published size product.
Doctoring that says untrusted controls cannot exist as constructed objects is false on this head.
Repair is already on successor #939 (0814458f): seed/verbose go through the same allowlist, trusted ints are written back, hostile __index__ coverage includes seed/verbose/q_*, and a 2-by-256 simulation shape check fails if n_items wraps. Prefer #939. Close this PR as superseded after #939 is green with a non-author approval.
Do not merge this head. Do not self-approve. Exact-head CI on 805df46 does not transfer to #939. OpenCode CHANGES_REQUESTED on e2375ce was coverage-evidence tooling, not this defect.
Sent by Cursor Automation: Fix Issues
|
|
||
| def __post_init__(self) -> None: | ||
| """Reject invalid simulation controls at construction.""" | ||
| self.validate() |
There was a problem hiding this comment.
validate() here still skips seed and never writes trusted integers back. A hostile or np.uint8 seed survives construction; int(config.seed) and config.seed + restart remain live callback/wrap sites. Successor #939 stores built-in ints after the allowlist.
| if value_type is int: | ||
| return value | ||
| if any(value_type is trusted_type for trusted_type in _NUMPY_INTEGER_SCALAR_TYPES): | ||
| return int(value) |
There was a problem hiding this comment.
int(value) is only used for local bounds. n_items still multiplies the stored fields, so np.uint8(16) * np.uint8(16) wraps to 0 after a successful validate. Write the trusted built-in ints back before simulate() reads n_items.
2 participants
Scope
Advances #872 by making public simulation and fit integer validation establish a package-trusted scalar identity before any caller-dispatchable coercion.
Test-first evidence
fb67ced09d8ee00542c05d56374537a9a7239751.4c81e4dc465312d13f044b9b47e14d839af6cc1a.31859346010reproduced the trust-boundary defect: Python 3.12/3.14 failed with the new callback regressions while Rust, package, fuzz and GPU jobs were independently green. The Python 3.12 suite reported 14 failures, 3670 passes and 2 skips; the failures covered caller__index__execution, accepted caller-defined integer subclasses, and fit controls that did not reject the adversarial values.e2375ce88ee0b456f483a0374db57aea894df4fa.Contract
Validation now accepts exact built-in integers plus genuine supported NumPy integer scalar identities, normalizing admitted NumPy values to built-in integers only after trust is established. It rejects booleans, caller-defined
intsubclasses, NumPy integer subclasses, and arbitrary__index__providers before their coercion hooks can execute.The hardened simulation controls are
n_persons,n_dims,items_per_dim, andlatent_dim. The hardened fit controls arelatent_dim,lbfgs_history,max_iter,n_restarts,q_theta,q_xi,q_u,m_steps,xi_points, andxi_seed; existing bounds and aggregate optimizer-work checks remain intact.This is validation/marshalling only. No likelihood, estimator, quadrature, optimizer, recovery, uncertainty, or other psychometric/statistical arithmetic moves into Python.
Exact current evidence
e2375ce88ee0b456f483a0374db57aea894df4fa.Protected integration still requires the live independent non-author approval, last-push approval, conversation-resolution, and all exact-head required checks at decision time. Automated checks do not replace that approval, and predecessor-head evidence does not transfer if the source head moves.
Summary by CodeRabbit
Bug Fixes
Documentation