UAT: ToolRegistryService.register_tool and remove_tool use fragile duck-typing fallback that masks errors and has unreachable dead code #3718

New issue

Open

opened 2026-04-05 22:17:47 +00:00 by freemo · 0 comments

freemo commented

2026-04-05 22:17:47 +00:00

Owner

Metadata

Branch: fix/tool-registry-service-duck-typing
Commit Message: fix(tool): remove fragile duck-typing fallback in ToolRegistryService register_tool and remove_tool
Milestone: Backlog
Parent Epic: #397

Background and Context

ToolRegistryService.register_tool() and remove_tool() in src/cleveragents/application/services/tool_registry_service.py use fragile duck-typing fallback patterns that contain unreachable dead code and can mask errors.

Current Behavior

`register_tool` (lines ~50-65):

def register_tool(self, tool: Any) -> Any:
    create_fn = getattr(self._tool_repo, "create", None)
    if callable(create_fn):
        return create_fn(tool)
    add_fn = getattr(self._tool_repo, "add", None)
    if callable(add_fn):
        return add_fn(tool)
    return self._tool_repo.create(tool)  # ← UNREACHABLE DEAD CODE

The last line return self._tool_repo.create(tool) is unreachable:

If create exists and is callable → returns from first branch
If create doesn't exist but add does → returns from second branch
If neither exists → the last line would raise AttributeError (not create not found, but the getattr already returned None)

