-
Notifications
You must be signed in to change notification settings - Fork 93
feat: Add configurable security policy support to Agent #427
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
0935302 to
27b1c1c
Compare
- Add security_policy_filename field to Agent model with default 'security_policy.j2' - Update system_prompt.j2 to use configurable security policy template - Add comprehensive tests for security policy configuration - Add example demonstrating configurable security policy usage - All tests pass and pre-commit hooks validated Co-authored-by: openhands <openhands@all-hands.dev>
27b1c1c to
040e162
Compare
- Merged security_policy.j2 from PR #422 with configurable security policy feature - Updated system_prompt.j2 to use configurable security_policy_filename in SECURITY section - Combined test suites from both branches in test_security_policy_integration.py - Maintained backward compatibility with default security_policy.j2 filename Co-authored-by: openhands <openhands@all-hands.dev>
…ctories - Updated test_security_policy_filename_in_system_message to copy security_risk_assessment.j2 - Updated test_configurable_security_policy_filename to copy security_risk_assessment.j2 - All tests now pass successfully Co-authored-by: openhands <openhands@all-hands.dev>
- Reduced from 7 tests to 3 focused tests - test_default_security_policy_filename: Verifies default filename - test_custom_security_policy_filename: Verifies custom filename can be set - test_custom_security_policy_in_system_message: Verifies custom policy is used in system message - Removed redundant and overly complex test cases - Tests will run in GitHub CI via sdk-tests job Co-authored-by: openhands <openhands@all-hands.dev>
- Implement FlexibleFileSystemLoader to support both relative and absolute paths - Update field descriptions to document absolute path support - Add comprehensive tests for absolute path functionality - Maintain backward compatibility with existing relative paths - Follow Jinja2 best practices for custom loaders Co-authored-by: openhands <openhands@all-hands.dev>
- Add absolute_path_example.py demonstrating filesystem-wide template loading - Update README.md with comprehensive path resolution documentation - Show both relative and absolute path usage patterns Co-authored-by: openhands <openhands@all-hands.dev>
- Remove trivial tests for default/custom security policy filenames - Remove monkeypatch mocking from custom security policy test - Keep test_security_policy_in_system_message as requested - Use absolute paths for both system prompt and security policy in custom test - All 4 tests now pass cleanly Co-authored-by: openhands <openhands@all-hands.dev>
- Remove test_absolute_path_security_policy() and test_absolute_path_system_prompt() - Absolute path functionality is already tested in test_custom_security_policy_in_system_message() - Remove absolute_path_example.py as it's redundant - Keep only essential tests: test_security_policy_in_system_message() and test_custom_security_policy_in_system_message() Co-authored-by: openhands <openhands@all-hands.dev>
- Remove shebang and main() function wrapper to match other examples - Use standard openhands.sdk imports and LLM configuration pattern - Add actual conversation demonstration instead of just configuration - Include callback pattern for collecting LLM messages - Use simpler docstring and print statements - Focus on demonstration rather than explanation - Follow the same structure as examples 01-03 Co-authored-by: openhands <openhands@all-hands.dev>
…ample - Remove references to deleted absolute_path_example.py - Simplify documentation by removing redundant code examples - Consolidate path resolution explanation into a brief mention - Update file list and running instructions - Make documentation more concise and focused Co-authored-by: openhands <openhands@all-hands.dev>
|
@OpenHands resolve any conflicts and make sure that tests still pass |
|
I'm on it! neubig can track my progress at all-hands.dev |
Co-authored-by: openhands <openhands@all-hands.dev>
…test The test was expecting the SECURITY_RISK_ASSESSMENT section to be present but wasn't providing a security analyzer. After the merge with main, the behavior changed so that this section is only included when there's an LLMSecurityAnalyzer present. Co-authored-by: openhands <openhands@all-hands.dev>
SummaryI have successfully resolved the conflicts and ensured all tests pass for the configurable-security-policy branch: ✅ Request Completion Checklist:
🔧 Changes Made:Single focused fix in
🧪 Verification:
The configurable security policy feature remains fully functional and backward compatible. The PR is now updated with the conflict resolution and test fix. |
|
Agent Server image for this PR Pull (multi-arch manifest): docker pull ghcr.io/all-hands-ai/agent-server:cb0f50aRun: docker run -it --rm \
-p 8000:8000 \
--name agent-server-cb0f50a \
ghcr.io/all-hands-ai/agent-server:cb0f50aThis tag is a multi-arch manifest (amd64/arm64). Your client pulls the right arch automatically. |
|
[Automatic Post]: It has been a while since there was any activity on this PR, are you still working on it? |
|
@OpenHands Fix merge conflicts for this |
|
I'm on it! neubig can track my progress at all-hands.dev |
Resolved conflicts in test_security_policy_integration.py: - Kept test_custom_security_policy_in_system_message from this branch (PR feature) - Integrated new test_security_policy_template_rendering from main - Updated all LLM constructors to include service_id parameter - Updated first test with new assertions from main - Added new imports required by main tests Co-authored-by: openhands <openhands@all-hands.dev>
SummaryI have successfully resolved the merge conflicts for PR #427 (configurable-security-policy branch) and pushed the changes to GitHub. What Was Done✅ Merge Conflicts Resolved:
Conflict Resolution DetailsThe conflict was in a single test file. I integrated changes from both branches:
Changes Pushed
The PR is now up-to-date with the main branch and ready for CI to run tests. The merge preserves the custom security policy feature while incorporating all the latest improvements from main. |
- Apply fix from commit 35d042e to make openhands.tools.glob import-safe - Provide stdlib-compatible glob/iglob/escape to avoid build-time import errors - Use lazy imports via __getattr__ to avoid import-time side effects - Fix import order in test_security_policy_integration.py Co-authored-by: openhands <openhands@all-hands.dev>
|
|
[Automatic Post]: It has been a while since there was any activity on this PR. @neubig, are you still working on it? If so, please go ahead, if not then please request review, close it, or request that someone else follow up. |
…y_filename The system_prompt.j2 template now requires security_policy_filename to be passed as a context variable. Updated the test to provide this required parameter. Co-authored-by: openhands <openhands@all-hands.dev>
Add documentation for custom security policy templates that allow users to define organization-specific risk assessment guidelines. Relates to OpenHands/software-agent-sdk#427
|
The |
Address review comment from xingyaoww Co-authored-by: openhands <openhands@all-hands.dev>
|
Fixed the The docs branch name didn't match the SDK branch name. I've:
The check-examples workflow should now pass once GitHub Actions picks up the new branch. You may need to trigger a re-run of the workflow. |
|
@OpenHands merge main, update the example number to be the appropriate most recent one, and then run the openhands CLI (https://github.com/OpenHands/OpenHands-CLI) in headless mode using this commit and write the log to a file. tell me the location of the log file demonstrating that with appropriate settings a custom security policy will be created and the agent will follow it. If you are not able to demonstrate this, then explain what additional changes would need to be made to make this possible. |
|
I'm on it! neubig can track my progress at all-hands.dev |
Resolved conflict in base.py by keeping both: - security_policy_filename from configurable-security-policy branch - model_name, model_family, model_variant from main branch Co-authored-by: openhands <openhands@all-hands.dev>
Co-authored-by: openhands <openhands@all-hands.dev>
SummaryI have completed all the requested tasks for PR #427 (configurable-security-policy): ✅ Checklist
📁 Log File LocationThe log file demonstrating the custom security policy feature is at: Key evidence from the log: 📝 Changes PushedThe branch has been pushed to GitHub with commits:
|
The test_custom_security_policy_in_system_message test was failing because it didn't copy the self_documentation.j2 template to the temp directory. The system_prompt.j2 template includes self_documentation.j2, so it needs to be present for the template to render correctly. Co-authored-by: openhands <openhands@all-hands.dev>
Add documentation for custom security policy templates that allow users to define organization-specific risk assessment guidelines. Relates to OpenHands/software-agent-sdk#427
The glob shadowing fix is unrelated to the configurable security policy feature and should be in a separate PR if needed. Co-authored-by: openhands <openhands@all-hands.dev>
|
Looks like there are a few issues preventing this PR from being merged!
If you'd like me to help, just leave a comment, like Feel free to include any additional details that might help me get this PR into a better state. You can manage your notification settings |
* docs: Add configurable security policy documentation Add documentation for custom security policy templates that allow users to define organization-specific risk assessment guidelines. Relates to OpenHands/software-agent-sdk#427 * Update security policy example number from 28 to 31 Address review comment from xingyaoww Co-authored-by: openhands <openhands@all-hands.dev> * Update security policy example number from 31 to 32 The example was renamed to 32 to avoid conflict with 31_iterative_refinement.py Co-authored-by: openhands <openhands@all-hands.dev> --------- Co-authored-by: openhands <openhands@all-hands.dev>
xingyaoww
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Adds
security_policy_filenamefield to Agent class for configurable security policies.security_policy.j2)security_policy_filenameparameterexamples/20_security_policy/for usageAgent 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:84425c1-pythonRun
All tags pushed for this build
About Multi-Architecture Support
84425c1-python) is a multi-arch manifest supporting both amd64 and arm6484425c1-python-amd64) are also available if needed