Skip to content

Conversation

@malhotra5
Copy link
Collaborator

@malhotra5 malhotra5 commented Nov 6, 2025

Summary

This PR does the following

  1. makes sure tool schemas is always has the security_risk field
  2. validates security_risk field at runtime.
    • when llm security analyzer is configured + security_risk is missing -> send an error event to correct itself (good for strong models)
    • when llm security analyzer is NOT configured + security_risk is missing -> default to SecurityRisk.UNKNOWN and proceed as usually
  3. move security analyzer to conversation state to support easier mid conversation switches

Benefits

  • Consistent schemas
  • Runtime enforcement of security_risk provides the desired flexibility
  • Graceful error handling (we send back an error event so the model has a chance to retry, good for stronger models)
  • No compromise on traceability (we have events registering when the security analyzer is added to a conversation; this is important since every event will contain security_risk field and they can be ignored depending on whether the analyzer has been configured)

The linked issue contains the following scenarios; this PR helps address each one of them

Scenario 1:
Security analyzer exists when starting conversation
System prompt includes tools with security_risk
Security analyzer is removed mid conversation
No risk field is expected, but LLM still believes it needs to pass this field because system prompt was not updated. Passing field will crash the conversation

In this scenario, the security_risk parameter will still exist. The LLM can continue passing the field. We simply won't interfere with the control loop even though the agent is passing risk values. The agent can also forget to pass a risk parameter which will be ignored.

Scenario 2:
Security analyzer was not included at start of conversation
System prompt does not include information on passing security_risk fields
Security analyzer is added later in the conversation
security_risk is a required field, but LLM is unaware of this requirement since system prompt did not include this information

This time security_risk information in the system prompt will always exist at the start of a conversation. The LLM is always aware of it. If analyzer is not configured and the LLM forgets to pass a value, we'll default to SecurityRisk.UNKNOWN and proceed as usual. When security analyzer is added later in the conversation, we'll make sure that at runtime an error event is raised if the LLM fails to pass the risk parameter.

Scenario 3:
Similar to 1, but:
The security analyzer is removed mid conversation
The conversation is fully reloaded
ActionEvents are created in history, and they have the old security_risk field
Validation fails, and leads to an infinite loop

The schema should always contain security_risk, so loading back events regardless of whether a section of conversation had or did not have a security analyzer should work.

Fixes #819

@malhotra5 can click here to continue refining the PR


Agent Server images for this PR

GHCR package: https://github.com/OpenHands/agent-sdk/pkgs/container/agent-server

Variants & Base Images

Variant Architectures Base Image Docs / Tags
java amd64, arm64 eclipse-temurin:17-jdk Link
python amd64, arm64 nikolaik/python-nodejs:python3.12-nodejs22 Link
golang amd64, arm64 golang:1.21-bookworm Link

Pull (multi-arch manifest)

# Each variant is a multi-arch manifest supporting both amd64 and arm64
docker pull ghcr.io/openhands/agent-server:8aaca4b-python

Run

docker run -it --rm \
  -p 8000:8000 \
  --name agent-server-8aaca4b-python \
  ghcr.io/openhands/agent-server:8aaca4b-python

All tags pushed for this build

ghcr.io/openhands/agent-server:8aaca4b-golang-amd64
ghcr.io/openhands/agent-server:8aaca4b-golang_tag_1.21-bookworm-amd64
ghcr.io/openhands/agent-server:8aaca4b-golang-arm64
ghcr.io/openhands/agent-server:8aaca4b-golang_tag_1.21-bookworm-arm64
ghcr.io/openhands/agent-server:8aaca4b-java-amd64
ghcr.io/openhands/agent-server:8aaca4b-eclipse-temurin_tag_17-jdk-amd64
ghcr.io/openhands/agent-server:8aaca4b-java-arm64
ghcr.io/openhands/agent-server:8aaca4b-eclipse-temurin_tag_17-jdk-arm64
ghcr.io/openhands/agent-server:8aaca4b-python-amd64
ghcr.io/openhands/agent-server:8aaca4b-nikolaik_s_python-nodejs_tag_python3.12-nodejs22-amd64
ghcr.io/openhands/agent-server:8aaca4b-python-arm64
ghcr.io/openhands/agent-server:8aaca4b-nikolaik_s_python-nodejs_tag_python3.12-nodejs22-arm64
ghcr.io/openhands/agent-server:8aaca4b-golang
ghcr.io/openhands/agent-server:8aaca4b-java
ghcr.io/openhands/agent-server:8aaca4b-python