Wait — actually the last line IS reachable if create exists but is NOT callable (e.g., it's a property or attribute). In that case, callable(create_fn) is False, add is also not callable, and then self._tool_repo.create(tool) is called — which would raise TypeError: 'X' object is not callable.

`remove_tool` (lines ~85-100):

def remove_tool(self, name: str) -> bool:
    delete_fn = getattr(self._tool_repo, "delete", None)
    if callable(delete_fn):
        return bool(delete_fn(name))
    remove_fn = getattr(self._tool_repo, "remove", None)
    if callable(remove_fn):
        return bool(remove_fn(name))
    return self._tool_repo.delete(name)  # ← Same issue

Same pattern — the last line is effectively dead code or will raise a confusing error.

Problems

Fragile design: The service should have a well-defined contract with its repository. Using getattr to probe for method names is a code smell that suggests the repository interface is not well-defined.
Unreachable dead code: The fallback return self._tool_repo.create(tool) at the end is effectively unreachable in normal operation, making the code harder to reason about.
Error masking: If neither create nor add exists, the error message will be confusing (AttributeError: 'X' object has no attribute 'create' from the last line, rather than a clear "repository does not implement required interface" error).
Type safety: The Any type annotations on tool and return type prevent static type checking from catching interface mismatches.

Expected Behavior

ToolRegistryService should call self._tool_repo.create(tool) directly (or whatever the canonical method name is per the ToolRegistryRepository interface), without duck-typing fallbacks. The repository interface should be well-defined.

Code Locations

src/cleveragents/application/services/tool_registry_service.py lines ~50-65 (register_tool)
src/cleveragents/application/services/tool_registry_service.py lines ~85-100 (remove_tool)

Acceptance Criteria

register_tool calls the repository method directly without duck-typing fallback
remove_tool calls the repository method directly without duck-typing fallback
No unreachable dead code in either method
Error messages are clear when the repository doesn't implement the required interface
nox -e typecheck passes

Subtasks

Determine the canonical method name for the ToolRegistryRepository interface (create or add)
Update register_tool to call the canonical method directly
Update remove_tool to call the canonical method directly (delete or remove)
Remove the duck-typing fallback patterns
Verify nox -e typecheck passes

Definition of Done

All subtasks completed
A Git commit is created with the exact first line: fix(tool): remove fragile duck-typing fallback in ToolRegistryService register_tool and remove_tool
The commit is pushed to branch fix/tool-registry-service-duck-typing
A Pull Request has been submitted, reviewed, and merged
All nox stages pass
Coverage >= 97%

Backlog note: This issue was discovered during autonomous UAT testing.
It does not block milestone completion and has been placed in the backlog
for human review and future milestone assignment.

Automated by CleverAgents Bot
Supervisor: UAT Testing | Agent: ca-uat-tester

## Metadata - **Branch**: `fix/tool-registry-service-duck-typing` - **Commit Message**: `fix(tool): remove fragile duck-typing fallback in ToolRegistryService register_tool and remove_tool` - **Milestone**: Backlog - **Parent Epic**: #397 ## Background and Context `ToolRegistryService.register_tool()` and `remove_tool()` in `src/cleveragents/application/services/tool_registry_service.py` use fragile duck-typing fallback patterns that contain unreachable dead code and can mask errors. ## Current Behavior ### `register_tool` (lines ~50-65): ```python def register_tool(self, tool: Any) -> Any: create_fn = getattr(self._tool_repo, "create", None) if callable(create_fn): return create_fn(tool) add_fn = getattr(self._tool_repo, "add", None) if callable(add_fn): return add_fn(tool) return self._tool_repo.create(tool) # ← UNREACHABLE DEAD CODE ``` The last line `return self._tool_repo.create(tool)` is unreachable: - If `create` exists and is callable → returns from first branch - If `create` doesn't exist but `add` does → returns from second branch - If neither exists → the last line would raise `AttributeError` (not `create` not found, but the `getattr` already returned `None`) Wait — actually the last line IS reachable if `create` exists but is NOT callable (e.g., it's a property or attribute). In that case, `callable(create_fn)` is `False`, `add` is also not callable, and then `self._tool_repo.create(tool)` is called — which would raise `TypeError: 'X' object is not callable`. ### `remove_tool` (lines ~85-100): ```python def remove_tool(self, name: str) -> bool: delete_fn = getattr(self._tool_repo, "delete", None) if callable(delete_fn): return bool(delete_fn(name)) remove_fn = getattr(self._tool_repo, "remove", None) if callable(remove_fn): return bool(remove_fn(name)) return self._tool_repo.delete(name) # ← Same issue ``` Same pattern — the last line is effectively dead code or will raise a confusing error. ## Problems 1. **Fragile design**: The service should have a well-defined contract with its repository. Using `getattr` to probe for method names is a code smell that suggests the repository interface is not well-defined. 2. **Unreachable dead code**: The fallback `return self._tool_repo.create(tool)` at the end is effectively unreachable in normal operation, making the code harder to reason about. 3. **Error masking**: If neither `create` nor `add` exists, the error message will be confusing (`AttributeError: 'X' object has no attribute 'create'` from the last line, rather than a clear "repository does not implement required interface" error). 4. **Type safety**: The `Any` type annotations on `tool` and return type prevent static type checking from catching interface mismatches. ## Expected Behavior `ToolRegistryService` should call `self._tool_repo.create(tool)` directly (or whatever the canonical method name is per the `ToolRegistryRepository` interface), without duck-typing fallbacks. The repository interface should be well-defined. ## Code Locations - `src/cleveragents/application/services/tool_registry_service.py` lines ~50-65 (`register_tool`) - `src/cleveragents/application/services/tool_registry_service.py` lines ~85-100 (`remove_tool`) ## Acceptance Criteria - [ ] `register_tool` calls the repository method directly without duck-typing fallback - [ ] `remove_tool` calls the repository method directly without duck-typing fallback - [ ] No unreachable dead code in either method - [ ] Error messages are clear when the repository doesn't implement the required interface - [ ] `nox -e typecheck` passes ## Subtasks - [ ] Determine the canonical method name for the `ToolRegistryRepository` interface (`create` or `add`) - [ ] Update `register_tool` to call the canonical method directly - [ ] Update `remove_tool` to call the canonical method directly (`delete` or `remove`) - [ ] Remove the duck-typing fallback patterns - [ ] Verify `nox -e typecheck` passes ## Definition of Done - [ ] All subtasks completed - [ ] A Git commit is created with the exact first line: `fix(tool): remove fragile duck-typing fallback in ToolRegistryService register_tool and remove_tool` - [ ] The commit is pushed to branch `fix/tool-registry-service-duck-typing` - [ ] A Pull Request has been submitted, reviewed, and merged - [ ] All nox stages pass - [ ] Coverage >= 97% > **Backlog note:** This issue was discovered during autonomous UAT testing. > It does not block milestone completion and has been placed in the backlog > for human review and future milestone assignment. --- **Automated by CleverAgents Bot** Supervisor: UAT Testing | Agent: ca-uat-tester

freemo referenced this issue

2026-04-05 22:18:21 +00:00

[Automated] CleverAgents Build Session - 2026-04-05 (Resumed) #3654

freemo referenced this issue

2026-04-05 22:20:24 +00:00

[Automated] CleverAgents Build Session - 2026-04-05 (Resumed) #3654

freemo referenced this issue

2026-04-05 22:20:38 +00:00

[Automated] CleverAgents Build Session - 2026-04-05 (Resumed) #3654

freemo referenced this issue

2026-04-05 22:22:14 +00:00

[Automated] CleverAgents Build Session - 2026-04-05 (Resumed) #3654