Skip to content
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

fix(agents-api): Make the sample work #467

Merged
merged 1 commit into from
Aug 26, 2024

Conversation

creatorrr
Copy link
Contributor

@creatorrr creatorrr commented Aug 26, 2024

Signed-off-by: Diwank Tomer [email protected]


🚀 This description was created by Ellipsis for commit abb36ba

Summary:

This PR updates base_evaluate to handle BaseModel instances, adds zip to allowed functions, modifies task execution logic, and adjusts tests and dependencies to ensure the sample task works correctly.

Key points:

  • base_evaluate.py: Converts BaseModel instances to dicts using model_dump and wraps values in a Box.
  • utils.py: Adds zip to ALLOWED_FUNCTIONS.
  • task_execution.py: Removes model_dump call on output and updates transition function.
  • pyproject.toml: Adds python-box dependency.
  • find_selector.yaml: Uses load_yaml for parsing YAML content.
  • test_find_selector.py: Updates tests to match new YAML parsing and output handling.

Generated with ❤️ by ellipsis.dev

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

👍 Looks good to me! Reviewed everything up to abb36ba in 19 seconds

More details
  • Looked at 145 lines of code in 6 files
  • Skipped 1 files when reviewing.
  • Skipped posting 2 drafted comments based on config settings.
1. agents-api/agents_api/activities/utils.py:11
  • Draft comment:
    Adding zip to ALLOWED_FUNCTIONS can be risky if inputs are not properly sanitized. Ensure that all inputs are validated to prevent code injection vulnerabilities.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable:
    The comment is relevant to the change made in the diff, which is the addition of 'zip' to ALLOWED_FUNCTIONS. It raises a valid concern about input validation, which is important for security. The comment is not asking for confirmation or making speculative statements, and it is actionable as it suggests ensuring input validation.
    The comment might be considered speculative since it assumes a risk without specific evidence of a vulnerability. However, it is a reasonable precaution given the context of allowing functions in an evaluator.
    Even though the comment is somewhat speculative, it is a valid security consideration when adding functions to an evaluator. It is better to err on the side of caution when it comes to security.
    The comment should be kept as it raises a valid security concern related to the change made in the diff.
2. agents-api/agents_api/workflows/task_execution.py:364
  • Draft comment:
    Ensure output is properly serialized if it's a Pydantic model. Removing model_dump might cause issues if output is not already a dictionary.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable:
    The comment is relevant to the change made in the diff, as it addresses the removal of 'model_dump'. The concern about serialization is valid because if 'output' is a Pydantic model, it might not be in the correct format without 'model_dump'.
    The comment might be considered speculative since it suggests a potential issue without confirming it. However, it is directly related to the change made in the diff.
    The comment is not speculative in a negative sense; it highlights a potential issue that could arise from the change, which is a valid concern.
    The comment should be kept as it addresses a potential issue directly related to the change made in the diff.

Workflow ID: wflow_ItU193fQYnidASpT


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

@whiterabbit1983 whiterabbit1983 merged commit 3457252 into dev-tasks-disable-routes Aug 26, 2024
2 of 5 checks passed
@whiterabbit1983 whiterabbit1983 deleted the x/sample-test-fix branch August 26, 2024 12:26
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.

2 participants