enh: implement new ruff standards - #967
Open
taylorwilsdon wants to merge 3 commits into
Open
Conversation
…into new_ruff # Conflicts: # gmail/gmail_tools.py
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughThis PR modernizes Python type annotations across authentication, core utilities, Google service integrations, and tool interfaces. It also pins Ruff, standardizes exception logging, adds timezone-aware attachment expiry handling, tightens selected validation paths, and updates related tests and schemas. ChangesRepository modernization
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| f"Failed to start the OAuth flow for {user_display_name}: {e}", | ||
| exc_info=True, | ||
| error_text = f"Could not initiate authentication for {user_display_name} due to an unexpected error: {e!s}" | ||
| logger.exception( |
| async def handle_auth_callback( | ||
| scopes: List[str], | ||
| scopes: list[str], | ||
| authorization_response: str, |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@auth/google_auth.py`:
- Around line 618-627: Update the exception handlers around the OAuth
authentication flow and the corresponding handling near the later referenced
path so neither logs nor raised user-facing errors interpolate the raw
exception. Log only sanitized, contextual messages without traceback details,
raise a generic actionable error, and preserve the original exception solely
through exception chaining.
In `@auth/oauth21_session_store.py`:
- Around line 111-120: Update SessionContext.metadata to use a default
consistent with its declared type, preferably by assigning
field(default_factory=dict) and removing the None normalization from
__post_init__; otherwise annotate it as dict[str, Any] | None while preserving
the existing initialization behavior.
In `@tests/auth/test_auth_info_middleware.py`:
- Line 13: Fix the unused method-argument warnings in set_state at
tests/auth/test_auth_info_middleware.py:13 and the corresponding method at
tests/auth/test_oauth21_session_store.py:210 by renaming or removing unused
arguments, or adding targeted per-line suppressions; do not rely on a global
ARG002 ignore.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: c9833478-bc71-4cb0-a31b-4a7077658638
📒 Files selected for processing (109)
.github/workflows/ruff.ymlauth/auth_info_middleware.pyauth/credential_store.pyauth/external_oauth_provider.pyauth/google_auth.pyauth/mcp_session_middleware.pyauth/oauth21_session_store.pyauth/oauth_callback_server.pyauth/oauth_config.pyauth/oauth_responses.pyauth/oauth_types.pyauth/permissions.pyauth/port_resolver.pyauth/scopes.pyauth/service_decorator.pycore/api_enablement.pycore/attachment_storage.pycore/comments.pycore/config.pycore/context.pycore/http_utils.pycore/log_formatter.pycore/server.pycore/tool_registry.pycore/tool_tier_loader.pycore/utils.pygappsscript/apps_script_tools.pygcalendar/calendar_helpers.pygcalendar/calendar_tools.pygchat/chat_tools.pygcontacts/contacts_helpers.pygcontacts/contacts_tools.pygdocs/docs_helpers.pygdocs/docs_markdown_writer.pygdocs/docs_structure.pygdocs/docs_tables.pygdocs/docs_tools.pygdocs/managers/__init__.pygdocs/managers/batch_operation_manager.pygdocs/managers/header_footer_manager.pygdocs/managers/table_operation_manager.pygdocs/managers/validation_manager.pygdocs/operation_schemas.pygdrive/drive_helpers.pygdrive/drive_tools.pygforms/forms_tools.pygmail/gmail_helpers.pygmail/gmail_tools.pygsearch/search_tools.pygsheets/__init__.pygsheets/sheets_helpers.pygsheets/sheets_tools.pygslides/slides_helpers.pygslides/slides_tools.pygtasks/tasks_tools.pymain.pypyproject.tomltests/auth/test_auth_info_middleware.pytests/auth/test_google_auth_callback_refresh_token.pytests/auth/test_google_auth_pkce.pytests/auth/test_google_auth_prompt_selection.pytests/auth/test_google_auth_refresh_persistence.pytests/auth/test_google_auth_stdio_preflight.pytests/auth/test_httplib2_timeout.pytests/auth/test_oauth21_session_store.pytests/auth/test_oauth_callback_server.pytests/core/test_comments.pytests/core/test_credentials_dir_permissions.pytests/core/test_pdf_image_utils.pytests/core/test_service_decorator_cleanup.pytests/core/test_start_google_auth.pytests/core/test_storage.pytests/core/test_user_google_email_defaults.pytests/core/test_validate_file_path.pytests/gappsscript/manual_test.pytests/gappsscript/test_apps_script_tools.pytests/gcalendar/test_focus_time.pytests/gcalendar/test_out_of_office.pytests/gchat/test_chat_tools.pytests/gcontacts/test_contacts_integration.pytests/gcontacts/test_contacts_tools.pytests/gcontacts/test_contacts_tools_v2.pytests/gcontacts/test_contacts_tools_v3.pytests/gdocs/golden/docs_tool_schemas.jsontests/gdocs/test_docs_markdown.pytests/gdocs/test_insert_doc_tab_response.pytests/gdocs/test_paragraph_style.pytests/gdocs/test_strikethrough.pytests/gdocs/test_suggestions_view_mode.pytests/gdocs/test_table_column_operations.pytests/gdocs/test_table_row_operations.pytests/gdrive/test_drive_file_content.pytests/gdrive/test_drive_tools.pytests/gdrive/test_url_download_limits.pytests/gforms/test_forms_tools.pytests/gmail/test_body_format.pytests/gmail/test_draft_gmail_message.pytests/gmail/test_forward_gmail_message.pytests/gmail/test_get_gmail_attachment_content.pytests/gmail/test_inline_image_attachments.pytests/gmail/test_modify_gmail_message_labels_schema.pytests/gmail/test_thread_ownership_helpers.pytests/gsheets/test_format_sheet_range.pytests/gsheets/test_resize_sheet_dimensions.pytests/helpers.pytests/test_cwe77_ruff_workflow.pytests/test_main_permissions_tier.pytests/test_permissions.pytests/test_scopes.py
💤 Files with no reviewable changes (3)
- tests/core/test_storage.py
- tests/auth/test_google_auth_prompt_selection.py
- tests/gdocs/test_insert_doc_tab_response.py
Comment on lines
618
to
627
| except FileNotFoundError as e: | ||
| error_text = f"OAuth client credentials not found: {e}. Please either:\n1. Set environment variables: GOOGLE_OAUTH_CLIENT_ID and GOOGLE_OAUTH_CLIENT_SECRET\n2. Ensure '{CONFIG_CLIENT_SECRETS_PATH}' file exists" | ||
| logger.error(error_text, exc_info=True) | ||
| logger.exception(error_text) | ||
| raise Exception(error_text) | ||
| except Exception as e: | ||
| error_text = f"Could not initiate authentication for {user_display_name} due to an unexpected error: {str(e)}" | ||
| logger.error( | ||
| f"Failed to start the OAuth flow for {user_display_name}: {e}", | ||
| exc_info=True, | ||
| error_text = f"Could not initiate authentication for {user_display_name} due to an unexpected error: {e!s}" | ||
| logger.exception( | ||
| f"Failed to start the OAuth flow for {user_display_name}", | ||
| ) | ||
| raise Exception(error_text) |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Do not expose raw OAuth exceptions in logs or user-facing errors.
error_text/error_msg include {e!s}, and logger.exception records the exception traceback as well. OAuth and client-configuration exceptions can contain credential or request data; CodeQL flags these paths. Log a sanitized message and raise a generic actionable error, preserving the original exception only through exception chaining.
Proposed fix
except FileNotFoundError as e:
- error_text = f"OAuth client credentials not found: {e}. Please either:\n1. Set environment variables: GOOGLE_OAUTH_CLIENT_ID and GOOGLE_OAUTH_CLIENT_SECRET\n2. Ensure '{CONFIG_CLIENT_SECRETS_PATH}' file exists"
- logger.exception(error_text)
- raise Exception(error_text)
+ logger.error("OAuth client credentials were not found", exc_info=False)
+ raise Exception(
+ "OAuth client credentials were not found. Configure the OAuth environment "
+ "variables or provide the client secrets file."
+ ) from e
except Exception as e:
- error_text = f"Could not initiate authentication for {user_display_name} due to an unexpected error: {e!s}"
- logger.exception(f"Failed to start the OAuth flow for {user_display_name}")
- raise Exception(error_text)
+ logger.error(
+ "Failed to start OAuth flow for %s (%s)",
+ user_display_name,
+ type(e).__name__,
+ )
+ raise Exception(
+ f"Could not initiate authentication for {user_display_name}."
+ ) from eAlso applies to: 1372-1375
🧰 Tools
🪛 GitHub Check: CodeQL
[failure] 620-620: Clear-text logging of sensitive information
This expression logs sensitive data (secret) as clear text.
[failure] 625-625: Clear-text logging of sensitive information
This expression logs sensitive data (password) as clear text.
This expression logs sensitive data (password) as clear text.
This expression logs sensitive data (password) as clear text.
This expression logs sensitive data (password) as clear text.
This expression logs sensitive data (password) as clear text.
This expression logs sensitive data (password) as clear text.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@auth/google_auth.py` around lines 618 - 627, Update the exception handlers
around the OAuth authentication flow and the corresponding handling near the
later referenced path so neither logs nor raised user-facing errors interpolate
the raw exception. Log only sanitized, contextual messages without traceback
details, raise a generic actionable error, and preserve the original exception
solely through exception chaining.
Source: Linters/SAST tools
Comment on lines
+111
to
120
| session_id: str | None = None | ||
| user_id: str | None = None | ||
| auth_context: Any | None = None | ||
| request: Any | None = None | ||
| metadata: dict[str, Any] = None | ||
| issuer: str | None = None | ||
|
|
||
| def __post_init__(self): | ||
| if self.metadata is None: | ||
| self.metadata = {} |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Keep SessionContext.metadata consistent with its default.
metadata is annotated as dict[str, Any] but defaults to None and is explicitly normalized in __post_init__. Use dict[str, Any] | None or field(default_factory=dict).
Proposed fix
- metadata: dict[str, Any] = None
+ metadata: dict[str, Any] | None = None📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| session_id: str | None = None | |
| user_id: str | None = None | |
| auth_context: Any | None = None | |
| request: Any | None = None | |
| metadata: dict[str, Any] = None | |
| issuer: str | None = None | |
| def __post_init__(self): | |
| if self.metadata is None: | |
| self.metadata = {} | |
| session_id: str | None = None | |
| user_id: str | None = None | |
| auth_context: Any | None = None | |
| request: Any | None = None | |
| metadata: dict[str, Any] | None = None | |
| issuer: str | None = None | |
| def __post_init__(self): | |
| if self.metadata is None: | |
| self.metadata = {} |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@auth/oauth21_session_store.py` around lines 111 - 120, Update
SessionContext.metadata to use a default consistent with its declared type,
preferably by assigning field(default_factory=dict) and removing the None
normalization from __post_init__; otherwise annotate it as dict[str, Any] | None
while preserving the existing initialization behavior.
| self.session_id = "session-123" | ||
|
|
||
| async def set_state(self, key, value, serializable=True): # noqa: ARG002 | ||
| async def set_state(self, key, value, serializable=True): |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
rg -n 'ARG002|select|extend-select|ignore' pyproject.toml
uvx ruff@0.16.0 check tests/auth/test_auth_info_middleware.py tests/auth/test_oauth21_session_store.py --select ARG002Repository: taylorwilsdon/google_workspace_mcp
Length of output: 572
🏁 Script executed:
#!/bin/bash
set -e
echo "===== pyproject ruff refs ====="
sed -n '1,190p' pyproject.toml | rg -n 'ARG002|ruff|select|extend-select|ignore|per-file-ignores' -C 2 || true
echo
echo "===== relevant files lines ====="
sed -n '1,35p' tests/auth/test_auth_info_middleware.py
sed -n '190,225p' tests/auth/test_oauth21_session_store.py
echo
echo "===== ruff availability/search for pinned version ====="
(command -v ruff && ruff --version || true)
rg -n "ruff|ARG002" pyproject.toml uv.lock README.md .github -S 2>/dev/null || trueRepository: taylorwilsdon/google_workspace_mcp
Length of output: 11518
🏁 Script executed:
#!/bin/bash
set -e
echo "===== test files exact content around target methods ====="
awk 'NR>=1 && NR<=30 {print NR": "$0}' tests/auth/test_auth_info_middleware.py
awk 'NR>=190 && NR<=222 {print NR": "$0}' tests/auth/test_oauth21_session_store.py
echo
echo "===== current local Ruff ARG002 behavior ====="
ruff check tests/auth/test_auth_info_middleware.py tests/auth/test_oauth21_session_store.py --select ARG002 --output-format=full || true
echo
echo "===== pyproject ignores summary ====="
awk '/^\[tool\.ruff\.lint\]|^\[tool\.ruff\.lint\.per-file-ignores\]|^ignore\s*=\s*\[|ARG002/' pyproject.toml || trueRepository: taylorwilsdon/google_workspace_mcp
Length of output: 3563
Fix the stale ARG002 suppressions
Ruff still reports these unused method arguments, and the Ruff config has no ARG002 ignore. Replace the removed noqa with either per-line suppressions or rename/remove the unused args:
tests/auth/test_auth_info_middleware.py#L13tests/auth/test_oauth21_session_store.py#L210
📍 Affects 2 files
tests/auth/test_auth_info_middleware.py#L13-L13(this comment)tests/auth/test_oauth21_session_store.py#L210-L210
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/auth/test_auth_info_middleware.py` at line 13, Fix the unused
method-argument warnings in set_state at
tests/auth/test_auth_info_middleware.py:13 and the corresponding method at
tests/auth/test_oauth21_session_store.py:210 by renaming or removing unused
arguments, or adding targeted per-line suppressions; do not rely on a global
ARG002 ignore.
14 tasks
7 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit