Repository navigation
fix(OMN-15700): derive savings consumer groups canonically - #2681
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe savings estimator subscription now uses a typed ChangesSavings estimator subscription identity
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
tests/integration/runtime/test_savings_estimator_subscription_group.py (1)
45-70: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winStrengthen the AST guard for the complete subscription contract.
The test only checks that
serviceandversionkeywords exist. A wrong value still passes and changes the canonical group identity. The call search is module-wide and checks only calls that already contain_savings_node_identity; a second savings subscription could retaingroup_idwithout being detected. Scope the search to the_input_topicloop and assert the intended service and version values.🤖 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/runtime/test_savings_estimator_subscription_group.py` around lines 45 - 70, Strengthen the AST assertions around the _input_topic loop: scope savings_subscribe_calls to subscribe calls within that loop rather than searching module-wide by _savings_node_identity alone, assert exactly one matching subscription, and validate its service and version keyword values against the intended canonical values instead of only checking their presence. Preserve the existing node_identity and group_id assertions.src/omnibase_infra/runtime/service_kernel.py (1)
1710-1711: 🗄️ Data Integrity & Integration | 🔵 TrivialPlan the consumer-group migration before rollout.
This change removes the explicit
savings-estimator.<topic>groups. Unless the resolver aliases the old values, the canonical identity creates new Kafka group IDs, so existing offsets do not transfer automatically. Confirm offset reset, replay, and idempotency behavior for all three input topics.🤖 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 `@src/omnibase_infra/runtime/service_kernel.py` around lines 1710 - 1711, Before rolling out the consumer-group identity change around node_identity and on_message, verify how the resolver maps the former savings-estimator.<topic> groups to the canonical identities. Define and validate the migration plan for all three input topics, including offset reset, replay, and idempotent processing behavior; preserve existing group IDs or explicitly handle offset transfer where required.
🤖 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 `@src/omnibase_infra/runtime/service_kernel.py`:
- Around line 1710-1711: Before rolling out the consumer-group identity change
around node_identity and on_message, verify how the resolver maps the former
savings-estimator.<topic> groups to the canonical identities. Define and
validate the migration plan for all three input topics, including offset reset,
replay, and idempotent processing behavior; preserve existing group IDs or
explicitly handle offset transfer where required.
In `@tests/integration/runtime/test_savings_estimator_subscription_group.py`:
- Around line 45-70: Strengthen the AST assertions around the _input_topic loop:
scope savings_subscribe_calls to subscribe calls within that loop rather than
searching module-wide by _savings_node_identity alone, assert exactly one
matching subscription, and validate its service and version keyword values
against the intended canonical values instead of only checking their presence.
Preserve the existing node_identity and group_id assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 8c39dee0-667a-4129-bb99-9f961023b138
📒 Files selected for processing (3)
src/omnibase_infra/runtime/service_kernel.pytests/integration/runtime/test_savings_estimator_subscription_group.pytests/unit/services/observability/savings_estimation/test_consumer.py
|
Addressed both review notes in head bc14b30 and the PR handoff: the AST guard is now scoped to the actual _input_topic loop and pins the full env/service/node/version identity contract. The rollout section now records that the denied legacy groups have no transferable committed offsets, the new groups start at the runtime's latest policy, and acceptance uses fresh unique sessions with deterministic source IDs and finalized-session deduplication. Focused guard tests, Ruff, and mypy remain green. |
|
Temporarily closed to relieve CI runner-fleet congestion (merge-sweep 2026-08-08, operator-approved — see ROLLING_WORK_LEDGER [mergesweep-0808-close]). Nothing is lost: branch, commits, and review state remain; this PR will be reopened in batches once the queue drains (reopen order recorded in the ledger). |
Pull request was closed
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/integration/runtime/test_savings_estimator_subscription_group.py (1)
80-95: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winCheck every
subscribecall in the loop.Lines [80-89] exclude calls that do not already contain
node_identity. A legacy call with onlygroup_idis therefore invisible to Lines [92-95]. Collect allsubscribecalls first, then assert that every call uses_savings_node_identityand omitsgroup_id.Suggested guard change
savings_subscribe_calls = [ node for node in ast.walk(savings_input_loops[0]) if isinstance(node, ast.Call) and _call_name(node.func) == "subscribe" - if any( - keyword.arg == "node_identity" - and isinstance(keyword.value, ast.Name) - and keyword.value.id == "_savings_node_identity" - for keyword in node.keywords - ) ] assert len(savings_subscribe_calls) == 1 assert all( - keyword.arg != "group_id" for keyword in savings_subscribe_calls[0].keywords + any( + keyword.arg == "node_identity" + and isinstance(keyword.value, ast.Name) + and keyword.value.id == "_savings_node_identity" + for keyword in node.keywords + ) + and all(keyword.arg != "group_id" for keyword in node.keywords) + for node in savings_subscribe_calls )🤖 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/runtime/test_savings_estimator_subscription_group.py` around lines 80 - 95, Update the savings_subscribe_calls collection to include every subscribe ast.Call within savings_input_loops[0], without filtering on node_identity. Then assert all collected calls use the _savings_node_identity Name for node_identity and omit group_id, while preserving the existing single-call assertion if required by the test.
🧹 Nitpick comments (1)
tests/integration/runtime/test_savings_estimator_subscription_group.py (1)
21-21: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAnchor
SERVICE_KERNEL_PATHto__file__.Line [21] depends on the current working directory. The test can fail when pytest starts outside the repository root. Build the repository path from
__file__.Suggested path fix
-SERVICE_KERNEL_PATH = Path("src/omnibase_infra/runtime/service_kernel.py") +REPO_ROOT = Path(__file__).parent.parent.parent.parent +SERVICE_KERNEL_PATH = REPO_ROOT / "src/omnibase_infra/runtime/service_kernel.py"Based on learnings, integration tests in this repository use lexical
Path(__file__).parent...traversal without.resolve().🤖 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/runtime/test_savings_estimator_subscription_group.py` at line 21, Update the SERVICE_KERNEL_PATH definition to derive the repository-relative path from __file__ using the existing lexical Path(__file__).parent traversal style, rather than relying on the current working directory; preserve the target service_kernel.py location.Source: Learnings
🤖 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/integration/runtime/test_savings_estimator_subscription_group.py`:
- Around line 111-125: Update the test around effective_group_ids to exercise
EventBusKafka.subscribe rather than calling the private
_resolve_effective_group_id directly. Provide a ModelNodeIdentity and a spy or
test double for the subscription registration, invoke subscribe for each
savings_config.consumed_topics topic, and assert that each registration uses the
expected effective subscription group without requiring a real broker.
---
Outside diff comments:
In `@tests/integration/runtime/test_savings_estimator_subscription_group.py`:
- Around line 80-95: Update the savings_subscribe_calls collection to include
every subscribe ast.Call within savings_input_loops[0], without filtering on
node_identity. Then assert all collected calls use the _savings_node_identity
Name for node_identity and omit group_id, while preserving the existing
single-call assertion if required by the test.
---
Nitpick comments:
In `@tests/integration/runtime/test_savings_estimator_subscription_group.py`:
- Line 21: Update the SERVICE_KERNEL_PATH definition to derive the
repository-relative path from __file__ using the existing lexical
Path(__file__).parent traversal style, rather than relying on the current
working directory; preserve the target service_kernel.py location.
🪄 Autofix
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 Plus
Run ID: d53f584f-1203-4def-9c68-ccfa033659db
📒 Files selected for processing (1)
tests/integration/runtime/test_savings_estimator_subscription_group.py
|
Correction applied after independent review: Evidence-Source now points to OCC#6192, the deploy assessment is a falsifiable exact-head content probe, and both Receipt Gate and the rerun Deploy Gate pass. I also added a behavioral regression at head 13cce04 proving every configured savings topic receives a distinct broker-facing .__t. group. OMN-15700 remains In Progress pending trusted OCC review, merge/deploy, and the live MSK/projection proof. |
✅ Hostile Reviewer — PASSEDBlocking findings (critical): 0 Gate semantics (pilot phase)
Powered by omniintelligence.review_pairing.cli_review — node-based adversarial review via HandlerLlmCliSubprocess (OMN-8468/OMN-8524) |
|
Follow-up: addressed CodeRabbit's valid test-quality finding. Head 0e21e0c now exercises the public subscribe(node_identity=...) path for every configured savings topic and captures the effective group at the consumer-start boundary. Focused integration tests pass 3/3; Ruff and mypy pass. OCC#6192 is refreshed through fed650ea for this exact product head. |
|
Heads up: this branch is now behind |
Advances the merge base to origin/dev @ 303e008 to unblock PR #2681 (OMN-15700) on the required CI Summary external-context check. The PR previously pinned merge SHA a90460f replayed over the Aug-6 base 4bf964a, which contained neither: - PR #2700 (dev squash 6b6cdc1): monitor_logs test timeout bump 10s -> 30s - PR #2714 (303e008): measured-comment fix for the same test Merging origin/dev in (not rebasing) so 0e21e0c stays in ancestry, keeping the prior hostile-review approval at that exact head citable against an unrewritten commit. No conflicts: git merge --no-commit --no-ff reported "Automatic merge went well" with a single auto-merged file (src/omnibase_infra/runtime/service_kernel.py, the savings-estimator seam file) and zero UU entries / conflict markers. Refs OMN-15700
…set_override seam PR #2681 (OMN-15700) landed on top of dev's #2705 (OMN-15789), which made EventBusKafka.subscribe() unconditionally pass auto_offset_reset_override= into _start_consumer_for_topic_unlocked(). The contractor's fake capture_consumer_start(topic, group_id) in test_savings_estimator_topics_keep_distinct_effective_consumer_groups predates that seam and only accepted the original two positional params, so the keyword-only argument raised TypeError deterministically in CI (Tests Split 3/15) — a seam collision between two individually correct, individually green PRs (OMN-14208 class), not a defect in either. Widen the fake's signature to field-match the real _start_consumer_for_topic_unlocked seam exactly (name + keyword-only + default None) and record the captured value alongside topic/group_id so the double mirrors the seam instead of swallowing it. No production code touched; the test's consumer-group-distinctness assertions are unchanged.
3 participants
Evidence-Ticket: OMN-15700
Summary
Replace the savings estimator's explicit
savings-estimator.<topic>Kafka consumer-group override with the runtime's typedModelNodeIdentityderivation path. This keeps the group environment-qualified and inside the existingonex-dev.*MSK IAM boundary; no IAM widening is included.Changes
Test plan
.__t.<topic>broker-facing groupRuntime startup gate
This touches
service_kernel.pyonly at an optional Kafka subscription call site; it does not change contract discovery, handler construction,wire_from_manifest, or readiness enforcement.wire_from_manifestwith production arguments (not changed by this PR)omninode-runtimein its governed runtime checksType safety checklist
dict[str, Any]fieldsStrict-gate ordering
Scope and residual proof
This PR is code-only evidence. After Jonah reviews and merges it, the staging runtime must be rebuilt/deployed. OMN-15700 remains In Progress until the three live inputs are exercised and the downstream cost/savings projection advances. No AWS, IAM, MSK topic, or Kubernetes mutation is part of this PR.
Refs OMN-15700
Summary by CodeRabbit
Bug Fixes
Tests
Consumer-group migration behavior
The former
savings-estimator.<topic>groups never passed MSK authorization, so they have no usable committed offsets to transfer. The runtime bus defaults a newly derived group tolatest; the deployment proof will therefore use fresh, uniquely identified post-rollout events on each of the three inputs rather than replaying history or resetting offsets manually. The estimator's source event ID is deterministic from session ID and schema version, and its finalized-session cache suppresses duplicate finalization during the bounded proof.Evidence-Source: OCC#6261