-
Notifications
You must be signed in to change notification settings - Fork 0
Description
Context
PR #5 introduced composite (tenant_id, name) keyed storage across ToolManager, ResourceManager, and PromptManager. The implementation is solid and backward compatible, but a few improvements were identified during review.
Follow-up items
-
add_templatedoesn't warn on duplicates — Unlikeadd_resource,add_tool, andadd_prompt,ResourceManager.add_template()silently overwrites an existing template for the same(tenant_id, uri_template)key. It should follow the same pattern: return the existing entry and optionally warn. -
list_*methods are O(n) scans — Everylist_tools/list_resources/list_promptscall iterates the entire dict filtering bytenant_id. Consider a nested dict structure (dict[str | None, dict[str, T]]) for O(1) lookups if tenant count grows. Low priority — fine for now. -
Existing test directly sets internal state —
test_resource_manager.py::TestResourceManager::test_get_resource_from_templatemanually assigns tomanager._templates[(None, template.uri_template)]instead of usingmanager.add_template(). Should be refactored to use the public API. -
No
remove_resource/remove_promptmethods —ToolManagerhasremove_toolwith tenant support, but the other two managers lack remove methods. Will likely be needed for tenant lifecycle management (e.g., tenant teardown). -
Thread safety of plain
dictstorage — The storage dicts are not thread-safe. If multiple async tasks register entries for different tenants concurrently, there could be race conditions. Pre-existing concern, but multi-tenancy increases the likelihood of concurrent access. -
Tests construct
Context()with no args —test_multi_tenancy_managers.pycreates bareContext()instances. IfContext.__init__signature changes in later iterations, these tests will break. Consider a test fixture or factory for constructing test contexts.