chore(P3): eliminate all actionable warnings + thread-safe close() (#27)

Warning cleanup (54 → 44, remaining are P4 architecture file-size):
- DeprecationWarning: wrap set_schema_version tests with deprecated_call()
- ResourceWarning: atexit cleanup for mcp_server file handles
- RuntimeWarning: MagicMock for sync is_initialized() (was AsyncMock)

Thread safety:
- MigrationManager.close(): _closed + _conn.close() both under mutex
- Eliminates TOCTOU race window

Test hygiene:
- Architecture boundary allowlist updated for mcp_server.py line shifts
- 31 skipped tests audited: 8 Windows/symlink, 23 missing deps (all valid)

Verified by 3-agent adversarial review (OOP v4.2.0):
- /8eyes (Opus 4.6): SHIP 0.95 — 5/5 attacks repelled
- /collab (GPT-5.4): SHIP after close() TOCTOU fix
- code-review (Sonnet): APPROVED

1,351 tests passing, 44 warnings (all P4 architecture scope).

Co-authored-by: Brian Krafft <bkrafft@microsoft.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Author: 3 people

openspace/mcp_server.py

@@ -17,6 +17,7 @@
 from __future__ import annotations
 
 import asyncio
+import atexit
 import inspect
 import json
 import logging
@@ -88,19 +89,36 @@ def __getattr__(self, name):
 # drain stderr. Heavy log/print output during execute_task fills the stderr
 # pipe buffer, blocking this process on write() → deadlock → timeout.
 # Redirect stderr to a log file on Windows to prevent this.
+_stderr_file = None
 if os.name == "nt":
     _stderr_file = open(_LOG_DIR / "mcp_stderr.log", "a", encoding="utf-8", buffering=1)
     sys.stderr = _stderr_file
 
 sys.stdout = _MCPSafeStdout(_real_stdout, sys.stderr)
 
+# Set up file handler for logging
+_log_file_handler = logging.FileHandler(_LOG_DIR / "mcp_server.log")
 logging.basicConfig(
     level=logging.INFO,
     format="%(asctime)s - %(name)s - %(levelname)s - %(message)s",
-    handlers=[logging.FileHandler(_LOG_DIR / "mcp_server.log")],
+    handlers=[_log_file_handler],
 )
 logger = logging.getLogger("openspace.mcp_server")
 
+
+def _cleanup_file_handles():
+    """Clean up file handles on exit."""
+    global _stderr_file, _log_file_handler
+    if _stderr_file:
+        _stderr_file.close()
+        _stderr_file = None
+    if _log_file_handler:
+        _log_file_handler.close()
+
+
+# Register cleanup handler
+atexit.register(_cleanup_file_handles)
+
 from mcp.server.fastmcp import FastMCP
 
 _fastmcp_kwargs: dict = {}

openspace/skill_engine/migration_manager.py

@@ -226,15 +226,16 @@ def _ensure_open(self) -> None:
 
     def close(self) -> None:
         """Close the connection if we own it."""
-        if self._closed:
-            return
-
-        if self._owns_conn:
-            self._closed = True
-            try:
-                self._conn.close()
-            except Exception:
-                pass
+        with self._mu:
+            if self._closed:
+                return
+            # Only set _closed and close conn if we own the connection
+            if self._owns_conn:
+                self._closed = True
+                try:
+                    self._conn.close()
+                except Exception:
+                    pass
 
         logger.debug("MigrationManager closed")
 

tests/test_architecture_boundaries.py

@@ -268,15 +268,15 @@ def test_mcp_handler_files_exist(self) -> None:
     # public property accessors as delegation is fully wired in Phase 4).
     _KNOWN_PRIVATE_ACCESS: frozenset[tuple[str, int, str]] = frozenset(
         {
-            ("openspace/mcp_server.py", 185, "_skill_store"),
-            ("openspace/mcp_server.py", 203, "_grounding_config"),
-            ("openspace/mcp_server.py", 296, "_skill_registry"),
-            ("openspace/mcp_server.py", 430, "_skill_registry"),
-            ("openspace/mcp_server.py", 734, "_skill_registry"),
-            ("openspace/mcp_server.py", 635, "_skill_registry"),
-            ("openspace/mcp_server.py", 753, "_skill_evolver"),
-            ("openspace/mcp_server.py", 737, "_skill_evolver"),
-            ("openspace/mcp_server.py", 419, "_grounding_config"),
+            ("openspace/mcp_server.py", 203, "_skill_store"),
+            ("openspace/mcp_server.py", 221, "_grounding_config"),
+            ("openspace/mcp_server.py", 314, "_skill_registry"),
+            ("openspace/mcp_server.py", 448, "_skill_registry"),
+            ("openspace/mcp_server.py", 752, "_skill_registry"),
+            ("openspace/mcp_server.py", 653, "_skill_registry"),
+            ("openspace/mcp_server.py", 771, "_skill_evolver"),
+            ("openspace/mcp_server.py", 755, "_skill_evolver"),
+            ("openspace/mcp_server.py", 437, "_grounding_config"),
         }
     )
 

tests/test_migration_manager.py

@@ -189,25 +189,29 @@ def test_initial_schema_version_is_zero(self, migration_manager):
 
     def test_set_and_get_schema_version(self, migration_manager):
         """Test setting and retrieving schema versions."""
-        migration_manager.set_schema_version(5)
+        with pytest.deprecated_call():
+            migration_manager.set_schema_version(5)
         version = migration_manager.get_schema_version()
         assert version == 5
 
-        migration_manager.set_schema_version(10)
+        with pytest.deprecated_call():
+            migration_manager.set_schema_version(10)
         version = migration_manager.get_schema_version()
         assert version == 10
 
     def test_set_negative_version_fails(self, migration_manager):
         """Test that setting negative version raises error."""
         with pytest.raises(ValueError, match="version must be non-negative"):
-            migration_manager.set_schema_version(-1)
+            with pytest.deprecated_call():
+                migration_manager.set_schema_version(-1)
 
     def test_version_persists_across_connections(self, temp_db_path):
         """Test that schema version persists when reopening database."""
         # Set version in first instance
         manager1 = MigrationManager(db_path=temp_db_path)
         manager1.initialize_schema()  # Create tables first
-        manager1.set_schema_version(7)
+        with pytest.deprecated_call():
+            manager1.set_schema_version(7)
         manager1.close()
 
         # Verify version persists in second instance
@@ -240,22 +244,25 @@ def test_migrate_from_zero_to_one(self, migration_manager):
 
     def test_migrate_same_version_is_noop(self, migration_manager):
         """Test that migrating to current version is no-op."""
-        migration_manager.set_schema_version(3)
+        with pytest.deprecated_call():
+            migration_manager.set_schema_version(3)
 
         # Should not raise error and version should remain unchanged
         migration_manager.migrate_to_version(3)
         assert migration_manager.get_schema_version() == 3
 
     def test_migrate_to_lower_version_fails(self, migration_manager):
         """Test that downgrading version is not allowed."""
-        migration_manager.set_schema_version(5)
+        with pytest.deprecated_call():
+            migration_manager.set_schema_version(5)
 
         with pytest.raises(ValueError, match="Cannot downgrade from version 5 to 3"):
             migration_manager.migrate_to_version(3)
 
     def test_unsupported_migration_path_fails(self, migration_manager):
         """Test that unsupported migration paths raise errors."""
-        migration_manager.set_schema_version(2)
+        with pytest.deprecated_call():
+            migration_manager.set_schema_version(2)
 
         with pytest.raises(RuntimeError, match="Migration from version 2 to 5 not supported"):
             migration_manager.migrate_to_version(5)
@@ -272,7 +279,8 @@ def test_ensure_current_schema_migrates_when_needed(self, migration_manager):
 
     def test_ensure_current_schema_handles_newer_version(self, migration_manager):
         """Test warning when database is newer than expected."""
-        migration_manager.set_schema_version(5)
+        with pytest.deprecated_call():
+            migration_manager.set_schema_version(5)
 
         # Should log warning but not fail
         with patch('openspace.skill_engine.migration_manager.logger') as mock_logger:
@@ -340,17 +348,22 @@ def test_operations_fail_when_closed(self, migration_manager):
         """Test that operations fail after manager is closed."""
         migration_manager.close()
 
-        operations = [
-            lambda: migration_manager.initialize_schema(),
-            lambda: migration_manager.get_schema_version(),
-            lambda: migration_manager.set_schema_version(1),
-            lambda: migration_manager.migrate_to_version(1),
-            lambda: migration_manager.ensure_current_schema(1),
-        ]
-        
-        for operation in operations:
-            with pytest.raises(RuntimeError, match="MigrationManager is closed"):
-                operation()
+        # Test each operation individually
+        with pytest.raises(RuntimeError, match="MigrationManager is closed"):
+            migration_manager.initialize_schema()
+            
+        with pytest.raises(RuntimeError, match="MigrationManager is closed"):
+            migration_manager.get_schema_version()
+            
+        with pytest.raises(RuntimeError, match="MigrationManager is closed"):
+            with pytest.deprecated_call():
+                migration_manager.set_schema_version(1)
+                
+        with pytest.raises(RuntimeError, match="MigrationManager is closed"):
+            migration_manager.migrate_to_version(1)
+            
+        with pytest.raises(RuntimeError, match="MigrationManager is closed"):
+            migration_manager.ensure_current_schema(1)
 
 
 class TestSkillStoreFacadeIntegration:

tests/test_traceback_safety.py

@@ -218,7 +218,7 @@ def test_correlation_id_in_log_and_response(self):
 
 def _make_openspace_mock():
     """Create a mock OpenSpace instance sufficient for mcp_server imports."""
-    mock = AsyncMock()
+    mock = MagicMock()
     mock.is_initialized.return_value = True
     mock._skill_registry = MagicMock()
     mock._skill_evolver = MagicMock()