Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review to trigger a review.
Tip: disable this comment in your organization's Code Review settings.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #4782 +/- ##
==========================================
+ Coverage 44.32% 45.40% +1.07%
==========================================
Files 813 829 +16
Lines 32736 33403 +667
Branches 5721 5721
==========================================
+ Hits 14511 15166 +655
- Misses 16224 16236 +12
Partials 2001 2001
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| def raise_invalid_input(error: _graphql_client.InvalidInputSelection): | ||
| first_error = error.errors[0] if error.errors else None | ||
| if first_error is None: | ||
| raise exceptions.InvalidInputError(error) | ||
| if first_error.name == "PolicyDoesNotExist": | ||
| raise exceptions.PolicyNotFoundError() | ||
| if first_error.name == "PolicyTitleConflict": | ||
| raise exceptions.PolicyTitleExistsError(error) | ||
| if first_error.name == "PolicyArnConflict": | ||
| raise exceptions.PolicyArnExistsError(error) | ||
| if first_error.name == "RoleHasTooManyPoliciesToAttach": | ||
| raise exceptions.RoleTooManyPoliciesError(error) | ||
| raise exceptions.InvalidInputError(error) | ||
|
|
||
|
|
||
| def raise_operation_error(error: _graphql_client.OperationErrorSelection): | ||
| raise exceptions.OperationError(error) |
There was a problem hiding this comment.
NoReturn type annotations
raise_invalid_input and raise_operation_error always raise exceptions but are not annotated with -> NoReturn. Without this annotation, type checkers assume these helpers can return normally, which leads to false positives downstream: in _handle_policy_result (and policies.delete), the branches that call them appear to fall through to the next if or the final raise AssertionError, making the function's -> types.Policy return type unsatisfiable from a static-analysis perspective.
| def raise_invalid_input(error: _graphql_client.InvalidInputSelection): | |
| first_error = error.errors[0] if error.errors else None | |
| if first_error is None: | |
| raise exceptions.InvalidInputError(error) | |
| if first_error.name == "PolicyDoesNotExist": | |
| raise exceptions.PolicyNotFoundError() | |
| if first_error.name == "PolicyTitleConflict": | |
| raise exceptions.PolicyTitleExistsError(error) | |
| if first_error.name == "PolicyArnConflict": | |
| raise exceptions.PolicyArnExistsError(error) | |
| if first_error.name == "RoleHasTooManyPoliciesToAttach": | |
| raise exceptions.RoleTooManyPoliciesError(error) | |
| raise exceptions.InvalidInputError(error) | |
| def raise_operation_error(error: _graphql_client.OperationErrorSelection): | |
| raise exceptions.OperationError(error) | |
| from typing import NoReturn | |
| def raise_invalid_input(error: _graphql_client.InvalidInputSelection) -> NoReturn: | |
| first_error = error.errors[0] if error.errors else None | |
| if first_error is None: | |
| raise exceptions.InvalidInputError(error) | |
| if first_error.name == "PolicyDoesNotExist": | |
| raise exceptions.PolicyNotFoundError() | |
| if first_error.name == "PolicyTitleConflict": | |
| raise exceptions.PolicyTitleExistsError(error) | |
| if first_error.name == "PolicyArnConflict": | |
| raise exceptions.PolicyArnExistsError(error) | |
| if first_error.name == "RoleHasTooManyPoliciesToAttach": | |
| raise exceptions.RoleTooManyPoliciesError(error) | |
| raise exceptions.InvalidInputError(error) | |
| def raise_operation_error(error: _graphql_client.OperationErrorSelection) -> NoReturn: | |
| raise exceptions.OperationError(error) |
The same annotation pattern should be applied in raise_operation_error at line 38–39.
| def delete(id: str) -> None: | ||
| """ | ||
| Delete a role from the registry. | ||
|
|
||
| Args: | ||
| id: Role ID. | ||
| """ | ||
| result = util.get_client().role_delete(id=id) | ||
| typename = result.typename__ | ||
| if typename == "RoleDeleteSuccess": | ||
| return None | ||
| if typename == "RoleDoesNotExist": | ||
| raise exceptions.RoleNotFoundError() | ||
| if typename == "RoleNameReserved": | ||
| raise exceptions.RoleNameReservedError(result) | ||
| raise exceptions.Quilt3AdminError(result) |
There was a problem hiding this comment.
RoleAssigned and RoleNameUsedBySsoConfig delete responses
The generated role_delete client method can return five distinct union members: RoleDeleteSuccess, RoleDoesNotExist, RoleNameReserved, RoleNameUsedBySsoConfig, and RoleAssigned. The current delete() only handles the first three explicitly; the last two fall through to the generic raise exceptions.Quilt3AdminError(result).
This isn't broken at runtime, but callers cannot distinguish "role is still assigned to users" from any other unexpected error, and the test suite's ROLE_DELETE_ERRORS fixture doesn't exercise these paths. Consider either:
- Adding dedicated exception classes (
RoleAssignedError,RoleSsoConfigConflictError) and handling them here, or - Documenting in the docstring that these produce a generic
Quilt3AdminError.
| ...PermissionSelection | ||
| } | ||
| roles { | ||
| ...ManagedRoleSelection | ||
| } |
There was a problem hiding this comment.
defaultRoleGet query is generated but not exposed via the admin API
The defaultRoleGet query is added to the GraphQL file and the corresponding client method default_role_get() is generated in client.py, but no wrapper function (e.g. roles.get_default()) is implemented in api/python/quilt3/admin/roles.py, and no tests exercise it.
If this query is intentionally left for future use, consider either adding a comment in the GraphQL file, or omitting it until the public API is ready to avoid dead generated code.
|
superseded by #4805 |
2 participants
Summary
Verification
Greptile Summary
This PR expands the
quilt3.adminPython module with full CRUD operations for roles (managed and unmanaged) and a new policies module, plusset_defaultfor roles. New GraphQL queries and mutations are added toqueries.graphql, theariadne-codegen-generated client is regenerated accordingly, thin Python wrappers are added inadmin/roles.pyandadmin/policies.py, and the test suite is extended with parametrized happy-path and error-path coverage.Key observations:
raise_invalid_inputandraise_operation_errorinutil.pyalways raise but are missing-> NoReturnannotations, which causes type-checker noise in the callers (_handle_policy_result,policies.delete).roles.delete()silently falls through to genericQuilt3AdminErrorforRoleAssignedandRoleNameUsedBySsoConfigresponses, which the generated client explicitly models but the test suite does not exercise.defaultRoleGetGraphQL query and generated client method are added in this PR but no corresponding public admin API function (roles.get_default()) is implemented or tested — the generated code is effectively dead.Confidence Score: 4/5
Important Files Changed
Sequence Diagram
sequenceDiagram participant Caller as Caller participant AdminAPI as admin.roles / admin.policies participant Util as admin.util participant Client as _graphql_client.Client participant GQL as GraphQL API Caller->>AdminAPI: roles.create_managed(name, policies) AdminAPI->>Client: role_create_managed(ManagedRoleInput) Client->>GQL: mutation roleCreateManaged GQL-->>Client: RoleCreateSuccess | RoleNameReserved | ... Client-->>AdminAPI: typed Union result AdminAPI->>AdminAPI: _handle_role_mutation_result(result) AdminAPI->>Util: parse_role_result(result.role) Util-->>AdminAPI: ManagedRole | UnmanagedRole AdminAPI-->>Caller: types.ManagedRole Caller->>AdminAPI: policies.create_managed(title, permissions, roles) AdminAPI->>Client: policy_create_managed(ManagedPolicyInput) Client->>GQL: mutation policyCreateManaged GQL-->>Client: Policy | InvalidInput | OperationError Client-->>AdminAPI: typed Union result AdminAPI->>AdminAPI: _handle_policy_result(result) alt typename == "Policy" AdminAPI->>Util: parse_policy_result(result) Util-->>AdminAPI: types.Policy AdminAPI-->>Caller: types.Policy else typename == "InvalidInput" AdminAPI->>Util: raise_invalid_input(result) Util-->>Caller: raises PolicyNotFoundError / PolicyTitleExistsError / ... else typename == "OperationError" AdminAPI->>Util: raise_operation_error(result) Util-->>Caller: raises OperationError endReviews (1): Last reviewed commit: "Document admin role and policy APIs" | Re-trigger Greptile