Repository navigation
fix(registration): add terminal-state guard to decide_heartbeat (OMN-4822) - #797
Merged
jonahgabriel merged 4 commits intoMar 13, 2026
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdded an early-return guard in Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
Comment Tip CodeRabbit can use OpenGrep to find security vulnerabilities and bugs across 17+ programming languages.OpenGrep is compatible with Semgrep configurations. Add an |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (4)
tests/unit/nodes/node_registration_orchestrator/services/test_decide_heartbeat_terminal_state_guard.py (4)
97-102: Update stale class docstring.The docstring states "These tests are RED until OMN-4822" but OMN-4822 is now implemented. Consider updating to reflect current state.
📝 Suggested update
"""decide_heartbeat must return no_op immediately for terminal states. - These tests are RED until OMN-4822 adds the terminal-state guard. - After OMN-4822, all tests in this class should go GREEN. + Validates the terminal-state guard added in OMN-4822. """🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/nodes/node_registration_orchestrator/services/test_decide_heartbeat_terminal_state_guard.py` around lines 97 - 102, Update the stale class docstring on TestDecideHeartbeatTerminalStateGuard: replace the outdated note about "These tests are RED until OMN-4822" with a current statement reflecting that OMN-4822 has been implemented and these tests should now pass (or simply remove the temporal note). Locate the class TestDecideHeartbeatTerminalStateGuard in the test_decide_heartbeat_terminal_state_guard.py file and edit its triple-quoted docstring to remove or reword the reference to OMN-4822 so the comment reflects the present state of the codebase.
225-250: Consider using@pytest.mark.parametrizefor cleaner test output.The manual loop works but loses pytest's per-parameter test isolation and reporting. Using parametrize would show each terminal state as a separate test case in output.
♻️ Suggested refactor using parametrize
+ `@pytest.mark.parametrize`( + "state", + [s for s in EnumRegistrationState if s.is_terminal()], + ids=lambda s: s.name, + ) + def test_terminal_state_returns_no_op(self, state: EnumRegistrationState) -> None: + """Each terminal state must produce no_op from decide_heartbeat.""" + service = RegistrationReducerService() + node_id = uuid4() + projection = make_projection(state, entity_id=node_id) + ctx = make_ctx() + + decision = service.decide_heartbeat( + projection=projection, + node_id=node_id, + heartbeat_timestamp=TEST_NOW, + ctx=ctx, + ) + + assert decision.action == "no_op", ( + f"Expected no_op for terminal state {state!r}, " + f"got action={decision.action!r}. " + "All terminal states must be guarded in decide_heartbeat (OMN-4822)." + ) - def test_all_terminal_states_return_no_op(self) -> None: - """Parametric: all is_terminal() states must produce no_op from decide_heartbeat.""" - service = RegistrationReducerService() - terminal_states = [s for s in EnumRegistrationState if s.is_terminal()] - - assert len(terminal_states) >= 2, ( - "Expected at least LIVENESS_EXPIRED and REJECTED to be terminal states" - ) - - for state in terminal_states: - node_id = uuid4() - projection = make_projection(state, entity_id=node_id) - ctx = make_ctx() - - decision = service.decide_heartbeat( - projection=projection, - node_id=node_id, - heartbeat_timestamp=TEST_NOW, - ctx=ctx, - ) - - assert decision.action == "no_op", ( - f"Expected no_op for terminal state {state!r}, " - f"got action={decision.action!r}. " - "All terminal states must be guarded in decide_heartbeat (OMN-4822)." - )Note: You may want to keep the assertion
len(terminal_states) >= 2as a separate sanity-check test if desired.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/nodes/node_registration_orchestrator/services/test_decide_heartbeat_terminal_state_guard.py` around lines 225 - 250, Refactor the test_all_terminal_states_return_no_op to use pytest.mark.parametrize for clearer per-state test reporting: extract terminal_states = [s for s in EnumRegistrationState if s.is_terminal()] and parametrize the test over that list, keeping the existing assertions and setup (use make_projection, make_ctx, TEST_NOW, uuid4 and call RegistrationReducerService().decide_heartbeat) so each terminal state is its own test case; you can keep the len(terminal_states) >= 2 sanity check as a separate test if desired.
5-21: Update stale docstring — tests now pass.The module docstring describes these as "Failing tests" and states "Currently FAILING" (lines 10-13), but per the PR objectives, the fix is now implemented and all 6 tests pass. This TDD-style documentation is now misleading.
📝 Suggested docstring update
-"""Failing tests for decide_heartbeat terminal-state guard (OMN-4819). +"""Tests for decide_heartbeat terminal-state guard (OMN-4822). These tests assert that decide_heartbeat returns no_op when the node's current registration state is terminal (LIVENESS_EXPIRED, REJECTED). -Currently FAILING: decide_heartbeat does not guard against terminal states. -It proceeds to extend the liveness deadline even when the node is in a -terminal state, which triggers spurious re-registration side effects in -the handler layer. - -Fix target: OMN-4822 — add terminal-state guard to decide_heartbeat. +The terminal-state guard (OMN-4822) ensures that heartbeats for nodes in +LIVENESS_EXPIRED or REJECTED states are ignored, preventing spurious +re-registration side effects in the handler layer. Related Tickets: - OMN-4817: UUID Type Mismatch & Stale-Registration Race Fix (epic) - - OMN-4819: This test file (write failing test) - - OMN-4822: Fix decide_heartbeat to return no_op for terminal states + - OMN-4819: Test file for terminal-state guard + - OMN-4822: Terminal-state guard implementation """🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/nodes/node_registration_orchestrator/services/test_decide_heartbeat_terminal_state_guard.py` around lines 5 - 21, Update the module docstring to remove "Failing tests" and "Currently FAILING" language and instead state that the decide_heartbeat terminal-state guard (OMN-4822) has been implemented and all tests now pass; reference decide_heartbeat and the related tickets (OMN-4819, OMN-4822) as the context for the tests' intent, and make the docstring describe the current behavior (tests assert no_op when registration state is terminal) rather than describing a failing state.
104-132: Remove stale "Currently FAILS" comment.Lines 111-112 state "Currently FAILS" but the test now passes. Same applies to similar comments in
test_rejected_returns_no_op(lines 140-141),test_no_op_reason_mentions_terminal(line 168), andtest_no_intents_emitted_for_terminal_state(line 202).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/nodes/node_registration_orchestrator/services/test_decide_heartbeat_terminal_state_guard.py` around lines 104 - 132, Remove the stale "Currently FAILS" sentence from the docstring in test_liveness_expired_returns_no_op and the same stale phrasing in the docstrings of test_rejected_returns_no_op, test_no_op_reason_mentions_terminal, and test_no_intents_emitted_for_terminal_state; locate each test by its function name (test_liveness_expired_returns_no_op, test_rejected_returns_no_op, test_no_op_reason_mentions_terminal, test_no_intents_emitted_for_terminal_state) and edit the triple-quoted docstring to delete the sentence that begins with "Currently FAILS" (or similar), leaving the rest of the explanatory text intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@tests/unit/nodes/node_registration_orchestrator/services/test_decide_heartbeat_terminal_state_guard.py`:
- Around line 97-102: Update the stale class docstring on
TestDecideHeartbeatTerminalStateGuard: replace the outdated note about "These
tests are RED until OMN-4822" with a current statement reflecting that OMN-4822
has been implemented and these tests should now pass (or simply remove the
temporal note). Locate the class TestDecideHeartbeatTerminalStateGuard in the
test_decide_heartbeat_terminal_state_guard.py file and edit its triple-quoted
docstring to remove or reword the reference to OMN-4822 so the comment reflects
the present state of the codebase.
- Around line 225-250: Refactor the test_all_terminal_states_return_no_op to use
pytest.mark.parametrize for clearer per-state test reporting: extract
terminal_states = [s for s in EnumRegistrationState if s.is_terminal()] and
parametrize the test over that list, keeping the existing assertions and setup
(use make_projection, make_ctx, TEST_NOW, uuid4 and call
RegistrationReducerService().decide_heartbeat) so each terminal state is its own
test case; you can keep the len(terminal_states) >= 2 sanity check as a separate
test if desired.
- Around line 5-21: Update the module docstring to remove "Failing tests" and
"Currently FAILING" language and instead state that the decide_heartbeat
terminal-state guard (OMN-4822) has been implemented and all tests now pass;
reference decide_heartbeat and the related tickets (OMN-4819, OMN-4822) as the
context for the tests' intent, and make the docstring describe the current
behavior (tests assert no_op when registration state is terminal) rather than
describing a failing state.
- Around line 104-132: Remove the stale "Currently FAILS" sentence from the
docstring in test_liveness_expired_returns_no_op and the same stale phrasing in
the docstrings of test_rejected_returns_no_op,
test_no_op_reason_mentions_terminal, and
test_no_intents_emitted_for_terminal_state; locate each test by its function
name (test_liveness_expired_returns_no_op, test_rejected_returns_no_op,
test_no_op_reason_mentions_terminal, test_no_intents_emitted_for_terminal_state)
and edit the triple-quoted docstring to delete the sentence that begins with
"Currently FAILS" (or similar), leaving the rest of the explanatory text intact.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a5d59f40-3ba8-49b4-bddb-f1fe7b0edca3
📒 Files selected for processing (2)
src/omnibase_infra/nodes/node_registration_orchestrator/services/registration_reducer_service.pytests/unit/nodes/node_registration_orchestrator/services/test_decide_heartbeat_terminal_state_guard.py
jonahgabriel
enabled auto-merge
March 12, 2026 23:26
…4822) When a heartbeat arrives for a node in a terminal state (LIVENESS_EXPIRED or REJECTED), decide_heartbeat now returns no_op immediately instead of emitting a PostgreSQL UPDATE intent that would erroneously extend the liveness deadline and trigger spurious re-registration in the handler. The guard uses the existing EnumRegistrationState.is_terminal() method, sourced from a shared enum — no hardcoded state strings. Makes OMN-4819 failing tests GREEN (6/6 pass). No regression in existing 29 heartbeat/reducer tests. Co-authored-with: OMN-4819 (failing test)
jonahgabriel
force-pushed
the
jonahgabriel/omn-4822-fix-decide_heartbeat-to-return-no_op-for-terminal-states
branch
from
March 13, 2026 13:04
ede37c5 to
09c8800
Compare
…o-return-no_op-for-terminal-states
…d (OMN-4822) Split TestHandlerNodeHeartbeatNonActiveNode parametrization: remove REJECTED (terminal state) from the non-active test and add a new TestHandlerNodeHeartbeatTerminalState class that asserts 0 intents for REJECTED and LIVENESS_EXPIRED, matching the decide_heartbeat guard added in the previous commit.
…o-return-no_op-for-terminal-states
jonahgabriel
added this pull request to the merge queue
Mar 13, 2026
jonahgabriel
deleted the
jonahgabriel/omn-4822-fix-decide_heartbeat-to-return-no_op-for-terminal-states
branch
March 13, 2026 15:41
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
decide_heartbeatinRegistrationReducerServiceLIVENESS_EXPIREDorREJECTEDstate, the function now returnsno_opimmediately instead of emitting a PostgreSQL UPDATE intentEnumRegistrationState.is_terminal()— sourced from shared enum, no hardcoded stringsRoot cause
decide_heartbeatonly guarded againstprojection is None. For any non-None projection, it proceeded to extend the liveness deadline regardless of the node's state. When a node's liveness expired and the next heartbeat arrived, the handler would apply the UPDATE intent and then trigger spurious re-registration.Change
src/omnibase_infra/nodes/node_registration_orchestrator/services/registration_reducer_service.pyAdded after the
Noneguard:Test plan
Linear
Closes OMN-4822 | Part of epic OMN-4817
Depends on OMN-4819 (failing test)
Summary by CodeRabbit
Bug Fixes
Tests