fix: address reviewer feedback - add error handling for invalid timeout env var

- Add try/catch for float() conversion to prevent ValueError crashes
- Prevent real server spawning in unit tests via Thread.start mocking
- Add test for invalid env var fallback behavior
- Preserve other env vars during testing (avoid clear=True)

Addresses feedback from Gemini, CodeRabbit, Qodo, and Copilot reviewers.

Co-authored-by: Mervin Praison <MervinPraison@users.noreply.github.com>

Author: praisonai-triage-agent[bot]

src/praisonai-agents/praisonaiagents/agents/agents.py

@@ -128,7 +128,11 @@ def run_server():
         thread.start()
 
         # Check for configurable timeout via environment variable
-        timeout = float(os.environ.get("PRAISONAI_SERVER_READY_TIMEOUT", "5.0"))
+        try:
+            timeout = float(os.environ.get("PRAISONAI_SERVER_READY_TIMEOUT", "5.0"))
+        except ValueError:
+            logger.warning("Invalid PRAISONAI_SERVER_READY_TIMEOUT value. Using default 5.0s.")
+            timeout = 5.0
         became_ready = ready_event.wait(timeout=timeout)
 
         if not became_ready:

src/praisonai-agents/tests/unit/agents/test_server_registry_timeout.py

@@ -18,15 +18,16 @@ def test_server_readiness_timeout_emits_warning(caplog):
     with patch.dict(registry._apps, {8000: mock_app}):
         with patch.dict(registry._ready_events, {8000: threading.Event()}):
             with patch.dict(registry._started, {8000: False}):
-                with caplog.at_level(logging.WARNING):
-                    # Use a very short timeout to ensure the test doesn't hang
-                    with patch.dict("os.environ", {"PRAISONAI_SERVER_READY_TIMEOUT": "0.1"}):
-                        result = registry.start_server_if_needed(8000, host="127.0.0.1")
-                
-                # The method should still return True but log a warning
-                assert result is True
-                assert "8000" in caplog.text
-                assert "did not become ready" in caplog.text.lower()
+                with patch("threading.Thread.start"):  # Prevent real server spawning
+                    with caplog.at_level(logging.WARNING):
+                        # Use a very short timeout to ensure the test doesn't hang
+                        with patch.dict("os.environ", {"PRAISONAI_SERVER_READY_TIMEOUT": "0.1"}):
+                            result = registry.start_server_if_needed(8000, host="127.0.0.1")
+                    
+                    # The method should still return True but log a warning
+                    assert result is True
+                    assert "8000" in caplog.text
+                    assert "did not become ready" in caplog.text.lower()
 
 
 def test_server_readiness_timeout_configurable_via_env():
@@ -40,16 +41,17 @@ def test_server_readiness_timeout_configurable_via_env():
     with patch.dict(registry._apps, {8001: mock_app}):
         with patch.dict(registry._ready_events, {8001: threading.Event()}):
             with patch.dict(registry._started, {8001: False}):
-                # Test with custom timeout
-                with patch.dict("os.environ", {"PRAISONAI_SERVER_READY_TIMEOUT": "0.05"}):
-                    import time
-                    start_time = time.time()
-                    result = registry.start_server_if_needed(8001, host="127.0.0.1")
-                    duration = time.time() - start_time
-                
-                # Should respect the custom timeout and complete quickly
-                assert result is True
-                assert duration < 1.0  # Should be much less than default 5s
+                with patch("threading.Thread.start"):  # Prevent real server spawning
+                    # Test with custom timeout
+                    with patch.dict("os.environ", {"PRAISONAI_SERVER_READY_TIMEOUT": "0.05"}):
+                        import time
+                        start_time = time.time()
+                        result = registry.start_server_if_needed(8001, host="127.0.0.1")
+                        duration = time.time() - start_time
+                    
+                    # Should respect the custom timeout and complete quickly
+                    assert result is True
+                    assert duration < 1.0  # Should be much less than default 5s
 
 
 def test_server_readiness_success_no_warning(caplog):
@@ -64,15 +66,16 @@ def test_server_readiness_success_no_warning(caplog):
     with patch.dict(registry._apps, {8002: mock_app}):
         with patch.dict(registry._ready_events, {8002: ready_event}):
             with patch.dict(registry._started, {8002: False}):
-                # Set the event immediately to simulate quick startup
-                ready_event.set()
-                
-                with caplog.at_level(logging.WARNING):
-                    result = registry.start_server_if_needed(8002, host="127.0.0.1")
-                
-                # Should succeed without warnings
-                assert result is True
-                assert "did not become ready" not in caplog.text.lower()
+                with patch("threading.Thread.start"):  # Prevent real server spawning
+                    # Set the event immediately to simulate quick startup
+                    ready_event.set()
+                    
+                    with caplog.at_level(logging.WARNING):
+                        result = registry.start_server_if_needed(8002, host="127.0.0.1")
+                    
+                    # Should succeed without warnings
+                    assert result is True
+                    assert "did not become ready" not in caplog.text.lower()
 
 
 def test_default_timeout_value():
@@ -87,12 +90,39 @@ def test_default_timeout_value():
     with patch.dict(registry._apps, {8003: mock_app}):
         with patch.dict(registry._ready_events, {8003: ready_event}):
             with patch.dict(registry._started, {8003: False}):
-                # Ensure no custom timeout is set
-                with patch.dict("os.environ", {}, clear=True):
-                    with patch("threading.Event.wait") as mock_wait:
-                        mock_wait.return_value = False
-                        result = registry.start_server_if_needed(8003, host="127.0.0.1")
-                
-                # Should have called wait with default timeout of 5.0
-                mock_wait.assert_called_once_with(timeout=5.0)
-                assert result is True
+                with patch("threading.Thread.start"):  # Prevent real server spawning
+                    # Ensure no custom timeout is set (but preserve other env vars)
+                    with patch.dict("os.environ", {"PRAISONAI_SERVER_READY_TIMEOUT": ""}, clear=False):
+                        with patch("threading.Event.wait") as mock_wait:
+                            mock_wait.return_value = False
+                            result = registry.start_server_if_needed(8003, host="127.0.0.1")
+                    
+                    # Should have called wait with default timeout of 5.0
+                    mock_wait.assert_called_once_with(timeout=5.0)
+                    assert result is True
+
+
+def test_invalid_timeout_env_var_fallback(caplog):
+    """Test that invalid PRAISONAI_SERVER_READY_TIMEOUT falls back to default with warning."""
+    registry = _AgentServerRegistry()
+    
+    # Create a mock FastAPI app
+    mock_app = MagicMock()
+    ready_event = threading.Event()
+    
+    # Register the app but don't set the ready event
+    with patch.dict(registry._apps, {8004: mock_app}):
+        with patch.dict(registry._ready_events, {8004: ready_event}):
+            with patch.dict(registry._started, {8004: False}):
+                with patch("threading.Thread.start"):  # Prevent real server spawning
+                    # Test with invalid timeout value
+                    with patch.dict("os.environ", {"PRAISONAI_SERVER_READY_TIMEOUT": "invalid"}):
+                        with caplog.at_level(logging.WARNING):
+                            with patch("threading.Event.wait") as mock_wait:
+                                mock_wait.return_value = False
+                                result = registry.start_server_if_needed(8004, host="127.0.0.1")
+                    
+                    # Should have fallen back to 5.0 and logged warning
+                    mock_wait.assert_called_once_with(timeout=5.0)
+                    assert result is True
+                    assert "Invalid PRAISONAI_SERVER_READY_TIMEOUT" in caplog.text