test: comprehensive E2E test suite#522
Closed
Coldaine wants to merge 30 commits into
Closed
Conversation
Integrated Google Apps Script API support to enable management and automation of Apps Script projects directly through the MCP server. New Features: - Created gappsscript module with 5 consolidated tools: * manage_script_project: Create, get, get content, update content * manage_script_version: Create, get, list versions * manage_script_deployment: Full CRUD for deployments * execute_script: Run Apps Script functions remotely * monitor_script_execution: List processes and get metrics - Added comprehensive Apps Script API scopes to auth/scopes.py: * script.projects - Project management * script.deployments - Deployment operations * script.metrics - Execution metrics * script.processes - Process monitoring - Configured script service in auth/service_decorator.py with proper service configuration and scope group mappings - Integrated script tools into main.py tool loading system * Added 'script' to CLI tool choices * Added script module import * Added ⚙️ icon for script service Implementation Details: - Consolidated 16 individual Node.js tools into 5 Python tools - Used operation parameter pattern for intuitive resource management - Follows existing codebase patterns (@require_google_service decorator) - Full async/await support with proper error handling - Comprehensive docstrings for all tools This adds Apps Script project management capabilities without breaking any existing functionality. The consolidation approach reduces tool count by 68% compared to the original Node.js implementation while maintaining full API coverage.
Consolidated Task management tools using operation parameter pattern: Before (12 tools): - list_task_lists, get_task_list, create_task_list, update_task_list, delete_task_list - list_tasks, get_task, create_task, update_task, delete_task, move_task - clear_completed_tasks After (3 tools): 1. manage_task_list - CRUD operations for task lists Operations: list | get | create | update | delete 2. manage_task - Full task management including movement Operations: list | get | create | update | delete | move Preserves all advanced features: - Structured task hierarchies with parent-child relationships - Multi-page pagination support - Comprehensive filtering (completed, deleted, hidden, etc.) - Due date boundary adjustments - Task positioning and movement between lists 3. clear_completed_tasks - Kept as distinct operation All existing functionality preserved: - Helper functions: get_structured_tasks, serialize_tasks, _adjust_due_max_for_tasks_api - StructuredTask class for hierarchical task representation - Complex pagination logic for large task lists - All API parameters and options maintained Benefits: - 75% reduction in tool count (12 → 3) - Consistent operation parameter pattern - Easier discoverability for LLM agents - Reduced cognitive load for API consumers - All functionality preserved, zero breaking changes
Documents full consolidation strategy for reducing 77 tools to 45 tools (41% reduction). Completed Work: - Apps Script: Added 5 new consolidated tools (ported from Node.js) - Tasks: Consolidated 12 → 3 tools (75% reduction) Remaining Work Documented: - Gmail: 12 → 6 tools (detailed implementation plan) - Docs: 14 → 7 tools (consolidation strategy) - Drive, Sheets, Forms, Slides, Chat, Search (Phase 2 & 3) Includes: - Detailed before/after tool listings - Implementation checklists - Code pattern examples - Helper function preservation notes - Success criteria and benefits - Commit history tracking This plan enables continuation of consolidation work with clear guidance.
Tool consolidation: - get_gmail_content (NEW): consolidates 5 tools * message, messages_batch, attachment, thread, threads_batch operations - manage_gmail_label (ENHANCED): added "list" operation * list, create, update, delete operations - modify_gmail_labels (NEW): consolidates 2 tools * single, batch operations Unchanged tools: - search_gmail_messages - send_gmail_message - draft_gmail_message All helper functions preserved. Full functionality maintained. Current progress: 77 → 62 tools (19% toward 45-tool target)
Co-authored-by: Coldaine <158332486+Coldaine@users.noreply.github.com>
Co-authored-by: Coldaine <158332486+Coldaine@users.noreply.github.com>
1. Fix manage_doc_operations scope: docs_read -> docs_write - batch_update operation requires write permissions - Prevents 'insufficient authentication scopes' errors 2. Fix TableOperationManager table targeting bug - Track created table by insertion index instead of assuming last - Prevents populating wrong table when inserting before existing ones - Adds _created_table_index tracking 3. Fix bullet list range calculation - Include newline in range (len(text) + 1) - Prevents INVALID_ARGUMENT error from Docs API 4. Fix image width/height defaults: 0 -> None - Makes image dimensions optional for auto-sizing - Prevents zero-sized images that Docs API rejects - Updates both docs_tools.py and docs_helpers.py
…011CV1exjSRJGijyVnq9kYoE Approved: Comprehensive tool consolidation (77→62 tools, 19% reduction) with Apps Script integration. Well-documented with consistent operation-based patterns. All functionality reportedly preserved. Note: Consider adding automated tests to verify functionality preservation in future consolidation phases.
- Added pytest infrastructure with unit and integration tests - Migrated live_test.py to tests/integration/test_gdocs_live.py - Added Bitwarden-based secret management scripts and Ansible role - Changed default port to 4100 - Added PauseMiddleware for maintenance mode - Updated documentation and manifest
Phase 2: Docs Consolidation, Testing Suite & Secret Management
* chore: remove tool tier system and expose all tools by default - Delete tier loader, registry, and tiers config - Simplify main tool loading logic - Update README and consolidation plan to reflect removal * feat: consolidate Drive tools and adopt discriminated union payloads - Add manage_drive_file & extended manage_drive_permissions - Refactor modify_doc_content and manage_task to use TypedDict unions - Update README and consolidation plan for new Drive + Docs/Tasks model * chore: update ancillary service tools and configuration - Adjust logging and server integration - Minor updates to Chat, Forms, Search, Sheets, Slides tools - pyproject dependency/version adjustments * chore(sheets): commit pending __init__ change post consolidation sweep
Validated that Google Desktop OAuth clients support localhost redirect URIs without requiring explicit configuration in Google Cloud Console. This confirms the existing documentation is accurate. Validated by: Patrick MacLyman (@Coldaine) Date: 2026-01-05 Official docs referenced: - https://developers.google.com/identity/protocols/oauth2/native-app - https://developers.google.com/identity/protocols/oauth2/resources/loopback-migration - https://support.google.com/cloud/answer/15549257 🤖 Generated with [Claude Code](https://claude.ai/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughPhase‑1 consolidation: many per-operation Google Workspace tools replaced with operation-driven APIs; Apps Script support added; tool‑tier system and registry removed; default server port changed from 8000→4100; Bitwarden-backed secret management, pause middleware, and extensive tests/docs were added. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User / CI
participant Loader as Secrets Loader Script
participant Map as Mapping File (JSON)
participant BW as Bitwarden CLI
participant Env as .env / Shell
participant MCP as MCP Server
User->>Loader: run loader (BW_SESSION present)
Loader->>Map: read mapping (env_key → item_id, field)
Loader->>BW: bw get item {item_id}
BW-->>Loader: item JSON (fields, login)
Loader->>Loader: extract requested field or fallback to login.username/password
alt field found
Loader->>Env: emit/write export ENV_KEY=value
else missing
Loader->>User: warn and continue
end
Loader->>MCP: start or provide .env for MCP process
MCP->>MCP: reads GOOGLE_OAUTH_CLIENT_ID/SECRET for OAuth flows
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 085137d7dc
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
76
to
80
| parser.add_argument('--tools', nargs='*', | ||
| choices=['gmail', 'drive', 'calendar', 'docs', 'sheets', 'chat', 'forms', 'slides', 'tasks', 'search'], | ||
| choices=['gmail', 'drive', 'calendar', 'docs', 'sheets', 'chat', 'forms', 'slides', 'tasks', 'search', 'script'], | ||
| help='Specify which tools to register. If not provided, all tools are registered.') | ||
| parser.add_argument('--tool-tier', choices=['core', 'extended', 'complete'], | ||
| help='Load tools based on tier level. Can be combined with --tools to filter services.') | ||
| parser.add_argument('--transport', choices=['stdio', 'streamable-http'], default='stdio', | ||
| help='Transport mode: stdio (default) or streamable-http') |
There was a problem hiding this comment.
This change drops --tool-tier from the CLI parser, but the deployment entrypoints still append that flag when TOOL_TIER is set (checked Dockerfile CMD and helm-chart/workspace-mcp/templates/deployment.yaml args), so configured environments now fail at process startup with an unrecognized argument instead of launching the server.
Useful? React with 👍 / 👎.
Comment on lines
27
to
29
| self.base_uri = os.getenv("WORKSPACE_MCP_BASE_URI", "http://localhost") | ||
| self.port = int(os.getenv("PORT", os.getenv("WORKSPACE_MCP_PORT", "8000"))) | ||
| self.port = int(os.getenv("PORT", os.getenv("WORKSPACE_MCP_PORT", "4100"))) | ||
| self.base_url = f"{self.base_uri}:{self.port}" |
There was a problem hiding this comment.
The fallback port was changed to 4100, but container/deployment defaults still assume 8000 (e.g., Dockerfile EXPOSE and health check use ${PORT:-8000}), so when neither PORT nor WORKSPACE_MCP_PORT is explicitly set, the app listens on 4100 while probes/clients target 8000 and report the service unhealthy.
Useful? React with 👍 / 👎.
Comment on lines
+550
to
+553
| service.projects() | ||
| .deployments() | ||
| .get(scriptId=script_id, deploymentId=deployment_id) | ||
| .execute |
There was a problem hiding this comment.
In the get_metrics branch, the code prepares metrics parameters but then calls projects().deployments().get(...), which returns deployment details rather than execution metrics; this makes metrics_granularity/metrics_fields ineffective and the operation cannot return the data its interface promises.
Useful? React with 👍 / 👎.
Comment on lines
+498
to
+500
| params = {"pageSize": page_size} | ||
| if page_token: | ||
| params["pageToken"] = page_token |
There was a problem hiding this comment.
The list_processes path requires a script_id argument but never adds it to the request parameters before calling service.processes().list(**params), so results are not constrained to the requested script and can include unrelated executions for the same user.
Useful? React with 👍 / 👎.
Contributor
There was a problem hiding this comment.
Pull request overview
Adds a large pytest-based test suite and expands supporting infrastructure (secret-loading utilities, middleware, and tool consolidation) for the Google Workspace MCP server.
Changes:
- Introduces extensive unit tests (Gmail/Drive/Docs/Calendar/OAuth2.1/security/rate-limiting) plus a live Docs integration test.
- Adds Bitwarden-based secret-loading scripts + Ansible skeleton and updates docs/config for this workflow.
- Refactors runtime behavior/tooling (default port to 4100, pause/maintenance middleware, tool-tier removal, new Apps Script + additional tool consolidations).
Reviewed changes
Copilot reviewed 65 out of 67 changed files in this pull request and generated 19 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/unit/test_security.py | New security-focused unit tests (headers, attachments, OAuth state/session security). |
| tests/unit/test_rate_limiting.py | New tests for retry/backoff + HTTP error handling + Gmail rate constants. |
| tests/unit/test_oauth21.py | New tests for OAuth2.1 session store/context and expiry normalization. |
| tests/unit/test_imports.py | New import smoke tests for docs/auth modules. |
| tests/unit/test_gmail_tools.py | New tests for Gmail helper functions (bodies/headers/attachments/message prep/URLs). |
| tests/unit/test_gmail_attachments.py | New tests focused on attachment extraction scenarios and edge cases. |
| tests/unit/test_drive_tools.py | New tests for Drive helper functions and query pattern detection. |
| tests/unit/test_docs_tools.py | New tests for Docs helpers and operation validation. |
| tests/unit/test_calendar_tools.py | New tests for Calendar helper validation/parsing functions. |
| tests/unit/conftest.py | New shared mocks/fixtures for Gmail/Calendar/Drive services. |
| tests/integration/test_gdocs_live.py | Live integration test for creating/reading a Doc. |
| tests/conftest.py | Session fixtures for integration tests and env loading. |
| secrets/bws_map.json | Adds BWS secret-id mapping file. |
| secrets/bitwarden_map.json | Adds Bitwarden item/field mapping template. |
| scripts/load_secrets_bws.py | Adds a BWS CLI-based env exporter. |
| scripts/load-secrets.sh | Adds shell helper to load Bitwarden secrets into current session. |
| scripts/load-secrets.ps1 | Adds PowerShell helper to load Bitwarden secrets into current session. |
| scripts/load-secrets-bws.ps1 | Adds PowerShell helper for BWS-based loading. |
| scripts/generate_env_file.py | Adds script to write fetched secrets into a .env file. |
| scripts/bitwarden_env_loader.py | Adds Python Bitwarden CLI loader emitting export ... lines. |
| ansible/roles/secret-management/tasks/main.yml | Adds Ansible role skeleton to render secrets into an env file. |
| ansible/deploy-secret-management.yml | Adds playbook wiring the secret-management role. |
| pyproject.toml | Adds pytest config; adjusts package-data config. |
| manifest.json | Updates default redirect URI + port defaults to 4100. |
| main.py | Removes tool-tier logic; adds script service; changes default port to 4100. |
| fastmcp_server.py | Removes tool-registry tier filtering wiring. |
| core/tool_tiers.yaml | Deletes tier configuration file. |
| core/tool_tier_loader.py | Deletes tier loader module. |
| core/tool_registry.py | Deletes tool registry module. |
| core/log_formatter.py | Removes tier-related formatting; currently leaves dead code in _enhance_message. |
| core/utils.py | Broadens API-not-enabled detection to include SERVICE_DISABLED. |
| core/config.py | Changes default port to 4100. |
| core/server.py | Adds PauseMiddleware and health status reflecting paused state. |
| core/pause_middleware.py | New maintenance-mode middleware (pause via env var or flag file). |
| auth/scopes.py | Adds Apps Script scopes and maps script tool scopes. |
| auth/service_decorator.py | Adds Apps Script service + scope mappings. |
| auth/oauth_config.py | Updates default port to 4100. |
| auth/oauth_callback_server.py | Updates callback server defaults to 4100. |
| gappsscript/appsscript_tools.py | Adds consolidated Apps Script tools (projects/versions/deployments/execute/monitor). |
| gappsscript/init.py | Adds Apps Script package init. |
| gsheets/sheets_tools.py | Consolidates spreadsheet list/get and create/add-sheet operations; introduces multi-service decorator usage. |
| gsheets/init.py | Updates exports to match consolidated Sheets tool surface. |
| gslides/slides_tools.py | Consolidates presentation/page/thumbnail into one tool. |
| gforms/forms_tools.py | Consolidates form get/settings and response get/list into fewer tools. |
| gchat/chat_tools.py | Consolidates message get/search into one tool. |
| gsearch/search_tools.py | Consolidates site-restricted search behavior into search_custom. |
| gdocs/docs_helpers.py | Updates typing/docstrings for optional image sizing params. |
| gdocs/managers/table_operation_manager.py | Tracks created table index to target correct table during population. |
| README.md | Removes tool-tier docs; adds Bitwarden secret-management documentation; updates tool lists. |
| TESTING_PLAN.md | Adds testing plan documentation. |
| UPSTREAM_SYNC_GUIDE.md | Adds upstream sync process doc. |
| TODO_MAINTENANCE.md | Adds maintenance TODOs and sync reminders. |
| CONSOLIDATION_PLAN.md | Updates consolidation plan/status documentation. |
| PR_DESCRIPTION.md | Adds Phase 1 PR description doc. |
| PR_DESCRIPTION_PHASE2.md | Adds Phase 2 PR description doc. |
| REVIEW_COMMENTS.md | Adds consolidated review notes doc. |
| bug_fixes_review.md | Adds bug-fix review summary doc. |
| approval_review.txt | Adds approval text doc. |
| review_strategic.txt | Adds consolidation strategy notes doc. |
| .gitignore | Ignores pause flag + generated env files. |
| .gitattributes | Enforces LF generally; CRLF for PowerShell scripts. |
| .env.example | Adds example env vars (port value currently mismatched with new defaults). |
Comments suppressed due to low confidence (1)
core/log_formatter.py:83
_enhance_message()returns immediately, making the pattern-matching logic below unreachable. This is dead code and likely an accidental early return (the laterreturn messagealready covers the fallback). Remove the early return or the unreachable block so the function’s behavior is unambiguous.
def _enhance_message(self, message: str) -> str:
"""Enhance the log message with better formatting."""
# Handle common patterns for better visual appeal
return message
# Enabled tools messages
if "Enabled tools set for scope management" in message:
tools = message.split(": ")[-1]
return f"Scope management configured for tools: {tools}"
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+13
to
+16
| import ssl | ||
| import asyncio | ||
| import pytest | ||
| from unittest.mock import MagicMock, AsyncMock, patch, PropertyMock |
There was a problem hiding this comment.
There are unused imports here (e.g., asyncio, PropertyMock) that will fail ruff check unless explicitly ignored. Please remove unused imports or add targeted noqa annotations if they are intentional.
Comment on lines
+70
to
+76
| if not value: | ||
| print(f"Warning: field '{field}' not found for item {item_id} (env {env_key})", file=sys.stderr) | ||
| continue | ||
| # Emit POSIX style export; Windows PowerShell script wraps its own logic | ||
| outputs.append(f"export {env_key}='{value}'") | ||
| for line in outputs: | ||
| print(line) |
There was a problem hiding this comment.
The loader prints export KEY='value' without escaping quotes/newlines in the secret value. A secret containing a single quote will produce invalid shell output (and can be dangerous if eval’d). Use robust escaping when generating shell commands, or output a machine-readable format (JSON) and let shell wrappers handle quoting safely.
Comment on lines
+1
to
+4
| { | ||
| "GOOGLE_OAUTH_CLIENT_ID": "81e4469b-851b-42b2-b03c-b393006d2659", | ||
| "GOOGLE_OAUTH_CLIENT_SECRET": "b36a25a6-2d68-48d6-885c-b393006d272f" | ||
| } |
There was a problem hiding this comment.
bws_map.json appears to contain real Bitwarden Secrets Manager secret IDs. Even though they aren't the secret values, committing live secret identifiers enables easier exfiltration for anyone with vault access and conflicts with the repo’s “no real secrets committed” guidance. Replace these with placeholders (like bitwarden_map.json) or move this file to an untracked example template.
Comment on lines
+24
to
+30
| data = json.loads(result.stdout) | ||
| value = data.get("value") | ||
| if value: | ||
| # Print commands to set env vars | ||
| print(f"export {env_key}='{value}'") | ||
| except subprocess.CalledProcessError as e: | ||
| print(f"# Error fetching {env_key}: {e.stderr}", file=sys.stderr) |
There was a problem hiding this comment.
The script emits export {env_key}='{value}' without escaping single quotes/newlines in the secret value. If a secret contains ' (or other shell-significant characters), the generated command becomes syntactically invalid (and can be unsafe if copy/pasted). Use a robust shell-escaping strategy when printing export lines (or emit JSON and let the caller handle quoting).
Comment on lines
+6
to
+45
| MAP_PATH = os.path.join(os.path.dirname(os.path.dirname(__file__)), "secrets", "bws_map.json") | ||
| ENV_PATH = os.path.join(os.path.dirname(os.path.dirname(__file__)), ".env") | ||
|
|
||
| def main(): | ||
| if not os.path.exists(MAP_PATH): | ||
| print(f"Error: {MAP_PATH} not found", file=sys.stderr) | ||
| sys.exit(1) | ||
|
|
||
| with open(MAP_PATH, 'r') as f: | ||
| mapping = json.load(f) | ||
|
|
||
| env_content = [] | ||
|
|
||
| # Read existing .env if it exists to preserve other values | ||
| if os.path.exists(ENV_PATH): | ||
| with open(ENV_PATH, 'r') as f: | ||
| for line in f: | ||
| if line.strip() and not any(line.startswith(k + "=") for k in mapping.keys()): | ||
| env_content.append(line.strip()) | ||
|
|
||
| print("Fetching secrets from Bitwarden...") | ||
| for env_key, secret_id in mapping.items(): | ||
| try: | ||
| result = subprocess.run( | ||
| ["bws", "secret", "get", secret_id], | ||
| capture_output=True, | ||
| text=True, | ||
| check=True | ||
| ) | ||
| data = json.loads(result.stdout) | ||
| value = data.get("value") | ||
| if value: | ||
| env_content.append(f"{env_key}={value}") | ||
| print(f"Loaded {env_key}") | ||
| except subprocess.CalledProcessError as e: | ||
| print(f"Error fetching {env_key}: {e.stderr}", file=sys.stderr) | ||
|
|
||
| with open(ENV_PATH, 'w') as f: | ||
| f.write("\n".join(env_content) + "\n") | ||
|
|
There was a problem hiding this comment.
This script writes fetched secrets to a persistent .env file on disk. That contradicts the “Values are never persisted to disk” guarantee described in bitwarden_env_loader.py’s module docstring and increases the risk of accidental leakage. Consider generating to a clearly-named ignored file (e.g. .env.generated) with safe permissions, or remove this script if persistence is not intended.
Comment on lines
+1
to
+4
| import pytest | ||
| import re | ||
| import os | ||
| from gdocs.docs_tools import create_doc, get_doc_content |
There was a problem hiding this comment.
os is imported but unused in this integration test file, which will fail ruff check (F401) unless ignored. Remove the unused import or add a targeted noqa if intentional.
Comment on lines
+536
to
+556
| elif operation == "get_metrics": | ||
| if not deployment_id or not metrics_granularity: | ||
| raise ValueError( | ||
| "'deployment_id' and 'metrics_granularity' are required for get_metrics" | ||
| ) | ||
|
|
||
| params = { | ||
| "scriptId": script_id, | ||
| "metricsGranularity": metrics_granularity, | ||
| } | ||
| if metrics_fields: | ||
| params["fields"] = metrics_fields | ||
|
|
||
| result = await asyncio.to_thread( | ||
| service.projects() | ||
| .deployments() | ||
| .get(scriptId=script_id, deploymentId=deployment_id) | ||
| .execute | ||
| ) | ||
|
|
||
| return f"Metrics data for deployment {deployment_id}:\n{result}" |
There was a problem hiding this comment.
monitor_script_execution(operation="get_metrics") validates deployment_id / metrics_granularity and builds a params dict, but then ignores all of it and calls projects().deployments().get(...) (no deployment_id in the request body or metrics API call). As written, this won’t return metrics and the required arguments are effectively unused. Either implement the correct Apps Script metrics endpoint call (and use metrics_granularity/fields), or drop the unused parameters/operation until supported.
Comment on lines
+14
to
+19
| import base64 | ||
| import hashlib | ||
| import pytest | ||
| from datetime import datetime, timezone | ||
| from unittest.mock import MagicMock, patch | ||
|
|
There was a problem hiding this comment.
This file has multiple unused imports (base64, datetime/timezone, MagicMock, patch, etc.). Since CI runs ruff check, these will fail linting unless ruff is configured to ignore F401 in tests. Remove the unused imports or add explicit noqa comments where intentional.
Comment on lines
+1
to
+13
| import pytest | ||
| import sys | ||
|
|
||
| def test_gdocs_imports(): | ||
| """ | ||
| Verify that gdocs tools can be imported successfully. | ||
| Equivalent to the old smoke_test.py. | ||
| """ | ||
| try: | ||
| from gdocs.docs_tools import modify_doc_content, insert_doc_elements, manage_doc_operations | ||
| assert True | ||
| except ImportError as e: | ||
| pytest.fail(f"Failed to import Docs tools: {e}") |
There was a problem hiding this comment.
sys is imported but unused, which will fail ruff check. Also, the try/except ImportError blocks plus assert True are redundant in pytest—importing at module scope (or just inside the test) is enough and failures will already fail the test.
Comment on lines
+18
to
+22
| python "$(dirname "$0")/bitwarden_env_loader.py" | while read -r line; do | ||
| if [[ "$line" == export* ]]; then | ||
| eval "$line" | ||
| var_name=$(echo "$line" | cut -d' ' -f2 | cut -d'=' -f1) | ||
| echo "Set $var_name" |
There was a problem hiding this comment.
The eval "$line" statement is evaluating shell commands constructed from Bitwarden data (bitwarden_env_loader.py emits lines like export KEY='value') without safely handling embedded single quotes. If any secret value or env key contains a single quote followed by shell metacharacters (e.g. '; rm -rf / #), this will break out of the quoted context and execute arbitrary commands in the current shell session when the script is sourced. To mitigate this, avoid using eval for setting environment variables from external data and ensure any secret values are properly escaped or assigned via a safe, non-evaluating mechanism.
![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: 20
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
gforms/forms_tools.py (1)
52-53:⚠️ Potential issue | 🟠 MajorUse
documentTitle(camelCase) in the Forms create payload.Line 53 uses
document_title, which does not match the Google Forms API v1 field naming (documentTitle) and will prevent the document title from being set.🛠️ Proposed fix
if document_title: - form_body["info"]["document_title"] = document_title + form_body["info"]["documentTitle"] = document_title🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@gforms/forms_tools.py` around lines 52 - 53, The payload uses snake_case "document_title" instead of the expected Google Forms API v1 camelCase field; update the assignment in the code that builds form_body (the block checking if document_title) to set form_body["info"]["documentTitle"] = document_title so the API receives the correct field name; no other logic changes are needed.core/server.py (1)
150-162:⚠️ Potential issue | 🟡 MinorRedundant handler execution when paused: middleware discards response.
The
PauseMiddlewarecallsawait call_next(request)to execute the health handler (line 37), but then ignores the returned response and returns its own minimal response (line 39):{"status": "paused", "service": "workspace-mcp"}. This means the handler runs and performs work (version check, transport mode lookup) that is immediately discarded when the service is paused.The responses are also inconsistent: when paused, clients receive only
statusandservicefields, not theversionandtransportfields the handler builds. Consider skipping the handler execution entirely when paused (usereturn JSONResponse(...)directly on line 36 instead of callingcall_next), or remove the check in the handler'sis_paused()branch since the middleware already controls this path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/server.py` around lines 150 - 162, PauseMiddleware currently calls await call_next(request) which executes the health_check handler even when paused, then discards its response and returns a minimal paused JSON; this causes redundant work and inconsistent responses. Fix by short-circuiting in PauseMiddleware: when is_paused() is true, return the paused JSONResponse directly from PauseMiddleware instead of calling call_next, or alternatively remove the is_paused() branch inside the health_check handler so the handler always builds the full response and PauseMiddleware is responsible for the paused response; update the code paths around PauseMiddleware.call_next, PauseMiddleware, health_check, and is_paused to ensure only one component produces the paused response.
🟡 Minor comments (19)
gsearch/search_tools.py-91-94 (1)
91-94:⚠️ Potential issue | 🟡 MinorValidate and normalize
sitesentries before query composition.The current check passes for values like
["", " "], which can produce malformedsite:tokens and weaken intended filtering.Suggested fix
if site_restrict: - if not sites: + normalized_sites = [ + site.strip() for site in (sites or []) + if isinstance(site, str) and site.strip() + ] + if not normalized_sites: raise ValueError("When site_restrict is True, provide a non-empty 'sites' list.") - site_query = " OR ".join([f"site:{site}" for site in sites]) + site_query = " OR ".join(f"site:{site}" for site in normalized_sites) params['q'] = f"{q} ({site_query})"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@gsearch/search_tools.py` around lines 91 - 94, The sites list may contain blank or whitespace-only entries that produce malformed site: tokens; before composing site_query in the branch that handles site_restrict (where variables/sites, site_query, params['q'] are used), normalize entries by stripping each site string and filtering out empty results, then if the resulting normalized_sites list is empty raise ValueError("When site_restrict is True, provide a non-empty 'sites' list.") and otherwise build site_query from normalized_sites (e.g., " OR ".join([f"site:{s}" for s in normalized_sites])) and set params['q'] accordingly.pyproject.toml-108-117 (1)
108-117:⚠️ Potential issue | 🟡 MinorAdd
asyncio_default_fixture_loop_scopeto pytest configuration.With
pytest-asyncio>=0.23.0, aPytestDeprecationWarningis triggered whenasyncio_default_fixture_loop_scopeis unset. Add the following to the[tool.pytest.ini_options]section:asyncio_default_fixture_loop_scope = "function"This explicitly sets the loop scope for async fixtures and prevents deprecation warnings in current and future versions of pytest-asyncio.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pyproject.toml` around lines 108 - 117, Add the missing pytest-asyncio configuration key to the [tool.pytest.ini_options] section by setting asyncio_default_fixture_loop_scope to "function" so async fixture loop scope is explicit and the PytestDeprecationWarning from pytest-asyncio>=0.23.0 is eliminated; update the existing block that contains minversion, addopts, testpaths, asyncio_mode, and markers to include this new key.bug_fixes_review.md-189-190 (1)
189-190:⚠️ Potential issue | 🟡 MinorUpdate PR reference for accurate traceability.
Line 189 references “PR
#2”, which does not match this change context and can mislead future reviewers.📝 Suggested edit
-All 4 critical bugs have been fixed and pushed to PR `#2`: +All 4 critical bugs have been fixed and pushed to PR `#522`:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bug_fixes_review.md` around lines 189 - 190, Update the incorrect PR reference string "All 4 critical bugs have been fixed and pushed to PR `#2`:" so it accurately reflects the target pull request for this change; locate that exact sentence in the file and replace "PR `#2`" with the correct PR identifier (or remove the reference if not applicable), then verify the updated text matches the actual PR number used for these fixes.PR_DESCRIPTION_PHASE2.md-279-281 (1)
279-281:⚠️ Potential issue | 🟡 MinorSpecify a language for the fenced code block (markdownlint MD040).
The commit-hash block should declare a language (or
text) to satisfy linting.✅ Minimal fix
-``` +```text 7a5a4ee - Consolidate Google Docs tools from 14 to 7 (50% reduction)</details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@PR_DESCRIPTION_PHASE2.mdaround lines 279 - 281, The fenced commit-hash
block lacks a language specifier which violates markdownlint MD040; update the
fenced code block containing "7a5a4ee - Consolidate Google Docs tools from 14 to
7 (50% reduction)" to include a language like "text" (i.e., change the opening
totext) so the block is explicitly typed and the linter passes.</details> </blockquote></details> <details> <summary>PR_DESCRIPTION_PHASE2.md-227-233 (1)</summary><blockquote> `227-233`: _⚠️ Potential issue_ | _🟡 Minor_ **Migration section is internally contradictory on breaking changes.** Line 228 says no breaking changes, while Line 232 says a new required parameter exists. Please reconcile this so migration expectations are unambiguous. <details> <summary>📝 Suggested wording cleanup</summary> ```diff -### Breaking Changes -None. All operations maintain the same functionality and API contracts as before. +### Breaking Changes +Minor API shape change: consolidated tools require an `operation` parameter. +Legacy behavior compatibility (if any) should be explicitly documented here. ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@PR_DESCRIPTION_PHASE2.md` around lines 227 - 233, The Migration section contradicts itself: the "### Breaking Changes" header currently says "None" but the "For Users" bullets state a "New operation parameter required"; update the document so the two statements align — either mark a breaking change and describe it under "### Breaking Changes" referencing the new required parameter (e.g., call out "New required parameter: <new operation parameter> — may require callers to update invocation"), or make the parameter backward-compatible and change the bullet to "New operation parameter available/optional"; ensure you edit the "### Breaking Changes" header and the "For Users" bullets (and the phrase "New operation parameter") to consistently reflect the true migration impact. ``` </details> </blockquote></details> <details> <summary>gdocs/managers/table_operation_manager.py-70-72 (1)</summary><blockquote> `70-72`: _⚠️ Potential issue_ | _🟡 Minor_ **Use the tracked insertion point to report `metadata.table_index` correctly.** You now track where the table was created, but metadata still reports the last table (Line 88), which is wrong when inserting before existing tables. <details> <summary>🛠️ Suggested fix</summary> ```diff metadata = { 'rows': rows, 'columns': cols, 'populated_cells': population_count, - 'table_index': len(fresh_tables) - 1 + 'table_index': next( + (i for i, t in enumerate(fresh_tables) if t.get('start_index', -1) >= index), + len(fresh_tables) - 1 + ) } ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@gdocs/managers/table_operation_manager.py` around lines 70 - 72, The metadata for the newly inserted table is still being populated using the last-table index instead of the tracked insertion point; update the code that builds the table metadata (where metadata.table_index is set after calling _create_empty_table) to use self._created_table_index (set before calling _create_empty_table) instead of the previous "last table" value so the reported metadata.table_index matches the actual insertion position; ensure any subsequent references/readers of metadata use that same self._created_table_index value. ``` </details> </blockquote></details> <details> <summary>tests/unit/test_rate_limiting.py-217-232 (1)</summary><blockquote> `217-232`: _⚠️ Potential issue_ | _🟡 Minor_ **API-enablement tests can pass even if enablement messaging regresses.** Because assertions are conditional (`if message:`), these cases won’t fail when known-service guidance unexpectedly becomes empty. <details> <summary>✅ Tighten assertions</summary> ```diff message = get_api_enablement_message( "SERVICE_DISABLED: Gmail API", "gmail" ) - # Should return a helpful message or None - # The function may return None if it can't match the service - if message: - assert "API" in message or "enable" in message.lower() + assert message, "Expected API enablement guidance for known service alias 'gmail'" + assert "API" in message or "enable" in message.lower() @@ message = get_api_enablement_message( "accessNotConfigured: Calendar API has not been used", "calendar" ) - if message: - assert "API" in message or "enable" in message.lower() + assert message, "Expected API enablement guidance for known service alias 'calendar'" + assert "API" in message or "enable" in message.lower() ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@tests/unit/test_rate_limiting.py` around lines 217 - 232, The tests use a conditional "if message:" which lets regressions pass when get_api_enablement_message returns None or empty; update the two tests that call get_api_enablement_message (the block that checks "SERVICE_DISABLED: Gmail API" and the block in test_access_not_configured_detection for "accessNotConfigured: Calendar API...") to assert the message is non-empty and contains expected text instead of guarding with an if: replace the conditional with assertions that message is truthy (not None/empty) and that it contains "API" or the word "enable" (case-insensitive) so the test fails if enablement guidance disappears. ``` </details> </blockquote></details> <details> <summary>README.md-465-472 (1)</summary><blockquote> `465-472`: _⚠️ Potential issue_ | _🟡 Minor_ **Add blank lines around the table under “Source of Truth”.** Markdownlint warning is valid (MD058), and this will keep docs checks clean. <details> <summary>📝 Minimal markdown fix</summary> ```diff ### Source of Truth + | Layer | Purpose | |-------|---------| | Bitwarden Item (record) | Stores actual secret value (client ID / client secret) | | Mapping File (`secrets/bitwarden_map.json`) | Declares which env var maps to which Bitwarden item + field | | Runtime Loader (`scripts/bitwarden_env_loader.py`) | Fetches items via CLI and emits `export` lines | | Shell Helpers (`scripts/load-secrets.sh` / `scripts/load-secrets.ps1`) | Convenience wrappers to populate current session | | Ansible Role (`ansible/roles/secret-management`) | CI/CD / deployment rendering to `.env.generated` | + ### Bitwarden Setup Steps ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@README.md` around lines 465 - 472, The Markdown table under the "Source of Truth" heading needs blank lines before and after it to satisfy MD058; edit README.md around the "Source of Truth" section and insert a single blank line immediately above the table start (the line with "| Layer | Purpose |") and a single blank line immediately after the table end (the line after "| Ansible Role (`ansible/roles/secret-management`) | CI/CD / deployment rendering to `.env.generated` |") so the table is separated from surrounding paragraphs and linter warnings are resolved. ``` </details> </blockquote></details> <details> <summary>gmail/gmail_tools.py-652-655 (1)</summary><blockquote> `652-655`: _⚠️ Potential issue_ | _🟡 Minor_ **Fix misleading attachment output text.** Lines 652–655 say full base64 data is available, but only the first 100 characters are returned in this response. <details> <summary>✏️ Suggested message correction</summary> ```diff result_lines = [ "Attachment downloaded successfully!", f"Message ID: {message_id}", f"Size: {size_kb:.1f} KB ({size_bytes} bytes)", "\nBase64-encoded content (first 100 characters shown):", f"{attachment['data'][:100]}...", - "\n\nThe full base64-encoded attachment data is available.", + "\n\nOnly a preview is included above (first 100 characters).", "To save: decode the base64 data and write to a file with the appropriate extension.", "\nNote: Attachment IDs are ephemeral. Always use IDs from the most recent message fetch." ] ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@gmail/gmail_tools.py` around lines 652 - 655, The message incorrectly claims the "full base64-encoded attachment data is available" while only returning attachment['data'][:100]; update the text near where the attachment preview is built (the strings that include "Base64-encoded content (first 100 characters shown):", f"{attachment['data'][:100]}...", and the following lines) to explicitly state that only the first 100 characters are included and the full data is omitted here (e.g., "Only the first 100 characters are shown; full base64 data is not included in this response."), and keep the save instruction about decoding and writing to a file unchanged. Ensure you modify the exact literals that produce the preview so they no longer claim full data availability. ``` </details> </blockquote></details> <details> <summary>.env.example-3-3 (1)</summary><blockquote> `3-3`: _⚠️ Potential issue_ | _🟡 Minor_ **Update the sample port to match the runtime default.** Line 3 sets `WORKSPACE_MCP_PORT=8000`, but the actual runtime default is `4100` (as configured in `main.py`, `core/config.py`, and `manifest.json`). This discrepancy can misconfigure local development setups. Update the value to `WORKSPACE_MCP_PORT=4100`. <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In @.env.example at line 3, Update the sample environment variable in .env.example so the example default matches runtime: change WORKSPACE_MCP_PORT from 8000 to 4100; locate the WORKSPACE_MCP_PORT entry in .env.example and replace its value with 4100 to align with defaults in main.py, core/config.py, and manifest.json. ``` </details> </blockquote></details> <details> <summary>gdrive/drive_tools.py-613-621 (1)</summary><blockquote> `613-621`: _⚠️ Potential issue_ | _🟡 Minor_ **Validate permission payload combinations before API call.** Lines 613–621 only check that `role` and `type` exist. Invalid combinations should be rejected upfront with clear errors. The current code doesn't validate: - Whether `type` is one of the allowed values (`user`, `group`, `domain`, `anyone`) - Whether `role` is one of the allowed values (`reader`, `commenter`, `writer`, `owner`, `organizer`, `fileOrganizer`) - Whether `emailAddress` is provided when required by `type` (`user` and `group` require it; `domain` requires a `domain` field instead; `anyone` must not have it) <details> <summary>✅ Suggested validation guard</summary> ```diff if not file_id: raise ValueError("Operation 'create' requires 'file_id'.") if not role or not type: raise ValueError("Operation 'create' requires 'role' and 'type'.") + allowed_types = {"user", "group", "domain", "anyone"} + allowed_roles = {"reader", "commenter", "writer", "organizer", "fileOrganizer", "owner"} + if type not in allowed_types: + raise ValueError(f"Invalid permission type '{type}'.") + if role not in allowed_roles: + raise ValueError(f"Invalid permission role '{role}'.") + if type in {"user", "group"} and not email_address: + raise ValueError("email_address is required when type is 'user' or 'group'.") + if type == "domain" and not domain: + raise ValueError("domain is required when type is 'domain'.") + if type == "anyone" and email_address: + raise ValueError("email_address must not be provided when type is 'anyone'.") ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@gdrive/drive_tools.py` around lines 613 - 621, The permission creation currently only checks presence of role and type; add validation in the function that builds permission_body (referencing the parameters role, type, email_address and the local permission_body) to enforce allowed values and required fields: validate that type is one of {"user","group","domain","anyone"} and role is one of {"reader","commenter","writer","owner","organizer","fileOrganizer"} and raise ValueError with a clear message if not; additionally enforce combination rules: if type is "user" or "group" require email_address be provided, if type is "domain" require a domain parameter (and reject if domain missing), and if type is "anyone" ensure email_address is not supplied; update error messages to indicate which field/combination is invalid before making the API call. ``` </details> </blockquote></details> <details> <summary>tests/unit/test_imports.py-1-2 (1)</summary><blockquote> `1-2`: _⚠️ Potential issue_ | _🟡 Minor_ **Remove unused import.** `sys` is imported but never used in this file. <details> <summary>🧹 Proposed fix</summary> ```diff import pytest -import sys ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@tests/unit/test_imports.py` around lines 1 - 2, Remove the unused import "sys" from tests/unit/test_imports.py; keep the existing "import pytest" line and ensure no other usages of sys remain in the file (remove the "import sys" statement that is currently unused). ``` </details> </blockquote></details> <details> <summary>tests/conftest.py-12-18 (1)</summary><blockquote> `12-18`: _⚠️ Potential issue_ | _🟡 Minor_ **Avoid hardcoding real email addresses.** The default email `pmaclyman@gmail.com` appears to be a real email address. Consider using a clearly fake placeholder or requiring the environment variable without a fallback to avoid potential PII exposure in the repository. <details> <summary>🛡️ Proposed fix</summary> ```diff `@pytest.fixture`(scope="session") def test_email(): """ Returns the email address to use for live tests. - Defaults to the one used in live_test.py if not set in env. + Must be set in environment for integration tests. """ - return os.getenv("TEST_GOOGLE_EMAIL", "pmaclyman@gmail.com") + email = os.getenv("TEST_GOOGLE_EMAIL") + if not email: + pytest.skip("TEST_GOOGLE_EMAIL not set. Skipping integration tests.") + return email ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@tests/conftest.py` around lines 12 - 18, The test_email fixture currently hardcodes a real-looking address; update the fixture test_email to avoid embedding PII by either (a) removing the default and forcing os.getenv("TEST_GOOGLE_EMAIL") to be provided (i.e., fail or raise if None) or (b) replacing the fallback with a clearly fake placeholder like "user@example.com" or "test@example.com"; adjust the logic in the test_email function to validate presence and raise a clear error if the env var is missing when opting to require it, and reference TEST_GOOGLE_EMAIL and the test_email fixture in your change. ``` </details> </blockquote></details> <details> <summary>CONSOLIDATION_PLAN.md-59-59 (1)</summary><blockquote> `59-59`: _⚠️ Potential issue_ | _🟡 Minor_ **Add language identifiers to fenced code blocks.** These fences are missing language tags and will continue failing markdown lint. <details> <summary>Proposed fix</summary> ```diff -``` +```text Task Lists: ... -``` +``` ``` </details> Also applies to: 124-124, 203-203 <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@CONSOLIDATION_PLAN.mdat line 59, Several fenced code blocks in
CONSOLIDATION_PLAN.md are missing language identifiers (causing markdown lint
failures); update each fenced block that contains the "Task Lists:" snippet (and
the other two similar blocks) to include an appropriate language tag such as
text (ormarkdown) immediately after the opening backticks so the fences
becometext ...; locate the three offending fences by searching for the
"Task Lists:" content and replace their openingwithtext.</details> </blockquote></details> <details> <summary>tests/integration/test_gdocs_live.py-37-37 (1)</summary><blockquote> `37-37`: _⚠️ Potential issue_ | _🟡 Minor_ **Use bare `raise` to preserve the original traceback.** Line 37 should re-raise with `raise`, not `raise e`. This maintains the original exception traceback for better debugging. <details> <summary>Proposed fix</summary> ```diff - raise e + raise ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@tests/integration/test_gdocs_live.py` at line 37, Replace the explicit re-raise "raise e" with a bare "raise" inside the except block in tests/integration/test_gdocs_live.py (the except that captures the variable e) so the original traceback is preserved; locate the except block where the variable e is caught and update the re-raise to use a bare raise instead of re-raising the caught exception object. ``` </details> </blockquote></details> <details> <summary>scripts/generate_env_file.py-36-39 (1)</summary><blockquote> `36-39`: _⚠️ Potential issue_ | _🟡 Minor_ **Secret values may need escaping for .env format.** If a secret contains special characters (newlines, `=`, quotes, or spaces), the .env file will be malformed. Consider quoting values or escaping special characters. <details> <summary>🛡️ Proposed fix to quote secret values</summary> ```diff if value: - env_content.append(f"{env_key}={value}") + # Quote values to handle special characters + escaped_value = value.replace("\\", "\\\\").replace('"', '\\"') + env_content.append(f'{env_key}="{escaped_value}"') print(f"Loaded {env_key}") ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@scripts/generate_env_file.py` around lines 36 - 39, The current env writer appends raw secret values (env_content.append(f"{env_key}={value}")) which breaks .env parsing for values with newlines, =, quotes or spaces; update the logic that builds the line for env_key to safely serialize value by escaping backslashes and double-quotes and replacing actual newlines with \n, then wrap the result in double quotes (e.g. produce env_key="escaped_value") so values containing spaces, = or special chars remain valid; reference the existing variables env_key and value in generate_env_file.py and apply this transformation before appending to env_content. ``` </details> </blockquote></details> <details> <summary>scripts/generate_env_file.py-43-44 (1)</summary><blockquote> `43-44`: _⚠️ Potential issue_ | _🟡 Minor_ **Consider restricting .env file permissions.** The .env file contains secrets but is created with default permissions (typically 0644), making it world-readable. The Ansible role in this PR sets 0600 for its generated files. <details> <summary>🔐 Proposed fix to restrict file permissions</summary> ```diff - with open(ENV_PATH, 'w') as f: - f.write("\n".join(env_content) + "\n") + import stat + with open(ENV_PATH, 'w') as f: + f.write("\n".join(env_content) + "\n") + os.chmod(ENV_PATH, stat.S_IRUSR | stat.S_IWUSR) # 0600 ``` Or use `os.open` with explicit mode: ```python fd = os.open(ENV_PATH, os.O_WRONLY | os.O_CREAT | os.O_TRUNC, 0o600) with os.fdopen(fd, 'w') as f: f.write("\n".join(env_content) + "\n") ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@scripts/generate_env_file.py` around lines 43 - 44, The .env file is written with open(ENV_PATH, 'w') which creates it with default (world-readable) permissions; change the write to create the file with 0600 by using os.open with flags os.O_WRONLY|os.O_CREAT|os.O_TRUNC and mode 0o600, then wrap the returned fd with os.fdopen to write env_content, or alternatively write as before and immediately call os.chmod(ENV_PATH, 0o600); update the code around ENV_PATH and env_content to use one of these approaches and ensure os is imported. ``` </details> </blockquote></details> <details> <summary>gsheets/sheets_tools.py-329-329 (1)</summary><blockquote> `329-329`: _⚠️ Potential issue_ | _🟡 Minor_ **Potential `KeyError` if `replies` is empty or missing.** Accessing `response["replies"][0]["addSheet"]["properties"]["sheetId"]` assumes the response structure is always present. If the batchUpdate fails silently or returns an unexpected structure, this will raise a `KeyError`. <details> <summary>🛡️ Suggested defensive access</summary> ```diff - sheet_id = response["replies"][0]["addSheet"]["properties"]["sheetId"] + replies = response.get("replies", []) + if not replies: + raise Exception("Unexpected API response: no replies returned from batchUpdate") + sheet_id = replies[0].get("addSheet", {}).get("properties", {}).get("sheetId", "Unknown") ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@gsheets/sheets_tools.py` at line 329, The assignment to sheet_id blindly assumes the batchUpdate response contains replies[0].addSheet.properties.sheetId; change it to defensively validate the response before indexing: check that response is a dict, that "replies" exists and is a non-empty list, and that the first reply contains "addSheet" -> "properties" -> "sheetId"; if any of these are missing, raise a clear exception or return a sensible default and include the full response for debugging. Update the code around the sheet_id assignment (the line creating sheet_id from response["replies"][0]["addSheet"]["properties"]["sheetId"]) to perform these presence checks and surface a helpful error message instead of letting a KeyError propagate. ``` </details> </blockquote></details> <details> <summary>gappsscript/appsscript_tools.py-497-520 (1)</summary><blockquote> `497-520`: _⚠️ Potential issue_ | _🟡 Minor_ **`list_processes` doesn't filter by `script_id` but references it in the message.** The `processes().list()` call at line 512-514 doesn't include `script_id` in the params, yet the "no processes found" message at line 520 references `script_id`. This could be misleading since the API returns all user processes, not just those for the specified script. <details> <summary>🔧 Suggested fix - update message or add filter</summary> ```diff if not processes: - return f"No processes found for script {script_id}" + return "No processes found matching the specified filters." - output = f"Script Processes ({len(processes)}):\n" + "\n".join(process_list) + output = f"Processes ({len(processes)}):\n" + "\n".join(process_list) ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@gappsscript/appsscript_tools.py` around lines 497 - 520, The list_processes branch builds params for service.processes().list but never includes the script_id filter yet returns a message mentioning script_id; update the behavior in the list_processes block to either add the script filter to params (e.g., include the appropriate key/value for script_id when present before calling service.processes().list in appsscript_tools.py) or change the "No processes found" message to not reference script_id (use a generic "No processes found" or include the actual filter criteria used). Locate the code around the list_processes branch and the call to service.processes().list(**params).execute and apply one of these fixes so the message accurately reflects the query. ``` </details> </blockquote></details> </blockquote></details> <details> <summary>🧹 Nitpick comments (13)</summary><blockquote> <details> <summary>core/log_formatter.py (1)</summary><blockquote> `78-91`: **Remove unreachable dead code.** The `return message` on line 78 makes lines 80-91 unreachable. If the message enhancement logic is being removed intentionally (as indicated by the broader changes removing tool-tier functionality), delete the dead code entirely rather than leaving it in place. <details> <summary>♻️ Proposed fix to remove dead code</summary> ```diff def _enhance_message(self, message: str) -> str: """Enhance the log message with better formatting.""" - # Handle common patterns for better visual appeal - return message - - # Enabled tools messages - if "Enabled tools set for scope management" in message: - tools = message.split(": ")[-1] - return f"Scope management configured for tools: {tools}" - - # Credentials directory messages - if "Credentials directory permissions check passed" in message: - path = message.split(": ")[-1] - return f"Credentials directory verified: {path}" - - # If no specific pattern matches, return the original message - return message ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@core/log_formatter.py` around lines 78 - 91, Remove the unreachable dead code after the early "return message" in core/log_formatter.py: delete the conditional blocks that check for "Enabled tools set for scope management" and "Credentials directory permissions check passed" (and their related path/tools extraction) since execution can never reach them; ensure only the single return message remains or, if enhanced formatting is still desired, move that logic before the early return or integrate it into the existing message formatting function so those conditions are reachable. ``` </details> </blockquote></details> <details> <summary>gtasks/tasks_tools.py (1)</summary><blockquote> `184-186`: **Keep `handle_http_errors` outermost for consistent behavior.** Decorator order here differs from other tool modules; placing `handle_http_errors` above `require_google_service` keeps error formatting/retry handling consistent. <details> <summary>♻️ Suggested decorator order</summary> ```diff `@server.tool`() # type: ignore -@require_google_service("tasks", ["tasks_read", "tasks"]) # type: ignore -@handle_http_errors("manage_task_list", service_type="tasks") # type: ignore +@handle_http_errors("manage_task_list", service_type="tasks") # type: ignore +@require_google_service("tasks", ["tasks_read", "tasks"]) # type: ignore async def manage_task_list( @@ `@server.tool`() # type: ignore -@require_google_service("tasks", ["tasks_read", "tasks"]) # type: ignore -@handle_http_errors("manage_task", service_type="tasks") # type: ignore +@handle_http_errors("manage_task", service_type="tasks") # type: ignore +@require_google_service("tasks", ["tasks_read", "tasks"]) # type: ignore async def manage_task( ``` </details> Also applies to: 360-362 <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@gtasks/tasks_tools.py` around lines 184 - 186, The decorator order on manage_task_list is inverted; move `@handle_http_errors`("manage_task_list", service_type="tasks") to be the outermost decorator (above `@require_google_service`("tasks", ["tasks_read", "tasks"])) so HTTP error formatting/retry handling is applied consistently, and make the same swap for the other occurrence referenced (the decorators at the block around lines 360-362) to match the module's standard order. ``` </details> </blockquote></details> <details> <summary>TESTING_PLAN.md (1)</summary><blockquote> `9-16`: **Consider referencing .env.example as a template.** The prerequisites section explains environment setup well. Consider adding a reference to `.env.example` as a starting template: ```diff - Create a `.env` file in the root directory with your OAuth credentials: + (See `.env.example` for a template) ``` GOOGLE_OAUTH_CLIENT_ID=... ``` <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@TESTING_PLAN.md` around lines 9 - 16, Update the "Environment Setup" section to reference the repository's .env.example as a template for required keys; specifically mention the .env.example file alongside the existing instructions and show that it contains the GOOGLE_OAUTH_CLIENT_ID, GOOGLE_OAUTH_CLIENT_SECRET, and TEST_GOOGLE_EMAIL placeholders so readers can copy/rename it to .env before filling values (refer to the "Environment Setup" heading and the listed variables). ``` </details> </blockquote></details> <details> <summary>scripts/load-secrets-bws.ps1 (1)</summary><blockquote> `20-20`: **Prefer stream-aware output over `Write-Host`.** For script composability and capture/redirect support, use `Write-Information` or `Write-Verbose` instead. <details> <summary>Proposed fix</summary> ```diff - Write-Host "Set $envKey" + Write-Information "Set $envKey" -InformationAction Continue ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@scripts/load-secrets-bws.ps1` at line 20, Replace the non-stream-aware Write-Host "Set $envKey" with a stream-aware cmdlet so output can be captured/redirected; for example use Write-Information "Set $envKey" (and optionally include -InformationAction Continue) or use Write-Verbose "Set $envKey" and ensure callers can enable verbose output. Update the single occurrence of the Write-Host call referencing $envKey to one of these alternatives and, if choosing Write-Verbose, ensure the script documents or sets appropriate verbose behavior. ``` </details> </blockquote></details> <details> <summary>scripts/load-secrets.ps1 (1)</summary><blockquote> `52-52`: **Use `Write-Information`/`Write-Verbose` instead of `Write-Host`.** This improves capture/redirection behavior and aligns with script-automation best practices. <details> <summary>Proposed fix</summary> ```diff - Write-Host "Set $envKey" -ForegroundColor Green + Write-Information "Set $envKey" -InformationAction Continue @@ - Write-Host "Placeholder item IDs not configured for: $($placeholders -join ', ')" -ForegroundColor Yellow + Write-Warning "Placeholder item IDs not configured for: $($placeholders -join ', ')" @@ -Write-Host "Secrets loaded into current PowerShell session." -ForegroundColor Cyan +Write-Information "Secrets loaded into current PowerShell session." -InformationAction Continue ``` </details> Also applies to: 56-56, 59-59 <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@scripts/load-secrets.ps1` at line 52, Replace the direct console writes using Write-Host (e.g. the statements like Write-Host "Set $envKey" -ForegroundColor Green) with structured output using Write-Information (or Write-Verbose) so messages can be captured/redirected by automation; specifically change those Write-Host calls to Write-Information "Set $envKey" -InformationAction Continue (or Write-Verbose "Set $envKey" if you prefer verbosity control) and update the other two identical Write-Host occurrences so all three use the new cmdlet. ``` </details> </blockquote></details> <details> <summary>ansible/deploy-secret-management.yml (1)</summary><blockquote> `6-9`: **Placeholder UUIDs require replacement before use.** The `REPLACE-WITH-BITWARDEN-ITEM-UUID-*` placeholders are clearly marked, but consider adding a comment in the playbook itself noting that these must be replaced. Per the AI summary, the role skips placeholder items, which provides a safety net. <details> <summary>📝 Suggested inline comment</summary> ```diff vars: + # IMPORTANT: Replace placeholder UUIDs below with actual Bitwarden item IDs bitwarden_secret_map: GOOGLE_OAUTH_CLIENT_ID: { item_id: "REPLACE-WITH-BITWARDEN-ITEM-UUID-CLIENT", field: "client_id" } GOOGLE_OAUTH_CLIENT_SECRET: { item_id: "REPLACE-WITH-BITWARDEN-ITEM-UUID-SECRET", field: "client_secret" } ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@ansible/deploy-secret-management.yml` around lines 6 - 9, Add an inline comment in the playbook next to the bitwarden_secret_map (and/or above it) that explicitly states the item_id placeholders (REPLACE-WITH-BITWARDEN-ITEM-UUID-CLIENT and REPLACE-WITH-BITWARDEN-ITEM-UUID-SECRET) must be replaced with real Bitwarden item UUIDs before running; reference the map keys (GOOGLE_OAUTH_CLIENT_ID and GOOGLE_OAUTH_CLIENT_SECRET) and note that the role will skip placeholder items while secret_output_dir remains ./secrets — this makes the required manual replacement clear to operators. ``` </details> </blockquote></details> <details> <summary>tests/unit/test_calendar_tools.py (1)</summary><blockquote> `12-12`: **Unused import: `MagicMock`.** The `MagicMock` import is not used anywhere in this test file. <details> <summary>🧹 Remove unused import</summary> ```diff import json import pytest -from unittest.mock import MagicMock from gcalendar.calendar_tools import ( ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@tests/unit/test_calendar_tools.py` at line 12, Remove the unused MagicMock import: edit the import statement that currently imports MagicMock and either delete MagicMock from the import list or remove the entire import line if it's no longer needed; ensure no other references to MagicMock remain in the test file (symbols to check: MagicMock). ``` </details> </blockquote></details> <details> <summary>tests/unit/test_gmail_tools.py (1)</summary><blockquote> `16-16`: **Unused imports from unittest.mock.** `MagicMock`, `AsyncMock`, `patch`, and `call` are imported but not used in this test file. <details> <summary>🧹 Remove unused imports</summary> ```diff import base64 import pytest -from unittest.mock import MagicMock, AsyncMock, patch, call from gmail.gmail_tools import ( ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@tests/unit/test_gmail_tools.py` at line 16, The test file imports unused names from unittest.mock (MagicMock, AsyncMock, patch, call); remove these unused imports from the import line in tests/unit/test_gmail_tools.py so only actually used symbols remain (e.g., if only unittest.mock is needed, delete MagicMock/AsyncMock/patch/call or replace with a direct import used elsewhere); ensure the file still imports any mocks actually referenced in functions like any test helpers before committing. ``` </details> </blockquote></details> <details> <summary>core/pause_middleware.py (1)</summary><blockquote> `34-36`: **Path matching may miss trailing slashes or query strings.** The exact match `path == "/health"` won't match `/health/` or `/health?check=1`. Consider using `path.rstrip("/") == "/health"` or `path.startswith("/health")` depending on your requirements. <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@core/pause_middleware.py` around lines 34 - 36, The equality check using path == "/health" can miss variants like "/health/", "/health?x=1" or "/health/?x=1"; update the check in the pause middleware where request.url.path is assigned to path so you normalize the path before comparison (e.g., strip query string and trailing slashes with something like take request.url.path up to the '?' then .rstrip('/') and compare to "/health") or, if you intend to match subpaths, use path.startswith("/health"); modify the if condition accordingly (refer to the variable path and the existing if path == "/health" check). ``` </details> </blockquote></details> <details> <summary>gappsscript/appsscript_tools.py (1)</summary><blockquote> `424-448`: **Redundant exception handling in `execute_script`.** The inner `try/except` block (lines 424-448) catches all exceptions and returns a string error message. However, the function is already decorated with `@handle_http_errors`, which provides standardized error handling. This inner handler may mask errors that should be propagated to the decorator for consistent error formatting. Consider removing the inner try/except or limiting it to handle only script-specific errors from the `result["error"]` check: <details> <summary>♻️ Suggested refactor</summary> ```diff - try: - result = await asyncio.to_thread( - service.scripts().run(scriptId=script_id, body=body).execute - ) - - if "error" in result: - error_details = result["error"] - return ( - f"Script execution failed:\n" - f"Error: {error_details.get('message', 'Unknown error')}\n" - f"Details: {error_details.get('details', [])}" - ) - - response = result.get("response", {}) - return_value = response.get("result") - - return ( - f"Script executed successfully.\n" - f"Function: {function_name}\n" - f"Return value: {return_value}" + result = await asyncio.to_thread( + service.scripts().run(scriptId=script_id, body=body).execute + ) + + if "error" in result: + error_details = result["error"] + return ( + f"Script execution failed:\n" + f"Error: {error_details.get('message', 'Unknown error')}\n" + f"Details: {error_details.get('details', [])}" ) - except Exception as e: - logger.error(f"Script execution error: {e}") - return f"Script execution failed: {str(e)}" + response = result.get("response", {}) + return_value = response.get("result") + + return ( + f"Script executed successfully.\n" + f"Function: {function_name}\n" + f"Return value: {return_value}" + ) ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@gappsscript/appsscript_tools.py` around lines 424 - 448, The inner broad try/except inside execute_script should be removed (or narrowed) so exceptions bubble to the `@handle_http_errors` decorator for consistent handling; locate the block around service.scripts().run(...).execute in execute_script and either delete the outer try/except and keep only the result/"error" handling and normal returns, or replace the catch with a specific exception type if you must handle only script-result parsing errors, ensuring you still re-raise unexpected exceptions to let handle_http_errors format them. ``` </details> </blockquote></details> <details> <summary>gdocs/docs_tools.py (2)</summary><blockquote> `571-578`: **Redundant exception handling duplicated across functions.** The `try/except` blocks at lines 571-578, 799-806, and 1119-1126 catch `HttpError` and generic `Exception`, re-raising them as `Exception`. However, all these functions are decorated with `@handle_http_errors`, which already provides standardized error handling for `HttpError`. This redundant handling may interfere with the decorator's error categorization (e.g., API enablement hints, re-auth guidance). <details> <summary>♻️ Consider removing inner exception handlers</summary> The inner try/except blocks can be removed since `@handle_http_errors` already handles: - `HttpError` with API-specific messaging - SSL transient errors with retry logic - General exceptions with user-friendly messages ```diff - except HttpError as error: - message = f"API error: {error}" - logger.error(message, exc_info=True) - raise Exception(message) - except Exception as e: - message = f"Unexpected error: {e}" - logger.exception(message) - raise Exception(message) ``` Let the decorator handle errors consistently across all tools. </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@gdocs/docs_tools.py` around lines 571 - 578, Remove the redundant inner try/except blocks that catch HttpError and re-raise generic Exception inside functions already decorated with `@handle_http_errors`; locate the catch/re-raise patterns (the except HttpError as error / except Exception as e blocks) in the functions containing those blocks (the occurrences around the shown diff and the similar blocks at the other two occurrences) and delete them so errors bubble up to the handle_http_errors decorator instead of being converted to plain Exception, keeping the surrounding logic intact and ensuring HttpError and other exceptions are not swallowed or re-wrapped locally. ``` </details> --- `973-975`: **Move `import json` to module level.** The `import json` statements at lines 973 and 991 are inside function bodies. For better performance (avoiding repeated import overhead) and code organization, move this to the module-level imports. <details> <summary>♻️ Move import to top of file</summary> ```diff import logging import asyncio import io +import json from typing import List, Dict, Any, Optional, Literal, TypedDict, Union ``` Then remove the inline imports at lines 973 and 991. </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@gdocs/docs_tools.py` around lines 973 - 975, Move the inline "import json" to the module-level imports and remove the two inline imports inside the functions; specifically add "import json" at the top of gdocs/docs_tools.py and delete the in-function imports that appear next to the code constructing the string "Document structure analysis for {document_id}:\n\n{json.dumps(result, indent=2)}..." and the other place that calls json.dumps near line ~991 so both functions use the top-level json import. ``` </details> </blockquote></details> <details> <summary>gsheets/sheets_tools.py (1)</summary><blockquote> `82-82`: **Consider using `ValueError` instead of `Exception` for input validation.** Lines 82, 111, 281, 308-309, and 336 raise generic `Exception` for input validation errors. Using `ValueError` would be more semantically appropriate and consistent with Python conventions for invalid arguments. <details> <summary>♻️ Example fix for line 82</summary> ```diff - raise Exception("Operation 'get' requires 'spreadsheet_id'.") + raise ValueError("Operation 'get' requires 'spreadsheet_id'.") ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@gsheets/sheets_tools.py` at line 82, Replace the generic Exception raises used for input validation with ValueError to follow Python conventions: change the raise Exception("Operation 'get' requires 'spreadsheet_id'.") and the other similar raises that use messages like "Operation 'append' requires 'spreadsheet_id' and 'range'.", "Missing required parameter 'range' for operation 'values_batch_get'.", the two raises around "operation requires 'spreadsheet_id' and 'range'" and "Operation 'clear' requires 'spreadsheet_id' and 'range'." to raise ValueError instead; locate these by searching for those exact error message strings in sheets_tools.py and update the raise statements to use ValueError while keeping the same messages. ``` </details> </blockquote></details> </blockquote></details> --- <details> <summary>ℹ️ Review info</summary> **Configuration used**: defaults **Review profile**: CHILL **Plan**: Pro <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between ca00c746343edf38512d5be3de6b2d13c8da8b76 and 085137d7dc2d63d2fea305acc6d1e5fdffdfeb0f. </details> <details> <summary>⛔ Files ignored due to path filters (1)</summary> * `uv.lock` is excluded by `!**/*.lock` </details> <details> <summary>📒 Files selected for processing (66)</summary> * `.env.example` * `.gitattributes` * `.gitignore` * `CONSOLIDATION_PLAN.md` * `PR_DESCRIPTION.md` * `PR_DESCRIPTION_PHASE2.md` * `README.md` * `REVIEW_COMMENTS.md` * `TESTING_PLAN.md` * `TODO_MAINTENANCE.md` * `UPSTREAM_SYNC_GUIDE.md` * `ansible/deploy-secret-management.yml` * `ansible/roles/secret-management/tasks/main.yml` * `approval_review.txt` * `auth/oauth_callback_server.py` * `auth/oauth_config.py` * `auth/scopes.py` * `auth/service_decorator.py` * `bug_fixes_review.md` * `core/config.py` * `core/log_formatter.py` * `core/pause_middleware.py` * `core/server.py` * `core/tool_registry.py` * `core/tool_tier_loader.py` * `core/tool_tiers.yaml` * `core/utils.py` * `fastmcp_server.py` * `gappsscript/__init__.py` * `gappsscript/appsscript_tools.py` * `gchat/chat_tools.py` * `gdocs/docs_helpers.py` * `gdocs/docs_tools.py` * `gdocs/managers/table_operation_manager.py` * `gdrive/drive_tools.py` * `gforms/forms_tools.py` * `gmail/gmail_tools.py` * `gsearch/search_tools.py` * `gsheets/__init__.py` * `gsheets/sheets_tools.py` * `gslides/slides_tools.py` * `gtasks/tasks_tools.py` * `main.py` * `manifest.json` * `pyproject.toml` * `review_strategic.txt` * `scripts/bitwarden_env_loader.py` * `scripts/generate_env_file.py` * `scripts/load-secrets-bws.ps1` * `scripts/load-secrets.ps1` * `scripts/load-secrets.sh` * `scripts/load_secrets_bws.py` * `secrets/bitwarden_map.json` * `secrets/bws_map.json` * `tests/conftest.py` * `tests/integration/test_gdocs_live.py` * `tests/unit/conftest.py` * `tests/unit/test_calendar_tools.py` * `tests/unit/test_docs_tools.py` * `tests/unit/test_drive_tools.py` * `tests/unit/test_gmail_attachments.py` * `tests/unit/test_gmail_tools.py` * `tests/unit/test_imports.py` * `tests/unit/test_oauth21.py` * `tests/unit/test_rate_limiting.py` * `tests/unit/test_security.py` </details> <details> <summary>💤 Files with no reviewable changes (4)</summary> * core/tool_tiers.yaml * core/tool_tier_loader.py * fastmcp_server.py * core/tool_registry.py </details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
Comment on lines
+15
to
+21
| - name: Fetch Bitwarden items | ||
| command: "bw get item {{ item.value.item_id }}" | ||
| register: bw_item | ||
| loop: "{{ dict(bitwarden_secret_map) | dict2items }}" | ||
| when: not item.value.item_id.startswith('REPLACE-WITH') | ||
| changed_when: false | ||
| failed_when: bw_item.rc != 0 |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
fd -t f "main.yml" ansible/roles/secret-management/tasks/Repository: taylorwilsdon/google_workspace_mcp
Length of output: 124
🏁 Script executed:
cat -n ansible/roles/secret-management/tasks/main.ymlRepository: taylorwilsdon/google_workspace_mcp
Length of output: 1973
Add no_log: true to prevent secrets from appearing in Ansible logs.
The Fetch Bitwarden items, Build env var map, and Create .env file tasks all process sensitive data (Bitwarden credentials, passwords, and environment variables) without masking output. Without no_log: true, Ansible logs will expose all secrets, defeating the security purpose of this role.
Proposed fix
- name: Fetch Bitwarden items
command: "bw get item {{ item.value.item_id }}"
register: bw_item
+ no_log: true
loop: "{{ dict(bitwarden_secret_map) | dict2items }}"
when: not item.value.item_id.startswith('REPLACE-WITH')
changed_when: false
failed_when: bw_item.rc != 0 - name: Build env var map
set_fact:
rendered_env: "{{ rendered_env | default({}) | combine({ item.item.key: resolved_value }) }}"
+ no_log: true
loop: "{{ bw_item.results }}" - name: Create .env file from secrets
copy:
dest: "{{ secret_output_dir | default('./') }}/.env.generated"
content: |
# Generated by secret-management role - DO NOT COMMIT
{% for k, v in rendered_env.items() %}{{ k }}={{ v }}
{% endfor %}
mode: '0600'
+ no_log: true📝 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
| - name: Fetch Bitwarden items | |
| command: "bw get item {{ item.value.item_id }}" | |
| register: bw_item | |
| loop: "{{ dict(bitwarden_secret_map) | dict2items }}" | |
| when: not item.value.item_id.startswith('REPLACE-WITH') | |
| changed_when: false | |
| failed_when: bw_item.rc != 0 | |
| - name: Fetch Bitwarden items | |
| command: "bw get item {{ item.value.item_id }}" | |
| register: bw_item | |
| no_log: true | |
| loop: "{{ dict(bitwarden_secret_map) | dict2items }}" | |
| when: not item.value.item_id.startswith('REPLACE-WITH') | |
| changed_when: false | |
| failed_when: bw_item.rc != 0 |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ansible/roles/secret-management/tasks/main.yml` around lines 15 - 21, The
three Ansible tasks that handle secrets—the tasks named "Fetch Bitwarden items",
"Build env var map", and "Create .env file"—need to have their outputs
suppressed to avoid leaking secrets; add no_log: true to each of those task
definitions (the task blocks that register bw_item and build the env var map and
the task that writes the .env file) so Ansible will mask stdout/stderr and
registered variables for those tasks while keeping the existing
changed_when/failed_when logic intact.
Comment on lines
+24
to
+27
| set_fact: | ||
| rendered_env: "{{ rendered_env | default({}) | combine({ item.item.key: ( ( ( ( ( ( ( bw_item.stdout | from_json ).fields | default([]) ) | selectattr('name','equalto', item.item.value.field ) | list )[0].value ) if ( ( ( ( bw_item.stdout | from_json ).fields | default([]) ) | selectattr('name','equalto', item.item.value.field ) | list ) | length ) > 0 else ( (bw_item.stdout | from_json).login.username if item.item.value.field in ['client_id','username','user'] else (bw_item.stdout | from_json).login.password ) ) ) }) }}" | ||
| loop: "{{ bw_item.results }}" | ||
| when: rendered_env is not defined or True |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "main.yml" -path "*/secret-management/tasks/*" 2>/dev/nullRepository: taylorwilsdon/google_workspace_mcp
Length of output: 126
🏁 Script executed:
git ls-files | grep -E "secret-management.*main\.yml"Repository: taylorwilsdon/google_workspace_mcp
Length of output: 124
🏁 Script executed:
cat -n ansible/roles/secret-management/tasks/main.ymlRepository: taylorwilsdon/google_workspace_mcp
Length of output: 1973
Use item.stdout (current loop result) instead of bw_item.stdout to access Bitwarden JSON data.
Line 25 references bw_item.stdout inside the loop loop: "{{ bw_item.results }}", but bw_item is the parent registered result object with only a .results array. The actual iteration data is in item.stdout. This causes undefined behavior and incorrect secret extraction.
Additionally, both the "Fetch Bitwarden items" task (line 15) and "Build env var map" task (line 24) must include no_log: true to prevent Bitwarden credentials from appearing in task output, callbacks, or logs.
Proposed fix for variable reference
- name: Build env var map
set_fact:
- rendered_env: "{{ rendered_env | default({}) | combine({ item.item.key: ( ( ( ( ( ( ( bw_item.stdout | from_json ).fields | default([]) ) | selectattr('name','equalto', item.item.value.field ) | list )[0].value ) if ( ( ( ( bw_item.stdout | from_json ).fields | default([]) ) | selectattr('name','equalto', item.item.value.field ) | list ) | length ) > 0 else ( (bw_item.stdout | from_json).login.username if item.item.value.field in ['client_id','username','user'] else (bw_item.stdout | from_json).login.password ) ) ) }) }}"
+ rendered_env: "{{ rendered_env | default({}) | combine({ item.item.key: resolved_value }) }}"
+ vars:
+ bw_json: "{{ item.stdout | from_json }}"
+ matched_fields: "{{ (bw_json.fields | default([])) | selectattr('name','equalto', item.item.value.field) | list }}"
+ resolved_value: >-
+ {{
+ matched_fields[0].value
+ if (matched_fields | length) > 0
+ else (
+ bw_json.login.username
+ if item.item.value.field in ['client_id','username','user']
+ else bw_json.login.password
+ )
+ }}
loop: "{{ bw_item.results }}"
- when: rendered_env is not defined or True
+ no_log: trueAlso add no_log: true to the "Fetch Bitwarden items" task (line 15).
📝 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
| set_fact: | |
| rendered_env: "{{ rendered_env | default({}) | combine({ item.item.key: ( ( ( ( ( ( ( bw_item.stdout | from_json ).fields | default([]) ) | selectattr('name','equalto', item.item.value.field ) | list )[0].value ) if ( ( ( ( bw_item.stdout | from_json ).fields | default([]) ) | selectattr('name','equalto', item.item.value.field ) | list ) | length ) > 0 else ( (bw_item.stdout | from_json).login.username if item.item.value.field in ['client_id','username','user'] else (bw_item.stdout | from_json).login.password ) ) ) }) }}" | |
| loop: "{{ bw_item.results }}" | |
| when: rendered_env is not defined or True | |
| set_fact: | |
| rendered_env: "{{ rendered_env | default({}) | combine({ item.item.key: resolved_value }) }}" | |
| vars: | |
| bw_json: "{{ item.stdout | from_json }}" | |
| matched_fields: "{{ (bw_json.fields | default([])) | selectattr('name','equalto', item.item.value.field) | list }}" | |
| resolved_value: >- | |
| {{ | |
| matched_fields[0].value | |
| if (matched_fields | length) > 0 | |
| else ( | |
| bw_json.login.username | |
| if item.item.value.field in ['client_id','username','user'] | |
| else bw_json.login.password | |
| ) | |
| }} | |
| loop: "{{ bw_item.results }}" | |
| no_log: true |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ansible/roles/secret-management/tasks/main.yml` around lines 24 - 27, In the
set_fact task that builds rendered_env (the block using rendered_env | combine
and looping over bw_item.results), replace references to bw_item.stdout with
item.stdout so each iteration reads the current result’s JSON (use item.stdout |
from_json ... instead of bw_item.stdout) to correctly extract Bitwarden fields;
also add no_log: true to both the "Fetch Bitwarden items" task (the task that
registers bw_item) and the "Build env var map" task (the set_fact task that
registers rendered_env) to prevent secret exposure in logs.
Comment on lines
+29
to
+31
| ### Recommendation: | ||
| **MERGE THIS NOW** and use it as the foundation for Phase 2 (Docs consolidation). | ||
|
|
Contributor
There was a problem hiding this comment.
Remove the unconditional merge directive from this artifact.
Line 30 says “MERGE THIS NOW” while the PR is explicitly marked “DO NOT MERGE”; this is a high-risk review-process contradiction.
🛠️ Suggested edit
-**MERGE THIS NOW** and use it as the foundation for Phase 2 (Docs consolidation).
+**DO NOT MERGE YET** — keep this as a progress/review artifact until merge criteria are explicitly met.📝 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
| ### Recommendation: | |
| **MERGE THIS NOW** and use it as the foundation for Phase 2 (Docs consolidation). | |
| ### Recommendation: | |
| **DO NOT MERGE YET** — keep this as a progress/review artifact until merge criteria are explicitly met. | |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@approval_review.txt` around lines 29 - 31, The approval note contains an
unconditional merge directive "MERGE THIS NOW" that contradicts the PR's "DO NOT
MERGE" status; remove or replace that phrase in the approval_review.txt content
so it no longer instructs an immediate merge—either delete the sentence or
change it to conditional language (e.g., "Approve for merge once PR status
changes" or "Recommend merge after resolving blockers") to align with the PR's
DO NOT MERGE marker.
Comment on lines
+414
to
+416
| **Total Tools:** ~45 (Target Met) | ||
| **Consolidation:** Complete across all services. | ||
| **Tool Tiers:** Removed (All tools enabled by default). |
Contributor
There was a problem hiding this comment.
Resolve contradictory project status statements.
The doc simultaneously says consolidation is complete/target met and also lists multiple services as planned/not started with total progress at 28%. This creates an incorrect source of truth.
Also applies to: 423-425, 512-526, 566-573
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@CONSOLIDATION_PLAN.md` around lines 414 - 416, The document contains
contradictory status statements — e.g., the summary block values "Total Tools",
"Consolidation", and "Tool Tiers" conflict with per-service progress and the 28%
overall progress; reconcile these by choosing the correct authoritative state
and updating all occurrences (the summary entries "Total Tools:",
"Consolidation:", "Tool Tiers:" and the per-service status lines referenced
around the other ranges) to match it: update the overall progress percentage to
reflect the actual summed per-service progress or update each service to match
the declared "Complete/Target Met", and ensure every duplicated status paragraph
(including the blocks around the other noted ranges) is made consistent so the
doc presents a single source of truth.
| try { | ||
| $json = bw get item $itemId | ConvertFrom-Json | ||
| } catch { | ||
| Write-Warning "Failed to fetch item $itemId for $envKey: $_" |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, check if the file exists and read line 35
if [ -f scripts/load-secrets.ps1 ]; then
echo "=== File found. Reading line 35 context ==="
sed -n '30,40p' scripts/load-secrets.ps1
echo ""
echo "=== Exact line 35 ==="
sed -n '35p' scripts/load-secrets.ps1
else
echo "File scripts/load-secrets.ps1 not found"
find . -name "load-secrets.ps1" 2>/dev/null
fiRepository: taylorwilsdon/google_workspace_mcp
Length of output: 519
🌐 Web query:
PowerShell variable interpolation syntax colon $envKey: parsing rules
💡 Result:
In expandable (double-quoted) PowerShell strings, the parser tries to form the longest valid variable reference starting at $. A colon (:) is special because it’s used in scoped / drive-qualified variable names such as $env:PATH or $global:var, and it’s also permitted in the “simple” variable-name form (so it can get “captured” into what PowerShell thinks the variable reference is). As a result, writing a variable immediately followed by : is ambiguous and often parsed not the way you intend. [1], [2]
What happens with $envKey:
If you write:
"$envKey:foo"PowerShell does not treat that as ($envKey) + ":foo"; it attempts to interpret : as part of a variable reference (scope/drive qualification or a longer variable token), which can yield empty output, the wrong variable, or even a “Variable reference is not valid” parser error in some cases. [1], [2], [3]
Correct ways to disambiguate
Use one of these to explicitly delimit the variable:
"${envKey}:foo" # braced variable name
"$($envKey):foo" # subexpressionThe braced form ${...} exists specifically to allow unambiguous variable references (including when adjacent punctuation would otherwise be parsed as part of the name). [1], [4]
Rule of thumb
If a variable in an expandable string is immediately followed by :, ., [ or other “identifier-ish” / special characters, prefer:
${var}(simple delimiter), or$($expr)(when you need an expression)
Sources: Stack Overflow explanation of : in variable names and ${} delimiting [1]; discussion/example of $user:$machine misparsing due to : [2]; example of "$ComputerName: ..." causing invalid variable reference parsing [3]; ${...} allowing arbitrary characters and disambiguation [4].
References
[1] https://stackoverflow.com/questions/8386219/powershell-string-formatting-why-is-the-colon-character-causing-my-variables-v
[2] https://winterdom.com/2008/02/20/watchoutforinpowershell
[3] https://www.packtpub.com/sa-br/product/mastering-powershell-scripting-9781805120278/chapter/variables-arrays-and-hashtables-3/section/variables-in-strings-ch03lvl1sec24
[4] https://www.red-gate.com/simple-talk/sysadmin/powershell/the-complete-guide-to-powershell-punctuation/
Fix invalid variable interpolation syntax in warning string.
Line 35 uses $envKey: which PowerShell misinterprets; the colon triggers scope-qualification parsing, causing empty output or parse errors.
Proposed fix
- Write-Warning "Failed to fetch item $itemId for $envKey: $_"
+ Write-Warning "Failed to fetch item $itemId for ${envKey}: $_"Use braced syntax ${envKey} to explicitly delimit the variable and prevent the colon from being parsed as part of the variable reference.
📝 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
| Write-Warning "Failed to fetch item $itemId for $envKey: $_" | |
| Write-Warning "Failed to fetch item $itemId for ${envKey}: $_" |
🧰 Tools
🪛 PSScriptAnalyzer (1.24.0)
[error] 35-35: Variable reference is not valid. ':' was not followed by a valid variable name character. Consider using ${} to delimit the name.
(InvalidVariableReferenceWithDrive)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/load-secrets.ps1` at line 35, The Write-Warning message uses
"$envKey:" which PowerShell interprets incorrectly because the colon is parsed
as scope; update the warning string in the Write-Warning call to use braced
variable syntax for envKey (e.g., ${envKey}) so the colon is treated as literal
text and the variables $itemId and $envKey interpolate correctly; modify the
Write-Warning invocation that currently contains "Failed to fetch item $itemId
for $envKey:" to use the braced form for envKey.
Comment on lines
+6
to
+24
| MAP_PATH="${MAP_PATH:-$(dirname "$0")/../secrets/bitwarden_map.json}" | ||
|
|
||
| if [[ -z "${BW_SESSION:-}" ]]; then | ||
| echo "BW_SESSION not set. Run: export BW_SESSION=\"$(bw unlock --raw)\"" >&2 | ||
| exit 1 | ||
| fi | ||
|
|
||
| if [[ ! -f "$MAP_PATH" ]]; then | ||
| echo "Mapping file not found: $MAP_PATH" >&2 | ||
| exit 2 | ||
| fi | ||
|
|
||
| python "$(dirname "$0")/bitwarden_env_loader.py" | while read -r line; do | ||
| if [[ "$line" == export* ]]; then | ||
| eval "$line" | ||
| var_name=$(echo "$line" | cut -d' ' -f2 | cut -d'=' -f1) | ||
| echo "Set $var_name" | ||
| fi | ||
| done |
Contributor
There was a problem hiding this comment.
This won’t reliably populate the caller’s environment when sourced.
There are three blocking shell issues here: path resolution via $0, subshell export loss from pipeline while, and unintended execution of bw unlock in the error message.
🛠️ Proposed shell-semantics fix
set -euo pipefail
-MAP_PATH="${MAP_PATH:-$(dirname "$0")/../secrets/bitwarden_map.json}"
+SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)"
+MAP_PATH="${MAP_PATH:-$SCRIPT_DIR/../secrets/bitwarden_map.json}"
if [[ -z "${BW_SESSION:-}" ]]; then
- echo "BW_SESSION not set. Run: export BW_SESSION=\"$(bw unlock --raw)\"" >&2
+ echo 'BW_SESSION not set. Run: export BW_SESSION="$(bw unlock --raw)"' >&2
exit 1
fi
@@
-python "$(dirname "$0")/bitwarden_env_loader.py" | while read -r line; do
+while read -r line; do
if [[ "$line" == export* ]]; then
eval "$line"
var_name=$(echo "$line" | cut -d' ' -f2 | cut -d'=' -f1)
echo "Set $var_name"
fi
-done
+done < <(python "$SCRIPT_DIR/bitwarden_env_loader.py")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/load-secrets.sh` around lines 6 - 24, The script fails to reliably
export variables into the caller environment because it uses $0 for path
resolution, pipes the python output into a while loop (creating a subshell so
exports are lost), and invokes bw in the error hint; fix by resolving script dir
with DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" and set MAP_PATH using
that DIR (MAP_PATH="${MAP_PATH:-$DIR/../secrets/bitwarden_map.json}"), check
BW_SESSION without running bw (print a safe hint like 'export BW_SESSION="$(bw
unlock --raw)"' but do not execute it), and avoid the pipeline/subshell by using
process substitution or direct eval — e.g. read lines from python with: while
read -r line; do ... done < <(python "$DIR/bitwarden_env_loader.py") or simply
eval "$(python "$DIR/bitwarden_env_loader.py")" while still only processing
export lines; ensure you reference MAP_PATH, BW_SESSION,
bitwarden_env_loader.py, and the while read loop when editing.
Comment on lines
+19
to
+21
| if [[ "$line" == export* ]]; then | ||
| eval "$line" | ||
| var_name=$(echo "$line" | cut -d' ' -f2 | cut -d'=' -f1) |
Contributor
There was a problem hiding this comment.
Avoid executing unvalidated dynamic shell with raw eval.
Given this processes dynamic secret values, add a strict format gate before evaluating lines.
🧱 Minimal hardening around eval
- if [[ "$line" == export* ]]; then
+ if [[ "$line" =~ ^export[[:space:]]([A-Za-z_][A-Za-z0-9_]*)= ]]; then
eval "$line"
- var_name=$(echo "$line" | cut -d' ' -f2 | cut -d'=' -f1)
+ var_name="${BASH_REMATCH[1]}"
echo "Set $var_name"
+ else
+ echo "Skipping unexpected loader output line" >&2
fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/load-secrets.sh` around lines 19 - 21, The current code unsafely runs
eval on untrusted lines (eval "$line") — instead, validate and parse the export
line before executing: ensure the string in variable line strictly matches the
format export <NAME>=<VALUE>, extract NAME and VALUE without eval, verify NAME
matches a safe identifier regex (e.g. ^[A-Za-z_][A-Za-z0-9_]*$) and that VALUE
contains no dangerous characters/expansions (no backticks, $(), newlines), then
set the variable using a safe export/declare invocation (e.g., export
"$NAME=$VALUE") rather than eval; reference the variables/operations line,
var_name and the current eval call when locating where to implement this
validation and safe assignment.
Comment on lines
+2
to
+3
| "GOOGLE_OAUTH_CLIENT_ID": "81e4469b-851b-42b2-b03c-b393006d2659", | ||
| "GOOGLE_OAUTH_CLIENT_SECRET": "b36a25a6-2d68-48d6-885c-b393006d272f" |
Contributor
There was a problem hiding this comment.
Avoid committing concrete vault item identifiers.
Lines 2–3 include environment-specific Bitwarden item IDs. Even without secret values, these identifiers are sensitive metadata and should not be hardcoded in a tracked default mapping file.
🔐 Safer template pattern
{
- "GOOGLE_OAUTH_CLIENT_ID": "81e4469b-851b-42b2-b03c-b393006d2659",
- "GOOGLE_OAUTH_CLIENT_SECRET": "b36a25a6-2d68-48d6-885c-b393006d272f"
+ "GOOGLE_OAUTH_CLIENT_ID": "<BITWARDEN_ITEM_ID_FOR_GOOGLE_OAUTH_CLIENT_ID>",
+ "GOOGLE_OAUTH_CLIENT_SECRET": "<BITWARDEN_ITEM_ID_FOR_GOOGLE_OAUTH_CLIENT_SECRET>"
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@secrets/bws_map.json` around lines 2 - 3, The file secrets/bws_map.json
currently contains concrete Bitwarden item IDs (GOOGLE_OAUTH_CLIENT_ID and
GOOGLE_OAUTH_CLIENT_SECRET) which are sensitive metadata; remove these hardcoded
IDs and replace them with non-sensitive placeholders or a template pattern
(e.g., "__BITWARDEN_ITEM_ID__" or null) and add a short comment/instructions in
the file header guiding developers to populate their local, untracked overrides
or use environment-specific config; ensure symbols GOOGLE_OAUTH_CLIENT_ID and
GOOGLE_OAUTH_CLIENT_SECRET remain present as keys but do not contain production
IDs so CI and docs still reference them safely.
| Integration test that creates a Google Doc and then reads it back. | ||
| Requires valid Google Credentials in the environment. | ||
| """ | ||
| print(f"\nTesting with email: {test_email}") |
Contributor
There was a problem hiding this comment.
Avoid printing live user identifiers in integration logs.
Line 21 and Line 45 emit email/document identifiers to stdout, which can leak PII into CI logs.
Proposed fix
- print(f"\nTesting with email: {test_email}")
+ # Keep integration logs non-PII
...
- print(f"Created Doc ID: {doc_id}")
+ # Avoid logging raw document identifiersAlso applies to: 45-45
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/integration/test_gdocs_live.py` at line 21, Remove or redact direct
printing of live user identifiers in tests/integration/test_gdocs_live.py:
replace the print statements that output test_email and any document identifier
(variables named test_email and whatever variable is printed at line 45) with
non-PII logging or no-op assertions; e.g., stop emitting the raw email/document
id to stdout, log only a redacted form (masking the local part) or a generic
message like "using test credentials" so the test behavior remains the same but
PII is not written to CI logs.
- Fix gforms/forms_tools.py: use camelCase documentTitle for API - Fix core/pause_middleware.py: remove redundant call_next() that discarded response - Fix core/log_formatter.py: remove unreachable code after early return - Fix TypedDict imports: use typing_extensions.TypedDict for Python < 3.12 compatibility All 221 unit tests pass.
Author
Review SummaryIssues Addressed ✅
Tests ✅
Duplicate PRs Check ✅Reviewed 23+ open community PRs. The following PRs have significant overlap with #522 and may have merge conflicts:
Note: PR #522 description still shows "DO NOT MERGE" - review and update description before merging. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (4)
core/pause_middleware.py (2)
28-30: Accept common truthy values for pause env parsing.
WORKSPACE_MCP_PAUSEDcurrently activates only for exact"true". Supporting common truthy values (1,yes,on) makes operational toggling less error-prone.♻️ Proposed refactor
def is_paused() -> bool: """Determine if the service is currently paused.""" - env_paused = os.getenv("WORKSPACE_MCP_PAUSED", "false").lower() == "true" + env_value = os.getenv("WORKSPACE_MCP_PAUSED", "false").strip().lower() + env_paused = env_value in {"1", "true", "yes", "on"} file_paused = os.path.exists(_pause_flag_file()) return env_paused or file_paused🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/pause_middleware.py` around lines 28 - 30, The env parsing in the pause check only treats WORKSPACE_MCP_PAUSED=="true" as truthy; update the logic in the function that sets env_paused (the code referencing env_paused and WORKSPACE_MCP_PAUSED) to accept common truthy values ("true","1","yes","on") case-insensitively (e.g., normalize to lower() and check membership in a set of truthy strings) while keeping the existing file-based check via _pause_flag_file(); ensure you only change the env_paused assignment so return env_paused or file_paused behavior is preserved.
36-42: Normalize health path before comparison.The bypass currently matches only
"/health". Normalizing trailing slashes avoids returning 503 for equivalent health URLs like"/health/".♻️ Proposed refactor
if is_paused(): - path = request.url.path + path = request.url.path.rstrip("/") or "/" # Allow health endpoint to report paused state differently if path == "/health": # Return paused indicator for health endpoint return JSONResponse( {"status": "paused", "service": "workspace-mcp"}, status_code=200 )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/pause_middleware.py` around lines 36 - 42, The health-path check in the pause middleware currently only matches "/health" and misses equivalent URLs like "/health/"; update the comparison in the handler that reads request.url.path (used to decide to return JSONResponse({"status": "paused", ...})) to normalize the path before comparing — e.g., compute a normalized_path = request.url.path.rstrip("/") (treating "/" carefully) or otherwise trim a single trailing slash, then compare normalized_path == "/health" so both "/health" and "/health/" return the paused health JSONResponse; reference the code handling request.url.path and the JSONResponse return to locate where to change the comparison.core/log_formatter.py (1)
163-172: Prevent duplicate file handlers on repeated configuration calls.Line 171 unconditionally adds a new
FileHandler; repeated invocations can duplicate log lines and inflate I/O.Suggested fix
file_handler = logging.FileHandler(log_file_path, mode="a") file_handler.setLevel(logging.DEBUG) @@ file_handler.setFormatter(file_formatter) - target_logger.addHandler(file_handler) + already_attached = any( + isinstance(h, logging.FileHandler) + and os.path.abspath(getattr(h, "baseFilename", "")) == os.path.abspath(log_file_path) + for h in target_logger.handlers + ) + if not already_attached: + target_logger.addHandler(file_handler)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/log_formatter.py` around lines 163 - 172, The FileHandler is always added to target_logger which causes duplicate handlers on repeated config; before creating or adding a new logging.FileHandler check target_logger.handlers for an existing logging.FileHandler with the same destination (handler.baseFilename) and reuse or skip adding if found, or update its level/formatter instead; reference the symbols file_handler, target_logger, logging.FileHandler and addHandler to locate and modify the logic so duplicate file handlers are not attached.gdocs/docs_tools.py (1)
665-672: Avoid re-wrapping exceptions inside a function already covered byhandle_http_errors.This block converts structured errors into generic
Exceptions, which degrades centralized status-aware handling and message quality. Prefer letting the decorator handle raw exceptions directly. Apply the same cleanup to the matching blocks at Lines 921-928 and Lines 1269-1276.Proposed simplification
- except HttpError as error: - message = f"API error: {error}" - logger.error(message, exc_info=True) - raise Exception(message) - except Exception as e: - message = f"Unexpected error: {e}" - logger.exception(message) - raise Exception(message) + except HttpError: + raise + except Exception: + raise🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@gdocs/docs_tools.py` around lines 665 - 672, The except blocks that catch HttpError and generic Exception currently wrap and raise new generic Exception objects, which defeats centralized handling by handle_http_errors; for each occurrence (the HttpError/generic Exception handlers shown and the matching blocks noted later) stop re-wrapping errors — keep any logging you want but re-raise the original exception using bare "raise" (not "raise Exception(message)"), or remove the generic except entirely so the original exception propagates to the handle_http_errors decorator; update the handlers in gdocs/docs_tools.py to log with exc_info=True or logger.exception(...) and then use "raise" to preserve exception type and traceback.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@core/log_formatter.py`:
- Around line 177-180: The except handler references log_file_path which may be
uninitialized if an earlier statement fails; initialize log_file_path to a
sensible default (e.g., None or an empty string) immediately before the try
block that contains the file logging setup in this module (the block that later
writes "CRITICAL: Failed to set up file logging to '{log_file_path}'"); ensure
any subsequent use in the except clause safely formats or checks log_file_path
(e.g., coalesce to "<unknown>" when None) so the original exception is not
masked by an UnboundLocalError.
- Around line 111-123: The code currently accesses handler.stream.name directly
in log_formatter.py (in the block that sets handler.setFormatter(formatter) and
when building console_handlers) which can raise AttributeError for custom
streams; update those checks to guard attribute access (e.g., use
getattr(handler.stream, "name", None)) or, better, compare the stream object
identity against sys.stderr and sys.stdout (import sys) instead of comparing
names; modify the conditional in the handler loop and the console_handlers list
comprehension to use these guarded checks (referencing handler, handler.stream,
formatter, root_logger, and console_handlers) so custom file-like streams
without .name no longer crash.
In `@gforms/forms_tools.py`:
- Around line 135-143: The payload passed to service.forms().setPublishSettings
(in the asyncio.to_thread call) is using the wrong flat fields; change
settings_body to include the nested publishSettings object with a publishState
containing both isPublished and isAcceptingResponses (e.g., {"publishSettings":
{"publishState": {"isPublished": ..., "isAcceptingResponses": ...}}}) and pass
updateMask="publishSettings.publishState" to setPublishSettings; also enforce
that if isAcceptingResponses is True you set isPublished=True so you never send
isAcceptingResponses=true with isPublished=false.
- Around line 43-49: The current create payload in forms.create (the form_body
built in the function) incorrectly sets info.description which the Google Forms
v1 forms.create endpoint rejects; remove adding form_body["info"]["description"]
before calling forms.create, keep only info.title and info.documentTitle, then
after successful creation call forms.batchUpdate with an updateFormInfo request
(using the created formId) to set the description field via the
UpdateFormInfoRequest; update the code paths that call forms.create and then
invoke forms.batchUpdate/updateFormInfo to apply the description when
description is provided.
In `@gtasks/tasks_tools.py`:
- Around line 411-414: The code passes uncapped max_results/results_remaining
into the Google Tasks API causing 400 errors; modify the logic around params =
{"tasklist": task_list_id} so that any maxResults sent to tasks.list is capped
at 100 (the API per-request max) — e.g., replace uses of max_results and
results_remaining when setting params["maxResults"] with min(max_results, 100)
or min(results_remaining, 100); update both the initial params assignment and
the block where results_remaining is used (references: params, max_results,
results_remaining, LIST_TASKS_MAX_RESULTS_MAX) to enforce this cap before
calling tasks.list.
---
Nitpick comments:
In `@core/log_formatter.py`:
- Around line 163-172: The FileHandler is always added to target_logger which
causes duplicate handlers on repeated config; before creating or adding a new
logging.FileHandler check target_logger.handlers for an existing
logging.FileHandler with the same destination (handler.baseFilename) and reuse
or skip adding if found, or update its level/formatter instead; reference the
symbols file_handler, target_logger, logging.FileHandler and addHandler to
locate and modify the logic so duplicate file handlers are not attached.
In `@core/pause_middleware.py`:
- Around line 28-30: The env parsing in the pause check only treats
WORKSPACE_MCP_PAUSED=="true" as truthy; update the logic in the function that
sets env_paused (the code referencing env_paused and WORKSPACE_MCP_PAUSED) to
accept common truthy values ("true","1","yes","on") case-insensitively (e.g.,
normalize to lower() and check membership in a set of truthy strings) while
keeping the existing file-based check via _pause_flag_file(); ensure you only
change the env_paused assignment so return env_paused or file_paused behavior is
preserved.
- Around line 36-42: The health-path check in the pause middleware currently
only matches "/health" and misses equivalent URLs like "/health/"; update the
comparison in the handler that reads request.url.path (used to decide to return
JSONResponse({"status": "paused", ...})) to normalize the path before comparing
— e.g., compute a normalized_path = request.url.path.rstrip("/") (treating "/"
carefully) or otherwise trim a single trailing slash, then compare
normalized_path == "/health" so both "/health" and "/health/" return the paused
health JSONResponse; reference the code handling request.url.path and the
JSONResponse return to locate where to change the comparison.
In `@gdocs/docs_tools.py`:
- Around line 665-672: The except blocks that catch HttpError and generic
Exception currently wrap and raise new generic Exception objects, which defeats
centralized handling by handle_http_errors; for each occurrence (the
HttpError/generic Exception handlers shown and the matching blocks noted later)
stop re-wrapping errors — keep any logging you want but re-raise the original
exception using bare "raise" (not "raise Exception(message)"), or remove the
generic except entirely so the original exception propagates to the
handle_http_errors decorator; update the handlers in gdocs/docs_tools.py to log
with exc_info=True or logger.exception(...) and then use "raise" to preserve
exception type and traceback.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
.kilo-prompt/mission.txt.kilo-prompt/review-mission.txtcore/log_formatter.pycore/pause_middleware.pygdocs/docs_tools.pygforms/forms_tools.pygtasks/tasks_tools.py
✅ Files skipped from review due to trivial changes (2)
- .kilo-prompt/review-mission.txt
- .kilo-prompt/mission.txt
Comment on lines
+111
to
+123
| if isinstance(handler, logging.StreamHandler) and handler.stream.name in [ | ||
| "<stderr>", | ||
| "<stdout>", | ||
| ]: | ||
| handler.setFormatter(formatter) | ||
|
|
||
| # If no console handler exists, create one | ||
| console_handlers = [h for h in root_logger.handlers | ||
| if isinstance(h, logging.StreamHandler) and h.stream.name in ['<stderr>', '<stdout>']] | ||
| console_handlers = [ | ||
| h | ||
| for h in root_logger.handlers | ||
| if isinstance(h, logging.StreamHandler) | ||
| and h.stream.name in ["<stderr>", "<stdout>"] | ||
| ] |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's look at the file structure and read the specific lines mentioned
fd core/log_formatter.pyRepository: taylorwilsdon/google_workspace_mcp
Length of output: 60
🏁 Script executed:
# Read the file to see the context around lines 111-123
cat -n core/log_formatter.py | sed -n '100,135p'Repository: taylorwilsdon/google_workspace_mcp
Length of output: 1469
🏁 Script executed:
# Check for all stream.name accesses in the file
rg -n '\.stream\.name' core/log_formatter.pyRepository: taylorwilsdon/google_workspace_mcp
Length of output: 222
🏁 Script executed:
# Also check if there are any custom stream implementations in the codebase
rg -n 'class.*Stream' --type pyRepository: taylorwilsdon/google_workspace_mcp
Length of output: 60
🌐 Web query:
Python logging StreamHandler stream.name attribute safety
💡 Result:
logging.StreamHandler(stream=...) only requires that stream provide write() and flush()—it can be any “file-like object”, not necessarily a real file object. Therefore, relying on handler.stream.name is not safe: many valid streams won’t have a .name attribute (e.g., io.StringIO, sockets, custom wrappers). [1]
If you need a stable identifier:
- Prefer the handler’s own name (instead of the stream’s): in modern Python you can look up handlers by name (
logging.getHandlerByName()/logging.getHandlerNames()), and the underlyingHandlerimplementation maintains an internal “map of handlers … used for handler lookup by name”. [2] - If you must display something about the stream, guard it:
getattr(handler.stream, "name", None)and treat it as optional. This matches the fact that the logging API does not promise.nameon streams—onlywrite/flush. [1] - If you specifically want the file path for file-based logging, check for a
FileHandlerrather than assuming.stream.name(sinceStreamHandlermay not be a file at all). [1]
Sources:
[1] Python docs: StreamHandler accepts any object supporting write() and flush() (no .name guarantee)
[2] Python docs: handlers are tracked in an internal map for lookup by handler name; logging.getHandlerByName()/getHandlerNames() exist (3.12+)
🏁 Script executed:
# Let's also check if the logging module is imported at the top of the file
head -20 core/log_formatter.pyRepository: taylorwilsdon/google_workspace_mcp
Length of output: 591
🏁 Script executed:
# Check if sys is imported and how StreamHandler is typically used
rg -n 'import logging|import sys|from logging' core/log_formatter.pyRepository: taylorwilsdon/google_workspace_mcp
Length of output: 108
Guard stream.name access to prevent formatter setup crashes with custom streams.
Lines 111 and 122 directly access handler.stream.name without checking if the attribute exists. The Python logging API accepts any file-like object with write() and flush() methods—many valid streams (e.g., io.StringIO, custom wrappers) won't have a .name attribute. This will raise AttributeError at runtime with such streams.
Replace stream.name comparisons with direct identity checks against sys.stderr and sys.stdout using getattr() guards:
Suggested fix
- for handler in root_logger.handlers:
- if isinstance(handler, logging.StreamHandler) and handler.stream.name in [
- "<stderr>",
- "<stdout>",
- ]:
+ for handler in root_logger.handlers:
+ stream = getattr(handler, "stream", None)
+ if isinstance(handler, logging.StreamHandler) and stream in [
+ sys.stderr,
+ sys.stdout,
+ ]:
handler.setFormatter(formatter)
- console_handlers = [
- h
- for h in root_logger.handlers
- if isinstance(h, logging.StreamHandler)
- and h.stream.name in ["<stderr>", "<stdout>"]
- ]
+ console_handlers = [
+ h
+ for h in root_logger.handlers
+ if isinstance(h, logging.StreamHandler)
+ and getattr(h, "stream", None) in [sys.stderr, sys.stdout]
+ ]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@core/log_formatter.py` around lines 111 - 123, The code currently accesses
handler.stream.name directly in log_formatter.py (in the block that sets
handler.setFormatter(formatter) and when building console_handlers) which can
raise AttributeError for custom streams; update those checks to guard attribute
access (e.g., use getattr(handler.stream, "name", None)) or, better, compare the
stream object identity against sys.stderr and sys.stdout (import sys) instead of
comparing names; modify the conditional in the handler loop and the
console_handlers list comprehension to use these guarded checks (referencing
handler, handler.stream, formatter, root_logger, and console_handlers) so custom
file-like streams without .name no longer crash.
Comment on lines
177
to
+180
| except Exception as e: | ||
| sys.stderr.write(f"CRITICAL: Failed to set up file logging to '{log_file_path}': {e}\n") | ||
| sys.stderr.write( | ||
| f"CRITICAL: Failed to set up file logging to '{log_file_path}': {e}\n" | ||
| ) |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find and read the configure_file_logging function to see the full context
fd -e py "log_formatter.py" -x wc -l {}Repository: taylorwilsdon/google_workspace_mcp
Length of output: 105
🏁 Script executed:
#!/bin/bash
# Read the configure_file_logging function with context around lines 177-180
cat -n core/log_formatter.py | sed -n '1,250p'Repository: taylorwilsdon/google_workspace_mcp
Length of output: 7739
Initialize log_file_path before the try block to prevent UnboundLocalError in exception handler.
If any statement before line 161 raises an exception, the except block on line 179 will attempt to use an undefined log_file_path variable, causing an UnboundLocalError that masks the root cause.
Suggested fix
def configure_file_logging(logger_name: str = None) -> bool:
# Check if stateless mode is enabled
stateless_mode = (
os.getenv("WORKSPACE_MCP_STATELESS_MODE", "false").lower() == "true"
)
if stateless_mode:
logger = logging.getLogger(logger_name)
logger.debug("File logging disabled in stateless mode")
return False
# Configure file logging for normal mode
+ log_file_path = "<unresolved>"
try:
target_logger = logging.getLogger(logger_name)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@core/log_formatter.py` around lines 177 - 180, The except handler references
log_file_path which may be uninitialized if an earlier statement fails;
initialize log_file_path to a sensible default (e.g., None or an empty string)
immediately before the try block that contains the file logging setup in this
module (the block that later writes "CRITICAL: Failed to set up file logging to
'{log_file_path}'"); ensure any subsequent use in the except clause safely
formats or checks log_file_path (e.g., coalesce to "<unknown>" when None) so the
original exception is not masked by an UnboundLocalError.
Comment on lines
+43
to
+49
| form_body: Dict[str, Any] = {"info": {"title": title}} | ||
|
|
||
| if description: | ||
| form_body["info"]["description"] = description | ||
|
|
||
| if document_title: | ||
| form_body["info"]["document_title"] = document_title | ||
| form_body["info"]["documentTitle"] = document_title |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Google Forms API v1 forms.create request body schema info.description allowed
💡 Result:
In the Google Forms API v1, the forms.create request body is a Form object, whose info field is an Info object that does include description in the general schema (info: { title, documentTitle, description }).[2]
However, for forms.create specifically, Google states that only form.info.title and form.info.document_title are used; all other fields (explicitly including the form description) are disallowed/ignored during creation.[1][3]
To set the description, create the form with a title first, then update it (e.g., via forms.batchUpdate / update form info).[1][3]
Example (create):
{
"info": {
"title": "My new form"
}
}[3]
forms.create request includes a disallowed field (info.description).
The Google Forms API v1 forms.create endpoint only accepts info.title and info.documentTitle; the description field is not permitted and will be ignored. Set description after form creation via batchUpdate with updateFormInfo.
Proposed fix
- if description:
- form_body["info"]["description"] = description
-
if document_title:
form_body["info"]["documentTitle"] = document_title
created_form = await asyncio.to_thread(
service.forms().create(body=form_body).execute
)
+
+ if description:
+ await asyncio.to_thread(
+ service.forms()
+ .batchUpdate(
+ formId=created_form["formId"],
+ body={
+ "requests": [
+ {
+ "updateFormInfo": {
+ "info": {"description": description},
+ "updateMask": "description",
+ }
+ }
+ ]
+ },
+ )
+ .execute
+ )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@gforms/forms_tools.py` around lines 43 - 49, The current create payload in
forms.create (the form_body built in the function) incorrectly sets
info.description which the Google Forms v1 forms.create endpoint rejects; remove
adding form_body["info"]["description"] before calling forms.create, keep only
info.title and info.documentTitle, then after successful creation call
forms.batchUpdate with an updateFormInfo request (using the created formId) to
set the description field via the UpdateFormInfoRequest; update the code paths
that call forms.create and then invoke forms.batchUpdate/updateFormInfo to apply
the description when description is provided.
Comment on lines
+135
to
+143
| settings_body = { | ||
| "publishAsTemplate": publish_as_template, | ||
| "requireAuthentication": require_authentication, | ||
| } | ||
|
|
||
| settings_body = { | ||
| "publishAsTemplate": publish_as_template, | ||
| "requireAuthentication": require_authentication | ||
| } | ||
| await asyncio.to_thread( | ||
| service.forms() | ||
| .setPublishSettings(formId=form_id, body=settings_body) | ||
| .execute |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Google Forms API v1 setPublishSettings request body schema
💡 Result:
forms.setPublishSettings (POST https://forms.googleapis.com/v1/forms/{formId}:setPublishSettings) request body (SetPublishSettingsRequest) schema: [1]
{
"publishSettings": {
"publishState": {
"isPublished": true,
"isAcceptingResponses": true
}
},
"updateMask": "publishState"
}Field details:
-
publishSettings (required):
PublishSettingsobject. [1]- publishState (optional):
PublishStateobject. When updatingpublishState, bothisPublishedandisAcceptingResponsesmust be set;isAcceptingResponses=truewithisPublished=falseis invalid. [2]- isPublished (boolean, required in updates): whether the form is published/visible. [2]
- isAcceptingResponses (boolean, required in updates): whether the form accepts responses (forced to
falseifisPublished=false). [2]
- publishState (optional):
-
updateMask (optional): FieldMask string; accepted values include
publishStateor"*". [1]
References: [1][2]
setPublishSettings payload shape is incorrect for the Forms API.
The current body is flat with incorrect field names (publishAsTemplate / requireAuthentication), but forms.setPublishSettings expects a nested publishSettings object containing a publishState object with isPublished and isAcceptingResponses fields. Additionally, updateMask must be specified. As written, the API will reject this operation.
When updating publishState, both isPublished and isAcceptingResponses must be set together; isAcceptingResponses=true with isPublished=false is invalid.
Proposed fix (schema-aligned shape)
-async def manage_form(
+async def manage_form(
service,
user_google_email: str,
operation: Literal["get", "update_settings"],
form_id: str,
- publish_as_template: bool = False,
- require_authentication: bool = False,
+ is_published: bool = True,
+ is_accepting_responses: bool = True,
) -> str:
@@
- settings_body = {
- "publishAsTemplate": publish_as_template,
- "requireAuthentication": require_authentication,
- }
+ settings_body = {
+ "publishSettings": {
+ "publishState": {
+ "isPublished": is_published,
+ "isAcceptingResponses": is_accepting_responses,
+ }
+ },
+ "updateMask": "publishState",
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@gforms/forms_tools.py` around lines 135 - 143, The payload passed to
service.forms().setPublishSettings (in the asyncio.to_thread call) is using the
wrong flat fields; change settings_body to include the nested publishSettings
object with a publishState containing both isPublished and isAcceptingResponses
(e.g., {"publishSettings": {"publishState": {"isPublished": ...,
"isAcceptingResponses": ...}}}) and pass
updateMask="publishSettings.publishState" to setPublishSettings; also enforce
that if isAcceptingResponses is True you set isPublished=True so you never send
isAcceptingResponses=true with isPublished=false.
Comment on lines
+411
to
+414
| params: Dict[str, Any] = {"tasklist": task_list_id} | ||
| if max_results is not None: | ||
| params["maxResults"] = max_results | ||
| if page_token: |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Google Tasks API v1 tasks.list maxResults parameter maximum limit documentation
💡 Result:
For Google Tasks API v1 tasks.list, the maxResults parameter has:
- Default: 20
- Maximum allowed: 100
This is documented in the REST reference for tasks.list [1] (also summarized in the general Tasks “Use Tasks Parameters” page [2]).
Sources:
[1] Google Tasks API v1 – tasks.list (REST reference) (developers.google.com)
[2] Google Tasks API – Parameters (maxResults max allowable value) (developers.google.com)
Citations:
- 1: https://developers.google.com/workspace/tasks/reference/rest/v1/tasks/list?utm_source=openai
- 2: https://developers.google.com/workspace/tasks/params?utm_source=openai
🏁 Script executed:
cd gtasks && head -460 tasks_tools.py | tail -60Repository: taylorwilsdon/google_workspace_mcp
Length of output: 2808
🏁 Script executed:
rg "LIST_TASKS_MAX_RESULTS" gtasks/tasks_tools.py -nRepository: taylorwilsdon/google_workspace_mcp
Length of output: 359
Cap tasks.list page size to 100 for each API call.
The maxResults parameter for tasks.list has a per-request maximum of 100. Current logic can send values up to 10,000 (via LIST_TASKS_MAX_RESULTS_MAX), causing API errors. At line 411-414, max_results is passed directly without capping. At line 457, results_remaining (which can exceed 100) is sent as maxResults, triggering 400 responses.
Proposed fix
- params: Dict[str, Any] = {"tasklist": task_list_id}
- if max_results is not None:
- params["maxResults"] = max_results
+ requested_total = (
+ min(max_results, LIST_TASKS_MAX_RESULTS_MAX)
+ if max_results is not None
+ else LIST_TASKS_MAX_RESULTS_DEFAULT
+ )
+ requested_total = max(1, requested_total)
+
+ params: Dict[str, Any] = {"tasklist": task_list_id}
+ params["maxResults"] = min(requested_total, 100)
@@
- results_remaining = (
- min(max_results, LIST_TASKS_MAX_RESULTS_MAX)
- if max_results
- else LIST_TASKS_MAX_RESULTS_DEFAULT
- )
+ results_remaining = requested_total
results_remaining -= len(tasks)
while results_remaining > 0 and next_page_token:
params["pageToken"] = next_page_token
- params["maxResults"] = str(results_remaining)
+ params["maxResults"] = min(results_remaining, 100)
result = await asyncio.to_thread(service.tasks().list(**params).execute)Also applies to: 447-457
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@gtasks/tasks_tools.py` around lines 411 - 414, The code passes uncapped
max_results/results_remaining into the Google Tasks API causing 400 errors;
modify the logic around params = {"tasklist": task_list_id} so that any
maxResults sent to tasks.list is capped at 100 (the API per-request max) — e.g.,
replace uses of max_results and results_remaining when setting
params["maxResults"] with min(max_results, 100) or min(results_remaining, 100);
update both the initial params assignment and the block where results_remaining
is used (references: params, max_results, results_remaining,
LIST_TASKS_MAX_RESULTS_MAX) to enforce this cap before calling tasks.list.
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
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.
DO NOT MERGE
Summary
Comprehensive unit test suite covering 219 tests across 8 test files:
Test Results
Summary by CodeRabbit
New Features
Improvements
Configuration
Bug Fixes
Documentation & Tests