Observer artifact creation

[wintonzheng]

Important

Add support for observer artifact creation in ArtifactManager with new schema models and database updates.

• Artifact Manager:
  • Adds create_observer_thought_artifact() in manager.py for creating observer thought artifacts.
  • Introduces _create_artifact() as a helper method in manager.py.
• Schemas:
  • Adds ObserverCruise and ObserverThought models in schemas/observers.py.
  • Defines ObserverCruiseStatus enum for cruise status.
• Database Models:
  • Renames ObserverCruise to ObserverCruiseModel and ObserverThought to ObserverThoughtModel in models.py.
• Storage:
  • Adds build_observer_thought_uri() and build_observer_cruise_uri() in base.py, local.py, and s3.py for URI construction.
• Misc:
  • Updates convert_to_artifact() in utils.py to handle new fields.

^{This description was created by for 3fca676. It will automatically update as commits are pushed.}

[ellipsis-dev]

👍 Looks good to me! Incremental review on 3fca676 in 31 seconds

More details

• Looked at 425 lines of code in 9 files
• Skipped 0 files when reviewing.
• Skipped posting 5 drafted comments based on config settings.

1. skyvern/forge/sdk/artifact/manager.py:111

• Draft comment:
  The use of Literal for the primary_key parameter is unnecessary. Consider using a simple string type instead for better readability and simplicity.
• Reason this comment was not posted:
  Confidence changes required: 50%
  The code in manager.py has a potential issue with the use of Literal for the primary_key parameter in update_artifact_data. The use of Literal is not necessary here and can be replaced with a simple string type, as it doesn't provide any additional type safety or functionality in this context.

2. skyvern/forge/sdk/artifact/manager.py:23

• Draft comment:
  Ensure that aio_task_primary_key is a valid key in upload_aiotasks_map to prevent potential KeyErrors.
• Reason this comment was not posted:
  Confidence changes required: 50%
  In manager.py, the _create_artifact method has a potential issue with the aio_task_primary_key parameter. It is used to index the upload_aiotasks_map, but there is no check to ensure that this key is valid or exists. This could lead to potential KeyErrors if the key is not present in the map.

3. skyvern/forge/sdk/artifact/manager.py:71

• Draft comment:
  Consider refactoring the repeated pattern of generating artifact_id and building a URI into a helper function to reduce code duplication.
• Reason this comment was not posted:
  Confidence changes required: 50%
  In manager.py, the create_artifact and create_observer_thought_artifact methods both generate a new artifact_id and then immediately use it to build a URI. This pattern is repeated and could be refactored into a helper function to reduce code duplication and improve maintainability.

4. skyvern/forge/sdk/artifact/storage/local.py:39

• Draft comment:
  Consider refactoring the file path and directory handling logic into a helper method to reduce code duplication in store_artifact and store_artifact_from_path.
• Reason this comment was not posted:
  Confidence changes required: 50%
  In local.py, the store_artifact and store_artifact_from_path methods both have similar logic for handling file paths and directories. This logic could be refactored into a helper method to reduce code duplication and improve maintainability.

5. skyvern/forge/sdk/artifact/storage/s3.py:42

• Draft comment:
  Consider adding error handling for async_client operations to manage potential upload failures.
• Reason this comment was not posted:
  Confidence changes required: 50%
  In s3.py, the store_artifact and store_artifact_from_path methods both use the async_client to upload files. If the async_client fails, there is no error handling to manage this situation, which could lead to unhandled exceptions.

Workflow ID: wflow_xCqiovgSWMfjigKu

---

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

[ellipsis-dev]

👍 Looks good to me! Reviewed everything up to 3fca676 in 1 minute and 26 seconds

More details

• Looked at 425 lines of code in 9 files
• Skipped 0 files when reviewing.
• Skipped posting 5 drafted comments based on config settings.

1. skyvern/forge/sdk/artifact/manager.py:111

• Draft comment:
  Consider using an Enum instead of Literal for the primary_key parameter to ensure type safety and avoid potential runtime errors.
• Reason this comment was not posted:
  Confidence changes required: 50%
  The use of Literal for the primary_key parameter in the update_artifact_data method is not ideal. It would be better to use an Enum to ensure type safety and avoid potential runtime errors.

2. skyvern/forge/sdk/artifact/manager.py:64

• Draft comment:
  Consider extracting the logic for generating artifact_id and uri into a helper function to reduce code duplication in create_artifact and create_observer_thought_artifact.
• Reason this comment was not posted:
  Confidence changes required: 50%
  The create_artifact and create_observer_thought_artifact methods have similar logic for generating artifact_id and uri. This logic can be extracted into a helper function to reduce code duplication.

3. skyvern/forge/sdk/artifact/storage/local.py:27

• Draft comment:
  Consider extracting the URI construction logic into a shared utility function to reduce code duplication between LocalStorage and S3Storage.
• Reason this comment was not posted:
  Confidence changes required: 50%
  The build_observer_thought_uri and build_observer_cruise_uri methods in LocalStorage and S3Storage have similar logic for constructing URIs. This logic can be extracted into a shared utility function to reduce code duplication.

4. skyvern/forge/sdk/artifact/storage/s3.py:30

• Draft comment:
  Consider extracting the URI construction logic into a shared utility function to reduce code duplication between LocalStorage and S3Storage.
• Reason this comment was not posted:
  Confidence changes required: 50%
  The build_observer_thought_uri and build_observer_cruise_uri methods in LocalStorage and S3Storage have similar logic for constructing URIs. This logic can be extracted into a shared utility function to reduce code duplication.

5. skyvern/forge/sdk/db/utils.py:150

• Draft comment:
  Ensure that artifact_model.artifact_type is a valid key in ArtifactType to avoid potential KeyError. Consider using get with a default value or handling the exception.
• Reason this comment was not posted:
  Confidence changes required: 50%
  The convert_to_artifact function uses ArtifactType[artifact_model.artifact_type.upper()] which assumes that the artifact_type is always a valid key in the ArtifactType enum. This could lead to a KeyError if the artifact_type is not valid. It would be safer to handle this potential error.

Workflow ID: wflow_fxvMvb5VsuQ2VlQF

---

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