-
Notifications
You must be signed in to change notification settings - Fork 0
Apply code review feedback: fix hardcoded paths, add safety checks, improve bootstrap logic #49
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Apply code review feedback: fix hardcoded paths, add safety checks, improve bootstrap logic #49
Conversation
Co-authored-by: Steake <530040+Steake@users.noreply.github.com>
Co-authored-by: Steake <530040+Steake@users.noreply.github.com>
Co-authored-by: Steake <530040+Steake@users.noreply.github.com>
009d580
into
find-out-the-main-tasks-left-9wLP6LwjQwb7
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR addresses 11 code review comments by fixing critical path portability issues, adding safety checks to prevent runtime errors, and improving code quality. The changes enhance system reliability by replacing hardcoded paths with dynamic resolution, adding defensive checks for potential AttributeErrors, and refactoring fragile bootstrap detection logic into a dedicated method.
Key Changes
- Path Portability: Replaced hardcoded
/workspace/GodelOSpaths with dynamic resolution usingos.path.dirname(os.path.abspath(__file__))and$(dirname "${BASH_SOURCE[0]}")for cross-environment compatibility - Safety Checks: Added
isinstance()validation before dictionary access and redundancy check before variance calculation to prevent potential runtime errors - Code Quality: Refactored bootstrap detection into
is_bootstrap_complete()method, improved async method detection withasyncio.iscoroutinefunction(), and properly definedmetadatafield in EmergentPattern dataclass
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| quick_verify.sh | Dynamic script directory resolution replacing hardcoded path |
| inline_test.py | Dynamic project root detection for path insertion |
| demo_consciousness.py | Dynamic project root detection for path insertion |
| backend/unified_server.py | Added isinstance check for safe dictionary access; replaced fragile hasattr checks with dedicated method call |
| backend/goal_management_system.py | Removed redundant inline import, using module-level logger |
| backend/core/unified_consciousness_engine.py | Added redundancy check and optimized variance calculation with extracted mean |
| backend/core/metacognitive_monitor.py | Enhanced documentation explaining partial integration status and future enhancement path |
| backend/core/knowledge_graph_evolution.py | Added metadata field to EmergentPattern dataclass, removed dynamic dict assignment |
| backend/core/consciousness_engine.py | Added is_bootstrap_complete() method with multi-aspect validation; documented bootstrap delay purposes |
| backend/core/cognitive_manager.py | Improved async method detection using asyncio.iscoroutinefunction() |
| if len(depth_history) > 0: | ||
| mean_depth = sum(depth_history) / len(depth_history) | ||
| depth_variance = sum((d - mean_depth)**2 for d in depth_history) / len(depth_history) | ||
| # Lower variance = higher stability | ||
| stability = max(0.0, min(1.0, 1.0 - (depth_variance / 4.0))) | ||
| current_state.recursive_awareness["strange_loop_stability"] = stability | ||
| else: | ||
| current_state.recursive_awareness["strange_loop_stability"] = 0.5 |
Copilot
AI
Nov 22, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The check if len(depth_history) > 0: on line 576 is redundant. Since the outer condition on line 574 ensures len(self.consciousness_history) > 5, and line 575 creates depth_history from the last 5 elements with self.consciousness_history[-5:], depth_history will always contain exactly 5 elements at this point. The additional check can be removed.
| if len(depth_history) > 0: | |
| mean_depth = sum(depth_history) / len(depth_history) | |
| depth_variance = sum((d - mean_depth)**2 for d in depth_history) / len(depth_history) | |
| # Lower variance = higher stability | |
| stability = max(0.0, min(1.0, 1.0 - (depth_variance / 4.0))) | |
| current_state.recursive_awareness["strange_loop_stability"] = stability | |
| else: | |
| current_state.recursive_awareness["strange_loop_stability"] = 0.5 | |
| mean_depth = sum(depth_history) / len(depth_history) | |
| depth_variance = sum((d - mean_depth)**2 for d in depth_history) / len(depth_history) | |
| # Lower variance = higher stability | |
| stability = max(0.0, min(1.0, 1.0 - (depth_variance / 4.0))) | |
| current_state.recursive_awareness["strange_loop_stability"] = stability |
Addresses 11 code review comments from PR #44 review 3495910323, fixing path portability issues, potential runtime errors, and code quality concerns.
Critical Fixes
Hardcoded paths replaced with dynamic resolution
quick_verify.sh: Uses$(dirname "${BASH_SOURCE[0]}")instead of/workspace/GodelOSinline_test.py,demo_consciousness.py: Usesos.path.dirname(os.path.abspath(__file__))Safety checks added
unified_server.py:2728: Addedisinstance()check before accessingphenomenal_experience.get()to preventAttributeErrorunified_consciousness_engine.py:576: Added explicitlen(depth_history) > 0check before variance calculationEmergentPattern dataclass metadata field
Eliminates dynamic attribute assignment via
__dict__.Redundant inline import removed
goal_management_system.py:264: Uses module-levelloggerinstead of inlineimport loggingCode Quality Improvements
Bootstrap check logic refactored
Replaces fragile multi-level
hasattrchecks with dedicated validation method.Async method detection improved
cognitive_manager.py: Usesasyncio.iscoroutinefunction()to properly detect async methods before awaitingDocumentation added
Variance calculation optimized
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.