About Multi-Architecture Support

  • Each variant tag (e.g., 8aaca4b-python) is a multi-arch manifest supporting both amd64 and arm64
  • Docker automatically pulls the correct architecture for your platform
  • Individual architecture tags (e.g., 8aaca4b-python-amd64) are also available if needed

- Modified tool schema generation to include security_risk field when add_security_risk_prediction=True for all tool types (including read-only tools)
- Updated LLM security analyzer validation to always require security_risk field when using LLMSecurityAnalyzer
- Added comprehensive test suite for security_risk validation behavior
- Fixed existing tests to reflect new behavior where security_risk is included for read-only tools when prediction is enabled
- Updated docstrings to clarify the new behavior

Co-authored-by: openhands <openhands@all-hands.dev>
@github-actions
Copy link
Contributor

github-actions bot commented Nov 6, 2025

Coverage

Coverage Report •
FileStmtsMissCoverMissing
openhands-agent-server/openhands/agent_server
   conversation_router.py974850%80–82, 96–97, 110, 121–123, 151–154, 165–168, 185, 189–191, 197, 211–213, 215, 217, 219–221, 234–238, 251–255, 270–273, 286, 289–291
   event_service.py2048657%52–53, 72–74, 77–82, 94, 110, 117, 119, 126–127, 135–138, 145–147, 159–160, 163–164, 166–168, 170, 175, 178, 182–184, 186, 188, 192–196, 200, 204–205, 207, 224–225, 272, 279–280, 282, 285–287, 289, 293–296, 300–303, 311–314, 333–334, 336–343, 345–346, 352, 358, 371–372
   models.py98297%56–57
openhands-sdk/openhands/sdk/agent
   agent.py1675567%80, 84, 141, 145–146, 153–154, 156–158, 160–162, 178, 193–195, 202–204, 206, 210, 213–214, 216, 223, 250, 255, 275, 282, 304, 309, 320, 323, 346, 356–357, 378–380, 382, 394–395, 400–401, 421–422, 427, 439–440, 445–446, 478, 485–486
   base.py1772486%163, 181, 187, 199–201, 227–228, 240–241, 247–249, 262, 270–271, 302, 346, 353, 366, 403–404, 414–415
openhands-sdk/openhands/sdk/conversation
   base.py69592%106, 111–112, 147, 158
   state.py1321290%175, 200, 247–249, 265–266, 295, 313, 322, 337, 343
openhands-sdk/openhands/sdk/conversation/impl
   local_conversation.py1895371%121, 123–124, 157–158, 181, 186, 198, 215, 223–225, 229–230, 282–283, 286, 293, 314–316, 319, 328, 344, 346, 348, 352, 354–356, 358, 360, 366–367, 380–381, 383, 385, 389–392, 409–410, 415, 420, 422, 427, 429–431, 449, 451
   remote_conversation.py37210472%55–61, 68–71, 100, 107, 115, 117–120, 130, 139, 143–144, 149–152, 187, 201, 218, 229, 238–239, 291, 311, 319, 331, 339–342, 345, 350–353, 355, 360–361, 366–370, 375–379, 384–387, 390, 401–402, 406, 410, 413, 484, 490, 492, 508, 510–511, 522, 539–540, 546, 561, 594, 596, 598–599, 603–604, 613–614, 623, 631, 636–638, 640, 643, 645–646, 663, 670, 676–677, 691–692, 699–700
TOTAL12046555253% 

malhotra5 and others added 26 commits November 6, 2025 10:53
…ation

- Created new SecurityAnalyzerConfigurationEvent class extending Event
- Added event type to EventType literal and exports
- Modified AgentBase.init_state to always emit SecurityAnalyzerConfigurationEvent
- Added comprehensive tests for event creation and emission
- Event tracks analyzer type (string name or None) and includes visualization methods

