Refactor: Always include risk fields #1052
Conversation
- 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 <[email protected]>
Coverage Report •
|
||||||||||||||||||||||||||||||
…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 <[email protected]>
- 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 <[email protected]>
…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 <[email protected]>
- 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 <[email protected]>
…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 <[email protected]>
- 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 <[email protected]>
…//github.com/OpenHands/software-agent-sdk into refactor/always-include-security-risk-fields
This reverts commit 160a6a2.
- 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 <[email protected]>
malhotra5
force-pushed
the
refactor/always-include-security-risk-fields
branch
from
3a8f6bb to
4d58824
Compare
| len(security_analyzer_configuration_events) == 0 | ||
| or security_analyzer_event != security_analyzer_configuration_events[-1] | ||
| ): | ||
| on_event(security_analyzer_event) |
There was a problem hiding this comment.
It seems to me, this security analyzer event solely act as some sort of flag?
Can we add a new field to ConversationState to keep this information instead of making it an event?
There was a problem hiding this comment.
So with this PR we always store security_risk param regardless of the security analyzer. So without knowing which analyzer was active at the time of the action being generated, a stored security_risk value is ambiguous: you can’t tell if it was enforced, advisory, or ignored.
SecurityAnalyzerConfigurationEvent provides a timestamped, minimal audit trail of the analyzer type in effect when the action was proposed. This is especially important as we don't record user confirmation events, they are implicitly assumed if the action was executed
There was a problem hiding this comment.
How about we keep a variable inside State that record the latest timestamp and the security analyzer type instead of emitting an event?
I'm hesitant about throwing this into events as this make the logic here very hard to parse
There was a problem hiding this comment.
ok sounds good! i figured it would be easier if all the information was in one place rather than having to cross reference
will make the change
There was a problem hiding this comment.
So one reason I can think of adding the information to agent event history, instead of the conversation state, is that the security_risk is a default specific to the Agent class. Other folks who build their own agent implementations will use the same conversation state but may not have a security_risk requirement the way we do. So for specificity is makes sense to isolate that customization to the history for the default agent, rather than the shared state object
There was a problem hiding this comment.
imo we already have confirmation_policy in the state and it kinda make sense to have SecurityAnalyzer related information stored there as well?
There was a problem hiding this comment.
ah that makes sense; in that case how about moving the security_analyzer out of the agent class as well?
mid conversation analyzer switches are also tough to do because the agent is immutable. if its in the state we can easily update it - wdyt?
There was a problem hiding this comment.
yep! i think that's a more reasonable thing to do (although this means we are making breaking change 1 day after cutting the v1 release haha 🤣 )
Co-authored-by: openhands <[email protected]>
- Add security_analyzer field to ConversationState class following confirmation_policy pattern - Remove security_analyzer field from Agent base class - Add backwards compatibility handling via custom model_validate method - Update Agent class to use conversation.state.security_analyzer instead of self.security_analyzer - Restore system_message as property for backwards compatibility, add get_system_message method - Update all Agent class methods to pass security_analyzer parameter where needed - Update is_confirmation_mode_active property to use state.security_analyzer - Add security_analyzer property to ConversationStateProtocol - Create comprehensive backwards compatibility tests - Update existing test fixtures to work with new architecture - All 1141 SDK tests passing, all pre-commit checks passing Co-authored-by: openhands <[email protected]>
Summary
This PR does the following
security_riskfieldsecurity_riskfield at runtime.security_riskis missing -> send an error event to correct itself (good for strong models)security_riskis missing -> default toSecurityRisk.UNKNOWNand proceed as usuallyBenefits
security_riskprovides the desired flexibilitysecurity_riskfield 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
In this scenario, the
security_riskparameter 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.This time
security_riskinformation 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 toSecurityRisk.UNKNOWNand 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.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
eclipse-temurin:17-jdknikolaik/python-nodejs:python3.12-nodejs22golang:1.21-bookwormPull (multi-arch manifest)
# Each variant is a multi-arch manifest supporting both amd64 and arm64 docker pull ghcr.io/openhands/agent-server:053baee-pythonRun
All tags pushed for this build
About Multi-Architecture Support
053baee-python) is a multi-arch manifest supporting both amd64 and arm64053baee-python-amd64) are also available if needed