fix: resolve mypy union-attr errors - part 5 (refs #11) #32
Conversation
- Add None checks before accessing self.process attributes - Fixes union-attr errors for Process | None type
…11) - Fix lines 448 and 510 with None checks
…py (refs #11) - Use explicit if/else instead of ternary operator for better type inference - Fixes union-attr errors for str | LogLevel type
- Add None check for results[0].item in domain_classifier.py - Add None check for self.kb in context_manager.py - Handle LLMResponse | AsyncIterator union in entity_extraction.py - Add type check for workflow_steps dict in phase4_test_suite.py
- Add None checks at start of _process_standard, _process_kb_enhanced, _process_analysis_only, _process_validation_only - Add None checks at start of _stage1_preprocess, _stage2_extract_entities, _stage3_resolve_entities - Add None checks before accessing self.state attributes - Fixes union-attr errors for AgentState | None type
…refs #11) - Add None checks in _stage4_build_graph, _stage5_generate_xml, _stage6_validate_phase4 - Add None check for validation_layer in _stage6_validate_phase4
There was a problem hiding this comment.
Pull Request Overview
This PR adds defensive null checks and type guards throughout the codebase to improve type safety and handle edge cases. The changes primarily focus on adding runtime validation for optional attributes that could potentially be None, as well as handling union types properly for LLM responses.
- Added null checks for
self.process,self.state,self.kb, andself.validation_layerbefore accessing their properties - Updated
entity_extraction.pyto handle the union typeLLMResponse | AsyncIterator[str]returned by the LLM client - Refactored ternary expressions to if-else blocks in
observability.pyfor improved readability
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 12 comments.
Show a summary per file
| File | Description |
|---|---|
| bpmn_agent/validation/phase4_test_suite.py | Added type guard to check workflow_steps is a dict before iteration |
| bpmn_agent/stages/xml_generation.py | Added null checks for self.process before accessing its properties in multiple methods |
| bpmn_agent/stages/entity_extraction.py | Added handling for union type return value from LLM client (LLMResponse or AsyncIterator) |
| bpmn_agent/knowledge/domain_classifier.py | Added null check for results[0].item before accessing it with .get() |
| bpmn_agent/knowledge/context_manager.py | Added null check for self.kb before calling its methods |
| bpmn_agent/core/observability.py | Refactored ternary operator to if-else block for clarity |
| bpmn_agent/agent/orchestrator.py | Added null checks for self.state and self.validation_layer in multiple stage methods |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if self.process is None: | ||
| raise ValueError("Process not initialized") |
There was a problem hiding this comment.
These null checks for self.process are redundant. The generate_xml method initializes self.process at line 162 (self.process = self._create_process(...)) and then immediately calls _build_xml_root at line 170, which calls _build_process_element. Since self.process is always initialized before these methods are called, these checks will never fail during normal execution.
Consider removing these redundant checks or moving them to the beginning of the generate_xml method if you want to ensure initialization.
| if self.process is None: | |
| raise ValueError("Process not initialized") |
| if self.state is None: | ||
| raise ValueError("AgentState not initialized") |
There was a problem hiding this comment.
This null check for self.state is redundant. The process() method always initializes self.state at line 119 before calling _process_kb_enhanced(). Since this is a private method that's only called from process(), self.state will always be initialized when this code executes.
Consider removing this redundant check to simplify the code.
| if self.state is None: | |
| raise ValueError("AgentState not initialized") |
| if self.state is None: | ||
| raise ValueError("AgentState not initialized") |
There was a problem hiding this comment.
This null check for self.state is redundant. The process() method always initializes self.state at line 119 before calling any stage methods. Since _stage6_validate_phase4() is a private method that's only called from processing methods invoked by process(), self.state will always be initialized when this code executes.
Consider removing this redundant check to simplify the code.
| if self.state is None: | |
| raise ValueError("AgentState not initialized") |
| if self.process is None: | ||
| raise ValueError("Process not initialized") |
There was a problem hiding this comment.
These null checks for self.process are redundant. The generate_xml method initializes self.process at line 162 (self.process = self._create_process(...)) and then immediately calls _convert_nodes and _organize_lanes at lines 165-167. Since self.process is always initialized before these methods are called, these checks will never fail during normal execution.
Consider removing these redundant checks or moving them to the beginning of the generate_xml method if you want to ensure initialization.
| if self.process is None: | |
| raise ValueError("Process not initialized") |
| if self.process is None: | ||
| raise ValueError("Process not initialized") |
There was a problem hiding this comment.
These null checks for self.process are redundant. The generate_xml method initializes self.process at line 162 (self.process = self._create_process(...)) and then immediately calls _build_xml_root at line 170, which calls _build_diagram_element. Since self.process is always initialized before these methods are called, these checks will never fail during normal execution.
Consider removing these redundant checks or moving them to the beginning of the generate_xml method if you want to ensure initialization.
| if self.process is None: | |
| raise ValueError("Process not initialized") |
| if self.state is None: | ||
| raise ValueError("AgentState not initialized") |
There was a problem hiding this comment.
This null check for self.state is redundant. The process() method always initializes self.state at line 119 before calling any stage methods. Since _stage2_extract_entities() is a private method that's only called from processing methods invoked by process(), self.state will always be initialized when this code executes.
Consider removing this redundant check to simplify the code.
| if self.state is None: | |
| raise ValueError("AgentState not initialized") |
| if self.state is None: | ||
| raise ValueError("AgentState not initialized") |
There was a problem hiding this comment.
This null check for self.state is redundant. The process() method always initializes self.state at line 119 before calling any stage methods. Since _stage3_resolve_entities() is a private method that's only called from processing methods invoked by process(), self.state will always be initialized when this code executes.
Consider removing this redundant check to simplify the code.
| if self.state is None: | |
| raise ValueError("AgentState not initialized") |
| if self.state is None: | ||
| raise ValueError("AgentState not initialized") |
There was a problem hiding this comment.
This null check for self.state is redundant. The process() method always initializes self.state at line 119 before calling any stage methods. Since _stage5_generate_xml() is a private method that's only called from processing methods invoked by process(), self.state will always be initialized when this code executes.
Consider removing this redundant check to simplify the code.
| try: | ||
| # Call LLM | ||
| from bpmn_agent.core.llm_client import LLMMessage | ||
| from bpmn_agent.core.llm_client import LLMMessage, LLMResponse |
There was a problem hiding this comment.
The imports LLMMessage and LLMResponse are placed inside the method, which means they will be imported on every method call. While Python caches imports, this is not idiomatic and makes the code less maintainable.
Consider moving these imports to the top of the file with the other imports (line 19 imports BaseLLMClient from the same module).
| if self.process is None: | ||
| raise ValueError("Process not initialized") |
There was a problem hiding this comment.
These null checks for self.process are redundant. The generate_xml method initializes self.process at line 162 (self.process = self._create_process(...)) and then immediately calls _convert_nodes and _organize_lanes at lines 165-167. Since self.process is always initialized before these methods are called, these checks will never fail during normal execution.
Consider removing these redundant checks or moving them to the beginning of the generate_xml method if you want to ensure initialization.
| if self.process is None: | |
| raise ValueError("Process not initialized") |
Summary
This PR continues addressing mypy type errors as part of Issue #11 (Code Quality & Technical Debt Cleanup), focusing on
union-attrerrors.Changes Made
✅ Resolved Error Types
Key Fixes
Process | None in xml_generation.py:
self.processattributesUnion[str, LogLevel] in observability.py:
Optional types in knowledge modules:
results[0].itemindomain_classifier.pyself.kbincontext_manager.pyLLMResponse | AsyncIterator in entity_extraction.py:
llm_client.call()LLMResponsevsAsyncIterator[str]and handle accordinglydict | str | None in phase4_test_suite.py:
workflow_stepsdictionaryAgentState | None in orchestrator.py:
_process_standard,_process_kb_enhanced,_process_analysis_only,_process_validation_only)_stage1_preprocess,_stage2_extract_entities,_stage3_resolve_entities,_stage4_build_graph,_stage5_generate_xml,_stage6_validate_phase4)validation_layerin_stage6_validate_phase4Impact
Remaining Errors (201)
operator: 43 errorsattr-defined: 27 errorsassignment: 19 errorsunion-attr: 17 errorsreturn-value: 11 errorsTesting