Skip to content

fix(models): seal model-spec record admission - #1064

Merged
seonghobae merged 6 commits into
mainfrom
fix/model-spec-record-admission-1063
Aug 24, 2026
Merged

fix(models): seal model-spec record admission#1064
seonghobae merged 6 commits into
mainfrom
fix/model-spec-record-admission-1063

Conversation

@seonghobae

@seonghobae seonghobae commented Aug 20, 2026

Copy link
Copy Markdown
Contributor

Closes #1063.

Defect

_resolve_model() already rejected hostile integer factor-count subclasses, but protected main@04d0bc21a2a20693bcf16108cd76d394fe844d23 still admitted caller-defined ExploratoryModel / ConfirmatoryModel subclasses through isinstance(...) before reading dimensions or loading_pattern. A subclass could therefore execute caller-controlled attribute callbacks while the package was still deciding whether the model specification was trusted.

RED → GREEN

  • RED c23be656e7af67b6e0aaeee7023ee2135ddf396d: focused resolution regressions construct hostile exploratory and confirmatory subclasses without invoking their constructors, arm only the relevant package field access, and require the existing package-owned TypeError with zero callbacks. The protected-main isinstance(...) implementation reaches those callbacks.
  • GREEN 9b897f247c9b4920dfe2a7e914c8280d135b6eae: replace subclass-permissive record admission with exact type(model) is ExploratoryModel / type(model) is ConfirmatoryModel checks before any record field read.
  • Trace/current head 96e114f92d570b2fda4f408e1f8d37b27e4a7b64: add the governed changelog fragment.

An isolated boundary proof confirms the predecessor path dispatches the hostile dimensions / loading_pattern callbacks, while the exact-type path returns the existing TypeError with zero watched callbacks. Hosted exact-head repository CI/security remains authoritative for integration.

Preserved contract

  • exact package-owned ExploratoryModel and ConfirmatoryModel instances retain their existing behavior;
  • exact built-in and supported concrete NumPy integer factor counts retain their existing behavior;
  • one-factor exploratory resolution and confirmatory row-count validation are unchanged;
  • multidimensional exploratory loading estimation remains explicitly unsupported and separately governed by feat(mirt): implement identified multidimensional exploratory loading estimation #633; this PR does not implement, emulate, or silently fall back for that estimator.

Ownership boundary

Python validation/marshalling only. No likelihood, loading estimation, gradients, optimizer, identification, rotation, factor retention, uncertainty, or structural-model-selection arithmetic changes. Production numerical ownership remains Rust-first.

Verification boundary

Current exact head is 96e114f92d570b2fda4f408e1f8d37b27e4a7b64 against protected main@04d0bc21a2a20693bcf16108cd76d394fe844d23, with exactly three changed files: python/fast_mlsirm/models.py, tests/test_cov_a_models.py, and docs/changelog.d/1063-model-spec-record-admission.md. Repository CI, CodeQL, Semgrep, and Security Scan are regenerating on this exact head. Keep this PR Draft until exact-current-head required CI/security/package/coverage and independent review evidence is terminal and clean; predecessor-head evidence does not transfer.

@coderabbitai

coderabbitai Bot commented Aug 20, 2026

Copy link
Copy Markdown
Contributor

Warning

Review limit reached

@seonghobae, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 56 minutes

Limit 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.

How can I continue?

Wait for the limit to reset, then comment @coderabbitai review or push new commits to the PR.

An organization admin can change what happens after included review limits in Billing.

How do review limits work?

CodeRabbit enforces per-developer PR review limits within each organization.

For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day window.

Please refer docs for additional details.

Review details
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 5d2dc57d-ab01-4497-9c59-04b137c0e1c9

📥 Commits

Reviewing files that changed from the base of the PR and between 04d0bc2 and dc65d76.

📒 Files selected for processing (3)
  • docs/changelog.d/1063-model-spec-record-admission.md
  • python/fast_mlsirm/models.py
  • tests/test_cov_a_models.py

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 20, 2026 06:23
@seonghobae
seonghobae enabled auto-merge (squash) August 20, 2026 06:23
@seonghobae

Copy link
Copy Markdown
Contributor Author

Current-head review request for 96e114f. Exact-type model-spec admission is scoped to validation and preserves the existing unsupported multidimensional estimator contract. Please review this head and regenerate all required checks. @opencode-agent review @cwl-noema-review review

@seonghobae

Copy link
Copy Markdown
Contributor Author

Current-head local review evidence for 96e114f92d570b2fda4f408e1f8d37b27e4a7b64: python -m pytest -q tests/test_cov_a_models.py passed (25 passed). The exact-type admission boundary was exercised before hostile model-spec attribute callbacks; no actionable issue found.

