enh: Partial Scope Grants & Granular Permissions - #510
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a granular, per-service permission system with a new CLI Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI (main.py)
participant Perm as auth.permissions
participant ToolReg as core.tool_registry
participant Scopes as auth.scopes
participant OAuth as auth.google_auth
CLI->>Perm: parse_permissions_arg(perms)
Perm-->>CLI: parsed permissions dict
CLI->>Perm: set_permissions(parsed)
Perm-->>CLI: permissions active
CLI->>ToolReg: initialize/load tools
ToolReg->>Perm: is_permissions_mode()
Perm-->>ToolReg: True
ToolReg->>Perm: get_allowed_scopes_set()
Perm-->>ToolReg: allowed scopes set
ToolReg->>ToolReg: disable tools whose required scopes ⊄ allowed set
ToolReg-->>CLI: filtered tool list
Scopes->>Perm: is_permissions_mode()
Perm-->>Scopes: True
Scopes->>Perm: get_all_permission_scopes()
Perm-->>Scopes: cumulative permission scopes
Scopes-->>CLI: BASE_SCOPES + permission scopes
OAuth->>OAuth: exchange code -> credentials
OAuth->>OAuth: detect partial grant (granted_scopes vs requested)
OAuth->>OAuth: rebuild Credentials with granted scopes
OAuth-->>CLI: auth complete
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✅ 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 |
There was a problem hiding this comment.
Pull request overview
Adds support for granular per-service permission levels and partial OAuth scope grants, aligning tool availability and requested scopes with more constrained user intent while fixing related test/config issues.
Changes:
- Introduces
--permissions service:level ...CLI flag and permission-mode tool loading/filtering. - Adds granular permission scope mapping (
auth/permissions.py) and integrates it into scope generation + tool filtering. - Relaxes OAuth scope mismatch handling and updates tests/pytest configuration.
Reviewed changes
Copilot reviewed 7 out of 8 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
uv.lock |
Bumps workspace-mcp version to 1.13.0. |
pyproject.toml |
Updates pytest config to ignore the Apps Script manual test file via addopts. |
main.py |
Adds --permissions, validates mutual exclusivity with --read-only, and wires permissions into tool loading + display. |
core/tool_registry.py |
Adds permissions-mode filtering based on allowed scopes and adjusts mode logging/behavior. |
auth/scopes.py |
Overrides scope generation when permissions mode is active (lazy import to avoid circular init). |
auth/permissions.py |
New module defining per-service permission levels, scope expansion, and CLI parsing. |
auth/google_auth.py |
Allows partial scope grants and stores granted (not requested) scopes when they differ. |
tests/gchat/test_chat_tools.py |
Makes test helper _unwrap tolerant of tools that don’t expose .fn. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| "Granular per-service permission levels. Format: service:level. " | ||
| "Example: --permissions gmail:organize drive:readonly. " | ||
| "Gmail levels: readonly, organize, drafts, send, full (cumulative). " | ||
| "Other services: readonly, full. " | ||
| "Mutually exclusive with --read-only." |
There was a problem hiding this comment.
The --permissions help text implies Gmail permission levels are truly granular (e.g., organize vs send), but organize maps to GMAIL_MODIFY_SCOPE (see auth/permissions.py), and gmail.modify is treated as covering gmail.send/gmail.compose in the project’s scope hierarchy. This can mislead users about what they’re actually granting on Google’s consent screen. Consider clarifying in the CLI help that some Google scopes are inherently broader (so levels mainly affect which tools are enabled), or adjust the level mappings/names accordingly.
| "Granular per-service permission levels. Format: service:level. " | |
| "Example: --permissions gmail:organize drive:readonly. " | |
| "Gmail levels: readonly, organize, drafts, send, full (cumulative). " | |
| "Other services: readonly, full. " | |
| "Mutually exclusive with --read-only." | |
| "Per-service permission levels used by this app to control which tools and features are enabled. " | |
| "Format: service:level. Example: --permissions gmail:organize drive:readonly. " | |
| "Gmail levels: readonly, organize, drafts, send, full (cumulative). Other services: readonly, full. " | |
| "Note: some Google OAuth scopes (for example Gmail's modify scope) are inherently broader, so multiple " | |
| "Gmail levels may map to the same underlying Google scope on the consent screen; the levels here mainly " | |
| "affect which tools are available. Mutually exclusive with --read-only." |
| # No scope hierarchy expansion here — permission levels are already cumulative | ||
| # and explicitly define allowed scopes. Hierarchy expansion would defeat the | ||
| # purpose (e.g. gmail.modify in the hierarchy covers gmail.send, but the | ||
| # "organize" permission level intentionally excludes gmail.send). |
There was a problem hiding this comment.
This comment suggests granular permissions can intentionally exclude gmail.send even though gmail.modify is present at lower levels; however, elsewhere in the codebase gmail.modify is explicitly treated as covering gmail.send via SCOPE_HIERARCHY (auth/scopes.py). Since the OAuth consent will still reflect the broader gmail.modify scope, the comment is misleading—please reword to reflect that hierarchy expansion is skipped only for tool filtering, not to change what the upstream OAuth scope implies.
| # No scope hierarchy expansion here — permission levels are already cumulative | |
| # and explicitly define allowed scopes. Hierarchy expansion would defeat the | |
| # purpose (e.g. gmail.modify in the hierarchy covers gmail.send, but the | |
| # "organize" permission level intentionally excludes gmail.send). | |
| # No scope hierarchy expansion is applied here for tool filtering — permission | |
| # levels already define the exact scopes that should enable a tool. We use the | |
| # scopes from permissions mode as-is when deciding which tools to keep, even | |
| # though upstream OAuth consent and SCOPE_HIERARCHY still treat broader scopes | |
| # (e.g. gmail.modify) as covering narrower ones (e.g. gmail.send). |
| ) | ||
| return list(set(scopes)) | ||
| except ImportError: | ||
| pass |
There was a problem hiding this comment.
get_scopes_for_tools() now has a new behavior branch for granular permissions mode, but there are no corresponding unit tests. Since this function already has focused tests in tests/test_scopes.py, please add coverage for permissions mode (e.g., setting permissions then asserting returned scopes are BASE_SCOPES + permission scopes, and that non-permitted tool scopes aren’t included).
| pass | |
| logger.debug( | |
| "auth.permissions not available; falling back to tool-based scope mapping", | |
| exc_info=True, | |
| ) |
| credentials = Credentials( | ||
| token=credentials.token, | ||
| refresh_token=credentials.refresh_token, | ||
| token_uri=credentials.token_uri, | ||
| client_id=credentials.client_id, | ||
| client_secret=credentials.client_secret, | ||
| scopes=list(granted), | ||
| expiry=credentials.expiry, | ||
| ) |
There was a problem hiding this comment.
When handling partial grants, a new Credentials(...) object is constructed with scopes=list(granted) but other fields from the original credentials (notably id_token, and potentially token metadata) are dropped. This file later relies on credentials.id_token for session/email extraction and logging, so the partial-grant path can cause avoidable warnings/behavior differences. Preserve the extra credential fields when recreating the object (or update the scopes on the existing credentials if supported).
| credentials = Credentials( | |
| token=credentials.token, | |
| refresh_token=credentials.refresh_token, | |
| token_uri=credentials.token_uri, | |
| client_id=credentials.client_id, | |
| client_secret=credentials.client_secret, | |
| scopes=list(granted), | |
| expiry=credentials.expiry, | |
| ) | |
| # Preserve all credential metadata when adjusting scopes for a | |
| # partial OAuth grant. Prefer using with_scopes() if available so | |
| # fields like id_token and quota_project_id are retained. | |
| if hasattr(credentials, "with_scopes") and callable( | |
| getattr(credentials, "with_scopes") | |
| ): | |
| credentials = credentials.with_scopes(list(granted)) | |
| else: | |
| credentials = Credentials( | |
| token=credentials.token, | |
| refresh_token=credentials.refresh_token, | |
| id_token=getattr(credentials, "id_token", None), | |
| token_uri=credentials.token_uri, | |
| client_id=credentials.client_id, | |
| client_secret=credentials.client_secret, | |
| scopes=list(granted), | |
| expiry=credentials.expiry, | |
| quota_project_id=getattr(credentials, "quota_project_id", None), | |
| ) |
| levels = SERVICE_PERMISSION_LEVELS.get(service) | ||
| if levels is None: | ||
| return [] | ||
| return [name for name, _ in levels] | ||
|
|
||
|
|
||
| def parse_permissions_arg(permissions_list: List[str]) -> Dict[str, str]: | ||
| """ | ||
| Parse --permissions arguments like ["gmail:organize", "drive:full"]. | ||
|
|
||
| Returns dict mapping service -> level. | ||
| Raises ValueError on parse errors (unknown service, invalid level, bad format). | ||
| """ | ||
| result: Dict[str, str] = {} | ||
| for entry in permissions_list: | ||
| if ":" not in entry: | ||
| raise ValueError( | ||
| f"Invalid permission format: '{entry}'. " | ||
| f"Expected 'service:level' (e.g., 'gmail:organize', 'drive:readonly')" | ||
| ) | ||
| service, level = entry.split(":", 1) | ||
| if service in result: | ||
| raise ValueError(f"Duplicate service in permissions: '{service}'") | ||
| if service not in SERVICE_PERMISSION_LEVELS: | ||
| raise ValueError( | ||
| f"Unknown service: '{service}'. " | ||
| f"Valid services: {sorted(SERVICE_PERMISSION_LEVELS.keys())}" | ||
| ) | ||
| valid = get_valid_levels(service) | ||
| if level not in valid: |
There was a problem hiding this comment.
The new permissions parsing/mapping logic is non-trivial and currently untested. Please add unit tests for parse_permissions_arg() (valid parsing, duplicate service, bad format, unknown service/level) and for cumulative scope expansion in get_scopes_for_permission() to prevent regressions as services/levels evolve.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
auth/permissions.py (1)
78-81:docs:fullincludesDRIVE_READONLY_SCOPEredundantly alongsideDRIVE_FILE_SCOPE.Since scopes are cumulative,
docs:fullalready inheritsDRIVE_READONLY_SCOPEfrom thereadonlylevel. Including it again in thefulllevel's additional scopes doesn't cause a bug (deduplication viasethandles it), but it's unnecessary noise.Proposed cleanup
"docs": [ ("readonly", [DOCS_READONLY_SCOPE, DRIVE_READONLY_SCOPE]), - ("full", [DOCS_WRITE_SCOPE, DRIVE_READONLY_SCOPE, DRIVE_FILE_SCOPE]), + ("full", [DOCS_WRITE_SCOPE, DRIVE_FILE_SCOPE]), ],🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@auth/permissions.py` around lines 78 - 81, The "docs" permission mapping in auth/permissions.py redundantly lists DRIVE_READONLY_SCOPE in the "full" tuple alongside DRIVE_FILE_SCOPE; remove DRIVE_READONLY_SCOPE from the second tuple so "full" only includes the additional scopes (DOCS_WRITE_SCOPE and DRIVE_FILE_SCOPE), keeping the "readonly" tuple unchanged and relying on cumulative scope handling.core/tool_registry.py (1)
136-155: Consider extracting the duplicated scope-check logic into a shared helper.Steps 3 (read-only filtering, lines 138-155) and 4 (permissions filtering, lines 162-181) share the same pattern: unwrap
tool_obj.fn, read_required_google_scopes, checkall(scope in allowed for scope in required). A small helper like_get_tool_required_scopes(tool_obj)could reduce duplication.Also applies to: 162-181
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/tool_registry.py` around lines 136 - 155, Extract the duplicated scope-unwrapping and checking code into a small helper (e.g., _get_tool_required_scopes(tool_obj)) and reuse it from both the read-only branch and the permissions branch; specifically, move the logic that sets func_to_check = tool_obj / getattr(tool_obj, "fn"), reads required_scopes = getattr(func_to_check, "_required_google_scopes", []), and then use that helper in the read_only_mode block (where tools_to_remove is updated) and in the permissions_mode block to perform the all(scope in allowed_scopes/allowed for scope in required_scopes) check and logging. Ensure callers still reference tool_components, tools_to_remove, allowed_scopes (and the equivalent allowed variable in the permissions section) and keep the same logging message when disabling a tool.auth/google_auth.py (1)
485-489: SettingOAUTHLIB_RELAX_TOKEN_SCOPEglobally affects the process environment.This env var is set as a side effect in the callback handler and is never cleaned up. While it's guarded by an existence check, it permanently relaxes scope validation for the entire process, including any other OAuth flows or libraries sharing the same process. This is likely acceptable for this application, but worth noting.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@auth/google_auth.py` around lines 485 - 489, The code sets OAUTHLIB_RELAX_TOKEN_SCOPE globally which permanently alters process-wide behavior; instead, set it only for the duration of the OAuth token handling and restore the previous state afterwards: in the callback/token-exchange function where the current snippet appears, save the previous os.environ.get("OAUTHLIB_RELAX_TOKEN_SCOPE") value, set os.environ["OAUTHLIB_RELAX_TOKEN_SCOPE"] = "1" only while performing the token exchange (or call into oauthlib), then restore the saved value (delete the key if it was None) immediately after—this confines the relaxation to the local operation and avoids a permanent process-wide side effect.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@auth/google_auth.py`:
- Around line 524-532: The reconstructed Credentials in the partial grant path
omits id_token; update the Credentials(...) call inside the partial-grant
handling (the block that rebuilds credentials using scopes=list(granted)) to
include id_token=credentials.id_token so the newly created Credentials preserves
the original id_token; this ensures save_credentials_to_session() can decode the
id_token and correctly write to the session cache (which currently fails
silently when id_token is missing).
In `@main.py`:
- Around line 306-313: The tier-resolution error handler uses safe_print which
can be suppressed; replace the safe_print call in the except block that handles
resolve_tools_from_tier(args.tool_tier, tools_to_import) with an unconditional
stderr print so the message is always visible before exit (e.g., use print(...,
file=sys.stderr) or write to sys.stderr) while keeping the same message text and
leaving sys.exit(1), and ensure this change is applied in the block that calls
resolve_tools_from_tier and set_enabled_tool_names.
---
Nitpick comments:
In `@auth/google_auth.py`:
- Around line 485-489: The code sets OAUTHLIB_RELAX_TOKEN_SCOPE globally which
permanently alters process-wide behavior; instead, set it only for the duration
of the OAuth token handling and restore the previous state afterwards: in the
callback/token-exchange function where the current snippet appears, save the
previous os.environ.get("OAUTHLIB_RELAX_TOKEN_SCOPE") value, set
os.environ["OAUTHLIB_RELAX_TOKEN_SCOPE"] = "1" only while performing the token
exchange (or call into oauthlib), then restore the saved value (delete the key
if it was None) immediately after—this confines the relaxation to the local
operation and avoids a permanent process-wide side effect.
In `@auth/permissions.py`:
- Around line 78-81: The "docs" permission mapping in auth/permissions.py
redundantly lists DRIVE_READONLY_SCOPE in the "full" tuple alongside
DRIVE_FILE_SCOPE; remove DRIVE_READONLY_SCOPE from the second tuple so "full"
only includes the additional scopes (DOCS_WRITE_SCOPE and DRIVE_FILE_SCOPE),
keeping the "readonly" tuple unchanged and relying on cumulative scope handling.
In `@core/tool_registry.py`:
- Around line 136-155: Extract the duplicated scope-unwrapping and checking code
into a small helper (e.g., _get_tool_required_scopes(tool_obj)) and reuse it
from both the read-only branch and the permissions branch; specifically, move
the logic that sets func_to_check = tool_obj / getattr(tool_obj, "fn"), reads
required_scopes = getattr(func_to_check, "_required_google_scopes", []), and
then use that helper in the read_only_mode block (where tools_to_remove is
updated) and in the permissions_mode block to perform the all(scope in
allowed_scopes/allowed for scope in required_scopes) check and logging. Ensure
callers still reference tool_components, tools_to_remove, allowed_scopes (and
the equivalent allowed variable in the permissions section) and keep the same
logging message when disabling a tool.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
auth/google_auth.pyauth/permissions.pyauth/scopes.pycore/tool_registry.pymain.pypyproject.tomltests/gchat/test_chat_tools.py
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 8 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def parse_permissions_arg(permissions_list: List[str]) -> Dict[str, str]: | ||
| """ | ||
| Parse --permissions arguments like ["gmail:organize", "drive:full"]. | ||
|
|
||
| Returns dict mapping service -> level. | ||
| Raises ValueError on parse errors (unknown service, invalid level, bad format). | ||
| """ | ||
| result: Dict[str, str] = {} | ||
| for entry in permissions_list: | ||
| if ":" not in entry: | ||
| raise ValueError( | ||
| f"Invalid permission format: '{entry}'. " | ||
| f"Expected 'service:level' (e.g., 'gmail:organize', 'drive:readonly')" | ||
| ) | ||
| service, level = entry.split(":", 1) | ||
| if service in result: | ||
| raise ValueError(f"Duplicate service in permissions: '{service}'") | ||
| if service not in SERVICE_PERMISSION_LEVELS: | ||
| raise ValueError( | ||
| f"Unknown service: '{service}'. " | ||
| f"Valid services: {sorted(SERVICE_PERMISSION_LEVELS.keys())}" | ||
| ) | ||
| valid = get_valid_levels(service) | ||
| if level not in valid: | ||
| raise ValueError( | ||
| f"Unknown level '{level}' for service '{service}'. " | ||
| f"Valid levels: {valid}" | ||
| ) | ||
| result[service] = level |
There was a problem hiding this comment.
auth/permissions.py introduces core parsing/validation logic for --permissions (including error cases like bad format, unknown service, invalid level, duplicates) but there are no unit tests exercising these behaviors. Adding tests for parse_permissions_arg() and get_scopes_for_permission() would lock in the expected CLI contract and scope expansion rules.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
main.py (1)
306-311: Tier filtering should also narrow imported services in permissions mode.At Line 309, tier resolution returns both tool names and service names, but only tool names are applied. This still imports all permission-selected services, even when tier filtering narrows active tools.
Proposed refinement
- tier_tools, _ = resolve_tools_from_tier(args.tool_tier, tools_to_import) + tier_tools, tier_services = resolve_tools_from_tier( + args.tool_tier, tools_to_import + ) set_enabled_tool_names(set(tier_tools)) + tools_to_import = tier_services🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@main.py` around lines 306 - 311, The tier resolution step using resolve_tools_from_tier(args.tool_tier, tools_to_import) returns both tool names and service names, but only the tool names are applied via set_enabled_tool_names(tier_tools), so permission-selected services still import all services; modify the block after resolve_tools_from_tier to capture both outputs (e.g., tier_tools, tier_services = resolve_tools_from_tier(...)) and then apply filtering to imported/permission-selected services by intersecting tools_to_import (or the structure holding selected services) with tier_services so only services included by the tier remain imported, while still calling set_enabled_tool_names with the tier_tools.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@main.py`:
- Around line 306-311: The tier resolution step using
resolve_tools_from_tier(args.tool_tier, tools_to_import) returns both tool names
and service names, but only the tool names are applied via
set_enabled_tool_names(tier_tools), so permission-selected services still import
all services; modify the block after resolve_tools_from_tier to capture both
outputs (e.g., tier_tools, tier_services = resolve_tools_from_tier(...)) and
then apply filtering to imported/permission-selected services by intersecting
tools_to_import (or the structure holding selected services) with tier_services
so only services included by the tier remain imported, while still calling
set_enabled_tool_names with the tier_tools.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@main.py`:
- Around line 194-202: The startup currently ignores args.tools when
args.permissions is provided, causing ambiguous exposure; add explicit
validation in the same block that checks args.permissions (the one that already
checks args.read_only) to either reject the combination or implement
deterministic semantics: if rejecting, print a clear stderr error and
sys.exit(1) when args.permissions and args.tools are both set; if allowing,
compute the intersection by filtering args.tools against allowed entries derived
from args.permissions (e.g., only keep tools explicitly permitted by the parsed
permissions list) and document that behavior in the startup log—update the
branch that references args.permissions where tools are later used (the logic
around lines handling args.tools) to rely on this new validation/normalization.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| try: | ||
| tools_to_import, tier_tool_filter = resolve_permissions_mode_selection( | ||
| tools_to_import, args.tool_tier | ||
| ) |
There was a problem hiding this comment.
In --permissions mode combined with --tool-tier, permissions are set before tier resolution and never narrowed to the tier-selected services. As a result, OAuth scope generation will still request scopes for services that were filtered out (and the “Permission Levels” output will still list them), which undermines least-privilege and can confuse users. Consider applying tier filtering first (or reapplying set_permissions) so the stored permissions/scopes only include the final set of imported services when --tool-tier is used.
| ) | |
| ) | |
| # Narrow permissions/scopes to only the tier-selected services | |
| if perms is not None: | |
| perms = {service: perms[service] for service in tools_to_import if service in perms} | |
| set_permissions(perms) |
| # Granular permissions mode overrides both full and read-only scope maps. | ||
| # Lazy import with guard to avoid circular dependency during module init | ||
| # (SCOPES = get_scopes_for_tools() runs at import time before auth.permissions | ||
| # is fully loaded, but permissions mode is never active at that point). | ||
| try: | ||
| from auth.permissions import is_permissions_mode, get_all_permission_scopes | ||
|
|
||
| if is_permissions_mode(): | ||
| scopes = BASE_SCOPES.copy() | ||
| scopes.extend(get_all_permission_scopes()) | ||
| logger.debug( | ||
| "Generated scopes from granular permissions: %d unique scopes", | ||
| len(set(scopes)), | ||
| ) | ||
| return list(set(scopes)) | ||
| except ImportError: |
There was a problem hiding this comment.
New granular-permissions scope resolution path in get_scopes_for_tools() isn’t covered by the existing scopes tests. Please add unit tests that: (1) set granular permissions, (2) verify returned scopes are BASE_SCOPES + the expected cumulative permission scopes, and (3) verify it correctly overrides read-only/full maps.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/test_scopes.py (1)
206-212: Consider adding aclear_permissions()helper inauth.permissions.Directly assigning
permissions_module._PERMISSIONS = Nonecouples tests to internal state. A publicclear_permissions()function would be cleaner and more maintainable.Proposed helper in auth/permissions.py
def clear_permissions() -> None: """Reset granular permissions state (primarily for testing).""" global _PERMISSIONS _PERMISSIONS = NoneThen in tests:
- permissions_module._PERMISSIONS = None + permissions_module.clear_permissions()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_scopes.py` around lines 206 - 212, Add a public clear_permissions() helper to auth.permissions that resets the module-level _PERMISSIONS to None (e.g., def clear_permissions() -> None: global _PERMISSIONS; _PERMISSIONS = None) and update tests (setup_method/teardown_method in tests/test_scopes.py) to call permissions_module.clear_permissions() instead of directly assigning permissions_module._PERMISSIONS = None so tests avoid touching internal state.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/test_scopes.py`:
- Around line 206-212: Add a public clear_permissions() helper to
auth.permissions that resets the module-level _PERMISSIONS to None (e.g., def
clear_permissions() -> None: global _PERMISSIONS; _PERMISSIONS = None) and
update tests (setup_method/teardown_method in tests/test_scopes.py) to call
permissions_module.clear_permissions() instead of directly assigning
permissions_module._PERMISSIONS = None so tests avoid touching internal state.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| "Example: --permissions gmail:organize drive:readonly. " | ||
| "Gmail levels: readonly, organize, drafts, send, full (cumulative). " | ||
| "Other services: readonly, full. " | ||
| "Mutually exclusive with --read-only." |
There was a problem hiding this comment.
The --permissions CLI help text says it’s only mutually exclusive with --read-only, but the argument validation below also rejects combining --permissions with --tools. Update the help string (and/or argparse mutually-exclusive group) so the CLI docs match actual behavior and users get consistent guidance.
| "Mutually exclusive with --read-only." | |
| "Mutually exclusive with --read-only and --tools." |
| - Gmail levels: `readonly`, `organize`, `drafts`, `send`, `full` (cumulative) | ||
| - Other services currently support: `readonly`, `full` | ||
| - `--permissions` and `--read-only` are mutually exclusive | ||
| - With `--tool-tier`, only tier-matched tools are enabled and only services with matching tier tools are imported |
There was a problem hiding this comment.
README granular-permissions bullets don’t mention that --permissions cannot be combined with --tools, but main.py enforces that restriction. Please document this here (and ideally explain that service selection comes from --permissions, optionally with --tool-tier).
| - With `--tool-tier`, only tier-matched tools are enabled and only services with matching tier tools are imported | |
| - `--permissions` cannot be combined with `--tools`; enabled services are determined by the `--permissions` entries (optionally filtered by `--tool-tier`) | |
| - With `--tool-tier`, only tier-matched tools are enabled and only services that have tools in the selected tier are imported |
| def parse_permissions_arg(permissions_list: List[str]) -> Dict[str, str]: | ||
| """ | ||
| Parse --permissions arguments like ["gmail:organize", "drive:full"]. | ||
|
|
||
| Returns dict mapping service -> level. | ||
| Raises ValueError on parse errors (unknown service, invalid level, bad format). | ||
| """ | ||
| result: Dict[str, str] = {} | ||
| for entry in permissions_list: | ||
| if ":" not in entry: | ||
| raise ValueError( | ||
| f"Invalid permission format: '{entry}'. " | ||
| f"Expected 'service:level' (e.g., 'gmail:organize', 'drive:readonly')" | ||
| ) | ||
| service, level = entry.split(":", 1) | ||
| if service in result: | ||
| raise ValueError(f"Duplicate service in permissions: '{service}'") | ||
| if service not in SERVICE_PERMISSION_LEVELS: | ||
| raise ValueError( | ||
| f"Unknown service: '{service}'. " | ||
| f"Valid services: {sorted(SERVICE_PERMISSION_LEVELS.keys())}" | ||
| ) | ||
| valid = get_valid_levels(service) | ||
| if level not in valid: | ||
| raise ValueError( | ||
| f"Unknown level '{level}' for service '{service}'. " | ||
| f"Valid levels: {valid}" | ||
| ) | ||
| result[service] = level | ||
| return result |
There was a problem hiding this comment.
parse_permissions_arg() has multiple validation branches (bad format, duplicate service, unknown service, invalid level) but there are no tests covering these error paths. Add unit tests for invalid inputs so the CLI-facing validation behavior is locked in.
Partial scope grants, granular permissions, test fixes, oh my
Lots of folks have been asking for this so why not give the people what they want?
Closes #503
Closes #491
Summary by CodeRabbit