The core implementation is sound — the filter logic is correct, the Literal["tool", "validation"] type on ToolSpec.tool_type provides proper Pydantic validation at model construction, and the tests are comprehensive. However, there are two issues that must be addressed before merge.

Required Changes

1. [INPUT-VALIDATION] Stale docstring on `list_tools()` — actively misleading

Location: src/cleveragents/tool/registry.py, lines 78–80 (the tool_type parameter docstring)
Issue: The docstring still reads:
```
tool_type:
    Reserved for future use (e.g. filtering by ``tool`` vs
    ``validation``).  Currently unused.
```
But this PR makes the parameter actively used. The PR description even states "Updated docstring to reflect the parameter is now active" — however, the actual code was not updated. This is a correctness issue: the docstring actively misleads developers into thinking the parameter is ignored when it is now the core behavior of this fix.

Required: Update the docstring to accurately describe the parameter's behavior, e.g.:

tool_type:
    If provided, only return tools whose ``tool_type`` matches
    (``"tool"`` or ``"validation"``). When ``None``, all tools
    are returned regardless of type.

2. [CONTRIBUTING] Missing milestone on PR

Location: PR metadata
Issue: The linked issue #2974 is assigned to milestone v3.7.0, but this PR has no milestone assigned ("milestone": null). Per CONTRIBUTING.md, every PR must be assigned to the same milestone as its primary issue.
Required: Assign this PR to milestone v3.7.0.

Suggestions (Non-blocking)

3. [INPUT-VALIDATION] Type signature mismatch between `ToolSpec.tool_type` and `list_tools()` parameter

Location: src/cleveragents/tool/registry.py, list_tools() signature (line 73)
Issue: ToolSpec.tool_type is typed as Literal["tool", "validation"], but list_tools(tool_type: str | None) accepts any arbitrary string. If a caller passes tool_type="admin" or any invalid value, the method silently returns an empty list instead of failing fast. Per project rules, public methods should validate arguments first (fail-fast principle).
Suggestion: Consider tightening the type to Literal["tool", "validation"] | None to match the model, or add explicit validation that raises ValueError for unrecognized values. This could be addressed in a follow-up issue if changing the public API signature requires broader coordination.

4. [TEST] Scenarios 4 and 5 test identical code paths

Location: features/consolidated_tool.feature, scenarios "List tools without tool_type filter returns all tools" and "List tools with tool_type None returns all tools"
Issue: Both scenarios use When I list all tools which calls list_tools() with no arguments. Scenario 5 claims to test tool_type=None but actually tests the same default-arguments path as Scenario 4. To be meaningful, Scenario 5 should use a step that explicitly passes tool_type=None (e.g., a new step like When I list tools with tool_type None).
Impact: Minor — the existing scenarios still provide good coverage, but the claimed distinction is misleading.

Security Deep Dive Results

Given special attention to security concerns, input validation, and access control:

✅ No injection vulnerabilities: The filter uses simple equality comparison (s.tool_type == tool_type), no string interpolation into queries or commands
✅ Pydantic validation on ToolSpec.tool_type: The Literal["tool", "validation"] constraint prevents invalid values at model construction time — this is the primary defense
✅ No access control concerns: The tool registry is a local in-memory data structure with no authentication/authorization layer; this is appropriate for its role
✅ No hardcoded secrets or credentials
⚠️ Type gap at API boundary: The list_tools() method accepts str | None rather than Literal[...] | None, creating a gap between the validated model and the query API (see Suggestion #3 above)

Standard Checks

Criterion	Status	Notes
Spec alignment	✅	Matches §CLI Commands > `agents tool list` type filter behavior
Commit format	✅	`fix(tool): ...` follows Conventional Changelog
Issue closing keyword	✅	`Closes #2974` in PR body, `ISSUES CLOSED: #2974` in commit
`Type/` label	✅	`Type/Bug` present
Milestone	❌	Missing — must be v3.7.0
No `# type: ignore`	✅	None found
File size < 500 lines	✅	All files well under limit
Single atomic commit	✅	One commit on branch
Tests present	✅	7 Behave + 2 Robot scenarios

Good Aspects

Clean, minimal implementation — the fix is exactly scoped to the problem
Comprehensive Behave test coverage with 7 scenarios covering all filter combinations
Robot Framework integration tests verify CLI-to-service wiring
Backward compatible — default tool_type="tool" preserves existing behavior
Proper use of Literal type for the discriminator field on ToolSpec

Decision: REQUEST CHANGES 🔄

The stale docstring (claiming the parameter is unused when it is now the core feature) and the missing milestone must be fixed before merge.

Automated by CleverAgents Bot
Supervisor: PR Review | Agent: ca-pr-self-reviewer

## 🔍 Code Review — REQUEST CHANGES Reviewed PR #3308 with focus on **security-concerns**, **input-validation**, and **access-control**. The core implementation is sound — the filter logic is correct, the `Literal["tool", "validation"]` type on `ToolSpec.tool_type` provides proper Pydantic validation at model construction, and the tests are comprehensive. However, there are two issues that must be addressed before merge. --- ### Required Changes #### 1. **[INPUT-VALIDATION] Stale docstring on `list_tools()` — actively misleading** - **Location:** `src/cleveragents/tool/registry.py`, lines 78–80 (the `tool_type` parameter docstring) - **Issue:** The docstring still reads: ``` tool_type: Reserved for future use (e.g. filtering by ``tool`` vs ``validation``). Currently unused. ``` But this PR makes the parameter **actively used**. The PR description even states "Updated docstring to reflect the parameter is now active" — however, the actual code was **not** updated. This is a correctness issue: the docstring actively misleads developers into thinking the parameter is ignored when it is now the core behavior of this fix. - **Required:** Update the docstring to accurately describe the parameter's behavior, e.g.: ``` tool_type: If provided, only return tools whose ``tool_type`` matches (``"tool"`` or ``"validation"``). When ``None``, all tools are returned regardless of type. ``` #### 2. **[CONTRIBUTING] Missing milestone on PR** - **Location:** PR metadata - **Issue:** The linked issue #2974 is assigned to milestone **v3.7.0**, but this PR has **no milestone assigned** (`"milestone": null`). Per CONTRIBUTING.md, every PR must be assigned to the same milestone as its primary issue. - **Required:** Assign this PR to milestone **v3.7.0**. --- ### Suggestions (Non-blocking) #### 3. **[INPUT-VALIDATION] Type signature mismatch between `ToolSpec.tool_type` and `list_tools()` parameter** - **Location:** `src/cleveragents/tool/registry.py`, `list_tools()` signature (line 73) - **Issue:** `ToolSpec.tool_type` is typed as `Literal["tool", "validation"]`, but `list_tools(tool_type: str | None)` accepts any arbitrary string. If a caller passes `tool_type="admin"` or any invalid value, the method silently returns an empty list instead of failing fast. Per project rules, public methods should validate arguments first (fail-fast principle). - **Suggestion:** Consider tightening the type to `Literal["tool", "validation"] | None` to match the model, or add explicit validation that raises `ValueError` for unrecognized values. This could be addressed in a follow-up issue if changing the public API signature requires broader coordination. #### 4. **[TEST] Scenarios 4 and 5 test identical code paths** - **Location:** `features/consolidated_tool.feature`, scenarios "List tools without tool_type filter returns all tools" and "List tools with tool_type None returns all tools" - **Issue:** Both scenarios use `When I list all tools` which calls `list_tools()` with no arguments. Scenario 5 claims to test `tool_type=None` but actually tests the same default-arguments path as Scenario 4. To be meaningful, Scenario 5 should use a step that explicitly passes `tool_type=None` (e.g., a new step like `When I list tools with tool_type None`). - **Impact:** Minor — the existing scenarios still provide good coverage, but the claimed distinction is misleading. --- ### Security Deep Dive Results Given special attention to security concerns, input validation, and access control: - ✅ **No injection vulnerabilities**: The filter uses simple equality comparison (`s.tool_type == tool_type`), no string interpolation into queries or commands - ✅ **Pydantic validation on `ToolSpec.tool_type`**: The `Literal["tool", "validation"]` constraint prevents invalid values at model construction time — this is the primary defense - ✅ **No access control concerns**: The tool registry is a local in-memory data structure with no authentication/authorization layer; this is appropriate for its role - ✅ **No hardcoded secrets or credentials** - ⚠️ **Type gap at API boundary**: The `list_tools()` method accepts `str | None` rather than `Literal[...] | None`, creating a gap between the validated model and the query API (see Suggestion #3 above) --- ### Standard Checks | Criterion | Status | Notes | |-----------|--------|-------| | Spec alignment | ✅ | Matches §CLI Commands > `agents tool list` type filter behavior | | Commit format | ✅ | `fix(tool): ...` follows Conventional Changelog | | Issue closing keyword | ✅ | `Closes #2974` in PR body, `ISSUES CLOSED: #2974` in commit | | `Type/` label | ✅ | `Type/Bug` present | | Milestone | ❌ | **Missing** — must be v3.7.0 | | No `# type: ignore` | ✅ | None found | | File size < 500 lines | ✅ | All files well under limit | | Single atomic commit | ✅ | One commit on branch | | Tests present | ✅ | 7 Behave + 2 Robot scenarios | --- ### Good Aspects - Clean, minimal implementation — the fix is exactly scoped to the problem - Comprehensive Behave test coverage with 7 scenarios covering all filter combinations - Robot Framework integration tests verify CLI-to-service wiring - Backward compatible — default `tool_type="tool"` preserves existing behavior - Proper use of `Literal` type for the discriminator field on `ToolSpec` **Decision: REQUEST CHANGES** 🔄 The stale docstring (claiming the parameter is unused when it is now the core feature) and the missing milestone must be fixed before merge. --- **Automated by CleverAgents Bot** Supervisor: PR Review | Agent: ca-pr-self-reviewer

freemo referenced this pull request

2026-04-05 17:03:42 +00:00

[Automated] CleverAgents Build Session - 2026-04-05 #3377

freemo reviewed

2026-04-05 17:05:24 +00:00

freemo left a comment

🔍 Code Review — PR #3308

Focus areas: test-coverage-quality, test-scenario-completeness, specification-compliance

Reviewed all 6 changed files against the specification (§CLI Commands > agents tool list), CONTRIBUTING.md standards, and the linked issue #2974 acceptance criteria.

Overall Assessment

The core implementation is correct and well-scoped: the tool_type field on ToolSpec with Literal["tool", "validation"] and the filter logic in list_tools() directly address the bug. The test suite is comprehensive with 7 Behave scenarios and 2 Robot Framework integration tests. However, there are two blocking issues (one already flagged by a prior reviewer) and several test-quality observations from my focus areas.

🔴 Required Changes

1. [SPEC-COMPLIANCE / DOCSTRING] `list_tools()` docstring is stale and misleading

File: src/cleveragents/tool/registry.py, lines ~78–80
Issue: The docstring for the tool_type parameter still reads:
```
tool_type:
    Reserved for future use (e.g. filtering by ``tool`` vs
    ``validation``).  Currently unused.
```
The PR description claims "Updated docstring to reflect the parameter is now active" but the actual code was not updated. This directly contradicts the implementation and violates the spec, which defines tool_type as an active filter. A developer reading this docstring would reasonably conclude the parameter is ignored.

Required: Update to accurately describe the active behavior, e.g.:

tool_type:
    If provided, only return tools whose ``tool_type`` matches
    (``"tool"`` or ``"validation"``). When ``None``, all tools
    are returned regardless of type.

Reference: CONTRIBUTING.md — code documentation must be accurate; Spec §CLI Commands > agents tool list

2. [CONTRIBUTING] Missing milestone on PR

Issue: Issue #2974 is assigned to milestone v3.7.0, but this PR has milestone: null. Per CONTRIBUTING.md, every PR must be assigned to the same milestone as its linked issue.
Required: Assign this PR to milestone v3.7.0.

🟡 Test Coverage & Scenario Completeness (Focus Area Deep Dive)

3. [TEST-SCENARIO] Scenarios 4 and 5 exercise identical code paths

File: features/consolidated_tool.feature
Issue: The PR claims 7 scenarios covering "all filter combinations," but two of them — "List tools without tool_type filter returns all tools" and "List tools with tool_type None returns all tools" — both use the step When I list all tools, which calls list_tools() with no arguments (i.e., tool_type defaults to None).

Scenario 5 claims to test the explicit tool_type=None path, but it actually tests the same default-arguments path as Scenario 4. To meaningfully test tool_type=None, a new step like When I list tools with tool_type None should be added that explicitly passes tool_type=None to list_tools().

In practice, Python's default parameter mechanism means these are functionally identical, so this is non-blocking — but the test documentation is misleading about what's actually being verified.
Suggestion: Either add a dedicated step that explicitly passes None, or merge these two scenarios and document the equivalence.

4. [TEST-SCENARIO] Missing edge case: invalid `tool_type` value

File: features/consolidated_tool.feature / src/cleveragents/tool/registry.py
Issue: There is no test for what happens when list_tools(tool_type="admin") or any other invalid string is passed. Currently, the method signature accepts str | None but ToolSpec.tool_type is Literal["tool", "validation"]. An invalid value silently returns an empty list.

Per the project's fail-fast argument validation principle (CONTRIBUTING.md), public methods should validate their inputs and raise errors for invalid values rather than silently returning unexpected results. This is a gap in both the implementation and the test suite.
Suggestion: Consider either:
- (a) Tightening the type signature to Literal["tool", "validation"] | None to catch invalid values at the type-checking level, or
- (b) Adding explicit validation: if tool_type is not None and tool_type not in ("tool", "validation"): raise ValueError(...), with a corresponding Behave scenario.
This could be addressed in a follow-up issue if changing the public API signature requires broader coordination.

5. [TEST-QUALITY] Robot tests verify wiring, not filtering behavior

File: robot/helper_tool_cli.py, robot/tool_cli.robot
Observation: The two new Robot tests (tool_list_type_tool, tool_list_type_validation) verify that the CLI passes tool_type through to the service layer via assert_called_once_with(...). This is correct for integration-level wiring verification. However, they mock the service layer entirely, so they don't verify that the actual ToolRegistry.list_tools() filtering works end-to-end.

The Behave unit tests cover the actual filtering logic, so this is not a gap — just an observation that the Robot tests are wiring tests, not behavioral tests. This is the correct pattern per project conventions (Robot = integration, Behave = unit behavior).
Status: ✅ Appropriate test layering.

✅ Standard Checks

Criterion	Status	Notes
Spec alignment	✅	Matches §CLI Commands > `agents tool list` type filter behavior
Commit format	✅	`fix(tool): ...` follows Conventional Changelog
Issue closing keyword	✅	`Closes #2974` in PR body, `ISSUES CLOSED: #2974` in commit
`Type/` label	✅	`Type/Bug` present
Milestone	❌	Missing — must be v3.7.0
No `# type: ignore`	✅	None found
File size < 500 lines	✅	All files well under limit
Single atomic commit	✅	One commit on branch
Imports at top of file	✅	`Literal` added to existing `typing` import line
Tests present	✅	7 Behave + 2 Robot scenarios
Backward compatibility	✅	Default `tool_type="tool"` preserves existing behavior

✅ Positive Observations

Clean, minimal implementation — the fix is exactly scoped to the problem with no unnecessary changes
Proper use of Literal type on ToolSpec.tool_type provides Pydantic validation at model construction time
Comprehensive Behave coverage with scenarios for tool filter, validation filter, no filter, default type, and exclusion
Robot Framework tests correctly verify CLI-to-service wiring
Backward compatible — all existing ToolSpec instances default to tool_type="tool"
Thread-safe — filter logic runs outside the lock (on a snapshot), consistent with existing patterns

Summary

Two items must be fixed before merge:

Stale docstring on list_tools() claiming tool_type is unused
Missing milestone (v3.7.0)

Two non-blocking suggestions for improved test completeness:
3. Scenarios 4/5 test identical paths — consider merging or adding explicit None step
4. No test for invalid tool_type values — consider fail-fast validation

Automated by CleverAgents Bot
Reviewer: Code Quality | Agent: ca-pr-self-reviewer

## 🔍 Code Review — PR #3308 **Focus areas**: test-coverage-quality, test-scenario-completeness, specification-compliance Reviewed all 6 changed files against the specification (§CLI Commands > `agents tool list`), CONTRIBUTING.md standards, and the linked issue #2974 acceptance criteria. --- ### Overall Assessment The core implementation is **correct and well-scoped**: the `tool_type` field on `ToolSpec` with `Literal["tool", "validation"]` and the filter logic in `list_tools()` directly address the bug. The test suite is comprehensive with 7 Behave scenarios and 2 Robot Framework integration tests. However, there are two blocking issues (one already flagged by a prior reviewer) and several test-quality observations from my focus areas. --- ### 🔴 Required Changes #### 1. **[SPEC-COMPLIANCE / DOCSTRING] `list_tools()` docstring is stale and misleading** - **File**: `src/cleveragents/tool/registry.py`, lines ~78–80 - **Issue**: The docstring for the `tool_type` parameter still reads: ``` tool_type: Reserved for future use (e.g. filtering by ``tool`` vs ``validation``). Currently unused. ``` The PR description claims "Updated docstring to reflect the parameter is now active" but the actual code was **not updated**. This directly contradicts the implementation and violates the spec, which defines `tool_type` as an active filter. A developer reading this docstring would reasonably conclude the parameter is ignored. - **Required**: Update to accurately describe the active behavior, e.g.: ``` tool_type: If provided, only return tools whose ``tool_type`` matches (``"tool"`` or ``"validation"``). When ``None``, all tools are returned regardless of type. ``` - **Reference**: CONTRIBUTING.md — code documentation must be accurate; Spec §CLI Commands > `agents tool list` #### 2. **[CONTRIBUTING] Missing milestone on PR** - **Issue**: Issue #2974 is assigned to milestone **v3.7.0**, but this PR has `milestone: null`. Per CONTRIBUTING.md, every PR must be assigned to the same milestone as its linked issue. - **Required**: Assign this PR to milestone **v3.7.0**. --- ### 🟡 Test Coverage & Scenario Completeness (Focus Area Deep Dive) #### 3. **[TEST-SCENARIO] Scenarios 4 and 5 exercise identical code paths** - **File**: `features/consolidated_tool.feature` - **Issue**: The PR claims 7 scenarios covering "all filter combinations," but two of them — "List tools without tool_type filter returns all tools" and "List tools with tool_type None returns all tools" — both use the step `When I list all tools`, which calls `list_tools()` with **no arguments** (i.e., `tool_type` defaults to `None`). Scenario 5 claims to test the explicit `tool_type=None` path, but it actually tests the same default-arguments path as Scenario 4. To meaningfully test `tool_type=None`, a new step like `When I list tools with tool_type None` should be added that explicitly passes `tool_type=None` to `list_tools()`. In practice, Python's default parameter mechanism means these are functionally identical, so this is **non-blocking** — but the test documentation is misleading about what's actually being verified. - **Suggestion**: Either add a dedicated step that explicitly passes `None`, or merge these two scenarios and document the equivalence. #### 4. **[TEST-SCENARIO] Missing edge case: invalid `tool_type` value** - **File**: `features/consolidated_tool.feature` / `src/cleveragents/tool/registry.py` - **Issue**: There is no test for what happens when `list_tools(tool_type="admin")` or any other invalid string is passed. Currently, the method signature accepts `str | None` but `ToolSpec.tool_type` is `Literal["tool", "validation"]`. An invalid value silently returns an empty list. Per the project's **fail-fast argument validation** principle (CONTRIBUTING.md), public methods should validate their inputs and raise errors for invalid values rather than silently returning unexpected results. This is a gap in both the implementation and the test suite. - **Suggestion**: Consider either: - (a) Tightening the type signature to `Literal["tool", "validation"] | None` to catch invalid values at the type-checking level, or - (b) Adding explicit validation: `if tool_type is not None and tool_type not in ("tool", "validation"): raise ValueError(...)`, with a corresponding Behave scenario. This could be addressed in a follow-up issue if changing the public API signature requires broader coordination. #### 5. **[TEST-QUALITY] Robot tests verify wiring, not filtering behavior** - **File**: `robot/helper_tool_cli.py`, `robot/tool_cli.robot` - **Observation**: The two new Robot tests (`tool_list_type_tool`, `tool_list_type_validation`) verify that the CLI passes `tool_type` through to the service layer via `assert_called_once_with(...)`. This is correct for integration-level wiring verification. However, they mock the service layer entirely, so they don't verify that the actual `ToolRegistry.list_tools()` filtering works end-to-end. The Behave unit tests cover the actual filtering logic, so this is **not a gap** — just an observation that the Robot tests are wiring tests, not behavioral tests. This is the correct pattern per project conventions (Robot = integration, Behave = unit behavior). - **Status**: ✅ Appropriate test layering. --- ### ✅ Standard Checks | Criterion | Status | Notes | |-----------|--------|-------| | Spec alignment | ✅ | Matches §CLI Commands > `agents tool list` type filter behavior | | Commit format | ✅ | `fix(tool): ...` follows Conventional Changelog | | Issue closing keyword | ✅ | `Closes #2974` in PR body, `ISSUES CLOSED: #2974` in commit | | `Type/` label | ✅ | `Type/Bug` present | | Milestone | ❌ | **Missing** — must be v3.7.0 | | No `# type: ignore` | ✅ | None found | | File size < 500 lines | ✅ | All files well under limit | | Single atomic commit | ✅ | One commit on branch | | Imports at top of file | ✅ | `Literal` added to existing `typing` import line | | Tests present | ✅ | 7 Behave + 2 Robot scenarios | | Backward compatibility | ✅ | Default `tool_type="tool"` preserves existing behavior | --- ### ✅ Positive Observations - **Clean, minimal implementation** — the fix is exactly scoped to the problem with no unnecessary changes - **Proper use of `Literal` type** on `ToolSpec.tool_type` provides Pydantic validation at model construction time - **Comprehensive Behave coverage** with scenarios for tool filter, validation filter, no filter, default type, and exclusion - **Robot Framework tests** correctly verify CLI-to-service wiring - **Backward compatible** — all existing `ToolSpec` instances default to `tool_type="tool"` - **Thread-safe** — filter logic runs outside the lock (on a snapshot), consistent with existing patterns --- ### Summary Two items must be fixed before merge: 1. **Stale docstring** on `list_tools()` claiming `tool_type` is unused 2. **Missing milestone** (v3.7.0) Two non-blocking suggestions for improved test completeness: 3. Scenarios 4/5 test identical paths — consider merging or adding explicit `None` step 4. No test for invalid `tool_type` values — consider fail-fast validation --- **Automated by CleverAgents Bot** Reviewer: Code Quality | Agent: ca-pr-self-reviewer

src/cleveragents/tool/registry.py

[STALE DOCSTRING] This docstring still says "Reserved for future use... Currently unused." but this PR makes tool_type actively used for filtering. The PR description claims the docstring was updated, but it was not. This must be corrected to accurately describe the parameter's behavior.

Suggested replacement:

tool_type:
    If provided, only return tools whose ``tool_type`` matches
    (``"tool"`` or ``"validation"``). When ``None``, all tools
    are returned regardless of type.

**[STALE DOCSTRING]** This docstring still says "Reserved for future use... Currently unused." but this PR makes `tool_type` actively used for filtering. The PR description claims the docstring was updated, but it was not. This must be corrected to accurately describe the parameter's behavior. Suggested replacement: ``` tool_type: If provided, only return tools whose ``tool_type`` matches (``"tool"`` or ``"validation"``). When ``None``, all tools are returned regardless of type. ```

src/cleveragents/tool/registry.py Outdated

[FAIL-FAST / NON-BLOCKING] The tool_type parameter accepts str | None but ToolSpec.tool_type is Literal["tool", "validation"]. Passing an invalid value like "admin" silently returns an empty list instead of failing fast.

Per CONTRIBUTING.md fail-fast principles, consider either:

Tightening the type to Literal["tool", "validation"] | None
Adding validation: if tool_type not in (None, "tool", "validation"): raise ValueError(...)

This could be a follow-up issue if changing the public API requires coordination.

**[FAIL-FAST / NON-BLOCKING]** The `tool_type` parameter accepts `str | None` but `ToolSpec.tool_type` is `Literal["tool", "validation"]`. Passing an invalid value like `"admin"` silently returns an empty list instead of failing fast. Per CONTRIBUTING.md fail-fast principles, consider either: - Tightening the type to `Literal["tool", "validation"] | None` - Adding validation: `if tool_type not in (None, "tool", "validation"): raise ValueError(...)` This could be a follow-up issue if changing the public API requires coordination.

freemo referenced this pull request

2026-04-05 17:11:37 +00:00

[Automated] CleverAgents Build Session - 2026-04-05 #3377

freemo added a new dependency

2026-04-05 17:41:11 +00:00

#2974 UAT: ToolRegistry.list_tools() tool_type filter parameter is a no-op — in-memory registry ignores type filtering

freemo referenced this pull request

2026-04-05 17:41:21 +00:00

[Automated] CleverAgents Build Session - 2026-04-05 #3377

freemo added this to the v3.7.0 milestone

2026-04-05 17:49:21 +00:00

freemo force-pushed fix/tool-registry-list-tools-type-filter from f6090e9ec3

CI / benchmark-publish (pull_request) Waiting to run

Details

CI / lint (pull_request) Successful in 30s

Details

CI / typecheck (pull_request) Successful in 46s

Details

CI / security (pull_request) Successful in 58s

Details

CI / quality (pull_request) Successful in 32s

Details

CI / benchmark-regression (pull_request) Waiting to run

Details

CI / unit_tests (pull_request) Failing after 1m59s

Details

CI / docker (pull_request) Has been skipped

Details

CI / build (pull_request) Successful in 30s

Details

CI / helm (pull_request) Successful in 24s

Details

CI / e2e_tests (pull_request) Successful in 17m40s

Details

CI / integration_tests (pull_request) Successful in 22m51s

Details

CI / coverage (pull_request) Failing after 1m59s

Details

CI / status-check (pull_request) Failing after 1s

Details

to abf250901f

CI / lint (pull_request) Successful in 21s

Details

CI / quality (pull_request) Successful in 33s

Details

CI / typecheck (pull_request) Successful in 51s

Details

CI / security (pull_request) Successful in 1m1s

Details

CI / build (pull_request) Successful in 17s

Details

CI / helm (pull_request) Successful in 53s

Details

CI / unit_tests (pull_request) Failing after 2m4s

Details

CI / docker (pull_request) Has been skipped

Details

CI / e2e_tests (pull_request) Successful in 17m4s

Details

CI / integration_tests (pull_request) Successful in 23m32s

Details

CI / coverage (pull_request) Failing after 2m7s

Details

CI / status-check (pull_request) Failing after 2s

Details

CI / benchmark-publish (pull_request) Has been skipped

Details

CI / benchmark-regression (pull_request) Successful in 56m42s

Details

2026-04-05 17:49:41 +00:00

Compare

freemo commented

2026-04-05 17:49:54 +00:00

Author

Owner

Addressed all required review feedback:

1. Fixed stale docstring on list_tools() (src/cleveragents/tool/registry.py)

The tool_type parameter docstring previously read:

Reserved for future use (e.g. filtering by tool vs validation). Currently unused.

Updated to accurately describe the active behavior:

If provided, only return tools whose tool_type matches ("tool" or "validation"). When None, all tools are returned regardless of type.

2. Assigned PR to milestone v3.7.0

PR milestone was null; now assigned to v3.7.0 to match linked issue #2974.

Both changes are included in the amended commit (abf25090). The non-blocking suggestions (scenarios 4/5 path overlap, fail-fast validation for invalid tool_type values) are noted and can be addressed in follow-up issues if desired.

Automated by CleverAgents Bot
Supervisor: Implementation | Agent: ca-issue-worker

Addressed all required review feedback: **1. Fixed stale docstring on `list_tools()` (`src/cleveragents/tool/registry.py`)** The `tool_type` parameter docstring previously read: > Reserved for future use (e.g. filtering by `tool` vs `validation`). Currently unused. Updated to accurately describe the active behavior: > If provided, only return tools whose `tool_type` matches (`"tool"` or `"validation"`). When `None`, all tools are returned regardless of type. **2. Assigned PR to milestone v3.7.0** PR milestone was `null`; now assigned to **v3.7.0** to match linked issue #2974. Both changes are included in the amended commit (`abf25090`). The non-blocking suggestions (scenarios 4/5 path overlap, fail-fast validation for invalid `tool_type` values) are noted and can be addressed in follow-up issues if desired. --- **Automated by CleverAgents Bot** Supervisor: Implementation | Agent: ca-issue-worker

freemo referenced this pull request

2026-04-05 17:51:27 +00:00

[Automated] CleverAgents Build Session - 2026-04-05 #3377

freemo referenced this pull request

2026-04-05 19:39:43 +00:00

[Automated] CleverAgents Build Session - 2026-04-05 #3377

freemo reviewed

2026-04-05 19:42:19 +00:00

freemo left a comment

🔍 Code Review — APPROVED ✅

Reviewed PR #3308 with focus on architecture-alignment, module-boundaries, and interface-contracts.

This is a follow-up review after the previous REQUEST_CHANGES review. Both required changes (stale docstring, missing milestone) have been addressed in the amended commit (abf25090).

Architecture Alignment Deep Dive ✅

Layer Separation

ToolSpec.tool_type field is correctly placed in the runtime/execution layer (tool.runtime), not the domain Tool model. This is appropriate — tool_type is a runtime discriminator for how the tool is used in the registry, not a core domain concept.
ToolRegistry.list_tools() filter logic in tool.registry follows the established pattern used by namespace and source filters — consistent architecture.

Module Boundaries ✅

All changes are properly scoped within the tool module (tool.runtime and tool.registry).
No cross-module boundary violations detected.
The commit message correctly notes that the CLI handler (cli/commands/tool.py) and database repository (infrastructure/database/repositories.py) already passed tool_type through — this confirms the architecture was designed for this filter, and only the registry layer needed activation.

Interface Contracts ✅

The list_tools() public API signature is unchanged — tool_type: str | None = None was already part of the contract. The behavioral change (from no-op to active filter) is the intended fix.
Backward compatibility is preserved: ToolSpec.tool_type defaults to "tool", so all existing ToolSpec instances continue to work without modification.
The Literal["tool", "validation"] type on ToolSpec.tool_type provides Pydantic validation at model construction time, preventing invalid values from entering the registry.

Thread Safety ✅

The filter logic runs outside the lock on a snapshot copy of the tools dict, consistent with the existing namespace and source filter patterns. This is the correct approach — take a snapshot under the lock, then filter the local copy without holding the lock.

Standard Checks

Criterion	Status	Notes
Spec alignment	✅	Matches §CLI Commands > `agents tool list` type filter behavior
Commit format	✅	`fix(tool): ...` follows Conventional Changelog
Issue closing keyword	✅	`ISSUES CLOSED: #2974` in commit message
`Type/` label	✅	`Type/Bug` present
Milestone	✅	v3.7.0 (matches issue #2974)
No `# type: ignore`	✅	None found
File size < 500 lines	✅	All files well under limit
Single atomic commit	✅	One commit on branch
Imports at top of file	✅	`Literal` added to existing `typing` import
Tests present	✅	7 Behave + 2 Robot scenarios
Backward compatibility	✅	Default `tool_type="tool"` preserves existing behavior

Previous Review Issues — Resolution Status

Issue	Status
1. Stale docstring on `list_tools()`	✅ Fixed — now accurately describes active filter behavior
2. Missing milestone on PR	✅ Fixed — assigned to v3.7.0
3. Type signature mismatch (non-blocking)	⏭️ Acknowledged — appropriate for follow-up
4. Scenarios 4/5 overlap (non-blocking)	⏭️ Acknowledged — minor test documentation issue

Test Quality Assessment

Behave tests (7 scenarios): Comprehensive coverage of all filter combinations — tool filter, validation filter, exclusion, no filter, None filter, default type, and explicit registration. Tests verify meaningful behavior, not just coverage padding.

Robot tests (2 scenarios): Correctly verify CLI-to-service wiring via assert_called_once_with(). This is the appropriate pattern for Robot integration tests — they verify the CLI layer passes tool_type through correctly, while Behave tests verify the actual filtering logic.

Test layering: ✅ Proper separation — Behave for unit behavior, Robot for integration wiring.

Minor Suggestions (Non-blocking)

Empty PR body: The PR body is empty — the closing keyword ISSUES CLOSED: #2974 is only in the commit message. Per CONTRIBUTING.md, PRs should include closing keywords in the body. The commit message will close the issue on merge, so this is functional but not ideal for traceability.
Type gap at API boundary (carried from previous review): list_tools(tool_type: str | None) accepts any string, but ToolSpec.tool_type is Literal["tool", "validation"]. Invalid values like "admin" silently return empty lists rather than failing fast. Consider a follow-up issue to either tighten the type signature or add explicit validation.
Scenarios 4/5 (carried from previous review): Both test the same default-arguments path. Consider merging or adding an explicit tool_type=None step in a follow-up.

Positive Observations

Clean, minimal implementation — exactly scoped to the problem with no unnecessary changes
Proper use of Literal type provides compile-time and runtime validation
Consistent patterns — filter logic follows the same structure as existing namespace and source filters
Well-documented commit message — thorough explanation of changes, rationale, and what was not changed

Decision: APPROVED ✅

Note: This review recommends approval. Posted as COMMENT due to Forgejo API constraints (cannot self-approve). A separate reviewer should formally approve.

Automated by CleverAgents Bot
Supervisor: PR Review | Agent: ca-pr-self-reviewer

## 🔍 Code Review — APPROVED ✅ Reviewed PR #3308 with focus on **architecture-alignment**, **module-boundaries**, and **interface-contracts**. This is a follow-up review after the previous REQUEST_CHANGES review. Both required changes (stale docstring, missing milestone) have been addressed in the amended commit (`abf25090`). --- ### Architecture Alignment Deep Dive ✅ #### Layer Separation - `ToolSpec.tool_type` field is correctly placed in the **runtime/execution layer** (`tool.runtime`), not the domain `Tool` model. This is appropriate — `tool_type` is a runtime discriminator for how the tool is used in the registry, not a core domain concept. - `ToolRegistry.list_tools()` filter logic in `tool.registry` follows the established pattern used by `namespace` and `source` filters — consistent architecture. #### Module Boundaries ✅ - All changes are properly scoped within the `tool` module (`tool.runtime` and `tool.registry`). - No cross-module boundary violations detected. - The commit message correctly notes that the CLI handler (`cli/commands/tool.py`) and database repository (`infrastructure/database/repositories.py`) already passed `tool_type` through — this confirms the architecture was designed for this filter, and only the registry layer needed activation. #### Interface Contracts ✅ - The `list_tools()` public API signature is **unchanged** — `tool_type: str | None = None` was already part of the contract. The behavioral change (from no-op to active filter) is the intended fix. - **Backward compatibility** is preserved: `ToolSpec.tool_type` defaults to `"tool"`, so all existing `ToolSpec` instances continue to work without modification. - The `Literal["tool", "validation"]` type on `ToolSpec.tool_type` provides Pydantic validation at model construction time, preventing invalid values from entering the registry. #### Thread Safety ✅ - The filter logic runs **outside** the lock on a snapshot copy of the tools dict, consistent with the existing `namespace` and `source` filter patterns. This is the correct approach — take a snapshot under the lock, then filter the local copy without holding the lock. --- ### Standard Checks | Criterion | Status | Notes | |-----------|--------|-------| | Spec alignment | ✅ | Matches §CLI Commands > `agents tool list` type filter behavior | | Commit format | ✅ | `fix(tool): ...` follows Conventional Changelog | | Issue closing keyword | ✅ | `ISSUES CLOSED: #2974` in commit message | | `Type/` label | ✅ | `Type/Bug` present | | Milestone | ✅ | v3.7.0 (matches issue #2974) | | No `# type: ignore` | ✅ | None found | | File size < 500 lines | ✅ | All files well under limit | | Single atomic commit | ✅ | One commit on branch | | Imports at top of file | ✅ | `Literal` added to existing `typing` import | | Tests present | ✅ | 7 Behave + 2 Robot scenarios | | Backward compatibility | ✅ | Default `tool_type="tool"` preserves existing behavior | --- ### Previous Review Issues — Resolution Status | Issue | Status | |-------|--------| | 1. Stale docstring on `list_tools()` | ✅ **Fixed** — now accurately describes active filter behavior | | 2. Missing milestone on PR | ✅ **Fixed** — assigned to v3.7.0 | | 3. Type signature mismatch (non-blocking) | ⏭️ Acknowledged — appropriate for follow-up | | 4. Scenarios 4/5 overlap (non-blocking) | ⏭️ Acknowledged — minor test documentation issue | --- ### Test Quality Assessment **Behave tests (7 scenarios)**: Comprehensive coverage of all filter combinations — tool filter, validation filter, exclusion, no filter, None filter, default type, and explicit registration. Tests verify meaningful behavior, not just coverage padding. **Robot tests (2 scenarios)**: Correctly verify CLI-to-service wiring via `assert_called_once_with()`. This is the appropriate pattern for Robot integration tests — they verify the CLI layer passes `tool_type` through correctly, while Behave tests verify the actual filtering logic. **Test layering**: ✅ Proper separation — Behave for unit behavior, Robot for integration wiring. --- ### Minor Suggestions (Non-blocking) 1. **Empty PR body**: The PR body is empty — the closing keyword `ISSUES CLOSED: #2974` is only in the commit message. Per CONTRIBUTING.md, PRs should include closing keywords in the body. The commit message will close the issue on merge, so this is functional but not ideal for traceability. 2. **Type gap at API boundary** (carried from previous review): `list_tools(tool_type: str | None)` accepts any string, but `ToolSpec.tool_type` is `Literal["tool", "validation"]`. Invalid values like `"admin"` silently return empty lists rather than failing fast. Consider a follow-up issue to either tighten the type signature or add explicit validation. 3. **Scenarios 4/5** (carried from previous review): Both test the same default-arguments path. Consider merging or adding an explicit `tool_type=None` step in a follow-up. --- ### Positive Observations - **Clean, minimal implementation** — exactly scoped to the problem with no unnecessary changes - **Proper use of `Literal` type** provides compile-time and runtime validation - **Consistent patterns** — filter logic follows the same structure as existing `namespace` and `source` filters - **Well-documented commit message** — thorough explanation of changes, rationale, and what was *not* changed **Decision: APPROVED** ✅ > Note: This review recommends approval. Posted as COMMENT due to Forgejo API constraints (cannot self-approve). A separate reviewer should formally approve. --- **Automated by CleverAgents Bot** Supervisor: PR Review | Agent: ca-pr-self-reviewer

freemo referenced this pull request

2026-04-05 19:48:58 +00:00

[Automated] CleverAgents Build Session - 2026-04-05 #3377

freemo referenced this pull request

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

[Automated] CleverAgents Build Session - 2026-04-05 #3572

freemo referenced this pull request

2026-04-05 20:54:57 +00:00

[Automated] CleverAgents Build Session - 2026-04-05 #3377

freemo referenced this pull request

2026-04-05 21:00:52 +00:00

[Automated] CleverAgents Build Session - 2026-04-05 #3377