Skip to content

fix(scoring): seal assessment aggregate record admission - #1052

Merged
seonghobae merged 5 commits into
mainfrom
fix/assessment-aggregate-admission-1051
Aug 24, 2026
Merged

fix(scoring): seal assessment aggregate record admission#1052
seonghobae merged 5 commits into
mainfrom
fix/assessment-aggregate-admission-1051

Conversation

@seonghobae

@seonghobae seonghobae commented Aug 19, 2026

Copy link
Copy Markdown
Contributor

Closes #1051.

Defect

build_assessment_spec() admitted ConstructSpec, RubricSpecification, and scoring-policy subclasses with isinstance(...) and then read provenance-bearing identifiers, fingerprints, or construct scopes. A caller-defined subclass could therefore run arbitrary attribute/fingerprint callbacks while an assessment aggregate was establishing trusted package provenance.

Fix

  • require exact package-owned ConstructSpec entries before construct-field reads;
  • require exact package-owned RubricSpecification entries before fingerprint derivation;
  • require the exact six package-owned policy record types before construct-scope reads;
  • retain the existing bounded collection, cross-reference, error-code, and fingerprint behavior for valid package records;
  • keep this entirely in Python validation/marshalling/provenance: no likelihood, estimator, calibration, fit, uncertainty, or other psychometric arithmetic changes.

Test-first evidence

  • RED: f5fa97bf3e92ce3adc2068a0663fc223abe13580 adds public build_assessment_spec() regressions for hostile construct, rubric, and validation-policy subclasses. Against protected main@04d0bc21a2a20693bcf16108cd76d394fe844d23, all three fail because hostile callbacks are reached (construct/policy raise directly; rubric callback is caught only after dispatch).
  • GREEN: 306b5513834c66285ef88a6f3606381a8edaeb24 seals aggregate admission with exact-type checks.
  • Changelog/current head: e6119d3c5a22d0a0d85192eead4a2f9a2f462b54.

Focused local proof on the exact protected-main source blob plus the GREEN patch: 25 passed across the new regression, scoring contract core/validation/public-callback/UTF-8 suites.

Keep this PR Draft until exact-head repository CI, security, package/coverage, and independent review evidence are terminal-success.

Summary by CodeRabbit

  • Bug Fixes

    • Assessment assembly now rejects unsupported specification, rubric, and scoring-policy subclasses with a clear validation error.
    • Prevented invalid inputs from triggering unintended property access or callbacks.
    • Preserved existing package records and cross-reference behavior.
  • Tests

    • Added regression coverage for unsafe subclass handling during assessment construction.
  • Documentation

    • Added a changelog entry describing the validation improvements.

@coderabbitai

coderabbitai Bot commented Aug 19, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Important

Review skipped

Auto incremental reviews are disabled on this repository.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 6072b600-029b-47a7-bed1-836384f943e1

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

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: db60bc65-aed6-4b4f-a1a1-94c0612cf01b

📥 Commits

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

📒 Files selected for processing (3)
  • docs/changelog.d/1051-assessment-aggregate-record-admission.md
  • python/fast_mlsirm/scoring/assessment.py
  • tests/test_scoring_assessment_aggregate_callback_safety.py

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


📝 Walkthrough

Walkthrough

Assessment assembly now requires exact package-owned classes for constructs, rubrics, and validation policies. New regression tests verify that hostile subclasses are rejected before guarded properties are accessed. A changelog entry documents the admission behavior.

Changes

Assessment aggregate admission

Layer / File(s) Summary
Canonical aggregate record validation
python/fast_mlsirm/scoring/assessment.py
Construct, rubric, and validation-policy validation now rejects subclasses and preserves the existing AssessmentSpec factory return path.
Hostile subclass regression coverage
tests/test_scoring_assessment_aggregate_callback_safety.py, docs/changelog.d/1051-assessment-aggregate-record-admission.md
Tests verify the expected error codes and zero guarded-property callbacks. The changelog documents the admission validation.

Estimated code review effort: 2 (Simple) | ~10 minutes

Merge Risk: ⚪ Minimal · up to cddae