@seonghobae

Copy link
Copy Markdown
Contributor Author

Current-head review request for b6fa02a1a2320a38cdc096762224557e9d36ad56: added docstrings to both newly introduced hostile model-spec admission regressions, and the focused model suite remains 25 passed with ruff/format checks clean. @opencode-agent review @cwl-noema-review review

@seonghobae

Copy link
Copy Markdown
Contributor Author

Current-head review request for b6fa02a.\n\n@opencode-agent review\n@cwl-noema-review review\n\nLocal focused model-spec test evidence is already posted; all completed repository/security checks are passing and remaining CI/Strix jobs are pending.

@seonghobae

Copy link
Copy Markdown
Contributor Author

Root-cause fix pushed at 0b0ef30f1d37d1e12d030c6d37b6814ac46b4bd1. The changelog fragment now has the required level-one title; focused tests/test_changelog_fragment_contract.py passes 8/8. Please review and validate this exact post-fix head; prior b6fa02a evidence is superseded. @opencode-agent review @cwl-noema-review review

@seonghobae

Copy link
Copy Markdown
Contributor Author

Correction: the exact current post-fix head is 0b0ef30. The changelog title fix and focused 8/8 contract proof are bound to this SHA; please disregard the previously mistyped long SHA in my comment.

@seonghobae

Copy link
Copy Markdown
Contributor Author

Correction: exact-head local validation for SHA 0b0ef30.

  • Model-spec record-admission regression: 25 passed locally. No source correction was found; regenerate exact-head protected review evidence.

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

@seonghobae

Copy link
Copy Markdown
Contributor Author

Reviewed exact current head 0b0ef30.

  • pytest -q tests/test_cov_a_models.py: 25 passed
  • ruff check python/fast_mlsirm/models.py tests/test_cov_a_models.py: passed
  • git diff --check: passed
  • No local defect found in the exact-type admission boundary; caller-defined model-spec subclasses are rejected before package-field access.

Please regenerate the protected OpenCode/Noema review evidence for this exact head. @opencode-agent review @cwl-noema-review review

@seonghobae
seonghobae marked this pull request as draft August 20, 2026 16:11
auto-merge was automatically disabled August 20, 2026 16:11

Pull request was converted to draft

@seonghobae

Copy link
Copy Markdown
Contributor Author

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

Please review exact current HEAD dc65d76.

@seonghobae seonghobae left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Current-head review update for dc65d76.

Found and fixed a real documentation gap in the newly added hostile model-record tests: both nested probe classes and their attribute hooks lacked docstrings. Added only those six lines; the model admission behavior remains unchanged. Focused model tests: 25 passed; Ruff and diff checks pass. The PR remains Draft, so no merge action was attempted.

@seonghobae

Copy link
Copy Markdown
Contributor Author

Reviewed exact HEAD dc65d76 with CodeGraph. The trust-boundary fix admits only exact package-owned exploratory/confirmatory records before reading fields, while preserving numeric factor counts and exact model behavior. The 25-model contract tests, Ruff, compilation, diff checks, and 100% model-module docstring coverage pass; hosted Python, Rust, GPU, fuzz, security, CodeQL, Noema, Strix, OpenCode, and coverage checks are green. Ready for normal protected auto-merge; no bypass used.

@seonghobae
seonghobae marked this pull request as ready for review August 21, 2026 02:32
@seonghobae
seonghobae enabled auto-merge (squash) August 21, 2026 02:32

@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

@seonghobae

Copy link
Copy Markdown
Contributor Author

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

Please issue independent formal reviews for exact current head dc65d76c2d4641bf95c98cd581316224ee32e8d against protected main@04d0bc21a2a20693bcf16108cd76d394fe844d23. All completed repository, Rust, GPU, fuzz, package, security, CodeQL, Semgrep, Strix, OpenCode, Noema, and coverage checks for this head are successful; Devin reports no issues. Bind any approval or finding to this SHA only. Do not transfer predecessor evidence, self-approve, or merge.

@opencode-agent opencode-agent Bot added priority: medium Normal-priority or P2 work status: needs-review Open pull request requiring current-head review or checks type: bug Defect or incorrect behavior labels Aug 22, 2026
@seonghobae
seonghobae merged commit 5f3ede9 into main Aug 24, 2026
38 checks passed
@seonghobae
seonghobae deleted the fix/model-spec-record-admission-1063 branch August 24, 2026 06:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

priority: medium Normal-priority or P2 work status: needs-review Open pull request requiring current-head review or checks type: bug Defect or incorrect behavior

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix(models): reject model-spec subclasses before provenance field access

1 participant