diff --git a/conductor/tracks/gui_decoupling_controller_20260302/debrief.md b/conductor/tracks/gui_decoupling_controller_20260302/debrief.md new file mode 100644 index 0000000..57be288 --- /dev/null +++ b/conductor/tracks/gui_decoupling_controller_20260302/debrief.md @@ -0,0 +1,50 @@ +# Comprehensive Debrief: GUI Decoupling Track (Botched Implementation) + +## 1. Track Overview +* **Track Name:** GUI Decoupling & Controller Architecture +* **Track ID:** `gui_decoupling_controller_20260302` +* **Primary Objective:** Decouple business logic from `gui_2.py` (3,500+ lines) into a headless `AppController`. + +## 2. Phase-by-Phase Failure Analysis + +### Phase 1: Controller Skeleton & State Migration +* **Status:** [x] Completed (with major issues) +* **What happened:** State variables (locks, paths, flags) were moved to `AppController`. `App` was given a `__getattr__` and `__setattr__` bridge to delegate to the controller. +* **Failure:** The delegation created a "Phantom State" problem. Sub-agents began treating the two objects as interchangeable, but they are not. Shadowing (where `App` has a variable that blocks `Controller`) became a silent bug source. + +### Phase 2: Logic & Background Thread Migration +* **Status:** [x] Completed (with critical regressions) +* **What happened:** Async loops, AI client calls, and project I/O were moved to `AppController`. +* **Failure 1 (Over-deletion):** Tier 3 workers deleted essential UI-thread handlers from `App` (like `_handle_approve_script`). This broke button callbacks and crashed the app on startup. +* **Failure 2 (Thread Violation):** A "fallback queue processor" was added to the Controller thread. This caused two threads to race for the same event queue. If the Controller won, the UI never blinked/updated, causing simulation timeouts. +* **Failure 3 (Property Erasure):** During surgical cleanups in this high-reasoning session, the `current_provider` getter/setter in `AppController` was accidentally deleted while trying to remove a redundant method. `App` now attempts to delegate to a non-existent attribute, causing `AttributeError`. + +### Phase 3: Test Suite Refactoring +* **Status:** [x] Completed (fragile) +* **What happened:** `conftest.py` was updated to patch `AppController` methods. +* **Failure:** The `live_gui` sandbox environment (isolated workspace) was broken because the Controller now eagerly checks for `credentials.toml` on startup. The previous agent tried to "fix" this by copying secrets into the sandbox, which is a security regression and fragile. + +### Phase 4: Final Validation +* **Status:** [ ] FAILED +* **What happened:** Integration tests and extended simulations fail or timeout consistently. +* **Root Cause:** Broken synchronization between the Controller's background processing and the GUI's rendering loop. The "Brain" (Controller) and "Limb" (GUI) are disconnected. + +## 3. Current "Fucked" State of the Codebase +* **`src/gui_2.py`:** Contains rendering but is missing critical property logic. It still shadows core methods that should be purely in the controller. +* **`src/app_controller.py`:** Missing core properties (`current_provider`) and has broken `start_services` logic. +* **`tests/conftest.py`:** Has a messy `live_gui` fixture that uses environment variables (`SLOP_CREDENTIALS`, `SLOP_MCP_ENV`) but points to a sandbox that is missing the actual files. +* **`sloppy.py`:** The entry point works but the underlying classes are in a state of partial migration. + +## 4. Immediate Recovery Plan (New Phase 5) + +### Phase 5: Stabilization & Cleanup +1. **Task 5.1: AST Synchronization Audit.** Manually (via AST) compare `App` and `AppController`. Ensure every property needed for the UI exists in the Controller and is correctly delegated by `App`. +2. **Task 5.2: Restore Controller Properties.** Re-implement `current_provider` and `current_model` in `AppController` with proper logic (initializing adapters, clearing stats). +3. **Task 5.3: Explicit Delegation.** Remove the "magic" `__getattr__` and `__setattr__`. Replace them with explicit property pass-throughs. This will make `AttributeError` visible during static analysis rather than runtime. +4. **Task 5.4: Fix Sandbox Isolation.** Ensure `live_gui` fixture in `conftest.py` correctly handles `credentials.toml` via `SLOP_CREDENTIALS` env var pointing to the root, and ensure `sloppy.py` respects it. +5. **Task 5.5: Event Loop Consolidation.** Ensure there is EXACTLY ONE `asyncio` loop running, owned by the Controller, and that the GUI thread only reads from `_pending_gui_tasks`. + +## 5. Technical Context for Next Session +* **Encoding issues:** `temp_conftest.py` and other git-shipped files often have UTF-16 or different line endings. Use Python-based readers to bypass `read_file` failures. +* **Crucial Lines:** `src/gui_2.py` line 180-210 (Delegation) and `src/app_controller.py` line 460-500 (Event Processing) are the primary areas of failure. +* **Mocking:** All `patch` targets in `tests/` must now be audited to ensure they hit the Controller, not the App. diff --git a/conductor/tracks/gui_decoupling_controller_20260302/plan.md b/conductor/tracks/gui_decoupling_controller_20260302/plan.md index 17dd6fd..9eb0856 100644 --- a/conductor/tracks/gui_decoupling_controller_20260302/plan.md +++ b/conductor/tracks/gui_decoupling_controller_20260302/plan.md @@ -22,4 +22,11 @@ - [ ] WHAT: `uv run pytest` - [ ] HOW: Ensure 100% pass rate. - [ ] SAFETY: Watch out for lingering thread closure issues. -- [ ] Task: Conductor - User Manual Verification 'Phase 4: Final Validation' (Protocol in workflow.md) \ No newline at end of file +- [ ] Task: Conductor - User Manual Verification 'Phase 4: Final Validation' (Protocol in workflow.md) + +## Phase 5: Stabilization & Cleanup (RECOVERY) +- [ ] Task: Task 5.1: AST Synchronization Audit +- [ ] Task: Task 5.2: Restore Controller Properties (Restore `current_provider`) +- [ ] Task: Task 5.3: Replace magic `__getattr__` with Explicit Delegation +- [ ] Task: Task 5.4: Fix Sandbox Isolation logic in `conftest.py` +- [ ] Task: Task 5.5: Event Loop Consolidation & Single-Writer Sync \ No newline at end of file diff --git a/run_repro.py b/run_repro.py new file mode 100644 index 0000000..ca20f0b --- /dev/null +++ b/run_repro.py @@ -0,0 +1,67 @@ +import time +import requests +import sys +import os + +# Ensure src/ is in path +project_root = os.path.dirname(os.path.abspath(__file__)) +src_path = os.path.join(project_root, "src") +sys.path.insert(0, src_path) + +from api_hook_client import ApiHookClient + +def run_repro(): + client = ApiHookClient("http://127.0.0.1:8999") + if not client.wait_for_server(timeout=15): + print("Failed to connect to GUI Hook Server.") + return + + print("[REPRO] Connected to GUI.") + + # 1. Reset and Setup + client.click("btn_reset") + time.sleep(1) + client.set_value("auto_add_history", True) + client.set_value("manual_approve", False) # Auto-approve for simulation + client.set_value("current_provider", "gemini_cli") + + mock_script = os.path.abspath("tests/mock_gemini_cli.py") + client.set_value("gcli_path", f'"{sys.executable}" "{mock_script}"') + + # 2. Trigger Chat + msg = "What is the current date and time? Answer in one sentence." + print(f"[REPRO] Sending message: {msg}") + client.set_value("ai_input", msg) + client.click("btn_gen_send") + + # 3. Wait and Monitor + start_time = time.time() + while time.time() - start_time < 30: + status = client.get_value("ai_status") + print(f"[REPRO] Status: {status}") + + if status == "error": + print("[REPRO] DETECTED ERROR STATUS!") + # Try to get more info if possible + break + + if status == "done": + print("[REPRO] Success! Status is done.") + break + + # Check events + events = client.get_events() + for ev in events: + print(f"[REPRO] Received Event: {ev.get('type')}") + + time.sleep(1) + + # 4. Check Session + session = client.get_session() + entries = session.get('session', {}).get('entries', []) + print(f"[REPRO] History Entries: {len(entries)}") + for i, entry in enumerate(entries): + print(f" {i}: [{entry.get('role')}] {entry.get('content')[:100]}...") + +if __name__ == "__main__": + run_repro() diff --git a/simulation/workflow_sim.py b/simulation/workflow_sim.py index 991edc2..13fd348 100644 --- a/simulation/workflow_sim.py +++ b/simulation/workflow_sim.py @@ -103,9 +103,10 @@ class WorkflowSimulator: print(f"\n[DEBUG] {elapsed:.1f}s - status: '{status}', roles: {roles}") last_debug_time = elapsed - if "error" in status: - print(f"\n[ABORT] GUI reported error status: {status}") - return last_ai_entry if last_ai_entry else {"role": "AI", "content": f"ERROR: {status}"} + if "error" in status: + resp = self.client.get_value("ai_response") + print(f"\n[ABORT] GUI reported error status: {status} | AI Response: {resp}") + return last_ai_entry if last_ai_entry else {"role": "AI", "content": f"ERROR: {status}"} # Turn completion logic: # 1. Transition: we were busy and now we are not, and the last role is AI. diff --git a/src/app_controller.py b/src/app_controller.py index 44387eb..faed00d 100644 --- a/src/app_controller.py +++ b/src/app_controller.py @@ -469,18 +469,7 @@ class AppController: asyncio.set_event_loop(self._loop) self._loop.create_task(self._process_event_queue()) - # Fallback: process queues even if GUI thread is idling/stuck - async def queue_fallback() -> None: - while True: - try: - # Headless/fallback queue processing - # Note: In GUI mode, App._gui_func still calls these for low latency. - self._process_pending_gui_tasks() - # _process_pending_history_adds might need more care regarding project state - except: pass - await asyncio.sleep(0.1) - - self._loop.create_task(queue_fallback()) + pass # Loop runs the process_event_queue task self._loop.run_forever() async def _process_event_queue(self) -> None: @@ -671,100 +660,6 @@ class AppController: return True return False - def _process_pending_gui_tasks(self) -> None: - if not self._pending_gui_tasks: - return - with self._pending_gui_tasks_lock: - tasks = self._pending_gui_tasks[:] - self._pending_gui_tasks.clear() - for task in tasks: - try: - action = task.get("action") - if action == "refresh_api_metrics": - self._refresh_api_metrics(task.get("payload", {})) - elif action == "handle_ai_response": - payload = task.get("payload", {}) - text = payload.get("text", "") - stream_id = payload.get("stream_id") - is_streaming = payload.get("status") == "streaming..." - if stream_id: - if is_streaming: - if stream_id not in self.mma_streams: self.mma_streams[stream_id] = "" - self.mma_streams[stream_id] += text - else: - self.mma_streams[stream_id] = text - if stream_id == "Tier 1": - if "status" in payload: - self.ai_status = payload["status"] - else: - if is_streaming: - self.ai_response += text - else: - self.ai_response = text - self.ai_status = payload.get("status", "done") - self._trigger_blink = True - if not stream_id: - self._token_stats_dirty = True - elif action == "mma_stream_append": - payload = task.get("payload", {}) - stream_id = payload.get("stream_id") - text = payload.get("text", "") - if stream_id: - if stream_id not in self.mma_streams: - self.mma_streams[stream_id] = "" - self.mma_streams[stream_id] += text - elif action == "mma_state_update": - payload = task.get("payload", {}) - self.mma_status = payload.get("status", "idle") - self.active_tier = payload.get("active_tier") - self.mma_tier_usage = payload.get("tier_usage", self.mma_tier_usage) - self.active_tickets = payload.get("tickets", []) - elif action == "mma_step_approval": - dlg = MMAApprovalDialog(str(task.get("ticket_id") or ""), str(task.get("payload") or "")) - self._pending_mma_approval = task - if "dialog_container" in task: - task["dialog_container"][0] = dlg - elif action == "mma_spawn_approval": - spawn_dlg = MMASpawnApprovalDialog( - str(task.get("ticket_id") or ""), - str(task.get("role") or ""), - str(task.get("prompt") or ""), - str(task.get("context_md") or "") - ) - self._pending_mma_spawn = task - if "dialog_container" in task: - task["dialog_container"][0] = spawn_dlg - except Exception as e: - print(f"Error executing GUI task: {e}") - - def stop_services(self): - """Stops background threads and async event loop.""" - if self._loop: - self._loop.call_soon_threadsafe(self._loop.stop) - if self._loop_thread: - self._loop_thread.join(timeout=2.0) - - @property - def current_provider(self) -> str: - return self._current_provider - - @current_provider.setter - def current_provider(self, value: str) -> None: - if value != self._current_provider: - self._current_provider = value - ai_client.reset_session() - ai_client.set_provider(value, self.current_model) - if value == "gemini_cli": - if not ai_client._gemini_cli_adapter: - ai_client._gemini_cli_adapter = ai_client.GeminiCliAdapter(binary_path=self.ui_gemini_cli_path) - else: - ai_client._gemini_cli_adapter.binary_path = self.ui_gemini_cli_path - if hasattr(self, 'hook_server'): - self.hook_server.start() - self.available_models = [] - self._fetch_models(value) - self._token_stats = {} - self._token_stats_dirty = True @property def current_model(self) -> str: diff --git a/src/shell_runner.py b/src/shell_runner.py index 8a8d2cc..cf2f4b5 100644 --- a/src/shell_runner.py +++ b/src/shell_runner.py @@ -15,6 +15,10 @@ _ENV_CONFIG: dict = {} def _load_env_config() -> dict: """Load mcp_env.toml from project root (sibling of this file or parent dir).""" + env_path = os.environ.get("SLOP_MCP_ENV") + if env_path and Path(env_path).exists(): + with open(env_path, "rb") as f: + return tomllib.load(f) candidates = [ Path(__file__).parent / "mcp_env.toml", Path(__file__).parent.parent / "mcp_env.toml", diff --git a/temp_conftest.py b/temp_conftest.py new file mode 100644 index 0000000..7138a3d Binary files /dev/null and b/temp_conftest.py differ diff --git a/temp_conftest2.py b/temp_conftest2.py new file mode 100644 index 0000000..7138a3d Binary files /dev/null and b/temp_conftest2.py differ diff --git a/temp_gui_old.py b/temp_gui_old.py new file mode 100644 index 0000000..4b27e69 Binary files /dev/null and b/temp_gui_old.py differ diff --git a/temp_gui_old2.py b/temp_gui_old2.py new file mode 100644 index 0000000..4b27e69 Binary files /dev/null and b/temp_gui_old2.py differ diff --git a/tests/conftest.py b/tests/conftest.py index 1844f74..1ff12b7 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -189,6 +189,11 @@ def live_gui() -> Generator[tuple[subprocess.Popen, str], None, None]: (temp_workspace / "manual_slop.toml").write_text("[project]\nname = 'TestProject'\n", encoding="utf-8") (temp_workspace / "conductor" / "tracks").mkdir(parents=True, exist_ok=True) + # Resolve absolute paths for shared resources + project_root = Path(os.getcwd()) + cred_file = project_root / "credentials.toml" + mcp_file = project_root / "mcp_env.toml" + # Preserve GUI layout for tests layout_file = Path("manualslop_layout.ini") if layout_file.exists(): @@ -209,7 +214,11 @@ def live_gui() -> Generator[tuple[subprocess.Popen, str], None, None]: # Use environment variable to point to temp config if App supports it, # or just run from that CWD. env = os.environ.copy() - env["PYTHONPATH"] = os.getcwd() + env["PYTHONPATH"] = str(project_root.absolute()) + if cred_file.exists(): + env["SLOP_CREDENTIALS"] = str(cred_file.absolute()) + if mcp_file.exists(): + env["SLOP_MCP_ENV"] = str(mcp_file.absolute()) process = subprocess.Popen( ["uv", "run", "python", "-u", gui_script, "--enable-test-hooks"], diff --git a/tests/test_api_hook_extensions.py b/tests/test_api_hook_extensions.py index a8efbd6..4f2c9ba 100644 --- a/tests/test_api_hook_extensions.py +++ b/tests/test_api_hook_extensions.py @@ -34,11 +34,11 @@ def test_get_indicator_state_integration(live_gui: Any) -> None: def test_app_processes_new_actions() -> None: import gui_2 - with patch('gui_2.load_config', return_value={}), \ + with patch('src.models.load_config', return_value={}), \ patch('gui_2.PerformanceMonitor'), \ patch('gui_2.session_logger'), \ - patch.object(gui_2.App, '_prune_old_logs'), \ - patch.object(gui_2.App, '_load_active_project'): + patch('src.app_controller.AppController._prune_old_logs'), \ + patch('src.app_controller.AppController._load_active_project'): app = gui_2.App() # Test set_value via _pending_gui_tasks # First we need to register a settable field for testing if not present diff --git a/tests/test_gui2_events.py b/tests/test_gui2_events.py index 94cd625..9d5ce60 100644 --- a/tests/test_gui2_events.py +++ b/tests/test_gui2_events.py @@ -8,19 +8,19 @@ from events import EventEmitter @pytest.fixture def app_instance() -> Generator[type[App], None, None]: """ - Fixture to create an instance of the gui_2.App class for testing. - It mocks functions that would render a window or block execution. - """ + Fixture to create an instance of the gui_2.App class for testing. + It mocks functions that would render a window or block execution. + """ if not hasattr(ai_client, 'events') or ai_client.events is None: ai_client.events = EventEmitter() with ( - patch('gui_2.load_config', return_value={'ai': {}, 'projects': {}}), + patch('src.models.load_config', return_value={'ai': {}, 'projects': {}}), patch('gui_2.save_config'), patch('gui_2.project_manager'), patch('gui_2.session_logger'), patch('gui_2.immapp.run'), - patch.object(App, '_load_active_project'), - patch.object(App, '_fetch_models'), + patch('src.app_controller.AppController._load_active_project'), + patch('src.app_controller.AppController._fetch_models'), patch.object(App, '_load_fonts'), patch.object(App, '_post_init') ): @@ -43,4 +43,4 @@ def test_app_subscribes_to_events(app_instance: type[App]) -> None: for call in calls: handler = call.args[1] assert hasattr(handler, '__self__') - assert handler.__self__ is app + assert handler.__self__ is app.controller diff --git a/tests/test_gui_events.py b/tests/test_gui_events.py index e6a870b..12c31f0 100644 --- a/tests/test_gui_events.py +++ b/tests/test_gui_events.py @@ -9,7 +9,7 @@ sys.path.append(os.path.abspath(os.path.join(os.path.dirname(__file__), "..", "s def test_gui_updates_on_event(app_instance: App) -> None: app_instance.last_md = "mock_md" - with patch.object(app_instance, '_refresh_api_metrics') as mock_refresh: + with patch.object(app_instance.controller, '_refresh_api_metrics') as mock_refresh: # Simulate event (bypassing events.emit since _init_ai_and_hooks is mocked) app_instance._on_api_event(payload={"text": "test"}) # Process tasks manually diff --git a/tests/test_headless_service.py b/tests/test_headless_service.py index 34de6c2..b92bb2d 100644 --- a/tests/test_headless_service.py +++ b/tests/test_headless_service.py @@ -9,10 +9,15 @@ from fastapi.testclient import TestClient class TestHeadlessAPI(unittest.TestCase): def setUp(self) -> None: - with patch('gui_2.session_logger.open_session'), \ + with patch('src.models.load_config', return_value={'ai': {'provider': 'gemini', 'model': 'gemini-2.5-flash-lite'}, 'projects': {}, 'gui': {'show_windows': {}}}), \ + patch('gui_2.session_logger.open_session'), \ patch('gui_2.ai_client.set_provider'), \ patch('gui_2.PerformanceMonitor'), \ - patch('gui_2.session_logger.close_session'): + patch('gui_2.session_logger.close_session'), \ + patch('src.app_controller.AppController._init_ai_and_hooks'), \ + patch('src.app_controller.AppController._fetch_models'), \ + patch('src.app_controller.AppController._prune_old_logs'), \ + patch('src.app_controller.AppController.start_services'): self.app_instance = gui_2.App() # Set a default API key for tests self.test_api_key = "test-secret-key" diff --git a/tests/test_layout_reorganization.py b/tests/test_layout_reorganization.py index 499b9ca..26e7165 100644 --- a/tests/test_layout_reorganization.py +++ b/tests/test_layout_reorganization.py @@ -41,7 +41,11 @@ def test_old_windows_removed_from_gui2(app_instance_simple: Any) -> None: def app_instance_simple() -> Any: from unittest.mock import patch from gui_2 import App - with patch('gui_2.load_config', return_value={}): + with patch('src.models.load_config', return_value={'ai': {}, 'projects': {}, 'gui': {'show_windows': {}}}), \ + patch('src.app_controller.AppController._init_ai_and_hooks'), \ + patch('src.app_controller.AppController._fetch_models'), \ + patch('src.app_controller.AppController._prune_old_logs'), \ + patch('src.app_controller.AppController.start_services'): app = App() return app diff --git a/tests/test_mma_dashboard_refresh.py b/tests/test_mma_dashboard_refresh.py index d7c4bdd..ce2eb11 100644 --- a/tests/test_mma_dashboard_refresh.py +++ b/tests/test_mma_dashboard_refresh.py @@ -7,15 +7,18 @@ from gui_2 import App def app_instance() -> Any: # We patch the dependencies of App.__init__ to avoid side effects with ( - patch('gui_2.load_config', return_value={'ai': {}, 'projects': {}}), + patch('src.models.load_config', return_value={'ai': {}, 'projects': {}}), patch('gui_2.save_config'), patch('gui_2.project_manager') as mock_pm, patch('gui_2.session_logger'), patch('gui_2.immapp.run'), - patch.object(App, '_load_active_project'), - patch.object(App, '_fetch_models'), + patch('src.app_controller.AppController._load_active_project'), + patch('src.app_controller.AppController._fetch_models'), patch.object(App, '_load_fonts'), - patch.object(App, '_post_init') + patch.object(App, '_post_init'), + patch('src.app_controller.AppController._prune_old_logs'), + patch('src.app_controller.AppController.start_services'), + patch('src.app_controller.AppController._init_ai_and_hooks') ): app = App() # Ensure project and ui_files_base_dir are set for _refresh_from_project diff --git a/tests/test_process_pending_gui_tasks.py b/tests/test_process_pending_gui_tasks.py index 14bacc4..eee4253 100644 --- a/tests/test_process_pending_gui_tasks.py +++ b/tests/test_process_pending_gui_tasks.py @@ -7,15 +7,18 @@ from gui_2 import App @pytest.fixture def app_instance() -> Generator[App, None, None]: with ( - patch('gui_2.load_config', return_value={'ai': {'provider': 'gemini', 'model': 'gemini-2.5-flash-lite'}, 'projects': {}}), + patch('src.models.load_config', return_value={'ai': {'provider': 'gemini', 'model': 'gemini-2.5-flash-lite'}, 'projects': {}}), patch('gui_2.save_config'), patch('gui_2.project_manager'), patch('gui_2.session_logger'), patch('gui_2.immapp.run'), - patch.object(App, '_load_active_project'), - patch.object(App, '_fetch_models'), + patch('src.app_controller.AppController._load_active_project'), + patch('src.app_controller.AppController._fetch_models'), patch.object(App, '_load_fonts'), patch.object(App, '_post_init'), + patch('src.app_controller.AppController._prune_old_logs'), + patch('src.app_controller.AppController.start_services'), + patch('src.app_controller.AppController._init_ai_and_hooks'), patch('ai_client.set_provider'), patch('ai_client.reset_session') ):