Skip to content

Commit 64b2853

Browse files
drernieclaude
andcommitted
docs: Address PR comment about admin naming convention fragility
Added comprehensive documentation for admin tool/resource naming conventions to address PR reviewer's concern about relying on naming patterns. Changes: - **Added Code Conventions section to CLAUDE.md**: Documents admin_* prefix convention for tools and admin://* URI scheme for resources - **Added inline comments**: Explains naming convention in utils.py and resources/__init__.py with notes about future decorator approach - **Documented enforcement**: Clarifies that code review enforces conventions and has_admin_credentials() performs actual credential verification Rationale: - Naming convention is reasonable trade-off vs decorator complexity - Convention is consistently followed throughout codebase - Documented for maintainability and code review enforcement - Suggests future decorator-based approach if convention becomes insufficient Addresses PR comment: "The filtering logic assumes admin tools are prefixed with 'admin_'. This could be made more robust by using a more explicit mechanism (decorator, metadata, or registry)." 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
1 parent 5f46b4e commit 64b2853

File tree

3 files changed

+25
-0
lines changed

3 files changed

+25
-0
lines changed

AGENTS.md

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -274,6 +274,25 @@ Key execution guidelines:
274274
- Suggest improvements that align with these principles
275275
- When unsure, ask for clarification rather than assuming
276276

277+
## Code Conventions
278+
279+
### Admin Tool and Resource Naming
280+
281+
**Convention**: Admin-only functionality follows consistent naming patterns for filtering based on user credentials.
282+
283+
- **Tools**: Admin tools use `admin_*` function name prefix (e.g., `admin_user_create`, `admin_role_list`)
284+
- **Resources**: Admin resources use `admin://*` URI scheme (e.g., `admin://users`, `admin://roles`)
285+
286+
**Rationale**: These conventions enable credential-based filtering at registration time without requiring decorators or metadata systems. Non-admin users see a clean interface without admin clutter.
287+
288+
**Enforcement**:
289+
- Code review ensures all admin functions follow naming convention
290+
- `has_admin_credentials()` in `QuiltService` performs actual credential verification (not just import checks)
291+
- Tools filtered in `utils.register_tools_from_module()` based on name prefix
292+
- Resources filtered in `resources.create_default_registry()` at registration time
293+
294+
**Future Consideration**: If naming convention becomes insufficient, consider decorator-based approach (`@admin_required`) or resource-level metadata for more robustness.
295+
277296
## Summary
278297

279298
The key is to write clean, testable, functional code that evolves through small, safe increments. Every change should be driven by a test that describes the desired behavior, and the implementation should be the simplest thing that makes that test pass. When in doubt, favor simplicity and readability over cleverness.

src/quilt_mcp/resources/__init__.py

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -60,6 +60,9 @@ def create_default_registry() -> ResourceRegistry:
6060
has_admin = service.has_admin_credentials()
6161

6262
# Register admin resources only if user has credentials
63+
# NOTE: Admin resources follow URI convention of 'admin://*' scheme.
64+
# This is enforced by code review and documented in CLAUDE.md.
65+
# Future: Consider resource-level metadata (@requires_admin) for more flexibility.
6366
if has_admin:
6467
registry.register("admin://users", AdminUsersResource())
6568
registry.register("admin://roles", AdminRolesResource())

src/quilt_mcp/utils.py

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -155,6 +155,9 @@ def make_predicate(mod: Any) -> Callable[[Any], bool]:
155155

156156
for name, func in functions:
157157
# Skip admin tools if user lacks admin credentials
158+
# NOTE: Admin tools follow naming convention of 'admin_*' prefix.
159+
# This is enforced by code review and documented in CLAUDE.md.
160+
# Future: Consider decorator-based approach (@admin_required) for more robustness.
158161
if name.startswith('admin_') and not has_admin:
159162
if verbose:
160163
print(f"Skipped admin tool: {module.__name__}.{name} (no admin credentials)", file=sys.stderr)

0 commit comments

Comments
 (0)