Observer code open source

[wintonzheng]

Important

Refactor observer functionality to skyvern module, updating routes, schemas, and exceptions, and adding local development support.

• Behavior:
  • Removed observer_router from app.py and deleted observer.py, observer_service.py, and observers.py from cloud.
  • Added observer_service.py in skyvern/forge/sdk/services to handle observer functionality.
  • Added run_local_observer.py in dev_scripts for local observer execution.
  • Added observer_cruise endpoint in agent_protocol.py for handling observer requests.
• Schemas:
  • Moved ObserverCruise, ObserverMetadata, and CruiseRequest to skyvern/forge/sdk/schemas/observers.py.
• Exceptions:
  • Moved UrlGenerationFailure to skyvern/exceptions.py.
• Prompts:
  • Renamed observer-related prompt files from cloud/prompts/cloud to skyvern/forge/prompts/skyvern.
• Misc:
  • Updated async_executor.py to include execute_cruise method for handling observer cruises.
  • Updated run_observer.py to use observer_service.run_observer_cruise.
  • Added docker-compose-local.yml and local_test.Dockerfile for local development setup.
  • Updated docker-compose.yml to use postgres:16 image.
  • Added entrypoint-skyvern-local.sh for local environment setup.
  • Increased memory and CPU in job-definition-staging.json.

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

[ellipsis-dev]

👍 Looks good to me! Incremental review on 249eefe in 1 minute and 32 seconds

More details

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

1. skyvern/forge/sdk/executor/async_executor.py:94

• Draft comment:
  Consider checking if background_tasks is not None before calling add_task to avoid potential NoneType errors.
• Reason this comment was not posted:
  Comment looked like it was already resolved.

2. skyvern/forge/sdk/services/observer_service.py:514

• Draft comment:
  Ensure that loop_value_extraction_goal is validated or checked for correctness before using it as a prompt to avoid unexpected behavior.
• Reason this comment was not posted:
  Comment did not seem useful.

3. skyvern/forge/sdk/services/observer_service.py:711

• Draft comment:
  Consider adding error handling for cases where generate_extraction_task_response does not contain a valid schema to prevent potential issues.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable:
  The code does not validate that schema exists or is valid before using it. However, looking at the code: 1. The schema is typed as Optional (dict[str, Any] | list | None) 2. Both ExtractionBlockYAML and ExtractionBlock appear to accept None as a valid schema value 3. There's no evidence that a missing schema would cause issues 4. The code is already handling other error cases explicitly where needed
  I may be missing some context about the ExtractionBlock requirements - perhaps a None or invalid schema causes issues downstream that aren't visible here.
  While that's possible, the typing and usage suggests schema is optional. Without clear evidence that missing schema causes problems, we should assume the current implementation is intentional.
  Delete the comment. The code appears to handle optional schema by design, and there's no clear evidence that additional error handling is needed.

4. skyvern/forge/sdk/services/observer_service.py:743

• Draft comment:
  Consider adding a check to ensure navigation_goal is not None before proceeding to avoid potential issues.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable:
  The navigation_goal parameter comes from plan which already has a default empty string value. Even if somehow navigation_goal was None, the NavigationBlock and NavigationBlockYAML would still be created - they don't have any validation that would fail on None. The code would continue to work. The comment is suggesting defensive programming but there's no clear failure case it would prevent.
  I could be wrong about the NavigationBlock/YAML handling of None values - there could be validation I'm not seeing in those classes.
  Looking at the usage, navigation_goal is just passed through to constructors and stored as a string field. Even if it was None, Python would handle that gracefully. There's no evidence of validation that would fail.
  The suggested check is unnecessary since navigation_goal already has a default empty string value and None would not cause issues.

5. skyvern/forge/sdk/routes/agent_protocol.py:1128

• Draft comment:
  Consider adding validation to ensure data.user_prompt is not None or empty to avoid potential issues.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable:
  Since CruiseRequest is a Pydantic model (imported from schemas), it likely already has built-in validation. FastAPI automatically validates request bodies using Pydantic. If user_prompt was required, it would be enforced by the schema. If it's optional, then None/empty would be valid values. Without seeing the CruiseRequest schema, we can't be certain either way, but FastAPI's built-in validation makes this comment likely unnecessary.
  I haven't seen the CruiseRequest schema definition, so I can't be 100% certain about its validation rules. There could be edge cases where additional validation would be valuable.
  Even without seeing the schema, FastAPI's built-in Pydantic validation is robust and would handle basic null/empty checks. If more complex validation was needed, it should be defined in the schema itself, not ad-hoc in the route.
  Delete the comment since FastAPI/Pydantic already provides robust input validation, and any additional validation requirements should be defined in the schema rather than the route handler.

