Release 070

AGENTS.md

@@ -274,6 +274,25 @@ Key execution guidelines:
 - Suggest improvements that align with these principles
 - When unsure, ask for clarification rather than assuming
 
+## Code Conventions
+
+### Admin Tool and Resource Naming
+
+**Convention**: Admin-only functionality follows consistent naming patterns for filtering based on user credentials.
+
+- **Tools**: Admin tools use `admin_*` function name prefix (e.g., `admin_user_create`, `admin_role_list`)
+- **Resources**: Admin resources use `admin://*` URI scheme (e.g., `admin://users`, `admin://roles`)
+
+**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.
+
+**Enforcement**:
+- Code review ensures all admin functions follow naming convention
+- `has_admin_credentials()` in `QuiltService` performs actual credential verification (not just import checks)
+- Tools filtered in `utils.register_tools_from_module()` based on name prefix
+- Resources filtered in `resources.create_default_registry()` at registration time
+
+**Future Consideration**: If naming convention becomes insufficient, consider decorator-based approach (`@admin_required`) or resource-level metadata for more robustness.
+
 ## Summary
 
 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.
@@ -381,7 +400,7 @@ The following permissions are granted for this repository:
 - **Use sub-agents** from `.claude/agents/` for complex workflow phases to prevent context loss
 - **NEVER update historical specs** - Spec files in `./spec/` are historical documentation of what was done at the time. Only update current operational files like README.md, Makefiles, and source code when fixing references
 
-**Release System
+**Release System:**
 
 - ✅ `make mcpb` - Creates MCPB package (.mcpb file)
 - ✅ `make mcpb-validate` - Validates MCPB package integrity
@@ -466,9 +485,153 @@ Required secrets for Docker operations:
 - `VERSION` for overriding image version tags
 - `DOCKER_IMAGE_NAME` for custom image naming (defaults to `quilt-mcp-server`)
 
+### 2025-09-21 Search Function Consolidation
+- **Legacy function removal**: Successfully consolidated 4 search functions down to 1 canonical `catalog_search` interface by removing 3 legacy functions (`packages_search`, `bucket_objects_search`, `bucket_objects_search_graphql`)
+- **TDD approach for removal**: Use failing tests to validate removal - create tests that expect ImportError after function removal, then remove functions to make tests pass
+- **Internal API consolidation**: Legacy functions were internal-only (not in main `__init__.py` exports), making removal safe with no external API impact
+- **Function replacement patterns**: Direct shims (like `packages_search`) can be removed immediately; wrapper functions need replacement with equivalent `catalog_search` calls using appropriate scope/backend parameters
+- **Import cleanup**: Remove unused imports after function removal (e.g., `QuiltService` no longer needed in `buckets.py` after removing search functions)
+- **Error message updates**: Update any error messages or suggestions to reference the consolidated function instead of removed ones
+- **Achievement**: 75% reduction in search API surface area while preserving 100% of functionality with enhanced capabilities
+
+### 2025-09-30 Migration of _list Functions to MCP Resources
+- **Legacy `_list` functions removed from public API exports**: `list_available_resources`, `list_metadata_templates`, `workflow_list`, `package_tools_list`, `tabulator_tables_list` are no longer exported from `quilt_mcp.__init__`
+- **Functions remain as implementation details**: All 5 functions still exist in their original locations and are called internally by their corresponding MCP resources
+- **Resource pattern for tests**: Tests should now use `from quilt_mcp.resources import SomeResource` and call `await SomeResource().list_items()` instead of importing/calling `_list` functions directly
+- **Resource mappings**:
+  - `list_available_resources()` → `S3BucketsResource().list_items()` (s3://buckets)
+  - `list_metadata_templates()` → `MetadataTemplatesResource().list_items()` (metadata://templates)
+  - `workflow_list()` → `WorkflowResource().list_items()` (workflow://workflows)
+  - `package_tools_list()` → `PackageToolsResource().list_items()` (package://tools)
+  - `tabulator_tables_list(bucket)` → `TabulatorTablesResource().list_items(bucket_name=bucket)` (tabulator://{bucket}/tables)
+- **Previously removed**: `admin_users_list`, `admin_roles_list`, `athena_databases_list`, `athena_workgroups_list` were fully deleted and replaced by `AdminUsersResource`, `AdminRolesResource`, `AthenaDatabasesResource`, `AthenaWorkgroupsResource`
+- **Import pattern**: `from quilt_mcp.resources.workflow import WorkflowResource` then `listing = asyncio.run(WorkflowResource().list_items())`
+- **Breaking change**: Removing from `__all__` is a breaking change for external code importing these functions from `quilt_mcp` package
+- **No functionality loss**: All functionality preserved via MCP resources which provide richer metadata and consistent interface
+
+### 2025-10-01 QuiltService Refactoring - Getter Methods to Operations (Issue #155)
+
+**Summary**: Successfully refactored QuiltService from exposing raw quilt3 modules via getter methods to providing operational abstractions with typed returns. This enables backend swapping and proper separation of concerns.
+
+**Key Changes**:
+- **27 new operational methods added**: Users, Roles, SSO, Tabulator administration
+- **5 getter methods deleted**: `get_users_admin()`, `get_roles_admin()`, `get_sso_config_admin()`, `get_tabulator_admin()`, `get_search_api()`
+- **35+ call sites migrated**: All tools updated to use new operational API
+- **Return types fixed**: Eliminated `Any` returns in favor of proper types (`dict[str, Any]`, `list[dict[str, Any]]`, etc.)
+
+**New QuiltService Methods**:
+
+*User Management (10 methods)*:
+- `list_users()` → `list[dict[str, Any]]`
+- `get_user(name)` → `dict[str, Any]`
+- `create_user(name, email, role, extra_roles)` → `dict[str, Any]`
+- `delete_user(name)` → `None`
+- `set_user_email(name, email)` → `dict[str, Any]`
+- `set_user_role(name, role, extra_roles, append)` → `dict[str, Any]`
+- `set_user_active(name, active)` → `dict[str, Any]`
+- `set_user_admin(name, admin)` → `dict[str, Any]`
+- `add_user_roles(name, roles)` → `dict[str, Any]`
+- `remove_user_roles(name, roles, fallback)` → `dict[str, Any]`
+- `reset_user_password(name)` → `dict[str, Any]`
+
+*Role Management (4 methods)*:
+- `list_roles()` → `list[dict[str, Any]]`
+- `get_role(name)` → `dict[str, Any]`
+- `create_role(name, permissions)` → `dict[str, Any]`
+- `delete_role(name)` → `None`
+
+*SSO Configuration (3 methods)*:
+- `get_sso_config()` → `str | None`
+- `set_sso_config(config)` → `dict[str, Any]`
+- `remove_sso_config()` → `dict[str, Any]`
+
+*Tabulator Administration (6 methods)*:
+- `get_tabulator_access()` → `bool`
+- `set_tabulator_access(enabled)` → `dict[str, Any]`
+- `list_tabulator_tables(bucket)` → `list[dict[str, Any]]`
+- `create_tabulator_table(bucket, name, config)` → `dict[str, Any]`
+- `delete_tabulator_table(bucket, name)` → `None`
+- `rename_tabulator_table(bucket, old, new)` → `dict[str, Any]`
+
+*Config & Package (2 methods)*:
+- `get_navigator_url()` → `str | None`
+- `delete_package(name, registry)` → `None`
+
+**Migration Patterns** (for future large-scale refactorings):
+
+1. **Phase-based execution**: Break refactoring into discrete phases (Foundation → Implementation → Migration → Cleanup)
+2. **Test first, always**: Add operational methods with tests BEFORE migrating call sites
+3. **Incremental migration**: Update call sites one file at a time, test after each
+4. **Delete in separate phase**: Keep old methods during transition, delete only after all migrations complete
+5. **Private helpers remain**: `_get_*_admin_module()` helpers stay private, only public getters deleted
+6. **Fix return types early**: Update return types from `Any` to proper types before deletion phase
+7. **Validate thoroughly**: Run full test suite after each phase, maintain 100% coverage
+
+**Exception Handling Pattern**:
+- Custom exception hierarchy in `services/exceptions.py`:
+  - `QuiltServiceError` (base)
+  - `AdminNotAvailableError` (when admin features unavailable)
+  - `UserNotFoundError`, `UserAlreadyExistsError` (user operations)
+  - `RoleNotFoundError`, `RoleAlreadyExistsError` (role operations)
+  - `PackageNotFoundError`, `BucketNotFoundError` (package/bucket operations)
+- Centralized error handling using `_require_admin()` helper
+- Consistent exception raising across all operational methods
+
+**Testing Strategy**:
+- Behavioral tests via public API only (no implementation details tested)
+- Separate test files by domain: `test_quilt_service_users.py`, `test_quilt_service_roles.py`, etc.
+- Mock external dependencies (admin modules) for unit tests
+- Test both success and error paths for every operation
+- Verify exception types and messages
+
+**Files Modified**:
+- `src/quilt_mcp/services/quilt_service.py` - Core service implementation
+- `src/quilt_mcp/tools/governance.py` - Admin operations (~25 functions updated)
+- `src/quilt_mcp/tools/tabulator.py` - Tabulator operations (~8 functions updated)
+- `src/quilt_mcp/resources/admin.py` - Admin resources (2 resources updated)
+- `src/quilt_mcp/tools/catalog.py` - Config operations (~1 function updated)
+- `src/quilt_mcp/tools/package_creation.py` - Package deletion (1 function updated)
+
+**Success Metrics**:
+- ✅ 100% test coverage maintained throughout (7 phases, 35+ commits)
+- ✅ All 35+ call sites successfully migrated
+- ✅ Zero behavioral changes (functional equivalence verified)
+- ✅ No `Any` return types in public API (except legacy methods scheduled for deletion)
+- ✅ Backend swapping architecturally possible (no raw module exposure)
+- ✅ Complete audit trail (each phase independently committable and revertable)
+
+**Key Learnings**:
+1. **Prefactoring pays off**: Adding comprehensive tests BEFORE refactoring enables confident changes
+2. **Phase boundaries are critical**: Each phase must be independently testable and committable
+3. **Don't rush deletion**: Keep old methods during migration to enable gradual rollout
+4. **Private helpers vs public getters**: Private `_get_*` helpers can stay for implementation, only public getters are anti-pattern
+5. **Type safety first**: Fix return types early - don't leave `Any` returns until cleanup phase
+6. **Test organization matters**: Domain-based test files (`test_*_users.py`) scale better than monolithic test files
+7. **Commit granularity**: Small, focused commits (RED/GREEN/REFACTOR) enable easy debugging and rollback
+
+**Anti-patterns Eliminated**:
+- ❌ Exposing raw module references via `get_*_admin()` methods
+- ❌ Returning `Any` types that hide implementation details
+- ❌ Forcing callers to check module availability and handle None returns
+- ❌ Leaking quilt3-specific types into tool layer
+- ❌ Inconsistent error handling across operations
+
+**Architecture Improvements**:
+- ✅ Clean separation of concerns (service layer owns quilt3 interaction)
+- ✅ Backend-agnostic interface (can swap quilt3 for different backend)
+- ✅ Consistent operational API across all domains
+- ✅ Centralized error handling with custom exceptions
+- ✅ Type-safe returns enable better IDE support and static analysis
+
+**For Future Reference**:
+- Specification document: `spec/Archive/155-isolate-quilt3/a3-specifications.md`
+- Orchestration plan: `spec/Archive/155-isolate-quilt3/a4-orchestration.md`
+- Original analysis: `spec/Archive/155-isolate-quilt3/a2-analysis.md`
+- Requirements: `spec/Archive/155-isolate-quilt3/a1-requirements.md`
+
 ## important-instruction-reminders
 
 Do what has been asked; nothing more, nothing less.
 NEVER create files unless they're absolutely necessary for achieving your goal.
 ALWAYS prefer editing an existing file to creating a new one.
-NEVER proactively create documentation files (*.md) or README files. Only create documentation files if explicitly requested by the User.
+NEVER proactively create documentation files (*.md) or README files. Only create documentation files if explicitly requested by the User.