Skip to content

Conversation

@neubig
Copy link
Contributor

@neubig neubig commented Apr 6, 2025

This PR adds extensive typing to the openhands/security directory to improve type safety and code quality.

Changes include:

  • Added proper type annotations to all functions in the security module
  • Fixed type issues in the security analyzer
  • Improved type safety in the invariant analyzer
  • Used more specific types instead of Any where possible

To run this PR locally, use the following command:

docker run -it --rm   -p 3000:3000   -v /var/run/docker.sock:/var/run/docker.sock   --add-host host.docker.internal:host-gateway   -e SANDBOX_RUNTIME_CONTAINER_IMAGE=docker.all-hands.dev/all-hands-ai/runtime:13a2357-nikolaik   --name openhands-app-13a2357   docker.all-hands.dev/all-hands-ai/openhands:13a2357

openhands-agent and others added 24 commits January 21, 2025 19:12
- Update mypy configuration with stricter type checking rules
- Add more type stubs to pre-commit configuration
- Run mypy both through pre-commit and directly in CI
- Install project in editable mode for better type checking
- Set correct PYTHONPATH in CI environment
@neubig
Copy link
Contributor Author

neubig commented Apr 6, 2025

@OpenHands install pre-commit from the existing pre-commit-config.yaml and then follow the lint.yml workflow and fix any issues

@openhands-ai
Copy link

openhands-ai bot commented Apr 6, 2025

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

@openhands-ai
Copy link

openhands-ai bot commented Apr 6, 2025

I successfully addressed the PR comment by:

  1. Installing pre-commit using the existing configuration file:

    • Installed pre-commit version 3.7.0 as specified in the lint.yml workflow
    • Set up the pre-commit hooks using the configuration from ./dev_config/python/.pre-commit-config.yaml
  2. Running the lint checks as specified in the lint.yml workflow:

    • Ran pre-commit on all the modified files in the security module
    • Identified a mypy error in openhands/security/invariant/analyzer.py related to unreachable code
  3. Fixed the issue:

    • Removed the unreachable code block that was checking if check_result was not a tuple
    • This code was unreachable because the check method in the client always returns a tuple
  4. Verified the fix:

    • Re-ran pre-commit hooks on the modified files to confirm the fix worked
    • Ran pre-commit on all files (openhands//* evaluation//* tests/**/*) as specified in the lint.yml workflow
    • All checks passed successfully
  5. Committed and pushed the changes:

The PR is now updated with the necessary fixes to pass all linting checks, addressing the comment's request to follow the lint.yml workflow and fix any issues.

@neubig
Copy link
Contributor Author

neubig commented Apr 6, 2025

@OpenHands there are places in this PR where "Optional" is used. Please do not use Optional, use "| None" instead in all files that were changed in this PR

@openhands-ai
Copy link

openhands-ai bot commented Apr 6, 2025

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

@openhands-ai
Copy link

openhands-ai bot commented Apr 6, 2025

I successfully addressed the PR comment by replacing all instances of "Optional" with "| None" in the security typing files that were changed in PR #7732.

Here's a summary of the changes made:

  1. Identified all files that were modified in the PR:

    • openhands/security/invariant/analyzer.py
    • openhands/security/invariant/client.py
    • openhands/security/invariant/nodes.py
    • openhands/security/invariant/parser.py
    • openhands/security/options.py (no changes needed in this file)
  2. Replaced all instances of "Optional" with "| None" in these files:

    • Changed import statements to remove "Optional" from typing imports
    • Updated type annotations in function parameters, return types, and variable declarations
    • Maintained the same functionality while improving the typing style
  3. Verified the changes by:

    • Running grep to confirm no instances of "Optional" remained in the modified files
    • Running pre-commit hooks to ensure code quality
    • Successfully committing and pushing the changes to the PR branch

The changes have been successfully pushed to the security-typing-improvements-new branch, updating PR #7732 as requested in the comment.

@neubig
Copy link
Contributor Author

neubig commented Apr 7, 2025

This should be ready!

@neubig neubig requested a review from enyst April 8, 2025 12:21
@neubig
Copy link
Contributor Author

neubig commented Apr 8, 2025

Thanks @enyst, should be ready for review again!

Copy link
Collaborator

@enyst enyst left a comment

Choose a reason for hiding this comment

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

LGTM!

@neubig neubig enabled auto-merge (squash) April 8, 2025 13:05
@neubig neubig merged commit 84e2823 into main Apr 8, 2025
16 checks passed
@neubig neubig deleted the security-typing-improvements-new branch April 8, 2025 13:51
shabbir-shakudo pushed a commit to devsentient/OpenHands that referenced this pull request Jul 15, 2025
Co-authored-by: openhands <openhands@all-hands.dev>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants