Repository navigation
feat(OMN-10769): pure condition evaluator for interactive onboarding transitions - #1547
Conversation
📝 WalkthroughWalkthroughThis PR adds comprehensive unit and integration test coverage for the onboarding condition evaluator. Unit tests extend membership operator checks with literal list support and state-based compound conditions; integration tests verify routing through an interactive onboarding policy using condition evaluation against dynamic state. ChangesCondition Evaluator Test Coverage Expansion
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@tests/unit/onboarding/test_condition_evaluator.py`:
- Around line 98-106: The test named test_not_in_literal_list is using the wrong
operator string ("in") so it never exercises the `not in` code path; update the
test's expression passed to evaluate_condition to use "not in" (for example
"deployment_mode not in [local, hybrid]") so evaluate_condition is invoked with
a `not in` operator and the assertion remains that the result is False; locate
this change in the test function test_not_in_literal_list in
tests/unit/onboarding/test_condition_evaluator.py which calls
evaluate_condition.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6c6edacb-87e2-4ad5-b426-58d2946c2b17
📒 Files selected for processing (3)
src/omnibase_infra/onboarding/condition_evaluator.pysrc/omnibase_infra/validation/infra_validators.pytests/unit/onboarding/test_condition_evaluator.py
…ing transitions evaluate_condition() supports ==, in [literal], in state_key, not in, and compound 'and'. Uses object instead of Any throughout. Unknown state keys raise ConditionEvaluationError. No eval()/exec(). Bumps INFRA_MAX_UNIONS to 149.
The condition evaluator resolves the LHS of `in` / `not in` expressions as a state key (not a literal string). Tests from the PR branch assumed literal LHS values. Updated tests to provide the LHS as a state key, matching the correct behavior of the regex-based evaluator from main.
5af449d to
a5b8b05
Compare
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 (1)
tests/integration/onboarding/test_condition_evaluator_policy_integration.py (1)
46-68:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd required integration test marker(s).
These tests are missing pytest markers. In this repo, tests must be explicitly marked; otherwise marker-based runs can skip them unintentionally. Please add
@pytest.mark.integrationon each test or set module-levelpytestmark.Suggested patch
import pytest import yaml from omnibase_infra.onboarding.condition_evaluator import evaluate_condition +pytestmark = pytest.mark.integration + POLICY_PATH = (As per coding guidelines: "Test files must use pytest markers: mark test functions with
@pytest.mark.unit,@pytest.mark.integration,@pytest.mark.slow,@pytest.mark.chaos, or@pytest.mark.performanceas appropriate".🤖 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/integration/onboarding/test_condition_evaluator_policy_integration.py` around lines 46 - 68, The two integration tests (test_policy_not_in_response_routes_local_without_llm_to_terminal and test_policy_not_in_selected_services_routes_hybrid_without_llm_to_terminal) are missing pytest markers; add `@pytest.mark.integration` above each test function or define a module-level pytestmark = [pytest.mark.integration] to ensure pytest recognizes them as integration tests, and import pytest at top if not already present; keep the existing test names and assertions unchanged.
🤖 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 `@tests/integration/onboarding/test_condition_evaluator_policy_integration.py`:
- Around line 46-68: The two integration tests
(test_policy_not_in_response_routes_local_without_llm_to_terminal and
test_policy_not_in_selected_services_routes_hybrid_without_llm_to_terminal) are
missing pytest markers; add `@pytest.mark.integration` above each test function or
define a module-level pytestmark = [pytest.mark.integration] to ensure pytest
recognizes them as integration tests, and import pytest at top if not already
present; keep the existing test names and assertions unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 99d4c604-bcb3-444a-90a5-c30e550d6a02
📒 Files selected for processing (3)
tests/integration/onboarding/__init__.pytests/integration/onboarding/test_condition_evaluator_policy_integration.pytests/unit/onboarding/test_condition_evaluator.py
✅ Files skipped from review due to trivial changes (2)
- tests/integration/onboarding/init.py
- tests/unit/onboarding/test_condition_evaluator.py
omnibase_core#1547 (round-#3 remediation of the msk-direct-broker-endpoint url-authority rule) merged to dev at 478e205d6f415adb2b5edd06b61f185279bba12e. Re-pins both url-authority-gate.yml git-SHA pins and the .pre-commit-config.yaml rev from the provisional branch-head SHA (75c851266b) to this real merge commit, per the plan disclosed in the prior commit on this branch. This PR is no longer blocked on #1547 landing (it has landed) but will still not go green on its own: the full-repo scan will find 3 NEW, non-baselined, non-suppressible violations in docker/docker-compose.gateway.yml:51-52 and docker/gateway/beta-gateway-canary.yaml:35 — the sanctioned gateway forwarder's own bastion-IP route, tracked on OMN-15694/OMN-15534. Cites OMN-15692.
Summary
evaluate_condition(expr, state)supporting==,in [literal],in state_key,not in, and compoundandNonecondition returnsTrue; unknown state keys raiseConditionEvaluationError(strict, no silent fallback)objectthroughout — noAnyin function signatures or generic containersinteractive_onboarding.yamlINFRA_MAX_UNIONSfrom 148→149 for thelist[object] | strcollection return typeTicket
OMN-10769 (child of OMN-10767 Interactive Onboarding Executor epic)
Test plan
uv run pytest tests/unit/onboarding/test_condition_evaluator.py -v— 10/10 passuv run mypy src/omnibase_infra/onboarding/condition_evaluator.py --strict— cleanSummary by CodeRabbit
Tests
Chores
Evidence-Source: 498cf95027d9d5661ee119de10575bd9fc55e392
Evidence-Ticket: OMN-10769