OLS-3813 Remove confidence/risk from eval schemas - #152
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (7)
🔗 Linked repositories identifiedCodeRabbit considers these linked repositories for cross-repo context during reviews:
💤 Files with no reviewable changes (3)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe evaluation schema removes ChangesEvaluation and remediation contract updates
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Comment |
![coderabbitai[bot]](https://avatars.github.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
evals/test_schemas.py (1)
14-19: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftRun this check through the live eval harness.
The changed fixtures feed a local schema test.
test_action_required_false_with_diagnosisdirectly validates an in-memory document at Lines 49-51. Move this test out ofevals/if it remains a unit test, or invoke the containerized live runner as shown inevals/test_skill_invocation.pyLines 21-50.As per path instructions, “Evals are integration-only checks and must run against live endpoints in the containerized harness rather than as ordinary unit tests.”
Also applies to: 41-41
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@evals/test_schemas.py` around lines 14 - 19, Update test_action_required_false_with_diagnosis so it runs through the containerized live eval harness, following the invocation pattern in test_skill_invocation.py, rather than validating an in-memory document as a unit test. If it must remain a unit test, move it out of evals/ and keep the eval fixture changes limited to integration coverage.Source: Path instructions
evals/schemas.py (1)
31-38: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winMake the schema removal enforceable.
The three objects omit
"additionalProperties": False, sojsonschema.validatestill acceptsconfidenceandrisk. If OLS-3813 requires rejection, add the guard to bothdiagnosisobjects andremediationPlan, add rejection tests, and update bothfind-tokenfixtures. The operator CRD still declares these fields, so do not describe them as removed from the overall contract until that schema is updated.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@evals/schemas.py` around lines 31 - 38, Add "additionalProperties": False to both diagnosis object definitions and the remediationPlan schema so undeclared fields such as confidence and risk are rejected by validation. Add tests covering rejection of those fields and update both find-token fixtures accordingly; do not claim the fields are removed from the overall contract while the operator CRD still declares them.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@evals/schemas.py`:
- Around line 31-38: Add "additionalProperties": False to both diagnosis object
definitions and the remediationPlan schema so undeclared fields such as
confidence and risk are rejected by validation. Add tests covering rejection of
those fields and update both find-token fixtures accordingly; do not claim the
fields are removed from the overall contract while the operator CRD still
declares them.
In `@evals/test_schemas.py`:
- Around line 14-19: Update test_action_required_false_with_diagnosis so it runs
through the containerized live eval harness, following the invocation pattern in
test_skill_invocation.py, rather than validating an in-memory document as a unit
test. If it must remain a unit test, move it out of evals/ and keep the eval
fixture changes limited to integration coverage.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 292ac77f-c003-4f11-a7b7-f7a1b047df94
📒 Files selected for processing (3)
evals/schemas.pyevals/test_schemas.pyevals/test_skill_invocation.py
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
openshift/lightspeed-agentic-operator(manual)
💤 Files with no reviewable changes (1)
- evals/test_skill_invocation.py
1fd54c0 to
bcf2bbc
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/test_routes.py (1)
263-263: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert that the removed field stays absent.
assert "Reversible: True" in textcan pass even if the formatter also emitsRisk:. Addassert "Risk:" not in text. Add a case withoutreversibleto verify the"unknown"fallback.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_routes.py` at line 263, Strengthen the assertions around the formatter test by verifying that "Risk:" is absent from the output alongside the existing "Reversible: True" check. Add a test case with no reversible value and assert the formatter uses the "unknown" fallback.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@tests/test_routes.py`:
- Line 263: Strengthen the assertions around the formatter test by verifying
that "Risk:" is absent from the output alongside the existing "Reversible: True"
check. Add a test case with no reversible value and assert the formatter uses
the "unknown" fallback.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: ac170918-ecaf-4975-842b-1378bbe988a9
📒 Files selected for processing (7)
evals/schemas.pyevals/test_schemas.pyevals/test_skill_invocation.pyevals/workspace/skills/find-token/tools/find-token.shevals/workspace/tools/find-token.shsrc/lightspeed_agentic/routes/query.pytests/test_routes.py
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
openshift/lightspeed-agentic-operator(manual)
💤 Files with no reviewable changes (3)
- evals/workspace/skills/find-token/tools/find-token.sh
- evals/workspace/tools/find-token.sh
- evals/test_skill_invocation.py
🚧 Files skipped from review as they are similar to previous changes (2)
- evals/test_schemas.py
- evals/schemas.py
vimalk78
left a comment
vimalk78
left a comment
There was a problem hiding this comment.
Adversarial Review: OLS-3813 — PASS (100/100)
Mode: jira | Round: 1
Adherence
5/5 acceptance criteria met.
Code Quality
Original review found 1 should-fix issue:
evals/workspace/tools/find-token.shandevals/workspace/skills/find-token/tools/find-token.shstill emitted"confidence"and"risk"in canned JSON output
Note (out of scope): src/lightspeed_agentic/routes/query.py:51 does plan['risk'] with direct dict access. When OLS-3811 makes risk optional in the operator CRD, this will KeyError if the operator sends an approved option without risk. Needs a separate fix.
All findings resolved. PR is ready for human review.
🤖 Reviewed with Claude Code
blublinsky
left a comment
blublinsky
left a comment
There was a problem hiding this comment.
Nit: tests/test_routes.py still passes "risk": "low" in several approvedOption.remediationPlan fixtures (lines 250, 279, 304, 326). The production code no longer reads it so tests pass, but the leftover keys are misleading — a future reader might assume risk is still part of the contract.
Suggest removing the "risk" entries from those fixture dicts for consistency with the schema change.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Vimal Kumar <vimal78@gmail.com>
bcf2bbc to
37d2ab6
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: blublinsky The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
approved
|
@vimalk78: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
2 participants
Summary
confidencefrom diagnosis schemas (top-level and per-option) inevals/schemas.pyriskfrom remediation plan schemarequiredarrays to exclude these fieldstest_schemas.pyandtest_skill_invocation.pyTest plan
make test— 212 tests passJira
https://redhat.atlassian.net/browse/OLS-3813
🤖 Generated with Claude Code