Co-authored-by: openhands <openhands@all-hands.dev>
- Test all 5 required scenarios with parameterized tests
- Case 1: LLM analyzer set, security risk passed, extracted properly
- Case 2: analyzer not set, security risk passed, extracted properly
- Case 3: LLM analyzer set, security risk not passed, ValueError raised
- Case 4: analyzer not set, security risk not passed, UNKNOWN returned
- Case 5: invalid security risk value passed, ValueError raised
- Include additional tests for error messages and argument mutation
- Follow existing test patterns and code style guidelines

Co-authored-by: openhands <openhands@all-hands.dev>
…ehavior in conversations

- Test new conversation initialization creates SystemPromptEvent and SecurityAnalyzerConfigurationEvent
- Test reinitializing with same analyzer type creates new events (different instances)
- Test reinitializing with same agent instance still creates new events
- Test switching between different analyzers creates appropriate events
- Test switching from no analyzer to analyzer creates events
- Test multiple reinitializations create correct event sequences
- Test event properties and methods validation
- Use parameterized tests and fixtures following existing patterns
- All 8 tests passing with proper edge case coverage

Co-authored-by: openhands <openhands@all-hands.dev>
- Updated test_conversation_event_id_validation to expect duplicate event at index 2 instead of 1
- Updated test_pause_basic_functionality to expect 2 events instead of 1
- Both tests now account for the new SecurityAnalyzerConfigurationEvent being added during conversation initialization

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

- Updated expected event count to account for additional SecurityAnalyzerConfigurationEvent
- When conversation is loaded from persistence, agent initialization adds one more event
- Changed assertion from original_event_count to original_event_count + 1

Co-authored-by: openhands <openhands@all-hands.dev>
- Added __eq__ method to SecurityAnalyzerConfigurationEvent to compare only analyzer_type
- This prevents duplicate events when the same analyzer configuration is used
- Updated test expectations to account for SecurityAnalyzerConfigurationEvent being added during initialization
- Fixed test_conversation_event_id_validation to expect duplicate at index 2 (after SystemPromptEvent and SecurityAnalyzerConfigurationEvent)
- Fixed test_conversation_persistence_lifecycle to expect same event count when loading from persistence

Co-authored-by: openhands <openhands@all-hands.dev>
- 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>
@malhotra5 malhotra5 force-pushed the refactor/always-include-security-risk-fields branch from 3a8f6bb to 4d58824 Compare November 7, 2025 16:09
@malhotra5
Copy link
Collaborator Author

QA'ed with CLI and seems to work well

@malhotra5 malhotra5 marked this pull request as ready for review November 11, 2025 16:26
@malhotra5 malhotra5 marked this pull request as draft November 11, 2025 17:02
@malhotra5 malhotra5 marked this pull request as ready for review November 11, 2025 17:14
Comment on lines +166 to +171
if "security_analyzer" in d and d["security_analyzer"]:
warnings.warn(
AGENT_SECURITY_ANALYZER_DEPRECATION_WARNING,
DeprecationWarning,
stacklevel=3,
)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@OpenHands is there any good python library that allow us to manage deprecation warning in a more systematic way?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm on it! xingyaoww can track my progress at all-hands.dev

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Summary

I answered your question about Python libraries for managing deprecation warnings systematically. Here are the recommendations:

Top 3 Recommended Libraries:

  1. deprecation (Best for your use case)

    • Lightweight and purpose-built for deprecation management
    • Supports version tracking (deprecated_in, removed_in, current_version)
    • Integrates seamlessly with Python's warnings module
    • Ideal for SDKs and libraries
    • Installation: pip install deprecation
  2. Deprecated (deprecated.py)

    • Feature-rich with excellent Sphinx documentation integration
    • Simple decorator syntax with version and reason tracking
    • Very popular (4M+ downloads/month)
    • Installation: pip install Deprecated
  3. debtcollector (From OpenStack)

    • Comprehensive solution for tracking both deprecations and technical debt
    • Advanced features like move notifications and removal tracking
    • Battle-tested in large projects
    • Installation: pip install debtcollector

Why deprecation is recommended for your codebase:

Looking at your current usage (multiple warnings.warn calls in agent/base.py, llm/llm.py, and conversation/conversation_stats.py), the deprecation library would provide:

  • Version tracking for deprecation timelines
  • Minimal overhead and dependencies
  • Easy migration from your current warnings usage
  • Clear, structured messaging
  • SDK-friendly implementation

All three libraries provide decorators that make deprecation management more systematic and maintainable than the built-in warnings module alone.

No code changes were made as this was an informational question.

View full conversation

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@OpenHands please add a test to tests/cross/test_remote_conversation_live_server.py where we spawns a real agent server to test this

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm on it! xingyaoww can track my progress at all-hands.dev

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Summary

I've successfully added an integration test to tests/cross/test_remote_conversation_live_server.py that spawns a real agent server to test the security_risk field functionality introduced in PR #1052.

Changes Made

Added test: test_security_risk_field_with_live_server (~148 lines)

  • Spawns a real agent server via the existing server_env fixture
  • Uses a monkeypatched LLM completion that returns tool calls without the security_risk field
  • Validates that:
    1. Actions execute successfully even when security_risk is missing (defaults to UNKNOWN)
    2. ActionEvent objects always have a security_risk attribute

Test Details

The test:

  • Creates an agent with LLMSecurityAnalyzer configured
  • Mocks LLM completions to return a finish tool call without providing the security_risk parameter
  • Verifies the action executes successfully (demonstrating the graceful default behavior)
  • Confirms that the resulting ActionEvent has the security_risk attribute

Validation

✅ Test passes successfully
✅ Pre-commit checks pass (ruff format, ruff lint, pycodestyle, pyright)
✅ Changes committed and pushed to the refactor/always-include-security-risk-fields branch

This integration test provides end-to-end validation of the security_risk field handling changes, ensuring the system gracefully handles cases where the field is missing while still maintaining backward compatibility.

View full conversation

@xingyaoww xingyaoww added the integration-test Runs the integration tests and comments the results label Nov 11, 2025
@github-actions
Copy link
Contributor

Hi! I started running the integration tests on your PR. You will receive a comment with the results shortly.

Copy link
Collaborator

@xingyaoww xingyaoww left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Will merge this after tests pass

@xingyaoww xingyaoww added the test-examples Run all applicable "examples/" files. Expensive operation. label Nov 11, 2025
@github-actions
Copy link
Contributor

github-actions bot commented Nov 11, 2025

🔄 Running Examples with openhands/claude-haiku-4-5-20251001

Last updated: 2025-11-11 18:07:13 UTC

Example Status Duration Cost
01_standalone_sdk/02_custom_tools.py ✅ PASS 27s $0.03
01_standalone_sdk/03_activate_skill.py ✅ PASS 12s $0.01
01_standalone_sdk/05_use_llm_registry.py ✅ PASS 10s $0.01
01_standalone_sdk/07_mcp_integration.py ❌ FAIL (exit: 1) 35s $0.00
01_standalone_sdk/09_pause_example.py ✅ PASS 13s $0.01
01_standalone_sdk/10_persistence.py ✅ PASS 38s $0.02
01_standalone_sdk/11_async.py ✅ PASS 42s $0.03
01_standalone_sdk/12_custom_secrets.py ✅ PASS 14s $0.01
01_standalone_sdk/13_get_llm_metrics.py ✅ PASS 33s $0.02
01_standalone_sdk/14_context_condenser.py ✅ PASS 202s $0.35
01_standalone_sdk/17_image_input.py ✅ PASS 21s $0.02
01_standalone_sdk/18_send_message_while_processing.py ✅ PASS 19s $0.01
01_standalone_sdk/19_llm_routing.py ✅ PASS 20s $0.02
01_standalone_sdk/20_stuck_detector.py ✅ PASS 19s $0.02
01_standalone_sdk/21_generate_extraneous_conversation_costs.py ✅ PASS 17s $0.01
01_standalone_sdk/22_anthropic_thinking.py ✅ PASS 15s $0.01
01_standalone_sdk/23_responses_reasoning.py ✅ PASS 46s $0.01
01_standalone_sdk/24_planning_agent_workflow.py ✅ PASS 221s $0.23
01_standalone_sdk/25_agent_delegation.py ❌ FAIL (exit: 1) 67s $0.00
01_standalone_sdk/26_custom_visualizer.py ✅ PASS 24s $0.00N/A
02_remote_agent_server/01_convo_with_local_agent_server.py ✅ PASS 69s $0.06
02_remote_agent_server/02_convo_with_docker_sandboxed_server.py ✅ PASS 136s $0.07
02_remote_agent_server/03_browser_use_with_docker_sandboxed_server.py ✅ PASS 67s $0.05
02_remote_agent_server/04_convo_with_api_sandboxed_server.py ✅ PASS 90s $0.02