Workflow ID: wflow_AczFZNu7fObPjcCI

---

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 249eefe in 2 minutes and 54 seconds

More details

• Looked at 1231 lines of code in 12 files
• Skipped 0 files when reviewing.
• Skipped posting 7 drafted comments based on config settings.

1. skyvern/forge/sdk/executor/async_executor.py:94

• Draft comment:
  Consider refactoring the repeated if background_tasks: check before background_tasks.add_task(...) calls into a utility function or handle it within add_task to reduce code duplication. This pattern is repeated in execute_task, execute_workflow, and execute_cruise.
• Reason this comment was not posted:
  Confidence changes required: 50%
  The code in async_executor.py has a repeated pattern where background_tasks is checked for None before adding a task. This pattern is repeated in multiple methods, and it would be more efficient to handle this check within the add_task method itself or create a utility function to handle this logic. This would reduce code duplication and potential errors.

2. skyvern/forge/sdk/services/observer_service.py:771

• Draft comment:
  Seeding the random generator with os.urandom(16) is unnecessary for generating random strings. Consider removing the random.seed() call for simplicity, as os.urandom() is already secure.
• Reason this comment was not posted:
  Confidence changes required: 50%
  In observer_service.py, the _generate_random_string function uses os.urandom(16) to seed the random generator. This is unnecessary because random.seed() is not needed for cryptographic purposes, and os.urandom() is already a secure random generator. This could be simplified by removing the seeding step.

3. skyvern/forge/sdk/services/observer_service.py:127

• Draft comment:
  Consider refactoring run_observer_cruise to have a single return point for better readability and maintainability. Currently, it has multiple return points with None.
• Reason this comment was not posted:
  Confidence changes required: 50%
  In observer_service.py, the run_observer_cruise function has multiple return points with None. This could be refactored to have a single return point for better readability and maintainability.

4. skyvern/forge/sdk/services/observer_service.py:126

• Draft comment:
  Instead of logging an error and returning None, consider raising an exception to ensure the caller is aware of the failure and can handle it appropriately. This applies to other similar return points in run_observer_cruise.
• Reason this comment was not posted:
  Confidence changes required: 50%
  In observer_service.py, the run_observer_cruise function logs an error and returns None if certain conditions are not met. However, it might be more appropriate to raise exceptions in these cases to ensure that the caller is aware of the failure and can handle it appropriately.

5. skyvern/forge/sdk/services/observer_service.py:151

• Draft comment:
  Consider explicitly setting a default value for int_max_iterations_override to avoid unexpected behavior when max_iterations_override is not provided. Currently, it defaults to None.
• Reason this comment was not posted:
  Confidence changes required: 50%
  In observer_service.py, the run_observer_cruise function uses int_max_iterations_override to determine the number of iterations. However, if max_iterations_override is not provided, it defaults to None, which could lead to unexpected behavior. It would be safer to explicitly set a default value for int_max_iterations_override.

6. skyvern/forge/sdk/services/observer_service.py:206

• Draft comment:
  Consider explicitly setting a default value for int_max_iterations_override to avoid unexpected behavior when max_iterations_override is not provided. Currently, it defaults to None.
• Reason this comment was not posted:
  Confidence changes required: 50%
  In observer_service.py, the run_observer_cruise function uses int_max_iterations_override to determine the number of iterations. However, if max_iterations_override is not provided, it defaults to None, which could lead to unexpected behavior. It would be safer to explicitly set a default value for int_max_iterations_override.

7. skyvern/forge/sdk/services/observer_service.py:154

• Draft comment:
  Consider explicitly setting a default value for int_max_iterations_override to avoid unexpected behavior when max_iterations_override is not provided. Currently, it defaults to None.
• Reason this comment was not posted:
  Confidence changes required: 50%
  In observer_service.py, the run_observer_cruise function uses int_max_iterations_override to determine the number of iterations. However, if max_iterations_override is not provided, it defaults to None, which could lead to unexpected behavior. It would be safer to explicitly set a default value for int_max_iterations_override.

Workflow ID: wflow_FsVKNMgbIEfzuisZ

---

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