This localized validation change prevents caller-defined record subclasses from executing during assessment aggregate admission while preserving valid-record behavior; no actionable merge-blocking risk remains after normal checks and review.

Possibly related issues

Possibly related PRs

🚥 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 scoring admission fix, which is the primary change in the pull request.
Linked Issues check ✅ Passed The exact-type checks and hostile-subclass regressions satisfy issue #1051 while preserving validation, cross-reference, and scoring boundaries.
Out of Scope Changes check ✅ Passed The code, tests, and changelog entry support issue #1051; no unrelated scoring, psychometric, or other out-of-scope changes are shown.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/assessment-aggregate-admission-1051

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 00:06

Copy link
Copy Markdown
Contributor Author

@coderabbitai review
@opencode-agent review

Please review exact current head cddaee047089a4d431cb5de463758e109d1a98fd against protected main@04d0bc21a2a20693bcf16108cd76d394fe844d23. Repository CI, Security Scan, CodeQL, and Semgrep are terminal-success on this exact head. The only concurrent delta from the originally documented e6119d3... lineage is a governed changelog-only adjustment; do not transfer predecessor-head evidence.

@coderabbitai

coderabbitai Bot commented Aug 20, 2026

Copy link
Copy Markdown
Contributor

@seonghobae I will review #1052 at cddaee047089a4d431cb5de463758e109d1a98fd against main@04d0bc21a2a20693bcf16108cd76d394fe844d23. I will treat the changelog-only delta independently and will not transfer predecessor-head evidence.

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

Copy link
Copy Markdown
Contributor Author

Current-head review for cddaee0: required checks are terminal-success on this exact head. Please reacquire same-head approval and do not transfer stale predecessor or coverage-infrastructure review state. No source failure is shown. @opencode-agent review @cwl-noema-review review

@seonghobae

Copy link
Copy Markdown
Contributor Author

Current-head review request for cddaee0.\n\n@opencode-agent review\n@cwl-noema-review review\n\nEvery reported check is passing, including CI, Rust, Python, package, GPU-smoke, fuzz, security, Strix, coverage, OpenCode, and Noema.

@seonghobae

Copy link
Copy Markdown
Contributor Author

Current HEAD verification request: cddaee047089a4d431cb5de463758e109d1a98fd

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

Please review this exact head and publish an approval or actionable finding. All repository checks are terminal-success; normal merge remains gated on independent approval.

@seonghobae

Copy link
Copy Markdown
Contributor Author

Exact-head review request for cddaee0.\n\n@opencode-agent review\n@cwl-noema-review review\n\nAll checks are terminal-success on this exact head; publish approval or actionable findings without transferring predecessor evidence.

@seonghobae

Copy link
Copy Markdown
Contributor Author

Exact-head local review for cddaee047089a4d431cb5de463758e109d1a98fd:

  • Assessment aggregate callback-safety plus scoring contract suites: 17 passed.
  • Ruff on changed implementation/regression files: passed.
  • git diff --check: passed.

The record admission is validated before untrusted scalar/text callbacks; no scoring arithmetic or Rust ownership changed. Please request current-head independent review and terminal protected Checks before normal merge. @opencode-agent review @cwl-noema-review review

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

Pull request was converted to draft

@seonghobae
seonghobae marked this pull request as ready for review August 21, 2026 02:42
@seonghobae

Copy link
Copy Markdown
Contributor Author

Reviewed exact HEAD 2de03fb9dc79fe5b1c23d7787dd58adf148d42ca after the minimal end-of-file fix. CodeGraph source review confirms exact package-record admission for constructs, rubrics, and scoring policies prevents subclass attribute/fingerprint callbacks before aggregate validation; focused callback-safety tests passed 3/3, Ruff, compileall, interrogate (100%), and diff checks passed. Hosted checks must rerun for this exact HEAD. No bypass or self-approval used.

@seonghobae
seonghobae enabled auto-merge (squash) August 21, 2026 02:42

@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 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
@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 merged commit a0e788f into main Aug 24, 2026
38 checks passed
@seonghobae
seonghobae deleted the fix/assessment-aggregate-admission-1051 branch August 24, 2026 06:02
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.

Harden AssessmentSpec aggregate record admission before provenance reads

1 participant