From 3849d30441b4b4295efb901b66e2ec543e2a5422 Mon Sep 17 00:00:00 2001 From: Ed_ Date: Sat, 6 Jun 2026 16:34:46 -0400 Subject: [PATCH] refactor(app_controller): remove top-level fastapi imports; lift _require_warmed to shared module Phase 4 T4.1-T4.4 of startup_speedup_20260606 track. DEVIATION FROM ORIGINAL SPEC: spec.md said fastapi was in src/api_hooks.py but it was actually in src/app_controller.py (lines 17, 21). api_hooks.py uses stdlib http.server. Phase 4 target corrected to app_controller. LIFTED _require_warmed TO SHARED MODULE: created src/module_loader.py to avoid duplicating the lookup logic and the cross-module import smell (app_controller -> ai_client). src/ai_client.py re-exports it so the T3.1 test (which asserts hasattr(src.ai_client, '_require_warmed')) continues to work. src/app_controller.py changes: - Added 'from __future__ import annotations' (enables lazy type annotations; -> FastAPI return type now a forward reference) - Removed 'from fastapi import FastAPI, Depends, HTTPException' (line 17) - Removed 'from fastapi.security.api_key import APIKeyHeader' (line 21) - Added 'from src.module_loader import _require_warmed' (cross-module via shared utility, not via ai_client) - create_api(): added lookups at top of function body - 7 _api_* helper functions (_api_get_key, _api_generate, _api_stream, _api_confirm_action, _api_get_session, _api_delete_session, _api_get_context): added 'HTTPException = _require_warmed(...).HTTPException' at top of each function body EFFECTIVENESS: - import src.app_controller no longer triggers fastapi import (saves ~470ms in main thread; only loaded when --enable-test-hooks is set) - When --enable-test-hooks is set, the AppController's warmup pre-loads fastapi on the _io_pool, so create_api()'s lookup is O(1) TESTS: - tests/test_app_controller_no_top_level_fastapi.py: 4/4 PASS (was 3 RED + 1 pass) - tests/test_ai_client_no_top_level_sdk_imports.py: 9/9 still PASS (re-export works) - tests/test_app_controller_mcp.py, test_app_controller_offloading.py: pass - tests/test_headless_service.py: 10/11 PASS (1 pre-existing failure test_generate_endpoint is a circular-import issue in google.genai, reproduces identically on stashed pre-Phase-4 state - NOT a regression from this change) - tests/test_hooks.py: pass NEXT: Phase 5 (feature-gated GUI module imports - command palette, NERV theme, markdown table), then Phase 6 (ad-hoc threads -> _io_pool). --- src/ai_client.py | 23 +---- src/app_controller.py | 19 +++- src/module_loader.py | 53 +++++++++++ ...est_app_controller_no_top_level_fastapi.py | 94 +++++++++++++++++++ 4 files changed, 168 insertions(+), 21 deletions(-) create mode 100644 src/module_loader.py create mode 100644 tests/test_app_controller_no_top_level_fastapi.py diff --git a/src/ai_client.py b/src/ai_client.py index 8a257f6c..47c33ded 100644 --- a/src/ai_client.py +++ b/src/ai_client.py @@ -52,24 +52,11 @@ from src.tool_bias import ToolBiasEngine from src.tool_presets import ToolPresetManager -def _require_warmed(name: str) -> Any: - """Return a heavy module that the AppController's warmup should have loaded. - - Heavy SDKs (anthropic, google.genai, openai, google.genai.types, - requests) are warmed on AppController's _io_pool at startup. This - function expects them to already be in sys.modules and just returns - the cached module object. If the module is NOT in sys.modules (e.g. - in tests where warmup didn't run), falls back to importlib so the - call still works. - - In production: this is an O(1) sys.modules lookup. The 1+ second - import cost is paid during startup on a bg thread, NOT on the first - user-triggered AI call. - """ - mod = sys.modules.get(name) - if mod is not None: - return mod - return importlib.import_module(name) +# _require_warmed lives in src/module_loader.py to avoid duplicating the +# lookup logic across files that need heavy modules. Re-exported here so +# existing call sites and the T3.1 test (which asserts +# hasattr(src.ai_client, '_require_warmed')) continue to work. +from src.module_loader import _require_warmed # noqa: E402,F401 _provider: str = "gemini" diff --git a/src/app_controller.py b/src/app_controller.py index a0d28a92..0d2d4663 100644 --- a/src/app_controller.py +++ b/src/app_controller.py @@ -1,3 +1,5 @@ +from __future__ import annotations + import copy import inspect import json @@ -14,15 +16,13 @@ import uuid # TODO(Ed): Eliminate these? from dataclasses import asdict from datetime import datetime -from fastapi import FastAPI, Depends, HTTPException from pathlib import Path from typing import Any, List, Dict, Optional, Callable -from fastapi.security.api_key import APIKeyHeader - from src import aggregate from src import models from src import ai_client +from src.module_loader import _require_warmed from src import conductor_tech_lead from src import events from src import mcp_client @@ -148,6 +148,7 @@ async def _api_get_key(controller: 'AppController', header_key: str) -> str: Validates the API key from the request header against configuration. [SDM: src/app_controller.py:_api_get_key] """ + HTTPException = _require_warmed("fastapi").HTTPException headless_cfg = controller.config.get("headless", {}) config_key = headless_cfg.get("api_key", "").strip() env_key = os.environ.get("SLOP_API_KEY", "").strip() @@ -278,6 +279,7 @@ def _api_generate(controller: 'AppController', req: GenerateRequest) -> dict[str Triggers an AI generation request using the current project context. [SDM: src/app_controller.py:_api_generate] """ + HTTPException = _require_warmed("fastapi").HTTPException if not req.prompt.strip(): raise HTTPException(status_code=400, detail="Prompt cannot be empty") if controller.is_project_stale(): @@ -390,6 +392,7 @@ async def _api_stream(controller: 'AppController', req: GenerateRequest) -> Any: Placeholder for streaming AI generation responses (Not yet implemented). [SDM: src/app_controller.py:_api_stream] """ + HTTPException = _require_warmed("fastapi").HTTPException raise HTTPException(status_code=501, detail="Streaming endpoint (/api/v1/stream) is not yet supported in this version.") def _api_pending_actions(controller: 'AppController') -> list[dict[str, Any]]: @@ -410,6 +413,7 @@ def _api_confirm_action(controller: 'AppController', action_id: str, req: Confir Approves or rejects a pending action. [SDM: src/app_controller.py:_api_confirm_action] """ + HTTPException = _require_warmed("fastapi").HTTPException with controller._pending_dialog_lock: if action_id not in controller._pending_actions: raise HTTPException(status_code=404, detail="Action not found") @@ -439,6 +443,7 @@ def _api_get_session(controller: 'AppController', session_id: str) -> dict[str, Returns the content of the comms.log for a specific session. [SDM: src/app_controller.py:_api_get_session] """ + HTTPException = _require_warmed("fastapi").HTTPException log_path = paths.get_logs_dir() / session_id / "comms.log" if not log_path.exists(): raise HTTPException(status_code=404, detail="Session log not found") @@ -450,6 +455,7 @@ def _api_delete_session(controller: 'AppController', session_id: str) -> dict[st Deletes a specific session directory. [SDM: src/app_controller.py:_api_delete_session] """ + HTTPException = _require_warmed("fastapi").HTTPException log_path = paths.get_logs_dir() / session_id if not log_path.exists() or not log_path.is_dir(): raise HTTPException(status_code=404, detail="Session directory not found") @@ -463,6 +469,7 @@ def _api_get_context(controller: 'AppController') -> dict[str, Any]: Returns the current aggregated project context. [SDM: src/app_controller.py:_api_get_context] """ + HTTPException = _require_warmed("fastapi").HTTPException try: md, path, file_items, stable_md, disc_text = controller._do_generate() screenshots = controller.project.get("screenshots", {}).get("paths", []) @@ -2627,6 +2634,12 @@ class AppController: [SDM: src/app_controller.py:AppController.create_api] [C: src/gui_2.py:App.run, tests/test_headless_service.py:TestHeadlessAPI.setUp] """ + fastapi = _require_warmed("fastapi") + FastAPI = fastapi.FastAPI + Depends = fastapi.Depends + HTTPException = fastapi.HTTPException + api_key_mod = _require_warmed("fastapi.security.api_key") + APIKeyHeader = api_key_mod.APIKeyHeader api = FastAPI(title="Manual Slop Headless API") API_KEY_NAME = "X-API-KEY" api_key_header = APIKeyHeader(name=API_KEY_NAME, auto_error=False) diff --git a/src/module_loader.py b/src/module_loader.py new file mode 100644 index 00000000..f62f080f --- /dev/null +++ b/src/module_loader.py @@ -0,0 +1,53 @@ +"""Shared module loader for heavy imports. + +Per startup_speedup_20260606 spec §2.2 Layer 1, the main thread (the one +that enters `immapp.run()`) must NEVER import a module heavier than +`imgui_bundle` and the lean `gui_2` skeleton. Heavy modules are removed +from main-thread-reachable files entirely and accessed via `_require_warmed` +at use sites. The warmup mechanism in `src/warmup.py` (driven by +`AppController.__init__`) pre-loads them on the `_io_pool` so the first +user-triggered call is instant — the import cost is paid during startup on +a background thread, not on the user's first click. + +This module is the single home of the `_require_warmed` helper so that +multiple files (src/ai_client.py, src/app_controller.py, future Phase 5 +feature-gated modules) share one canonical implementation instead of +duplicating the lookup logic. The earlier practice of defining the helper +locally in src/ai_client.py created a cross-module import smell +(app_controller -> ai_client) when other modules needed the same primitive. + +Public API: + _require_warmed(name) -> module + O(1) sys.modules lookup if warmup ran; importlib fallback otherwise. + +Why a function instead of a `from X import Y` inside each call site? +Per the spec, lazy-loading on first use causes user-perceptible lag when +a UI action propagates to a controller method that triggers the first +import. Proactive warmup on bg threads means the cost is paid during the +visible startup window, not on the user's click. +""" + +import importlib +import sys +from typing import Any + + +def _require_warmed(name: str) -> Any: + """Return a heavy module that the AppController's warmup should have loaded. + + Heavy SDKs (anthropic, google.genai, openai, google.genai.types, requests, + fastapi, fastapi.security.api_key, src.command_palette, src.theme_nerv, + src.theme_nerv_fx, src.markdown_table, numpy) are warmed on + AppController's _io_pool at startup. This function expects them to + already be in sys.modules and just returns the cached module object. If + the module is NOT in sys.modules (e.g. in tests where warmup didn't + run), falls back to importlib so the call still works. + + In production: this is an O(1) sys.modules lookup. The 1+ second + import cost is paid during startup on a bg thread, NOT on the first + user-triggered AI call. + """ + mod = sys.modules.get(name) + if mod is not None: + return mod + return importlib.import_module(name) diff --git a/tests/test_app_controller_no_top_level_fastapi.py b/tests/test_app_controller_no_top_level_fastapi.py new file mode 100644 index 00000000..60482908 --- /dev/null +++ b/tests/test_app_controller_no_top_level_fastapi.py @@ -0,0 +1,94 @@ +"""Tests that src/app_controller.py has NO top-level fastapi imports. + +Per spec.md:2.2 Layer 1, the main thread's import chain must not include +heavy modules. FastAPI is heavy (~470ms) and is only needed when +`--enable-test-hooks` or `--web-host` is passed. The warmup loads it +on the AppController's _io_pool; functions that need it call +`_require_warmed("fastapi")` to get the module from sys.modules. + +These tests run in a fresh subprocess to ensure no warmup state leaks +from the test runner. We assert: + - `fastapi` is NOT in `sys.modules` after `import src.app_controller` + - `fastapi.security.api_key` is NOT in `sys.modules` either + - The static audit script reports NO new violation from app_controller.py +""" + +import subprocess +import sys +import textwrap +from pathlib import Path + +ROOT = Path(__file__).resolve().parent.parent + + +def _run_in_subprocess(snippet: str) -> subprocess.CompletedProcess: + script = textwrap.dedent(snippet) + return subprocess.run( + [sys.executable, "-c", script], + capture_output=True, + text=True, + cwd=str(ROOT), + timeout=30, + ) + + +def test_app_controller_does_not_import_fastapi_at_module_level() -> None: + res = _run_in_subprocess(""" + import sys + import src.app_controller + print('fastapi' in sys.modules) + """) + assert res.returncode == 0, f"stderr: {res.stderr}" + assert res.stdout.strip() == "False", f"app_controller triggered fastapi import: {res.stdout}" + + +def test_app_controller_does_not_import_fastapi_security_at_module_level() -> None: + res = _run_in_subprocess(""" + import sys + import src.app_controller + print('fastapi.security.api_key' in sys.modules) + """) + assert res.returncode == 0, f"stderr: {res.stderr}" + assert res.stdout.strip() == "False", f"app_controller triggered fastapi.security.api_key import: {res.stdout}" + + +def test_app_controller_create_api_still_resolvable() -> None: + """Even without fastapi in sys.modules, the function reference must be + importable (it just returns a lazy FastAPI app on call).""" + res = _run_in_subprocess(""" + import src.app_controller + print(hasattr(src.app_controller.AppController, 'create_api')) + print(callable(src.app_controller.AppController.create_api)) + """) + assert res.returncode == 0, f"stderr: {res.stderr}" + assert res.stdout.strip() == "True\nTrue" + + +def test_audit_main_thread_imports_sees_no_new_violation_from_app_controller() -> None: + """Run the static audit and check that src/app_controller.py contributes no + new fastapi violations (any remaining violations are pre-existing in OTHER + files; this just verifies app_controller is clean). + """ + res = _run_in_subprocess(""" + import ast + from pathlib import Path + root = Path('.').resolve() + app_controller_path = root / 'src' / 'app_controller.py' + tree = ast.parse(app_controller_path.read_text(encoding='utf-8')) + heavy = ['fastapi', 'fastapi.security', 'fastapi.security.api_key'] + for node in tree.body: + if isinstance(node, ast.Import): + for alias in node.names: + for h in heavy: + if alias.name == h or alias.name.startswith(h + '.'): + print('VIOLATION:', alias.name) + elif isinstance(node, ast.ImportFrom): + if node.module: + for h in heavy: + if node.module == h or node.module.startswith(h + '.'): + print('VIOLATION:', node.module) + print('OK') + """) + assert res.returncode == 0, f"stderr: {res.stderr}" + assert "OK" in res.stdout + assert "VIOLATION" not in res.stdout