address greptile review: short-secret guard, distinct test values, replace isinstance patch with stub subclass

litellm/secret_managers/hashicorp_secret_manager.py

@@ -538,8 +538,8 @@ class HashicorpSecretManager(BaseSecretManager):
                 )
                 if new_secret_value_from_vault != new_secret_value:
                     # Mask key values to avoid logging raw API keys
-                    masked_expected = f"...{new_secret_value[-4:]}" if new_secret_value else "<empty>"
-                    masked_got = f"...{new_secret_value_from_vault[-4:]}" if new_secret_value_from_vault else "<empty>"
+                    masked_expected = f"...{new_secret_value[-4:]}" if new_secret_value and len(new_secret_value) > 4 else "<redacted>"
+                    masked_got = f"...{new_secret_value_from_vault[-4:]}" if new_secret_value_from_vault and len(new_secret_value_from_vault) > 4 else "<redacted>"
                     verbose_logger.error(
                         "New secret value mismatch. Expected: %s, Got: %s",
                         masked_expected,

tests/litellm_utils_tests/test_hashicorp.py

@@ -737,7 +737,7 @@ async def test_hashicorp_secret_manager_rotate_secret_value_mismatch(hashicorp_s
         mock_get_response_new = MagicMock()
         mock_get_response_new.json.return_value = {
             "data": {
-                "data": {"key": "different-value"},  # Different from expected
+                "data": {"key": "different-ABCD"},  # Different from expected
             }
         }
         mock_get_response_new.raise_for_status.return_value = None
@@ -748,18 +748,18 @@ async def test_hashicorp_secret_manager_rotate_secret_value_mismatch(hashicorp_s
         
         current_secret_name = f"old-secret-{uuid.uuid4()}"
         new_secret_name = f"new-secret-{uuid.uuid4()}"
-        new_secret_value = "expected-value"
-        
+        new_secret_value = "expected-XYZW"
+
         response = await hashicorp_secret_manager.async_rotate_secret(
             current_secret_name=current_secret_name,
             new_secret_name=new_secret_name,
             new_secret_value=new_secret_value,
         )
-        
+
         # Verify error response
         assert response["status"] == "error"
         assert "mismatch" in response["message"].lower()
         # Raw key values should be masked — only last 4 chars shown
-        assert "expected-value" not in response["message"]
-        assert "...alue" in response["message"]  # last 4 of "expected-value"
-        assert "...alue" in response["message"]  # last 4 of "different-value"
+        assert "expected-XYZW" not in response["message"]
+        assert "...XYZW" in response["message"]  # last 4 of "expected-XYZW"
+        assert "...ABCD" in response["message"]  # last 4 of "different-ABCD"

tests/test_litellm/proxy/hooks/test_key_management_event_hooks.py

@@ -13,6 +13,7 @@ import pytest
 sys.path.insert(0, os.path.abspath("../../../.."))
 
 from litellm.proxy.hooks.key_management_event_hooks import KeyManagementEventHooks
+from litellm.secret_managers.base_secret_manager import BaseSecretManager
 
 
 class TestKeyManagementEventHooksIndependentOperations:
@@ -390,6 +391,28 @@ class TestRotateVirtualKeyInSecretManager:
         mock_secret_manager.async_rotate_secret.assert_not_called()
 
 
+class _StubSecretManager(BaseSecretManager):
+    """Minimal concrete BaseSecretManager for testing — passes isinstance checks naturally."""
+
+    def __init__(self):
+        pass
+
+    async def async_read_secret(self, secret_name, optional_params=None, timeout=None):
+        raise NotImplementedError
+
+    def sync_read_secret(self, secret_name, optional_params=None):
+        raise NotImplementedError
+
+    async def async_write_secret(self, secret_name, secret_value, description=None, optional_params=None):
+        raise NotImplementedError
+
+    async def async_delete_secret(self, secret_name, optional_params=None):
+        raise NotImplementedError
+
+    async def async_rotate_secret(self, current_secret_name, new_secret_name, new_secret_value, optional_params=None):
+        raise NotImplementedError
+
+
 class TestRotateVirtualKeyErrorHandling:
     """Tests that _rotate_virtual_key_in_secret_manager propagates error dicts as exceptions."""
 
@@ -397,36 +420,24 @@ class TestRotateVirtualKeyErrorHandling:
     async def test_rotate_raises_on_error_dict(self):
         """Test that error dict from async_rotate_secret is raised as ValueError."""
         from litellm.types.secret_managers.main import KeyManagementSystem, KeyManagementSettings
-        from litellm.secret_managers.base_secret_manager import BaseSecretManager
         import litellm
 
-        mock_secret_manager = MagicMock(spec=BaseSecretManager)
-        mock_secret_manager.async_rotate_secret = AsyncMock(
+        stub = _StubSecretManager()
+        stub.async_rotate_secret = AsyncMock(
             return_value={"status": "error", "message": "New secret value mismatch. Expected: ...3lcA, Got: ...zt4w"}
         )
 
-        litellm.secret_manager_client = mock_secret_manager
+        litellm.secret_manager_client = stub
         litellm._key_management_system = KeyManagementSystem.HASHICORP_VAULT
         litellm._key_management_settings = KeyManagementSettings(
             store_virtual_keys=True,
             prefix_for_stored_virtual_keys="litellm/",
         )
 
-        import builtins
-        original_isinstance = builtins.isinstance
-
-        def mock_isinstance(obj, cls):
-            if cls == BaseSecretManager and obj == mock_secret_manager:
-                return True
-            return original_isinstance(obj, cls)
-
         with patch.object(
             KeyManagementEventHooks,
             "_get_secret_manager_optional_params",
             return_value=None,
-        ), patch(
-            "litellm.proxy.hooks.key_management_event_hooks.isinstance",
-            side_effect=mock_isinstance,
         ):
             with pytest.raises(ValueError, match="Secret manager rotation failed"):
                 await KeyManagementEventHooks._rotate_virtual_key_in_secret_manager(
@@ -440,36 +451,24 @@ class TestRotateVirtualKeyErrorHandling:
     async def test_rotate_success_dict_does_not_raise(self):
         """Test that a success response from async_rotate_secret does not raise."""
         from litellm.types.secret_managers.main import KeyManagementSystem, KeyManagementSettings
-        from litellm.secret_managers.base_secret_manager import BaseSecretManager
         import litellm
 
-        mock_secret_manager = MagicMock(spec=BaseSecretManager)
-        mock_secret_manager.async_rotate_secret = AsyncMock(
+        stub = _StubSecretManager()
+        stub.async_rotate_secret = AsyncMock(
             return_value={"request_id": "abc", "data": {}}
         )
 
-        litellm.secret_manager_client = mock_secret_manager
+        litellm.secret_manager_client = stub
         litellm._key_management_system = KeyManagementSystem.HASHICORP_VAULT
         litellm._key_management_settings = KeyManagementSettings(
             store_virtual_keys=True,
             prefix_for_stored_virtual_keys="litellm/",
         )
 
-        import builtins
-        original_isinstance = builtins.isinstance
-
-        def mock_isinstance(obj, cls):
-            if cls == BaseSecretManager and obj == mock_secret_manager:
-                return True
-            return original_isinstance(obj, cls)
-
         with patch.object(
             KeyManagementEventHooks,
             "_get_secret_manager_optional_params",
             return_value=None,
-        ), patch(
-            "litellm.proxy.hooks.key_management_event_hooks.isinstance",
-            side_effect=mock_isinstance,
         ):
             # Should not raise
             await KeyManagementEventHooks._rotate_virtual_key_in_secret_manager(