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

Open source TOTP #1677

Merged
merged 1 commit into from
Jan 30, 2025
Merged

Open source TOTP #1677

merged 1 commit into from
Jan 30, 2025

Conversation

wintonzheng
Copy link
Contributor

@wintonzheng wintonzheng commented Jan 30, 2025

Important

Add TOTP support with new API route, parsing template, and database storage for TOTP codes.

  • API Changes:
    • Add totp_router to api_app.py with prefix /api/v1/totp.
    • New endpoint save_totp_code in totp.py to save TOTP codes.
  • Template:
    • Add parse-verification-code.j2 for parsing 2FA/MFA codes from messages.
  • Database:
    • Add create_totp_code function in client.py to store TOTP codes in the database.

This description was created by Ellipsis for 1d6e28c. It will automatically update as commits are pushed.

<!-- ELLIPSIS_HIDDEN -->

> [!IMPORTANT]
> Move TOTP functionality from `cloud` to `skyvern`, including routing, database methods, and prompts.
>
>   - **Routing**:
>     - Remove `totp_router` from `cloud/app.py` and add it to `skyvern/forge/api_app.py`.
>   - **Database**:
>     - Remove `create_totp_code` from `cloud/db/db_client.py` and add it to `skyvern/forge/sdk/db/client.py`.
>     - Remove `TOTPCodeModel` import from `cloud/db/db_client.py` and add it to `skyvern/forge/sdk/db/client.py`.
>   - **Prompts**:
>     - Rename `cloud/prompts/cloud/parse-verification-code.j2` to `skyvern/forge/prompts/skyvern/parse-verification-code.j2`.
>   - **Misc**:
>     - Rename `cloud/routes/totp.py` to `skyvern/forge/sdk/routes/totp.py` and update imports accordingly.
>
> <sup>This description was created by </sup>[<img alt="Ellipsis" src="https://img.shields.io/badge/Ellipsis-blue?color=175173">](https://www.ellipsis.dev?ref=Skyvern-AI%2Fskyvern-cloud&utm_source=github&utm_medium=referral)<sup> for f469f4850f253c90865c3795f1257a4f42f4370b. It will automatically update as commits are pushed.</sup>

<!-- ELLIPSIS_HIDDEN -->
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! Incremental review on 1d6e28c in 1 minute and 26 seconds

More details
  • Looked at 134 lines of code in 4 files
  • Skipped 0 files when reviewing.
  • Skipped posting 3 drafted comments based on config settings.
1. skyvern/forge/sdk/routes/totp.py:42
  • Draft comment:
    Consider adding error handling for the LLM API call in parse_totp_code to manage potential exceptions or errors that might occur during the API call.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable:
    The LLM API call could fail for various reasons (network issues, API limits, etc). Currently there's no explicit error handling. However, we don't know if app.SECONDARY_LLM_API_HANDLER already handles errors internally. We also don't know what the standard error handling patterns are in this codebase. The comment is reasonable but speculative without more context.
    I don't know if the LLM handler already has built-in error handling. The codebase might have global error handling or specific patterns I'm unaware of.
    While the concern is valid, keeping this comment without understanding the existing error handling infrastructure could lead to unnecessary or redundant error handling.
    Delete the comment since we don't have strong evidence that additional error handling is needed without understanding the broader error handling context.
2. skyvern/forge/sdk/db/client.py:1927
  • Draft comment:
    Consider adding exception handling for database operations in create_totp_code to manage potential SQLAlchemy errors or other exceptions.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable:
    While the suggestion is technically valid since exception handling could be added, I notice that:
  1. Many similar methods in the file don't have explicit exception handling
  2. The method is already wrapped in an async context manager which provides some error handling
  3. SQLAlchemy will raise exceptions by default which will propagate up
  4. The comment doesn't specify what types of exceptions to handle or how to handle them
    The comment points out a real inconsistency in error handling patterns across the codebase. Some methods have explicit error handling while others don't. Maybe this inconsistency should be addressed.
    While there is an inconsistency, this kind of architectural decision about error handling patterns should be made at the codebase level, not through one-off PR comments about individual methods. The current approach of letting errors propagate up seems intentional.
    The comment should be deleted. While technically correct, it suggests a pattern change that should be handled at an architectural level, not through individual method comments. The current error handling approach seems intentional.
3. skyvern/forge/prompts/skyvern/parse-verification-code.j2:11
  • Draft comment:
    Remove comments from the JSON output format in the parse-verification-code.j2 template to ensure valid JSON.
  • Reason this comment was not posted:
    Based on historical feedback, this comment is too similar to comments previously marked by users as bad.

Workflow ID: wflow_IYC0IWrQmA34wRld


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

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 1d6e28c in 1 minute and 55 seconds

More details
  • Looked at 134 lines of code in 4 files
  • Skipped 0 files when reviewing.
  • Skipped posting 3 drafted comments based on config settings.
1. skyvern/forge/sdk/routes/totp.py:44
  • Draft comment:
    Ensure code_resp is a dictionary and contains the 'code' key before accessing it to avoid potential errors.
  • Reason this comment was not posted:
    Comment did not seem useful.
2. skyvern/forge/sdk/db/client.py:1927
  • Draft comment:
    Consider adding exception handling for database operations to manage potential errors gracefully.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable:
  1. The create_totp_code function uses async with self.Session() which provides basic error handling.
  2. Looking at similar functions in the file (e.g. create_workflow_parameter, create_action), they don't have explicit try/except blocks.
  3. The Session context manager already handles basic database errors.
  4. The comment is vague and doesn't specify what types of errors should be handled or how.
  5. If error handling was really needed, it would be better to handle it consistently across all similar functions.
    The comment points out a real difference between this function and some other functions in the file that do have explicit error handling. Maybe there's a good reason those other functions have extra error handling?
    While some functions do have explicit error handling, they are typically more complex operations or have special error cases. This is a simple CRUD operation similar to many others in the file that rely on the Session context manager. Adding inconsistent error handling could make the code more confusing.
    The comment should be deleted. The suggestion is too vague to be actionable, and the current error handling through the Session context manager is consistent with similar functions in the codebase.
3. skyvern/forge/prompts/skyvern/parse-verification-code.j2:11
  • Draft comment:
    Remove comments from the JSON structure to ensure it is valid JSON.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable:
    This is a valid concern since the prompt explicitly states "MAKE SURE YOU OUTPUT VALID JSON... no comments (//)". However, these comments are in the example/template showing what fields mean - they aren't in the actual output format. The AI model will understand to not include comments in its output. The comments here serve as documentation.
    The comment could be correct if we want to be extremely strict about showing only valid JSON in examples. Having comments in an example could theoretically confuse an AI model.
    The comments are clearly part of the documentation/explanation, not the actual output format. Line 7 already explicitly tells the AI not to include comments in its output.
    The comment should be deleted as it misunderstands the purpose of the comments in the example JSON structure, which are for documentation rather than part of the output format.

Workflow ID: wflow_KUNWozgPCEOQFRNi


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

@suchintan suchintan merged commit d522c57 into main Jan 30, 2025
7 checks passed
@suchintan suchintan deleted the suchintan.open-source-totp branch January 30, 2025 06:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants