Repository navigation
docs(arch): formalize registration FSM contract [OMN-938] - #221
Conversation
Define formal FSM contract for Dual Registration Reducer (OMN-889): - 10 states: unregistered, validating, registering_postgres, postgres_registered, registering_consul, registered, partial_registered, deregistering, deregistered, failed - 16 state transitions with triggers and priorities - 12 guard conditions with expressions - Complete Mermaid state diagram - Intent emission mapping for Consul and PostgreSQL - Implementation notes and error handling strategies Dependencies: OMN-912 (Core Intents), OMN-913 (Payload Models) Blocks: OMN-889 (Reducer Implementation), OMN-942 (Test Suite)
|
Important Review skippedReview was skipped as selected files did not have any reviewable changes. 💤 Files selected but had no reviewable changes (1)
You can disable this status message by setting the WalkthroughAdds a formal Registration FSM Contract (v1.0.0) specifying a deterministic Dual Registration reducer for node registration to PostgreSQL (source of truth) and Consul (service discovery): defines 10 states, 17 transitions, triggers, guards, intents, timeouts, checkpointing, error/recovery flows, and implementation guidance for OMN-889. Also adds an Agent Instructions guideline discouraging running agents in background mode (CLAUDE.md). Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Operator
participant FSM as Registration FSM
participant Executor as Intent Executor
participant PG as PostgreSQL
participant CONS as Consul
Operator->>FSM: startRegistration()
FSM->>Executor: emit ModelPostgresUpsertRegistrationIntent
Executor->>PG: upsert registration
alt Postgres success
PG-->>Executor: success
Executor-->>FSM: intentOutcome(success)
Note right of FSM: synthetic CONTINUE triggers Consul step
FSM->>Executor: emit ModelConsulRegisterIntent (CONTINUE)
Executor->>CONS: register service
alt Consul success
CONS-->>Executor: success
Executor-->>FSM: intentOutcome(success)
FSM->>FSM: transition -> RegisteredOperational
else Consul failure
CONS-->>Executor: failure
Executor-->>FSM: intentOutcome(failure)
FSM->>FSM: transition -> PartialRegistration
FSM->>Executor: schedule retry/compensate intents (backoff)
end
else Postgres failure
PG-->>Executor: failure
Executor-->>FSM: intentOutcome(failure)
FSM->>FSM: transition -> Error / Retry (with backoff)
end
Note over FSM,Executor: Timeouts and orchestrator-managed recovery can trigger Deregister/rollback paths
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
Comment |
Pull Request Review: Registration FSM Contract [OMN-938]Overall AssessmentStatus: ✅ APPROVED with minor suggestions This is an excellent, comprehensive FSM contract specification that follows ONEX architectural patterns. The document is well-structured, detailed, and provides clear implementation guidance for OMN-889. The Mermaid diagrams and extensive documentation make this highly maintainable. Strengths1. Architectural Alignment ✅
2. Comprehensive State Coverage ✅
3. Dual Registration Logic ✅
4. Documentation Quality ✅
Issues & Concerns1. Terminal State Inconsistency
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
docs/architecture/REGISTRATION_FSM_CONTRACT.md (1)
740-779: Clarify guard expression syntax and validation rules.The guard definitions use a simple 3-token syntax (
field operator value), but the implementation details are sparse. Consider adding:
- Type system: How are field types (boolean, string, number) determined at runtime?
- Field resolution: How are guard field names (e.g.,
postgres_applied,retry_count) resolved from FSM context?- Operator semantics: Document whether operators like
existscheck for null/undefined, key presence in objects, or array membership.- Error cases: What happens if a guard field is undefined or an operator is unsupported?
This will help implementers of OMN-889 validate guards correctly.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
docs/architecture/REGISTRATION_FSM_CONTRACT.md(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-19T18:46:12.150Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T18:46:12.150Z
Learning: Applies to **/*reducer*.py : Use ModelIntent pattern for REDUCER nodes with pure FSM transitions
Applied to files:
docs/architecture/REGISTRATION_FSM_CONTRACT.md
🪛 LanguageTool
docs/architecture/REGISTRATION_FSM_CONTRACT.md
[style] ~311-~311: To form a complete sentence, be sure to include a subject.
Context: ... **Purpose**: Complete failure state. Can be retried viaRETRY` trigger. --- #...
(MISSING_IT_THERE)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
- GitHub Check: Tests (Split 20/20)
- GitHub Check: Tests (Split 16/20)
- GitHub Check: Tests (Split 18/20)
- GitHub Check: Tests (Split 15/20)
- GitHub Check: Tests (Split 13/20)
- GitHub Check: Tests (Split 10/20)
- GitHub Check: Tests (Split 17/20)
- GitHub Check: Tests (Split 19/20)
- GitHub Check: Tests (Split 9/20)
- GitHub Check: Tests (Split 8/20)
- GitHub Check: Tests (Split 11/20)
- GitHub Check: Tests (Split 2/20)
- GitHub Check: Tests (Split 14/20)
- GitHub Check: Tests (Split 5/20)
- GitHub Check: Tests (Split 12/20)
- GitHub Check: Tests (Split 3/20)
- GitHub Check: Tests (Split 4/20)
- GitHub Check: Tests (Split 1/20)
- GitHub Check: Tests (Split 6/20)
- GitHub Check: claude-review
🔇 Additional comments (4)
docs/architecture/REGISTRATION_FSM_CONTRACT.md (4)
344-363: Clarify intent emission semantics in transition actions.Transition details include
action_type: emit_intentwith nestedaction_config(e.g., lines 359–361), but the relationship to the explicit Intent Emission section (lines 883–950) is unclear.Ambiguity: Are the log_event/log_metric intents in transition actions actually emitted by the Reducer, or are they examples of what could be emitted? The Intent Emission Matrix seems to define the canonical intents, yet transition details also specify intent emissions.
Recommendation:
- Clarify whether
action_configdescribes emitted intents or is pseudo-code for documentation.- If they are emitted, ensure they don't conflict with the Intent Emission Matrix.
- If they are documentation only, move them to an "Associated Actions" section to avoid confusion.
Also applies to: 419-440
721-721: Validate wildcard transition scope and priority.Transition 16 (
global_error_handler) usesfrom_state: "*"withpriority: 0(lowest). This enables transitions from any non-terminal state, but the contract doesn't explicitly:
- Exclude terminal states: Do
registeredandderegisteredallowFATAL_ERRORtransitions? The note says "from any non-terminal state," but this should be explicit in the transition definition.- Priority ordering: Clarify how priority interacts with wildcards. If multiple transitions are valid, is priority the tiebreaker?
- Atomicity: Are wildcard transitions atomic, or can other transitions fire concurrently?
🔎 Suggested clarification
Update the transition definition to explicitly exclude terminal states:
transition_name: global_error_handler from_state: "*" # Any state EXCEPT terminal states (registered, deregistered) to_state: failed trigger: FATAL_ERROR priority: 0 note: "Wildcard wildcard does not apply to terminal states" is_atomic: trueAnd add a subsection to the Transitions section:
**Wildcard Transition Semantics**: - Wildcard `from_state: "*"` matches all states EXCEPT terminal states (`registered`, `deregistered`). - When multiple transitions are valid, priority determines precedence (highest first). - Atomic transitions cannot be preempted.Also applies to: 736-736
784-830: Verify Mermaid diagram renders correctly in GitHub.The Mermaid
stateDiagram-v2diagram is well-formed, but GitHub's Mermaid rendering has evolving support and occasional edge-case rendering issues (e.g., with note blocks, special characters, or very large diagrams).Action: After merging, verify in the GitHub PR preview that:
- The diagram renders without errors or warnings.
- All states and transitions are visible.
- Note annotations render correctly.
If rendering issues occur, consider providing an alternative ASCII flow diagram (which you've already included at lines 832–879, which is excellent).
1-1107: Excellent comprehensive FSM specification document.This document meets all acceptance criteria and provides clear, detailed guidance for OMN-889 implementation:
✓ All 10 states documented across 4 categories with YAML metadata
✓ All 16 transitions with triggers, guards, and actions
✓ 12 guard definitions with expression syntax
✓ Mermaid diagram + simplified ASCII flow for clarity
✓ Intent emission semantics and correlation tracking
✓ Validation rules and error handling scenarios
✓ Implementation guidance including purity constraints and FSM configuration
✓ Cross-references to dependencies (OMN-912, OMN-913) and related architecture docsAlignment with ONEX patterns: The document correctly emphasizes Reducer purity (line 991) and Intent emission for side effects, aligning with the ModelIntent pattern. The separation of retry logic to Effect nodes (line 1008) is architecturally sound.
Structure quality: The YAML metadata for each state and transition makes the contract machine-readable and implementable. The recovery scenarios (lines 1053–1080) are instructive.
Minor refinements needed: Guard expression syntax, retry limit standardization, intent emission semantics, and wildcard transition scope (flagged in separate comments above).
| required: true | ||
| - condition_name: retry_count_valid | ||
| condition_type: expression | ||
| expression: "retry_count < 3" |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Standardize and document retry limit enforcement.
The retry limit < 3 is hardcoded in three guard definitions across different transition contexts (retry_consul, retry_postgres, retry_from_failed). This creates maintenance risk if the limit changes.
Recommendation:
- Define a global constant (e.g.,
MAX_RETRY_COUNT = 3) in a configuration or constants section. - Reference it consistently across all guards.
- Document whether exceeding the limit triggers the
RETRY_EXHAUSTEDerror code (line 982).
🔎 Proposed structure
Add a new section before "Guards" to define FSM constants:
## FSM Constants
| Constant | Value | Description |
|----------|-------|-------------|
| MAX_RETRY_COUNT | 3 | Maximum number of retry attempts before giving up |
| VALIDATION_TIMEOUT_MS | 5000 | Timeout for validation state |
| REGISTRATION_TIMEOUT_MS | 10000 | Timeout for registration states |
| DEREGISTRATION_TIMEOUT_MS | 15000 | Timeout for deregistration state |Then update guard expressions to reference the constant:
-expression: "retry_count < 3"
+expression: "retry_count < MAX_RETRY_COUNT"Also applies to: 585-585, 683-683
🤖 Prompt for AI Agents
In docs/architecture/REGISTRATION_FSM_CONTRACT.md around lines 556, 585, and 683
(and note line 982 for the error code), the retry limit "retry_count < 3" is
hardcoded in multiple guards; add a new "FSM Constants" section immediately
before the "Guards" section that defines MAX_RETRY_COUNT = 3 (and any related
timeouts if desired), update each guard expression to reference MAX_RETRY_COUNT
instead of the literal 3, and update the documentation at line 982 to explicitly
state whether exceeding MAX_RETRY_COUNT triggers the RETRY_EXHAUSTED error code
so all references stay consistent and maintainable.
…OMN-938] Fixes identified in PR #221 code review: MAJOR fixes: - Fix terminal state inconsistency: `registered` now correctly marked as `is_terminal: false` to allow graceful shutdown transition - Add Retry Counter Management section with YAML spec (storage, increment_on, reset_on, max_value) MINOR fixes: - Fix sentence fragment at line 311 ("Can be retried" -> "The registration can be retried") - Add Trigger Catalog section documenting all 15 FSM triggers with sources - Add CONTINUE trigger semantics note explaining auto-emission - Add deregistration ordering note (parallel intent emission) - Update wildcard transition note clarifying terminal state exclusion NITPICK fixes: - Add Guard Implementation Details section with: - Type Resolution (boolean, numeric, string fields) - Field Resolution (FSM context structure) - Operator Semantics table (all operators documented) - Error Handling (undefined field, unsupported operator, type mismatch)
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
docs/architecture/REGISTRATION_FSM_CONTRACT.md (2)
311-311: Fix sentence fragment in failed state description.This was previously flagged and needs correction: rewrite "Can be retried via
RETRYtrigger." as a complete sentence.🔎 Proposed fix
-**Purpose**: Complete failure state. Can be retried via `RETRY` trigger. +**Purpose**: Complete failure state. It can be retried via `RETRY` trigger.
560-560: Standardize and document retry limit enforcement.The retry limit
< 3is hardcoded across three transitions (lines 560, 585, 687), and line 982 referencesRETRY_EXHAUSTEDerror without explicitly documenting its trigger condition. This was flagged previously and should be refactored to use a constant for maintainability.Recommendation: Add an FSM Constants section before "Guards" (around line 744) to centralize these values, then update all guard expressions and error documentation to reference the constant consistently.
🔎 Proposed structure
Add this section before line 744:
## FSM Constants | Constant | Value | Description | |----------|-------|-------------| | MAX_RETRY_COUNT | 3 | Maximum number of retry attempts before giving up |Then update each guard at lines 560, 585, and 687:
- expression: "retry_count < 3" + expression: "retry_count < MAX_RETRY_COUNT"And clarify line 1055 to explicitly tie the constant to the error:
-| `RETRY_EXHAUSTED` | Maximum retry count exceeded | Manual intervention | +| `RETRY_EXHAUSTED` | retry_count >= MAX_RETRY_COUNT | Manual intervention or `ABANDON` |Also applies to: 585-585, 687-687, 982-982
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
docs/architecture/REGISTRATION_FSM_CONTRACT.md(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-19T18:46:12.170Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T18:46:12.170Z
Learning: Applies to **/*reducer*.py : Use ModelIntent pattern for REDUCER nodes with pure FSM transitions
Applied to files:
docs/architecture/REGISTRATION_FSM_CONTRACT.md
🪛 LanguageTool
docs/architecture/REGISTRATION_FSM_CONTRACT.md
[style] ~801-~801: To form a complete sentence, be sure to include a subject.
Context: ...xternal/Internal | Retry after failure. Can be triggered by operator intervention o...
(MISSING_IT_THERE)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
- GitHub Check: Tests (Split 20/20)
- GitHub Check: Tests (Split 19/20)
- GitHub Check: Tests (Split 12/20)
- GitHub Check: Tests (Split 13/20)
- GitHub Check: Tests (Split 15/20)
- GitHub Check: Tests (Split 18/20)
- GitHub Check: Tests (Split 2/20)
- GitHub Check: Tests (Split 3/20)
- GitHub Check: Tests (Split 16/20)
- GitHub Check: Tests (Split 9/20)
- GitHub Check: Tests (Split 7/20)
- GitHub Check: Tests (Split 11/20)
- GitHub Check: Tests (Split 14/20)
- GitHub Check: Tests (Split 8/20)
- GitHub Check: Tests (Split 4/20)
- GitHub Check: Tests (Split 17/20)
- GitHub Check: Tests (Split 1/20)
- GitHub Check: Tests (Split 6/20)
- GitHub Check: Tests (Split 5/20)
- GitHub Check: claude-review
PR Review: Registration FSM Contract [OMN-938]OverviewThis PR introduces a comprehensive FSM contract for dual registration (Consul + PostgreSQL) management. The document is well-structured, thorough, and follows ONEX architectural patterns. ✅ Strengths1. Excellent Documentation Structure
2. Architectural Alignment
3. Comprehensive Coverage
4. Implementation Guidance
🔍 Issues & RecommendationsCRITICAL: State Type InconsistencyLocation: Line 59, 199-201 The registered state has conflicting metadata - state_type says terminal but is_terminal is false. Impact: Creates confusion about state behavior. Documentation says success state allowing graceful shutdown but YAML contradicts itself. Recommendation: Change state_type from terminal to success, OR document why terminal is used despite is_terminal being false. MAJOR: Missing Transition from partial_registered to failedLocation: Transition table (lines 322-341) The FSM lacks a transition from partial_registered to failed when retry count is exhausted. Current behavior: When retry_count >= 3 in partial_registered, guards block transitions 9 and 10. Only options are RECOVERY_COMPLETE or FATAL_ERROR. Problem: If retries exhausted but recovery not complete, FSM is stuck with no natural path to failed state. Recommendation: Add transition 17 for retry_exhausted, OR document that operators must use FATAL_ERROR trigger when retries exhausted. MAJOR: Guard Expression Validation GapLocation: Lines 765-782, 812-852 The guard expression syntax allows complex operators (matches, contains, in) but implementation details do not specify:
Recommendation: Add subsection under Guard Expression Syntax documenting literal syntax for strings, booleans, numbers, arrays, and regex patterns. MINOR: Ambiguous CONTINUE Trigger SemanticsLocation: Lines 330, 471-491, 797 The CONTINUE trigger is described as automatically emitted by the Reducer but lacks clarity:
Recommendation: Expand note at line 490 to clarify timing, error handling, and logging requirements. MINOR: Incomplete Error Code MappingLocation: Lines 1050-1057 Validation errors table lists 5 error codes but document references additional codes like GUARD_EVALUATION_ERROR and GUARD_TYPE_ERROR. Recommendation: Add complete error code appendix including all FSM-specific error codes with descriptions and context fields. MINOR: Missing Timeout HandlingLocation: Lines 97, 131, 176, 254 Several states define timeout_ms but no timeout transition defined. What happens when state timeout expires? Recommendation: Add timeout transition or document timeout behavior in Implementation Notes. 🎯 Final VerdictCode Quality: ⭐⭐⭐⭐⭐ (5/5)Architectural Alignment: ⭐⭐⭐⭐⭐ (5/5)Completeness: ⭐⭐⭐⭐ (4/5)Implementability: ⭐⭐⭐⭐ (4/5)✅ Approval RecommendationStatus: APPROVE with minor revisions This is excellent work that will unblock OMN-889. Issues identified are mostly clarifications and edge cases that can be addressed in follow-up commit or PR. Must-Fix Before Merge:
Should-Fix Soon (can be follow-up):
Nice-to-Have:
Great work on this comprehensive specification! The FSM contract is well-designed and will provide solid guidance for the Dual Registration Reducer implementation. Reviewed by: Claude Code (Sonnet 4.5) |
…tract [OMN-938] Critical: - Fix state type inconsistency: change `registered` from terminal to success type - Add `success_states` field to FSM subcontract configuration Major: - Add Transition 17 (retry_exhausted) from partial_registered to failed - Add RETRY_EXHAUSTED trigger for retry limit enforcement - Document guard expression validation rules (load-time and runtime) - Add comprehensive retry limit enforcement documentation Minor: - Fix sentence fragments in failed state and RETRY trigger descriptions - Clarify CONTINUE trigger semantics as internal auto-progression - Add complete error code mapping (6 new codes) - Add timeout handling documentation with recovery strategies - Update state diagrams to show retry exhaustion path Nitpick: - Add guard validation examples and error codes
PR Review: Registration FSM Contract [OMN-938]SummaryThis is an excellent architectural documentation PR that formalizes the FSM contract for dual registration (Consul + PostgreSQL). The document is comprehensive, well-structured, and adheres to ONEX framework conventions. I have some suggestions to further improve clarity and correctness. ✅ Strengths1. Comprehensive Coverage
2. Architecture Alignment
3. Error Handling & Recovery
4. Documentation Quality
🔍 Issues & RecommendationsCRITICAL: State Transition InconsistencyIssue: Transition Table shows 17 transitions, but the document states "The Registration FSM defines 17 transitions" everywhere, yet the transition count is correct at 17. However, there's a logical inconsistency in the Lines 333-334, 551, 580: Transitions 9 and 10 both use the
Problem: Line 551 shows Transition 9 uses Recommendation:
Suggested Fix (Option 1): # Transition 9
trigger: RETRY_CONSUL # Make it explicit
conditions:
- postgres_applied == true
- consul_applied == false # Make XOR explicit
# Trigger Catalog update
| `RETRY_CONSUL` | External | Retry Consul specifically when PostgreSQL succeeded |HIGH: Guard Expression AmbiguityLines 557-558, 586-587: The guards use validation_rules:
- "postgres_applied != consul_applied" # XOR conditionIssue: The XOR validation rule should mean exactly one is true and one is false when entering Recommendation: Add explicit negative checks to guards for clarity: # Transition 9 (retry Consul)
conditions:
- postgres_applied == true
- consul_applied == false # Explicit check
- retry_count < 3
# Transition 10 (retry PostgreSQL)
conditions:
- consul_applied == true
- postgres_applied == false # Explicit check
- retry_count < 3This makes the XOR condition explicit and prevents potential runtime confusion. MEDIUM: Timeout Trigger InconsistencyLines 1349-1353: The timeout trigger mapping recommends:
Issue: These triggers have guards checking Recommendation:
Suggested Addition (after line 1351): **Orchestrator Timeout Responsibilities**:
When emitting a timeout trigger, the Orchestrator must:
1. Set the corresponding `*_applied` field to `false` in FSM context
2. Optionally set a `*_error` field with timeout details
3. Log the timeout event with duration and state contextMEDIUM: CONTINUE Trigger SemanticsLines 491, 883: The Issue: This is a bit ambiguous - does the Reducer emit Recommendation: Clarify the implementation pattern: Option A: Reducer explicitly returns def delta(state, event):
if event == POSTGRES_SUCCEEDED:
return ("postgres_registered", [ContinueIntent()])Option B: FSM runtime auto-progresses (epsilon transition) The `CONTINUE` trigger is an **epsilon transition** - the FSM runtime
automatically evaluates this transition without waiting for an external
event when entering `postgres_registered` state.I suspect Option B is the intent, but the document should clarify this. If so, add to line 491: **Note**: The `CONTINUE` trigger represents an **epsilon transition**
(automatic progression). The FSM runtime automatically attempts this
transition when entering `postgres_registered` without waiting for an
external event. No external trigger source is required.MINOR: Inconsistent TerminologyLines 59, 216: State types use "success" vs "terminal":
Issue: The terminology might confuse readers. Recommendation: Clarify the state type taxonomy: ### State Type Classification
| Type | Meaning | Terminal? | Outgoing Transitions |
|------|---------|-----------|---------------------|
| **initial** | Starting state | No | Many |
| **operational** | Active processing | No | Many |
| **snapshot** | Checkpoint state | No | Few (1-2) |
| **success** | Goal achieved, awaiting shutdown | No | One (DEREGISTER only) |
| **error** | Recoverable failure | No | Few (retry/abandon) |
| **terminal** | Final state, no exit | Yes | Zero |Add this table after line 64 for clarity. MINOR: Guard Operator Example MissingLine 932: The operator semantics table includes Recommendation: Add a regex example to the Guard Definitions table (line 778) or Guard Expression Syntax examples (line 806): MINOR: Missing Error Code ContextLines 1140-1151: Validation Errors table includes error codes like Recommendation: Add a subsection documenting these error scenarios: ### Intent Emission Errors
**Error**: `INTENT_EMISSION_FAILED`
**Cause**: Failed to construct or emit an intent during a state transition entry action.
**Examples**:
- Missing required field in intent payload
- Target Effect service not registered in container
- Intent serialization failure
**Recovery**:
- Trigger `FATAL_ERROR` to transition to `failed` state
- Log full context including state, transition, and failed intent🎨 Code Quality & Best Practices✅ Follows ONEX Conventions
✅ Type Safety
✅ Thread Safety
🔒 Security Considerations✅ Good
|
…MN-938] - Add detailed CONTINUE trigger semantics with implementation patterns - Expand guard expression syntax with comprehensive examples - Add cross-references from retry transitions to enforcement section - Clarify terminal vs success state distinction for registered state - Add Phase column to validation errors table - Document GUARD_FIELD_UNDEFINED error and strict mode behavior - Add 4 additional workflow scenarios (validation failure, retry exhaustion, fatal error, timeout recovery)
Registration FSM Contract Review [OMN-938]Executive SummaryVerdict: ✅ APPROVE with Minor Recommendations This is an exceptionally well-crafted FSM contract specification that demonstrates deep understanding of the ONEX architecture. The document provides comprehensive coverage of the dual registration lifecycle with excellent implementation guidance for OMN-889. Quality Score: 9.5/10
✅ Major Strengths1. Architectural Excellence
2. Comprehensive State Machine Design
3. Documentation Quality
4. Error Handling & Observability
🔍 Issues & Recommendations1. CRITICAL: State Terminal Classification
|
| Category | Score | Comments |
|---|---|---|
| Architecture Alignment | 10/10 | Perfect adherence to ONEX patterns |
| Completeness | 9/10 | Minor clarifications needed |
| Code Quality | 10/10 | Excellent YAML/Markdown formatting |
| Documentation | 10/10 | Outstanding explanatory notes |
| Test Coverage | 9/10 | Blocks OMN-942, guidance provided |
| Security | 10/10 | No issues detected |
| Performance | 10/10 | Efficient design |
| Maintainability | 10/10 | Clear structure, good cross-refs |
Overall: 9.5/10
✅ Recommendation
APPROVE - This is production-ready documentation. The minor recommendations are enhancements, not blockers. Excellent work! 🚀
The FSM contract provides clear, implementable guidance for OMN-889. The dual registration pattern with partial recovery is well-designed and will serve as a template for future FSM contracts in the ONEX framework.
Reviewer: Claude Code (Sonnet 4.5)
Review Date: 2025-12-19
Time Spent: 25 minutes
Lines Reviewed: 1749
Files Changed: 1
…MN-938] - Add error handling note for CONTINUE trigger failures - Add cross-reference from Guards section to Retry Limit Enforcement - Clarify nested field support (top-level only) in guard expressions - Clarify RETRY_EXHAUSTED trigger is generated by Orchestrator
Pull Request Review: Registration FSM ContractOverall Assessment: EXCELLENT ✅ This is an exceptionally well-crafted formal FSM specification that demonstrates mastery of the ONEX Reducer pattern and provides comprehensive implementation guidance. The document quality is production-ready. Strengths1. Architectural Adherence ⭐⭐⭐⭐⭐
2. Comprehensive State Modeling ⭐⭐⭐⭐⭐
3. Guard Expression System ⭐⭐⭐⭐⭐
4. CONTINUE Trigger Semantics ⭐⭐⭐⭐⭐Lines 491-527 provide exemplary documentation of internal trigger mechanics:
5. Retry Logic & Timeout Handling ⭐⭐⭐⭐⭐
6. Error Recovery Flows ⭐⭐⭐⭐
7. Documentation Quality ⭐⭐⭐⭐⭐
Issues & RecommendationsCritical Issues: None ✅Moderate Issues1. Guard Expression Limitation - Nested Field Access (Line 899)Issue: The contract states nested field access is NOT supported ( Recommendation: Consider adding an optional nested field syntax for v1.1: Rationale: Flattening context adds boilerplate and potential naming collisions. If the guard evaluator can walk object graphs, the FSM contract becomes cleaner. Action: Document this as a future enhancement or provide clear guidance on context flattening patterns. 2. Unclear RETRY_EXHAUSTED Trigger Source (Lines 1005, 809)Issue: The trigger catalog (line 1005) states RETRY_EXHAUSTED is "Generated by the Orchestrator", but Transition 17 documentation (line 809) says "The Reducer should then emit a Conflict: Who emits this trigger - Reducer or Orchestrator? Recommendation: Clarify the trigger source. Suggested resolution: RETRY_EXHAUSTED:
source: Orchestrator
condition: Detects Reducer returned without state change due to guard failure (retry_count >= 3)
action: Emits RETRY_EXHAUSTED to transition partial_registered -> failedUpdate line 809 transition note to match this clarification. 3. Missing Guard on Transition 11 (RECOVERY_COMPLETE)Issue: Transition 11 ( Edge Case: What if an external event sets both flags to Recommendation: Add a guard condition to verify the recovery path: conditions:
- condition_name: both_applied
condition_type: expression
expression: "postgres_applied == true"
required: true
- condition_name: consul_now_applied
condition_type: expression
expression: "consul_applied == true"
required: true
# Add this guard:
- condition_name: recovery_via_retry
condition_type: expression
expression: "recovery_attempt exists true"
required: trueOr document that the Orchestrator is responsible for only emitting Minor Issues4. XOR Validation on partial_registered State (Line 239)Issue: Validation rule Recommendation: Document that this XOR check is a runtime invariant enforced by the Reducer when entering # Reducer should enforce XOR invariant on entry to partial_registered
assert postgres_applied != consul_applied, "partial_registered requires exactly one component failed"5. Timeout Metric Labels (Line 1574)Issue: Histogram metric Recommendation: Use separate metrics or bounded enum for outcome: metrics:
- name: registration_state_duration_ms
type: histogram
labels: [state] # Remove outcome to reduce cardinality
- name: registration_state_outcome_total
type: counter
labels: [state, outcome] # Separate counter for outcomes
values: [success, timeout, validation_failure, postgres_failure, consul_failure]6. Guard Validation Error Handling (Line 979)Issue: Runtime behavior states "No exception is raised for undefined fields (fail-safe behavior)", but Error Handling section lists Clarification Needed: When is Recommendation: Update line 979 to: This matches the note at line 1270. 7. Missing Correlation ID Propagation ExamplesIssue: Implementation Notes (line 1296) mention correlation_id tracking but no examples show propagation through context updates. Recommendation: Add correlation ID handling to retry counter example: # Current example (line 1389)
new_context = context.model_copy(
update={"retry_count": retry_count + 1}
)
# Enhanced example
new_context = context.model_copy(
update={
"retry_count": retry_count + 1,
"correlation_id": context.correlation_id, # Preserve correlation
"retry_timestamp": datetime.utcnow(), # Add for debugging
}
)Suggestions for Future Enhancement
Code Quality AssessmentType Safety: ✅ Excellent
Error Handling: ✅ Comprehensive
Testing Considerations: ⭐⭐⭐⭐⭐This contract provides excellent test coverage guidance:
Documentation: ✅ Production-Ready
Security Considerations✅ No security issues identified The guard expression syntax is properly constrained:
Performance ConsiderationsPositive:
Watch:
Test Coverage RecommendationsBased on CLAUDE.md requirement (60% minimum): Essential Tests (must have):
Recommended Tests (should have): Alignment with CLAUDE.md✅ Fully compliant with project standards:
Final RecommendationAPPROVE with minor clarifications This PR is ready to merge after addressing the 2 moderate issues:
The minor issues and future enhancements can be tracked in follow-up tickets. Congratulations on an exceptional specification document! This will serve as an excellent blueprint for OMN-889 implementation. Estimated Implementation EffortBased on this contract:
Reviewed by: Claude Sonnet 4.5 |
…tract [OMN-938] - Add State Type Terminology section clarifying success vs terminal states - Add Dual Registration Order section documenting PostgreSQL-first ordering - Add Intent Emission Error Details with CONTINUE trigger failure handling - Improve state descriptions (failed, RETRY trigger, validating, etc.) - Add no-background-agents rule to CLAUDE.md Agent Instructions
Code Review - PR #221: Registration FSM ContractSummaryThis PR introduces a comprehensive FSM (Finite State Machine) contract for the Dual Registration Reducer (OMN-889), providing formal specification for managing node registration to both Consul and PostgreSQL. The documentation is exceptionally thorough and well-structured. ✅ Strengths1. Outstanding Documentation Quality
2. Architectural Excellence
3. Robust Error Handling
4. CONTINUE Trigger Innovation
5. Retry Limit Enforcement
6. Alignment with ONEX Conventions
🔍 Areas for Improvement1. State Recoverability InconsistencyIssue: Table at line 82 shows
Concern: If Recommendation: Add a "Recoverability" section explaining what this field means:
2. Guard Expression Syntax - Missing Whitespace Edge CasesIssue: Line 990-991 states: "retry_count< 3" # GUARD_SYNTAX_ERROR: operator attached to fieldConcern: This conflicts with line 867-869 which says expressions are split into "exactly 3 tokens" separated by whitespace. If Recommendation: Clarify the tokenization rules:
3. CONTINUE Trigger - Two-Step Observability TradeoffIssue: Line 528 mentions two-step execution "makes the checkpoint state observable but adds latency." Concern: The tradeoff isn't fully explored:
Recommendation: Add a subsection under "CONTINUE Trigger Semantics" titled "When to Use Two-Step Execution" with guidance on:
4. Timeout Recovery - Deregistering Timeout SemanticsIssue: Line 1593 states that Concern: This creates orphaned resources if cleanup fails:
Recommendation: Consider adding:
5. Guard Field Resolution - Nested FieldsIssue: Line 943 states "Nested field access (e.g., Concern: This limits guard expressiveness. What if context structure changes and fields become nested? Recommendation:
6. Error Code Table CompletenessIssue: Lines 1297-1313 define validation error codes, but some errors mentioned in the document don't appear in the table:
Recommendation: Add a comprehensive "Error Code Reference" appendix listing ALL error codes with:
7. Metrics Section MissingIssue: The document mentions metrics at lines 1610-1622 (timeout metrics) but doesn't provide a comprehensive metrics catalog. Recommendation: Add a "Metrics & Observability" section documenting ALL emitted metrics:
Each metric should include:
8. CLAUDE.md Change - Minor WordingIssue: Line 79 in CLAUDE.md states: Concern: The phrase "parallel polymorphic agents" might be unclear to new contributors. Recommendation: Rephrase for clarity: 🛡️ Security Considerations✅ No security concerns identified:
🧪 Test Coverage AssessmentMissing Test GuidanceObservation: The "Test Plan" in the PR description only covers documentation validation:
Recommendation: Add a "Testing Strategy" section to the document covering:
📊 Performance Considerations✅ Well-designed for performance:
Minor concern: Retry backoff is mentioned (line 1339) but not specified:
Recommendation: Add a "Retry Backoff Configuration" section with recommended parameters. ✨ Best Practices Observed
🎯 Final VerdictAPPROVE ✅This is an exemplary architectural document that sets a high bar for FSM contract specifications. The level of detail, clarity, and implementation guidance is outstanding. Blockers: NoneThe issues identified are minor clarifications and enhancements that can be addressed in follow-up PRs or inline edits. Recommended Actions (Priority Order):
🙏 KudosExceptional work on this contract, @jonahgabriel! The Lines reviewed: 1,961 (1,959 new + 2 modified) Ready to merge pending minor clarifications above. 🚀 |
…tract [OMN-938] Comprehensive documentation enhancements addressing all review feedback: CRITICAL: - Clarified state type terminology (success vs terminal states) - Added whitespace-delimited operator requirements with valid/invalid examples - Documented observability vs latency tradeoffs for CONTINUE trigger - Added severity levels (Warning/Error/Critical) to validation errors table MAJOR: - Added missing error codes: INTENT_PAYLOAD_ERROR, EFFECT_NOT_FOUND, INTENT_SERIALIZATION_ERROR, INTERNAL_TRANSITION_ERROR - Added context preprocessing guidance for nested field limitation - Enhanced retry limit enforcement documentation with guard semantics - Added error handling best practices section (6 best practices) MINOR: - Added comprehensive metrics reference with types, emission points, thresholds - Documented nested field limitation as v1.0 scope with workaround - Added deregistration failure handling and orphan cleanup guidance - Clarified is_recoverable semantics for success states NITPICK: - Added guard validation flow diagram - Enhanced token separation rules documentation - Added valid/invalid format examples for guard expressions
Pull Request Review: Registration FSM Contract [OMN-938]SummaryThis PR introduces a comprehensive 2,735-line FSM (Finite State Machine) contract specification for the Dual Registration Reducer. The document is exceptionally thorough, well-structured, and demonstrates strong technical depth in formal state machine design for ONEX node registration. ✅ Strengths1. Outstanding Documentation Quality
2. Thoughtful Architecture Design
3. Exceptional Error Handling
4. Guard Expression System
5. Observability & Operations
6. Practical Implementation Guidance
🔍 Observations & Minor Suggestions1. Document Length (2,735 lines)Observation: While comprehensive, the document is very long. This may impact maintainability and discoverability. Suggestions:
2. CONTINUE Trigger ComplexityObservation: The CONTINUE trigger implementation section (lines 608-678) is highly detailed with single-step vs two-step execution patterns, latency comparisons, and crash recovery considerations. Strength: This level of detail is excellent for implementers who need to make informed tradeoff decisions. Suggestion: Consider adding a TL;DR summary at the top of the CONTINUE section: **TL;DR**: Use single-step execution (recommended for production, <10ms latency). Use two-step only for debugging (adds 50-100ms but makes checkpoint observable).3. Nested Field Limitation (v1.0)Observation: Lines 1096-1148 document that guard expressions don't support nested field access ( Strength: The limitation is well-documented with clear workarounds and rationale. Question: Is there a plan to implement nested field support in v2.0? If so, consider adding a reference to a tracking ticket. 4. Mermaid Diagram RenderingObservation: The state diagram (lines 1393-1440) uses Verification needed: Confirm this renders correctly in GitHub's Markdown renderer. The simplified ASCII flow diagram (lines 1444-1489) is an excellent fallback. Suggestion: Add a note in the PR description or commit message confirming successful rendering. 5. Deregistration Best-Effort CleanupObservation: Lines 339-383 document that deregistration failures proceed to terminal state with best-effort cleanup, potentially leaving orphaned resources. Strength: The garbage collection recommendation (lines 356-379) is excellent - provides concrete SQL query example. Suggestion: Consider referencing this pattern in the "Implementation Notes" section (lines 1621-1646) so implementers don't miss it. 🔐 Security Considerations✅ No Security Concerns Identified
🧪 Test Coverage RecommendationsWhile this is a specification document, here are testing suggestions for the implementation (OMN-889):
📊 Performance Considerations✅ Well-Designed for Performance
💡 Optimization OpportunityParallel deregistration: Lines 325-327 note that 🎯 Code Quality Assessment
✅ Recommendation: APPROVEThis PR represents excellent technical documentation work. The Registration FSM Contract is comprehensive, well-reasoned, and provides strong guidance for implementation. The minor suggestions above are enhancements for future iterations, not blockers for v1.0. Specific approval criteria met:
Next steps:
📚 Related CLAUDE.md Guidance Followed
Excellent work on this specification! 🎉 Review completed by Claude Code on 2025-12-19 |
…OMN-938] - Add TL;DR summary to CONTINUE trigger section for quick reference - Add v2.0 tracking note for nested field access enhancement - Add Mermaid rendering note with fallback reference - Cross-reference garbage collection in Implementation Notes - Add Document Organization section with future extraction guidance
Pull Request Review: Registration FSM Contract [OMN-938]OverviewThis PR introduces a comprehensive 2,752-line FSM contract specification for the Dual Registration Reducer. This is an architectural specification document, not an implementation. Overall, this is excellent work - comprehensive, well-structured, and thoughtful. The document demonstrates deep understanding of FSM patterns and ONEX architecture. ✅ Strengths1. Exceptional Documentation Quality
2. Thoughtful Architecture
3. Robust Error Handling
4. Guard Expression Design
5. Observability & Metrics
🔍 Code Quality AssessmentAdherence to CLAUDE.md Standards✅ PASS - All project conventions followed:
Type Safety✅ PASS - YAML contracts use Pydantic models:
Documentation Standards✅ EXCELLENT - Exceeds expectations:
|
| State | Trigger | Guard | Limit Enforcement |
|---|---|---|---|
failed |
RETRY |
retry_count < 3 |
Transition silently blocked (guard returns false) |
partial_registered |
RETRY |
retry_count < 3 |
Orchestrator emits RETRY_EXHAUSTED → failed |
Why the Difference?
- From
partial_registered: Orchestrator detects guard failure and emitsRETRY_EXHAUSTEDtrigger (Transition 17) - From
failed: Guard failure blocks transition, noRETRY_EXHAUSTEDtrigger
This asymmetry is confusing and may cause implementation bugs.
Recommendation:
- Add Transition 18:
failed→deregisteredonRETRY_EXHAUSTED(analogous to Transition 17) - Update Orchestrator Behavior: Emit
RETRY_EXHAUSTEDwhen guard fails on ANY retry transition, not just frompartial_registered - Rationale: Consistent behavior is easier to test and reason about
Severity: Medium (consistency issue that may cause bugs)
5. Missing Metrics for Deregistration Failures
Location: Lines 1995-2168 (Metrics Catalog)
Issue: The metrics catalog defines success metrics (deregistration_complete) but no failure metrics:
- Missing:
deregistration_consul_failure_total - Missing:
deregistration_postgres_failure_total - Missing:
deregistration_timeout_total
These are critical for alerting on orphaned resources (see Issue #1).
Recommendation:
Add 3 metrics to the catalog:
deregistration_consul_failure_total:
type: counter
labels: [node_id, deployment_id, error_type]
description: "Consul deregistration failed during graceful shutdown"
deregistration_postgres_failure_total:
type: counter
labels: [node_id, deployment_id, error_type]
description: "PostgreSQL deregistration failed during graceful shutdown"
deregistration_timeout_total:
type: counter
labels: [node_id, deployment_id, phase]
description: "Deregistration exceeded 15s timeout (best-effort cleanup applied)"Severity: Medium (observability gap for production operations)
6. Minor: CLAUDE.md Change is Unrelated
Location: CLAUDE.md line 79
Change: Added instruction "NEVER run agents in background mode"
Issue: This is a process improvement, not related to FSM contract work. Ideally should be a separate PR for cleaner git history.
Recommendation: Accept for this PR (it's a one-line change), but future process improvements should be separate PRs.
Severity: Trivial (process suggestion)
🧪 Test Coverage Considerations
Current Status: This PR is documentation only (no implementation).
For OMN-889 Implementation, ensure test coverage for:
- All 17 Transitions: Unit tests for each transition with valid guards
- Guard Failures: Test retry limit enforcement (Issue TKT-003: Validate Import Resolution and Eliminate Unused Dependencies #4 above)
- Timeout Handling: Integration tests with mock delays
- CONTINUE Failures: Test internal transition error path (Issue feat(tests): Complete TKT-002 ONEXContainer validation with comprehensive test suite #3)
- Deregistration Failures: Test best-effort cleanup (Issue Complete SPI Migration and Core Stabilization: LogLevel Standardization + ONEX Architecture #1)
- Metrics Emission: Verify all 8+ metrics are emitted correctly
Recommendation: Add "Test Plan" section to FSM contract document listing required test scenarios for implementers.
🔐 Security Concerns
✅ NO SECURITY ISSUES IDENTIFIED
- No credentials or secrets in YAML configs
- Correlation IDs for audit logging (good practice)
- Idempotent operations resist replay attacks
- Structured errors don't leak sensitive data
📊 Performance Considerations
Positive:
- Single-Step CONTINUE Execution (lines 537-566): <10ms latency vs 50-100ms for two-step
- Timeout Values: Reasonable (5s validation, 10s registration, 15s deregistration)
- Guard Evaluation: O(1) for 3-token expressions
Potential Issues:
- No Batch Registration: Each node registers individually (may be bottleneck at scale)
- Retry Backoff Missing: The contract mentions "backoff guidance" (line 29) but doesn't specify exponential backoff parameters
Recommendation:
- Add Retry Backoff Section: Document recommended backoff strategy (e.g., 1s, 2s, 4s for 3 retries)
- Batch Registration: File future enhancement issue if node registration at scale becomes a bottleneck
Severity: Low (performance is likely adequate for v1.0)
🎯 Acceptance Criteria Review
Comparing against PR description:
| Criterion | Status | Evidence |
|---|---|---|
| All states documented (10 states) | ✅ PASS | Lines 96-440 |
| All transitions documented (16 transitions) | Lines 449-467 | |
| Transition guards defined (12 guards) | ✅ PASS | Guard expressions throughout |
| State diagram included (Mermaid) | ✅ PASS | Lines 1408-1455 |
Note: PR description says "16 state transitions" but the Transition Table (line 449) lists 17 transitions. This is likely because Transition 17 (retry_exhausted) was added during authoring. Update PR description to reflect 17 transitions.
📝 Documentation Suggestions
1. Add Quick Reference Table
Add a 1-page "Quick Reference" section at the end with:
- State summary table (10 states with 1-line descriptions)
- Trigger summary table (14 triggers with sources)
- Common recovery paths (3-4 most frequent scenarios)
2. Add Implementation Checklist
For OMN-889 implementers, add:
## Implementation Checklist
- [ ] Implement all 10 states with entry/exit actions
- [ ] Implement all 17 transitions with guards
- [ ] Implement flatten_context() for guard evaluation
- [ ] Emit all 8+ metrics
- [ ] Handle CONTINUE failures with FATAL_ERROR path
- [ ] Implement idempotent PostgreSQL upsert
- [ ] Implement idempotent Consul registration
- [ ] Add garbage collection job for orphaned resources
- [ ] Test all 8 recovery scenarios
- [ ] Verify correlation_id propagation3. Cross-Reference to ONEX Terminology
Line 37 references docs/standards/onex_terminology.md, but the FSM uses terms like "Intent", "Reducer", "Effect" without defining them. Add:
> **Terminology**: For canonical definitions of Intent, Reducer, Effect, and other ONEX concepts, see [ONEX Terminology Guide](../standards/onex_terminology.md).Severity: Trivial (usability improvements)
🚀 Final Verdict
Overall Assessment: APPROVE with Recommendations
This is high-quality work that demonstrates:
- Deep FSM expertise
- Thoughtful error handling design
- Production-ready observability patterns
- Excellent documentation skills
Required Changes (Before Merge):
⚠️ Add deregistration failure metrics (Issue feat: implement complete 4-node canary deployment architecture #5) - 10 minutes⚠️ Fix retry limit asymmetry (Issue TKT-003: Validate Import Resolution and Eliminate Unused Dependencies #4) - Add Transition 18 or document rationale - 15 minutes⚠️ Update PR description (17 transitions, not 16) - 2 minutes
Recommended Changes (Can Be Follow-Up PRs):
- Add garbage collection job requirement to OMN-889 acceptance criteria (Issue Complete SPI Migration and Core Stabilization: LogLevel Standardization + ONEX Architecture #1)
- File enhancement issue for nested guard field access (Issue Add Claude Code GitHub Workflow #2)
- Add retry backoff parameters to FSM configuration (Performance section)
- Add Quick Reference and Implementation Checklist (Documentation suggestions)
Blocking Issues: None
🏆 Kudos
Exceptional work on:
- CONTINUE trigger semantics (single-step vs two-step execution analysis)
- State type terminology (success vs terminal state distinction)
- Error handling best practices (correlation IDs, structured logging, graceful degradation)
- Guard validation flow diagram (clear 2-phase validation process)
This document will serve as an excellent reference for future FSM contracts in the ONEX framework.
Reviewer: Claude Sonnet 4.5 (Code Review Agent)
Review Date: 2025-12-19
Contract Version: 1.0.0
Recommendation: Approve after addressing required changes
Address 50+ PR review issues across 8 categories: CRITICAL (5): - Add three-dimensional state classification system - Document mandatory whitespace in guard expressions - Add latency decision matrix for CONTINUE trigger - Define severity levels (warning/error/critical) - Document deregistration cleanup with GC examples MAJOR (14): - Add retry limit enforcement cross-references - Clarify RETRY_EXHAUSTED trigger source (Orchestrator) - Add guard error recovery scenarios (9, 10) - Complete error code table with all codes - Add 11 new metrics with alerting thresholds MINOR (30+): - Fix 16 sentence fragments in Trigger Catalog - Add recovery terminology disambiguation section - Enhance Table of Contents with subsections - Add comprehensive edge case examples NITPICK (1): - Extensively document guard expression syntax +937 lines of documentation enhancements
PR Review: Registration FSM Contract v1.0.0OverviewThis PR introduces a comprehensive FSM (Finite State Machine) contract for the Dual Registration Reducer (OMN-889), managing node registration to both Consul and PostgreSQL. The documentation is exceptionally thorough and well-structured. ✅ Strengths1. Exceptional Documentation Quality
2. Architectural Alignment with ONEX Principles
3. Robust Error Handling
4. Guard Expression System
5. State Diagram & Transitions
6. Production Readiness
🔍 Areas for Improvement1. Minor InconsistenciesRecovery Terminology (Lines 148-172)The "Recovery Terminology Disambiguation" section is excellent but could be positioned earlier. Consider moving it immediately after "State Type Terminology" (around line 77) since it clarifies concepts used throughout the document. Recommendation: Move lines 148-172 to precede "State Definitions" (before line 174) for better information flow. 2. Guard Expression Whitespace - UX ConsiderationThe mandatory whitespace requirement for guard expressions is correctly implemented but may cause friction for users accustomed to compact syntax. While the decision is sound (simpler parser, predictable behavior), consider these enhancements: Recommendation 1 - Helpful Error Messages: Recommendation 2 - Auto-formatting Tool (Future Enhancement): poetry run onex-lint --fix registration_fsm.yaml
# Auto-corrects: retry_count<3 → retry_count < 3Not a blocker - The current specification is clear and well-justified. 3. CONTINUE Trigger - Crash Recovery AmbiguityLines 961-973 discuss crash recovery during CONTINUE processing but leave some ambiguity:
Question: What if the crash occurs after PostgreSQL registration completes but before the state transition is persisted? This could lead to:
Recommendation: Add a subsection clarifying atomicity of state persistence: ### Crash Recovery - State Persistence Atomicity
**Critical Assumption**: This contract assumes the Orchestrator persists FSM state
transitions **atomically** with intent emission acknowledgment. If state persistence
is NOT atomic with effect application:
1. Use **event sourcing** - persist the trigger/event, replay on recovery
2. Or use **two-phase commit** - ensure state + effect results are atomic
3. Or accept **at-least-once semantics** - rely on Effect idempotency
See: `docs/architecture/ORCHESTRATOR_PERSISTENCE_GUARANTEES.md`Impact: Medium - This affects correctness guarantees during crashes. Should be clarified for implementers. 4. Deregistration Failure - Operational ConcernsLines 411-700+ cover deregistration failure handling with best-effort cleanup and GC jobs. This is excellent and necessary, but raises operational questions: Missing Guidance:
Recommendation: Add a subsection: ### Garbage Collection - Operational Runbook
#### GC Job Failure Handling
- **Transient errors** (network timeouts): Retry with exponential backoff (max 3 retries)
- **Persistent errors** (DB unavailable): Alert ops team, job will retry on next schedule
- **Batch size tuning**: If GC times out, reduce `batch_size` from 100 to 50/25
#### Alerting Thresholds
| Metric | Warning | Critical | Action |
|--------|---------|----------|--------|
| Orphaned nodes | >50 | >200 | Review node churn rate; check for mass failures |
| GC job failure | 2 consecutive | 5 consecutive | Check database/Consul health |
| GC duration | >60s | >120s | Reduce batch size or optimize query |
#### Manual Remediation
If automated GC fails, use these manual commands:
```sql
-- PostgreSQL: Manually delete orphaned registration
DELETE FROM node_registrations WHERE node_id = '<orphaned_node_id>';# Consul: Manually deregister service
consul services deregister -id=<service_id>Impact: Low-Medium - Operational guidance helps production teams handle failures. 5. Test Coverage - Not IncludedThe PR is documentation-only (no implementation). The test plan checklist is minimal: - [ ] Mermaid diagram renders correctly in GitHub
- [ ] Document structure follows existing architecture docs patterns
- [ ] Contract is implementable for OMN-889Recommendation: When OMN-889 (implementation) is created, ensure:
Suggested Test Structure: # tests/unit/reducer/test_registration_fsm.py
class TestRegistrationFSMTransitions:
@pytest.mark.parametrize("state,trigger,expected_state,guards", [
("unregistered", "REGISTER", "validating", {"payload_valid": True}),
("validating", "VALIDATION_SUCCESS", "registering_postgres", {}),
# ... 17 transitions
])
def test_transition(self, state, trigger, expected_state, guards):
"""Test each FSM transition follows contract."""
...
# tests/integration/reducer/test_registration_e2e.py
@pytest.mark.integration
async def test_full_registration_happy_path():
"""Test complete registration flow with real dependencies."""
# Uses testcontainers for Consul + PostgreSQL
...Impact: High - Tests are critical for FSM correctness. Not a blocker for this doc PR, but critical for OMN-889. 6. Observability - Metrics & LoggingThe contract mentions metrics and logging throughout but lacks a centralized reference: Recommendation: Add a section (or link to external doc): ## Observability & Monitoring
### Metrics Catalog
| Metric | Type | Labels | Description |
|--------|------|--------|-------------|
| `registration_attempts_total` | Counter | `environment`, `deployment_id` | Total registration attempts |
| `registration_success_total` | Counter | `environment` | Successful registrations (reached `registered` state) |
| `registration_partial_total` | Counter | `component` (postgres/consul) | Partial failures by component |
| `registration_retry_total` | Counter | `from_state`, `retry_count` | Retry attempts |
| `registration_duration_seconds` | Histogram | `outcome` (success/failed/partial) | Time from `unregistered` to terminal state |
| `fsm_state_transitions_total` | Counter | `from_state`, `to_state`, `trigger` | All FSM transitions (debugging) |
### Logging Standards
All FSM state transitions should emit structured logs:
```json
{
"timestamp": "2025-12-19T10:15:30Z",
"level": "INFO",
"fsm": "registration",
"event": "state_transition",
"from_state": "validating",
"to_state": "registering_postgres",
"trigger": "VALIDATION_SUCCESS",
"correlation_id": "reg-abc123",
"node_id": "node-prod-001",
"duration_ms": 42
}Impact: Medium - Improves production debugging and alerting. 7. CLAUDE.md Change - Minor EnhancementThe change to CLAUDE.md (line 79) is good but could be more specific: - **NEVER** run agents in background mode (`run_in_background: true`) - run parallel polymorphic agents in the foreground instead by dispatching multiple agents in a single messageRecommendation: Add rationale for clarity: - **NEVER** run agents in background mode (`run_in_background: true`) - Background agents
can fail silently and complicate error handling. Instead, run parallel polymorphic agents
in the foreground by dispatching multiple agents in a single message (see Agent SDK docs).Impact: Low - Helps developers understand why, not just what. 🔒 Security Considerations1. Garbage Collection - Race ConditionsThe PostgreSQL GC job (lines 465-700) uses SELECT node_id FROM node_registrations
WHERE last_heartbeat < NOW() - $1::interval
FOR UPDATE SKIP LOCKEDGood: Prevents lock contention with concurrent GC jobs ✅ Potential Issue: If a node is legitimately down but heartbeat hasn't stopped (e.g., heartbeat process stuck), the GC won't clean it up until heartbeat stops. Recommendation: Add a secondary condition: WHERE (
last_heartbeat < NOW() - $1::interval
OR (status = 'deregistering' AND updated_at < NOW() - INTERVAL '1 hour')
)Impact: Low - Edge case, but worth considering. 2. Consul Deregistration - Missing Health Check CleanupLines 430-434 mention orphaned health check definitions:
The document states: "Automatic cleanup with service deregistration" Question: What if service deregistration times out but the health check was registered separately? Consul may continue polling. Recommendation: Clarify in the Consul GC job example: # Deregister service (includes associated health checks)
await client.put(f"{consul_addr}/v1/agent/service/deregister/{service_id}")
# Explicitly deregister health checks if service deregistration may have failed
checks = await client.get(f"{consul_addr}/v1/agent/checks")
orphaned_checks = [c for c in checks if c["ServiceID"] == service_id]
for check_id in orphaned_checks:
await client.put(f"{consul_addr}/v1/agent/check/deregister/{check_id}")Impact: Low - Consul usually handles this, but explicit cleanup is safer. 📋 Code Quality & Best Practices✅ Adherence to ONEX StandardsBased on
Action Item for OMN-889: Ensure implementation follows ONEX container/DI patterns. ✅ Documentation Quality Metrics
🎯 Recommendations SummaryMust-Have (Before Merge)None - the PR is merge-ready as-is. Should-Have (Nice to Have)
Could-Have (Future Enhancements)
✅ Final VerdictAPPROVED ✅ This is an exemplary FSM contract specification. The documentation quality exceeds typical architecture documents by a significant margin. The three-dimensional state classification system, comprehensive error handling, and production-ready GC implementations demonstrate deep architectural thinking. Why Approve?
Post-Merge Actions
Great work! This sets a high bar for FSM contract specifications in the ONEX ecosystem. 🎉 Reviewed by Claude Code | Following ONEX architecture standards from |
Comprehensive documentation improvements for Registration FSM Contract: **CRITICAL Issues:** - Add CONTINUE trigger crash recovery vs retry-after-failure disambiguation - Enhance deregistration best-effort cleanup with orphan resource table - Fix state type inconsistency with consistent trigger names **MAJOR Issues:** - Standardize retry limit enforcement with RETRY_EXHAUSTED source clarified - Document Orchestrator (not Reducer) generates RETRY_EXHAUSTED trigger - Add missing partial_registered -> failed transition via RETRY_EXHAUSTED - Complete error code table with phase and severity info - Add comprehensive FATAL_ERROR path documentation (~175 lines) - Add guard validation error handling subsection (~100 lines) **MINOR Issues:** - Add Guard Syntax Quick Reference section (~52 lines) - Document all 15+ metrics with type, emission timing, alerting thresholds - Add correlation ID propagation examples (~200 lines) - Fix incomplete ASCII diagrams and duplicate separators - Verify document structure matches project patterns **NITPICK:** - Clarify guard expression syntax and validation rules Document grew from ~2,735 to 4,308 lines (+749 insertions, -11 deletions)
Pull Request Review: Registration FSM Contract [OMN-938]Overall AssessmentVerdict: ✅ Approve with Minor Suggestions This is an exceptionally well-crafted FSM contract specification. The document demonstrates deep architectural thinking, comprehensive edge case handling, and outstanding documentation quality. The 4,308-line document is thorough without being redundant, providing a complete reference for implementing the Dual Registration Reducer (OMN-889). Strengths 🎯1. Exceptional State Modeling
2. Comprehensive Error Handling
3. Outstanding Documentation Quality
4. Practical Implementation Guidance
5. Strong Architectural Alignment
Potential Issues & Suggestions1. Minor: Postgres vs PostgreSQL Consistency 🟡Issue: The document uses both "Postgres" and "PostgreSQL" inconsistently. Suggestion: Standardize on "PostgreSQL" in prose, "postgres" in identifiers (state names, transitions already follow this). Impact: Low - does not affect correctness, just consistency. 2. Minor: CLAUDE.md Change Needs Context 🟡File: Change: Added guideline about not running agents in background mode. Issue: This is valuable but appears unrelated to the FSM contract. PR description doesn't mention CLAUDE.md updates. Suggestion: Add a note to PR description explaining why this guideline was added. 3. Enhancement: Guard Expression Cookbook 🔵The guard expression reference is comprehensive for validation errors but could benefit from more positive examples showing common patterns (retry limits, boolean checks, XOR conditions). Impact: Low priority - current documentation is already excellent. 4. Question: Validation State Timeout Strategy ❓Location: Suggestion: Add a note clarifying that validation MUST be local (no I/O) to justify the 5-second timeout. This prevents implementers from accidentally adding blocking I/O to validation. Security Considerations 🔒1. Orphaned Resource Cleanup ✅
2. PostgreSQL Injection Risk ✅
3. Consul API Security
|
| State | Timeout | Analysis |
|---|---|---|
validating |
5s | ✅ Generous for local validation |
registering_postgres |
10s | ✅ Reasonable for DB upsert |
registering_consul |
10s | ✅ Reasonable for Consul API |
deregistering |
15s | ✅ Extra buffer for dual cleanup |
2. PostgreSQL GC Performance ✅
- Excellent use of
FOR UPDATE SKIP LOCKED(prevents lock contention) - Batch size limiting (prevents unbounded deletions)
- Transaction-scoped (atomic cleanup)
- Status: Production-ready ✅
3. Retry Limit of 3 ✅
- Maximum 3 attempts is appropriate
- Recommendation: Consider adding exponential backoff guidance
Test Coverage
The PR test plan covers rendering, structure, and implementability. For the implementation ticket (OMN-889), suggest adding:
-
Contract Load-Time Validation Tests
- All guard expressions parse correctly
- Invalid guards fail with correct error codes
-
Transition Coverage Tests
- All 16 transitions are reachable
- Terminal state has no outgoing transitions
-
Error Flow Tests
- All 10 error scenarios
- Retry exhaustion triggers correct transition
-
Garbage Collection Tests
- PostgreSQL GC with mocked stale records
- Consul GC with mocked critical services
- Idempotency verification
Adherence to CLAUDE.md ✅
Project Standards Compliance:
- ✅ Uses proper Pydantic v2 patterns
- ✅ Follows ONEX Reducer purity guarantees
- ✅ Intent emission follows ModelIntent pattern
- ✅ Error handling uses ModelOnexError (implied by error codes)
- ✅ No backwards-incompatible changes (new document)
Documentation Standards:
- ✅ Version number and last updated date present
- ✅ Related ticket numbers cross-referenced
- ✅ Clear purpose and scope definition
- ✅ Comprehensive table of contents
Architecture Alignment:
- ✅ Follows ONEX four-node architecture (Reducer → Effect → Orchestrator)
- ✅ Pure FSM semantics (no I/O in Reducer)
- ✅ Protocol-driven DI implied
- ✅ Structured errors with error codes
Recommendations Summary
Must Fix (Blockers) 🔴
None - document is ready to merge.
Should Fix (High Value) 🟡
- Standardize PostgreSQL vs Postgres terminology in prose (5 min)
- Add context for CLAUDE.md change in PR description (2 min)
Nice to Have (Enhancements) 🔵
- Add Guard Expression Cookbook subsection (15 min)
- Add validation I/O prohibition note (5 min)
- Add security note to Consul GC example (10 min)
- Add retry backoff recommendation (10 min)
Future Work (Post-Merge) ⚪
- Extract reusable patterns to standalone guides if FSM pattern is reused
- Link to registration payload models once OMN-913 merges
Conclusion
This is exceptional work. The Registration FSM Contract demonstrates:
- Deep understanding of FSM theory and ONEX architecture
- Comprehensive edge case analysis (orphaned resources, crash recovery, timeout handling)
- Outstanding documentation quality (4,308 lines, zero fluff)
- Production-ready operational guidance (GC jobs, metrics, alerting)
The minor suggestions above are truly minor - they would make a great document even better, but none are blockers.
Recommendation: ✅ Approve and merge after addressing items #1 and #2 (terminology + PR description context). Items #3-6 can be addressed in follow-up commits or during implementation (OMN-889).
Reviewed by: Claude Code (Sonnet 4.5)
Review Date: 2025-12-20
Reviewed Commits: fe316dd (PR #221)
Great work, @jonahgabriel! 🎉
1 participant
Summary
Acceptance Criteria
Dependencies
Blocks
Test Plan
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.