Fix failing unit tests for security risk extraction

- Fix _extract_security_risk method to always pop security_risk from arguments before checking readOnlyHint
- Update test calls to include the new readOnlyHint parameter (3rd argument)
- Add comprehensive test for readOnlyHint=True scenario
- Ensure security_risk is properly removed from arguments in all cases

Co-authored-by: openhands <openhands@all-hands.dev>

Author: openhands-agent

openhands-sdk/openhands/sdk/agent/agent.py

@@ -313,14 +313,14 @@ def _extract_security_risk(
         tool_name: str,
         readOnlyHint: bool,
     ) -> risk.SecurityRisk:
+        requires_sr = isinstance(self.security_analyzer, LLMSecurityAnalyzer)
+        raw = arguments.pop("security_risk", None)
+
         # Default risk value for action event
         # Tool is marked as read-only so security risk can be ignored
         if readOnlyHint:
             return risk.SecurityRisk.UNKNOWN
 
-        requires_sr = isinstance(self.security_analyzer, LLMSecurityAnalyzer)
-        raw = arguments.pop("security_risk", None)
-
         # Raises exception if failed to pass risk field when expected
         # Exception will be sent back to agent as error event
         # Strong models like GPT-5 can correct itself by retrying

tests/sdk/agent/test_extract_security_risk.py

@@ -95,9 +95,9 @@ def test_extract_security_risk(
 
     if should_raise:
         with pytest.raises(ValueError):
-            agent._extract_security_risk(arguments, tool_name)
+            agent._extract_security_risk(arguments, tool_name, False)
     else:
-        result = agent._extract_security_risk(arguments, tool_name)
+        result = agent._extract_security_risk(arguments, tool_name, False)
         assert result == expected_result
 
         # Verify that security_risk was popped from arguments
@@ -115,7 +115,7 @@ def test_extract_security_risk_error_messages(agent_with_llm_analyzer):
     with pytest.raises(
         ValueError, match="Failed to provide security_risk field in tool 'test_tool'"
     ):
-        agent_with_llm_analyzer._extract_security_risk(arguments, tool_name)
+        agent_with_llm_analyzer._extract_security_risk(arguments, tool_name, False)
 
 
 def test_extract_security_risk_arguments_mutation():
@@ -133,7 +133,7 @@ def test_extract_security_risk_arguments_mutation():
     arguments = {"param1": "value1", "security_risk": "LOW", "param2": "value2"}
     original_args = arguments.copy()
 
-    result = agent._extract_security_risk(arguments, "test_tool")
+    result = agent._extract_security_risk(arguments, "test_tool", False)
 
     # Verify result
     assert result == SecurityRisk.LOW
@@ -159,8 +159,31 @@ def test_extract_security_risk_with_empty_arguments():
     )
 
     arguments = {}
-    result = agent._extract_security_risk(arguments, "test_tool")
+    result = agent._extract_security_risk(arguments, "test_tool", False)
 
     # Should return UNKNOWN when no analyzer and no security_risk
     assert result == SecurityRisk.UNKNOWN
     assert arguments == {}  # Should remain empty
+
+
+def test_extract_security_risk_with_readonly_hint():
+    """Test _extract_security_risk with readOnlyHint=True."""
+    agent = Agent(
+        llm=LLM(
+            usage_id="test-llm",
+            model="test-model",
+            api_key=SecretStr("test-key"),
+            base_url="http://test",
+        ),
+        security_analyzer=LLMSecurityAnalyzer(),
+    )
+
+    # Test with readOnlyHint=True - should return UNKNOWN regardless of security_risk
+    arguments = {"param1": "value1", "security_risk": "HIGH"}
+    result = agent._extract_security_risk(arguments, "test_tool", True)
+
+    # Should return UNKNOWN when readOnlyHint is True
+    assert result == SecurityRisk.UNKNOWN
+    # security_risk should still be popped from arguments
+    assert "security_risk" not in arguments
+    assert arguments["param1"] == "value1"