❌ Some tests failed

Total: 24 | Passed: 22 | Failed: 2

View full workflow run

@github-actions
Copy link
Contributor

🧪 Integration Tests Results

Overall Success Rate: 93.8%
Total Cost: $0.69
Models Tested: 4
Timestamp: 2025-11-11 17:47:38 UTC

📁 Detailed Logs & Artifacts

Click the links below to access detailed agent/LLM logs showing the complete reasoning process for each model. On the GitHub Actions page, scroll down to the 'Artifacts' section to download the logs.

📊 Summary

Model Success Rate Tests Passed Skipped Total Tests Cost
litellm_proxy_gpt_5_mini_2025_08_07 100.0% 8/8 0 8 $0.05
litellm_proxy_claude_sonnet_4_5_20250929 100.0% 8/8 0 8 $0.41
litellm_proxy_deepseek_deepseek_chat 87.5% 7/8 1 8 $0.03
litellm_proxy_moonshot_kimi_k2_thinking 87.5% 7/8 1 8 $0.20

📋 Detailed Results

litellm_proxy_gpt_5_mini_2025_08_07

  • Success Rate: 100.0% (8/8)
  • Total Cost: $0.05
  • Run Suffix: litellm_proxy_gpt_5_mini_2025_08_07_41101cc_gpt5_mini_run_N8_20251111_174328

litellm_proxy_claude_sonnet_4_5_20250929

  • Success Rate: 100.0% (8/8)
  • Total Cost: $0.41
  • Run Suffix: litellm_proxy_claude_sonnet_4_5_20250929_41101cc_sonnet_run_N8_20251111_174323

litellm_proxy_deepseek_deepseek_chat

  • Success Rate: 87.5% (7/8)
  • Total Cost: $0.03
  • Run Suffix: litellm_proxy_deepseek_deepseek_chat_41101cc_deepseek_run_N8_20251111_174318
  • Skipped Tests: 1

Skipped Tests:

  • t08_image_file_viewing: This test requires a vision-capable LLM model. Please use a model that supports image input.

litellm_proxy_moonshot_kimi_k2_thinking

  • Success Rate: 87.5% (7/8)
  • Total Cost: $0.20
  • Run Suffix: litellm_proxy_moonshot_kimi_k2_thinking_41101cc_kimi_k2_run_N8_20251111_174322
  • Skipped Tests: 1

Skipped Tests:

  • t08_image_file_viewing: This test requires a vision-capable LLM model. Please use a model that supports image input.

openhands-agent and others added 2 commits November 11, 2025 17:58
This test validates the fix for issue #819 where security_risk field handling
was inconsistent. It spawns a real agent server and tests that:
1. Actions can be executed without security_risk (defaults to UNKNOWN)
2. ActionEvent always has a security_risk attribute

The test uses a monkeypatched LLM completion to return a finish tool call
without the security_risk field, ensuring the system handles it gracefully.

Co-authored-by: openhands <openhands@all-hands.dev>
@xingyaoww xingyaoww enabled auto-merge (squash) November 11, 2025 18:00
@xingyaoww xingyaoww disabled auto-merge November 11, 2025 18:00
Copy link
Collaborator

@xingyaoww xingyaoww left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@xingyaoww xingyaoww enabled auto-merge (squash) November 11, 2025 18:01
@xingyaoww xingyaoww merged commit a656ca7 into main Nov 11, 2025
15 checks passed
@xingyaoww xingyaoww deleted the refactor/always-include-security-risk-fields branch November 11, 2025 18:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

integration-test Runs the integration tests and comments the results test-examples Run all applicable "examples/" files. Expensive operation.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Comprehensive overview for difficulties with security analyzer

4 participants