feat: add tasks:manage permission level - #540
Merged
taylorwilsdon merged 8 commits intoMar 5, 2026
Merged
Conversation
…ng other writes The consolidated manage_task tool bundles create/update/delete/move into a single tool, making it impossible to deny just the delete action via tool tiers or scope-based filtering. This adds: - A `manage` permission level for tasks (between readonly and full) - A SERVICE_DENIED_ACTIONS registry mapping (service, level) to denied actions - An is_action_denied() helper that tools call before executing actions - Guards in manage_task and manage_task_list that reject denied actions Usage: --permissions tasks:manage Allows create, update, move. Denies delete. tasks:full remains unchanged (all actions allowed). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds Tasks granular permission levels ("readonly", "manage", "full"), a per-service denied-actions mapping and is_action_denied API, enforces denied task actions in gtasks entry points (raises UserInputError), and updates README and tests to document/verify behavior. Changes
Sequence Diagram(s)sequenceDiagram
participant User as "User / Client"
participant TasksAPI as "gtasks.tasks_tools"
participant Perms as "auth.permissions"
participant Store as "TaskStore"
User->>TasksAPI: request(action: create/update/move/delete/clear_completed)
TasksAPI->>Perms: is_action_denied("tasks", action)?
alt denied
Perms-->>TasksAPI: true
TasksAPI-->>User: raise UserInputError (action denied)
else allowed
Perms-->>TasksAPI: false
TasksAPI->>Store: perform action
Store-->>TasksAPI: result
TasksAPI-->>User: success/response
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
![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: 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 `@auth/permissions.py`:
- Around line 139-143: SERVICE_DENIED_ACTIONS currently denies only "delete" for
the "tasks" -> "manage" scope, leaving the destructive "clear_completed" action
allowed; update the SERVICE_DENIED_ACTIONS entry for the "tasks" key (under the
"manage" subkey) to include "clear_completed" in the frozenset alongside
"delete" so that both destructive actions are denied by the permission
enforcement logic.
ℹ️ Review info
Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b0834a6a-ab7f-437b-9c0d-a73d539ed71d
📒 Files selected for processing (4)
README.mdauth/permissions.pygtasks/tasks_tools.pytests/test_permissions.py
Addresses CodeRabbit review — clear_completed is destructive and should be blocked alongside delete at the manage permission level. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
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 `@tests/test_permissions.py`:
- Around line 139-180: Tests in TestIsActionDenied leak module-global permission
state because set_permissions() mutates shared state; add a per-test reset so
each test starts with a clean state by adding a teardown_method (or
setup_method) on the TestIsActionDenied class that calls set_permissions(None)
(or call set_permissions(None) at start of each test) to ensure set_permissions
is cleared between tests; locate TestIsActionDenied and add the
teardown_method(self, method) that invokes set_permissions(None) to avoid
order-dependent failures.
ℹ️ Review info
Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c053f92c-3863-4f73-84b5-1e5bca68d8ea
📒 Files selected for processing (2)
auth/permissions.pytests/test_permissions.py
- Add docstrings to test methods for coverage threshold - Add autouse fixture to reset permission state between tests Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/test_permissions.py (1)
131-135: Expandtasks:fullassertion to verify full cumulative inheritance.This test currently checks write scope only; asserting readonly scope too will better guard cumulative behavior regressions.
Suggested test tweak
def test_tasks_full_includes_write_scope(self): """Full level should include write scope from manage.""" scopes = get_scopes_for_permission("tasks", "full") assert TASKS_SCOPE in scopes + assert TASKS_READONLY_SCOPE in scopes🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_permissions.py` around lines 131 - 135, The test test_tasks_full_includes_write_scope should also assert that the readonly permission is included to validate cumulative inheritance; after calling get_scopes_for_permission("tasks", "full") and asserting TASKS_SCOPE is in scopes, add an assertion that TASKS_READONLY_SCOPE (or the constant representing the tasks readonly scope used elsewhere) is also in scopes so the test verifies both write and readonly scopes are present for "tasks:full".
🤖 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_permissions.py`:
- Around line 131-135: The test test_tasks_full_includes_write_scope should also
assert that the readonly permission is included to validate cumulative
inheritance; after calling get_scopes_for_permission("tasks", "full") and
asserting TASKS_SCOPE is in scopes, add an assertion that TASKS_READONLY_SCOPE
(or the constant representing the tasks readonly scope used elsewhere) is also
in scopes so the test verifies both write and readonly scopes are present for
"tasks:full".
Addresses CodeRabbit Review 3 nitpick: verify TASKS_READONLY_SCOPE is present at full level, confirming cumulative scope expansion. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/test_permissions.py (1)
137-140: Consider moving this test toTestParsePermissionsArg.This test validates
parse_permissions_arg()behavior but is placed inTestGetScopesForPermission. Moving it to the appropriate class would improve test organization and discoverability.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_permissions.py` around lines 137 - 140, The test method test_tasks_manage_is_valid_level belongs with other parse_permissions_arg tests: move the method from the TestGetScopesForPermission test case into the TestParsePermissionsArg test class and keep its body unchanged (calling parse_permissions_arg(["tasks:manage"]) and asserting {"tasks": "manage"}) so it lives next to other parse_permissions_arg-related tests for better organization.
🤖 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_permissions.py`:
- Around line 137-140: The test method test_tasks_manage_is_valid_level belongs
with other parse_permissions_arg tests: move the method from the
TestGetScopesForPermission test case into the TestParsePermissionsArg test class
and keep its body unchanged (calling parse_permissions_arg(["tasks:manage"]) and
asserting {"tasks": "manage"}) so it lives next to other
parse_permissions_arg-related tests for better organization.
…nsArg Addresses CodeRabbit Review 4 nitpick: the test validates parse_permissions_arg() so it belongs with that test class. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
✅ Actions performedReviews resumed. |
…into feat/tasks-manage-permission
Owner
|
Appreciate the diligent PR feedback updates! Will get this merged today. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
managepermission level for the tasks service (betweenreadonlyandfull) that grants write access but denies destructive actions (deleteandclear_completed)SERVICE_DENIED_ACTIONSregistry andis_action_denied()helper so tools can enforce action-level restrictions based on the configured permission levelmanage_taskandmanage_task_listagainst denied actionsMotivation
The consolidation of
create_task,update_task,delete_task, andmove_taskinto a singlemanage_tasktool means that destructive actions can no longer be excluded via tool tiers or scope-based filtering — all actions share the same OAuth scope. This adds a permission level that lets operators allow task writes while blocking destructive operations.Usage
Design
tasks:fullbehaviour is unchanged;manageis opt-inSERVICE_DENIED_ACTIONScan be extended for other services/levels without changing the helperTest plan
tests/test_permissions.py)--permissions tasks:manageblocks delete and clear_completed via MCP client--permissions tasks:fullstill allows all actions🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests
Documentation