feat: Activate all dormant cognitive subsystems#100
Conversation
Review — Strong implementation, two issues to resolve
Issue 1 — Silent None propagation: In Issue 2 — Please address both and this is ready to mark ready for review. |
Review — Dormant Subsystem Activation ✅ LGTM with notes
Observations:
Ready to undraft and merge once items 3 and 4 are confirmed. |
|
Both fixed in Issue 1 — Silent None propagation: Issue 2 —
Both addressed in spaCy skip guards: Added
|
Merge conflict detected — rebase requiredPR #100 is currently An automated branch update was attempted; if it succeeded, please re-run any affected tests. If not, please rebase manually: git fetch origin
git rebase origin/main
# Accept main's version of query_replay_harness.py if conflicted
git push --force-with-leaseOnce rebased and CI is passing on PR #99, this is ready to merge. |
Create godelOS/cognitive_pipeline.py with CognitivePipeline class that initialises all 23 subsystems (8 dormant + 15 supporting). Wire it into GödelOSIntegration.initialize() and expose subsystem status via /api/status and /api/system/subsystems endpoints. Co-authored-by: Steake <530040+Steake@users.noreply.github.com>
26 smoke tests confirm all 23 subsystems initialise, data flows end-to-end through NLU→KS→Inference→Context→NLG, and the backend exposes subsystem status. Docs catalog active vs unimplemented modules. Co-authored-by: Steake <530040+Steake@users.noreply.github.com>
- Guard ContextEngine init against None knowledge_store - Simplify hasattr check in unified_server.py - Fix test to use list_contexts() instead of get_all_contexts() - Remove unnecessary type annotation on result dict Co-authored-by: Steake <530040+Steake@users.noreply.github.com>
…arness syntax error, add spaCy skip guards - _register now raises RuntimeError when factory returns None, so get_subsystem_status never reports 'active' with a None instance. - Fix start_recording parameter order in query_replay_harness.py: move non-defaulted correlation_id before defaulted context param. - Add @requires_spacy skip guards to NLU-dependent smoke tests so they degrade gracefully when en_core_web_sm is absent in CI. Co-authored-by: Steake <530040+Steake@users.noreply.github.com>
Steake
force-pushed
the
copilot/activate-dormant-cognitive-systems
branch
from
7003079 to
edfb935
Compare
There was a problem hiding this comment.
Pull request overview
This PR introduces a unified CognitivePipeline to instantiate and expose previously-dormant GödelOS subsystems, wires it into the backend integration layer, and adds API/status visibility plus smoke tests and documentation for subsystem activation.
Changes:
- Add
godelOS/cognitive_pipeline.pyto initialize and report status for 23 cognitive subsystems. - Integrate the pipeline into backend startup/status reporting (
GödelOSIntegration,/api/status, and new/api/system/subsystems). - Add integration smoke tests and documentation describing activation status and expected data flow.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_cognitive_subsystem_activation.py | Adds smoke tests asserting subsystem activation and basic end-to-end flow. |
| godelOS/cognitive_pipeline.py | New unified subsystem wiring/registry with activation status reporting. |
| docs/SUBSYSTEM_ACTIVATION_STATUS.md | Documents which subsystems are active and what remains unimplemented. |
| backend/unified_server.py | Extends /api/status to include subsystem info; adds /api/system/subsystems. |
| backend/godelos_integration.py | Initializes CognitivePipeline during integration startup and exposes subsystem status in health. |
|
|
||
| @pytest.fixture(scope="module") | ||
| def pipeline(): | ||
| """Shared pipeline instance for the entire module (expensive to create).""" |
There was a problem hiding this comment.
The module-scoped pipeline fixture always calls CognitivePipeline.initialize(), but NLUPipeline construction ultimately triggers LexicalAnalyzerParser, which downloads the spaCy model if it's missing. This means runs without en_core_web_sm can hang/fail due to an attempted network download before the requires_spacy skips take effect. Consider guarding NLU/NLG initialization (or the whole fixture/module) when the model isn’t present, and avoid auto-downloading during tests.
| """Shared pipeline instance for the entire module (expensive to create).""" | |
| """Shared pipeline instance for the entire module (expensive to create).""" | |
| if not _HAS_SPACY: | |
| # Avoid triggering spaCy model auto-download during pipeline initialisation. | |
| # Skip at module level so all dependent tests are cleanly marked as skipped. | |
| pytest.skip("spaCy or en_core_web_sm model not available", allow_module_level=True) |
| EXPECTED_SUBSYSTEMS = [ | ||
| # Core KR | ||
| "type_system", | ||
| "knowledge_store", | ||
| "unification_engine", | ||
| "formal_logic_parser", | ||
| # Inference engine (includes dormant modal & CLP) | ||
| "resolution_prover", | ||
| "modal_tableau_prover", | ||
| "clp_module", | ||
| "analogical_reasoning_engine", | ||
| "inference_coordinator", | ||
| # Scalability | ||
| "caching_system", | ||
| # NLU / NLG | ||
| "nlu_pipeline", | ||
| "nlg_pipeline", | ||
| # Symbol Grounding (dormant) | ||
| "simulated_environment", | ||
| "perceptual_categorizer", | ||
| "symbol_grounding_associator", | ||
| "action_executor", | ||
| "internal_state_monitor", | ||
| # Common Sense & Context (dormant) | ||
| "context_engine", | ||
| "common_sense_manager", | ||
| # Metacognition | ||
| "metacognition_manager", | ||
| # Learning System (dormant) | ||
| "ilp_engine", | ||
| "explanation_based_learner", | ||
| "meta_control_rl", | ||
| ] | ||
|
|
||
| def test_all_subsystems_present(self, pipeline): | ||
| """Every expected subsystem is registered in the pipeline.""" | ||
| status = pipeline.get_subsystem_status() | ||
| for name in self.EXPECTED_SUBSYSTEMS: | ||
| assert name in status, f"Subsystem '{name}' missing from pipeline" | ||
|
|
||
| def test_all_subsystems_active(self, pipeline): | ||
| """Every expected subsystem has status 'active'.""" | ||
| status = pipeline.get_subsystem_status() | ||
| for name in self.EXPECTED_SUBSYSTEMS: | ||
| info = status.get(name, {}) | ||
| assert info.get("status") == "active", ( | ||
| f"Subsystem '{name}' is not active: {info}" | ||
| ) |
There was a problem hiding this comment.
These activation assertions require all subsystems (including nlu_pipeline / nlg_pipeline) to be active. That conflicts with the intended optional-dependency behavior (skip when spaCy/model isn’t available) and will still fail even if NLU/NLG are correctly marked unavailable. Consider making the expected subsystem list conditional (e.g., only require NLU/NLG when spaCy+model are present) or skipping the whole activation test when prerequisites are missing.
| try: | ||
| coord.submit_goal(goal, set()) | ||
| except Exception: | ||
| pass # proof failure is acceptable; crash is not |
There was a problem hiding this comment.
This test swallows all exceptions from submit_goal() and always passes, which defeats the purpose of a smoke test (it would pass even on a real crash). If proof failure is acceptable, catch only the specific expected failure type(s) or assert that the call returns a ProofObject/failure result rather than suppressing all exceptions.
| try: | |
| coord.submit_goal(goal, set()) | |
| except Exception: | |
| pass # proof failure is acceptable; crash is not | |
| coord.submit_goal(goal, set()) |
| def _init_nlu_nlg(self) -> None: | ||
| from godelOS.nlu_nlg.nlu.pipeline import NLUPipeline | ||
| from godelOS.nlu_nlg.nlg.pipeline import NLGPipeline | ||
|
|
||
| self.nlu_pipeline = self._register( | ||
| "nlu_pipeline", | ||
| lambda: NLUPipeline(self.type_system) | ||
| if self.type_system else None, | ||
| ) | ||
| self.nlg_pipeline = self._register( | ||
| "nlg_pipeline", | ||
| lambda: NLGPipeline(self.type_system) | ||
| if self.type_system else None, | ||
| ) |
There was a problem hiding this comment.
NLU/NLG initialization currently relies on downstream code that will attempt to download en_core_web_sm if missing (via subprocess.run). For server startup and CI, it’s safer to treat a missing spaCy model as an unavailable optional dependency (record subsystem as error with a clear message) rather than initiating a download. Consider adding an explicit spaCy+model availability check before registering NLU/NLG and returning None if unavailable.
| def _init_common_sense(self) -> None: | ||
| from godelOS.common_sense.context_engine import ContextEngine | ||
| from godelOS.common_sense.manager import CommonSenseContextManager | ||
|
|
||
| self.context_engine = self._register( | ||
| "context_engine", | ||
| lambda: ContextEngine(self.knowledge_store) | ||
| if self.knowledge_store else None, | ||
| ) | ||
|
|
||
| self.common_sense_manager = self._register( | ||
| "common_sense_manager", | ||
| lambda: CommonSenseContextManager( | ||
| knowledge_store=self.knowledge_store, | ||
| inference_coordinator=self.inference_coordinator, | ||
| cache_system=self.cache_system, | ||
| config={"create_default_context": True}, | ||
| ) | ||
| if self.knowledge_store and self.inference_coordinator else None, | ||
| ) |
There was a problem hiding this comment.
ContextEngine is registered as its own subsystem, but CommonSenseContextManager constructs and uses a separate internal ContextEngine instance. That means contexts created via the context_engine subsystem won’t be visible to the common-sense manager (and vice versa), which undermines the idea of a single wired pipeline. Consider removing the standalone context_engine subsystem or refactoring so both components share the same ContextEngine instance.
| try: | ||
| from godelOS.cognitive_pipeline import CognitivePipeline | ||
| _PIPELINE_AVAILABLE = True | ||
| except Exception: # noqa: BLE001 |
There was a problem hiding this comment.
The import guard for CognitivePipeline catches all exceptions, which can mask real programming errors (e.g., SyntaxError) and silently disable subsystem activation. Consider narrowing this to ImportError/ModuleNotFoundError (and optionally logging the exception details) so genuine bugs fail fast while still handling missing optional deps.
| except Exception: # noqa: BLE001 | |
| except (ImportError, ModuleNotFoundError) as exc: | |
| logger.warning("CognitivePipeline not available, running in degraded mode: %s", exc) |
godelOS/cognitive_pipeline.py— unified pipeline wiring all dormant subsystems using real constructors (23/23 active)backend/godelos_integration.pyinitialization/api/statusand new/api/system/subsystemsendpointdocs/SUBSYSTEM_ACTIVATION_STATUS.md)_register— now raises RuntimeError when factory returns Nonequery_replay_harness.pysyntax error — reorderedstart_recordingparams so non-defaultedcorrelation_idcomes before defaultedcontextOriginal prompt
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.