mypy-pylint-fixed #21

						
				@@ -286,3 +283,3 @@

				        # Extract result

				        if hasattr(result, "messages") and result.messages:

				            return result.messages[-1].get("content", "")

				            content = result.messages[-1].get("content", "")

brent.edwards commented

2025-10-27 18:16:56 +00:00

In line 279 (I cannot leave a comment on code you didn't write) is

      result = await graph.execute(
         {"messages": [{"role": "user", "content": message}], "metadata": context}
     )

mypy --strict src reports:

src/cleveragents/agents/composite.py:279: error: Item "None" of "LangGraph | None" has no attribute "execute"  [union-attr]

In line 279 (I cannot leave a comment on code you didn't write) is > result = await graph.execute( > {"messages": [{"role": "user", "content": message}], "metadata": context} > ) `mypy --strict src` reports: ``` src/cleveragents/agents/composite.py:279: error: Item "None" of "LangGraph | None" has no attribute "execute" [union-attr] ```

brent.edwards marked this conversation as resolved

src/cleveragents/core/application.py Outdated

						
				@@ -450,3 +482,1 @@

				                f"Registered {len(templates.get('agents', {}))} agent templates, "

				                f"{len(templates.get('graphs', {}))} graph templates, "

				                f"{len(templates.get('streams', {}))} stream templates"

				                "Registered %d agent templates, %d graph templates, %d stream templates",

brent.edwards commented

2025-10-27 18:18:20 +00:00

On line 478 (I cannot leave comments on lines you did not change) is the line

              self.template_registry.register_all_templates(self.config.templates)

mypy --strict src reports:

src/cleveragents/core/application.py:478: error: "object" has no attribute "register_all_templates"  [attr-defined]

On line 478 (I cannot leave comments on lines you did not change) is the line > self.template_registry.register_all_templates(self.config.templates) `mypy --strict src` reports: ``` src/cleveragents/core/application.py:478: error: "object" has no attribute "register_all_templates" [attr-defined] ```

brent.edwards commented

2025-10-29 02:39:32 +00:00

Still missing this problem from mypy --strict src. Here's what I get:

src/cleveragents/core/application.py:481: error: "object" has no attribute "register_all_templates"  [attr-defined]

Still missing this problem from `mypy --strict src`. Here's what I get: ``` src/cleveragents/core/application.py:481: error: "object" has no attribute "register_all_templates" [attr-defined] ```

brent.edwards commented

2025-10-29 18:40:47 +00:00

It's still here this morning:

src/cleveragents/core/application.py:481: error: "object" has no attribute "register_all_templates"  [attr-defined]

It's still here this morning: ``` src/cleveragents/core/application.py:481: error: "object" has no attribute "register_all_templates" [attr-defined] ```

aditya commented

2025-10-30 13:36:56 +00:00

@brent.edwards
Fixed the mypy --strict issues.

@brent.edwards Fixed the mypy --strict issues.

src/cleveragents/templates/graph_templates.py Outdated

						
				@@ -56,3 +57,3 @@

				            graph_def["name"] = self.name

				        return graph_def

				        return cast(Dict[str, Any], graph_def)

brent.edwards commented

2025-10-27 18:13:22 +00:00

    return cast(Dict[str, Any], graph_def)

According to mypy --strict src:

src/cleveragents/templates/graph_templates.py:59: error: Redundant cast to "dict[str, Any]"  [redundant-cast]

> return cast(Dict[str, Any], graph_def) According to `mypy --strict src`: ``` src/cleveragents/templates/graph_templates.py:59: error: Redundant cast to "dict[str, Any]" [redundant-cast] ```

brent.edwards marked this conversation as resolved

src/cleveragents/templates/inline_jinja_handler.py Outdated

						
				@@ -283,3 +280,3 @@

				        result = self._apply_templates_recursive(result, templates, context)

				        return result

				        return cast(Dict[str, Any], result)

brent.edwards commented

2025-10-27 18:11:44 +00:00

      return cast(Dict[str, Any], result)

According to mypy --strict src:

src/cleveragents/templates/inline_jinja_handler.py:282: error: Redundant cast to "dict[str, Any]"  [redundant-cast]

> return cast(Dict[str, Any], result) According to `mypy --strict src`: ``` src/cleveragents/templates/inline_jinja_handler.py:282: error: Redundant cast to "dict[str, Any]" [redundant-cast] ```

brent.edwards marked this conversation as resolved

src/cleveragents/templates/stream_templates.py Outdated

						
				@@ -52,3 +53,3 @@

				            stream_def["name"] = self.name

				        return stream_def

				        return cast(Dict[str, Any], stream_def)

brent.edwards commented

2025-10-27 18:12:33 +00:00

    return cast(Dict[str, Any], stream_def)

According to mypy --strict src:

src/cleveragents/templates/stream_templates.py:55: error: Redundant cast to "dict[str, Any]"  [redundant-cast]

> return cast(Dict[str, Any], stream_def) According to `mypy --strict src`: ``` src/cleveragents/templates/stream_templates.py:55: error: Redundant cast to "dict[str, Any]" [redundant-cast] ```

brent.edwards marked this conversation as resolved

aditya added 1 commit 2025-10-28 08:09:19 +00:00

fix: fix rest of the mypy --strict and pylint errors a8a394fab5

khird reviewed 2025-10-28 20:24:24 +00:00

khird left a comment

To achieve meaningful mypy compliance, the use of cast to subvert its warnings should be reduced, or at least there should be comments justifying why each usage is safe despite mypy's warning. The other issues are optional and won't block my approval of this MR.

src/cleveragents/agents/llm.py Outdated

						
				@@ -100,3 +99,3 @@

				                    kwargs["api_key"] = api_key

				                return ChatAnthropic(**kwargs)

				            elif self.provider.lower() == "google":

				            if self.provider.lower() == "google":

khird commented

2025-10-28 18:05:29 +00:00

These if/elif blocks would probably be more clearly expressed as one block of code that operates on a data structure like

{   "openai": {
        "renames": {},
        "class": ChatOpenAI},
    "anthropic": {
        "renames": {},
        "class": ChatAnthropic},
    "google": {
        "renames": {
            "max_tokens": "max_output_tokens",
            "api_key": "google_api_key"},
        "class": ChatGoogleGenerativeAPI}}

and decides what members of common_kwargs (if any) to mangle based on the renames field.

These if/elif blocks would probably be more clearly expressed as one block of code that operates on a data structure like ``` { "openai": { "renames": {}, "class": ChatOpenAI}, "anthropic": { "renames": {}, "class": ChatAnthropic}, "google": { "renames": { "max_tokens": "max_output_tokens", "api_key": "google_api_key"}, "class": ChatGoogleGenerativeAPI}} ``` and decides what members of `common_kwargs` (if any) to mangle based on the `renames` field.

aditya commented

2025-10-30 13:15:19 +00:00

@khird
Replaced if/elif blocks with a data-driven provider_config dictionary that maps providers to their parameter renames and classes, using a unified flow for all providers.

@khird Replaced if/elif blocks with a data-driven provider_config dictionary that maps providers to their parameter renames and classes, using a unified flow for all providers.

khird marked this conversation as resolved

src/cleveragents/langgraph/nodes.py Outdated

						
				@@ -205,2 +228,3 @@

				        return {}

				    async def _execute_tool(self, state: GraphState) -> Dict[str, Any]:

				    async def _execute_tool(self, _state: GraphState) -> Dict[str, Any]:

khird commented

2025-10-28 18:49:34 +00:00

I assume this makes some automated tool shut up about you not using the argument state anywhere in the function body. Is there any reason not to simply remove it from the function definition? If there are places that rely on being able to call a function that might be self._execute_tool() or might be self._execute_conditional() and construct their argument list to include a GraphState, this change would cause it to break if they set it by keyword. If you absolutely can't remove it, using *args and **kwargs might be more forgiving.

I assume this makes some automated tool shut up about you not using the argument `state` anywhere in the function body. Is there any reason not to simply remove it from the function definition? If there are places that rely on being able to call a function that might be `self._execute_tool()` or might be `self._execute_conditional()` and construct their argument list to include a GraphState, this change would cause it to break if they set it by keyword. If you absolutely can't remove it, using `*args` and `**kwargs` might be more forgiving.

aditya commented

2025-10-30 13:16:10 +00:00

@khird
Fixed by removing the unused state parameter entirely from _execute_tool() since it wasn't being used in the function body, while keeping it in _execute_conditional() where it's actually needed.

@khird Fixed by removing the unused state parameter entirely from _execute_tool() since it wasn't being used in the function body, while keeping it in _execute_conditional() where it's actually needed.

khird commented

2025-10-31 14:39:40 +00:00

same issue in _execute_state(). Not a blocker, so I'm approving

src/cleveragents/reactive/config_parser.py Outdated

						
				@@ -156,2 +156,3 @@

				                        if default_value.isdigit():

				                            return default_value

				                        elif default_value.replace(".", "").isdigit():

				                        if default_value.replace(".", "").isdigit():

khird commented

2025-10-28 18:27:57 +00:00

What is the point of this test? If to match decimal numbers, should probably be replace(x, y, 1). If to match IP addresses, this doesn't work on IPv6. And in any case, if the condition isn't satisfied, we do the same thing (i.e. returning the default value) anyway when we fall through.

aditya commented

2025-10-30 13:16:47 +00:00

Fixed by removing the redundant decimal check since it was doing the same thing as the fall-through case - returning the default value as a string. @khird

khird marked this conversation as resolved

src/cleveragents/reactive/config_parser.py Outdated

						
				@@ -173,2 +171,3 @@

				            if config.isdigit():

				                return int(config)

				            elif config.replace(".", "").isdigit() and config.count(".") == 1:

				            if config.replace(".", "").isdigit() and config.count(".") == 1:

khird commented

2025-10-28 19:04:08 +00:00

So I guess the answer here is that it's a decimal number. I note that negatives aren't handled; may or may not be an issue.

So I guess the answer [here](https://git.cleverthis.com/cleveragents/cleveragents-core/pulls/21/files#issuecomment-30424) is that it's a decimal number. I note that negatives aren't handled; may or may not be an issue.

aditya commented

2025-10-30 13:17:25 +00:00

Fixed by using lstrip('-') to handle negative numbers and replace(".", "", 1) to properly handle decimal numbers with only one decimal point, addressing the negative number issue you mentioned. @khird

khird marked this conversation as resolved

src/cleveragents/reactive/stream_router.py Outdated

						
				@@ -332,2 +336,2 @@

				            import threading

				            import asyncio as async_io  # pylint: disable=reimported,import-outside-toplevel

				            import threading  # pylint: disable=import-outside-toplevel

khird commented

2025-10-28 19:14:09 +00:00

It's not clear why reimporting modules is necessary, or why they wouldn't be at the top level. Maybe comment to justify why do it this way that requires circumventing pylint + mypy?

aditya commented

2025-10-30 13:19:29 +00:00

@khird
fixed the re importing modules, only keeping it on places where there is circular import issue.

@khird fixed the re importing modules, only keeping it on places where there is circular import issue.

khird marked this conversation as resolved

src/cleveragents/templates/enhanced_registry.py

						
				@@ -6,3 +6,4 @@

				from pathlib import Path

				from typing import TYPE_CHECKING

				from typing import Any

				from typing import Dict

khird commented

2025-10-28 19:36:25 +00:00

typing.Dict is deprecated in favour of dict in recent versions of python, including all the versions CleverAgents claims to support.

aditya commented

2025-10-30 13:20:29 +00:00

@khird
Replaced the Dict[..] with dict[..] and removed the import statement.

@khird Replaced the Dict[..] with dict[..] and removed the import statement.

khird marked this conversation as resolved

src/cleveragents/templates/enhanced_registry.py Outdated

						
				@@ -174,1 +173,3 @@

				            return template.instantiate(params, self, context or InstantiationContext())

				            return cast(

				                Dict[str, Any],

				                template.instantiate(params, self, context or InstantiationContext()),

khird commented

2025-10-28 19:34:54 +00:00

Please add a comment here explaining why fooling mypy with a cast() is necessary - if you know that template.instantiate() is going to return a dict[str, Any] then why not just annotate it as such in the base submodule? And if not, please explain why it's safe to proceed as if this were true, despite mypy's warning.

Please add a comment here explaining why fooling mypy with a `cast()` is necessary - if you know that `template.instantiate()` is going to return a `dict[str, Any]` then why not just annotate it as such in the base submodule? And if not, please explain why it's safe to proceed as if this were true, despite mypy's warning.

aditya commented

2025-10-30 13:21:04 +00:00

@khird
Replaced cast() with runtime validation that checks isinstance(result, dict) and raises ValueError with clear error message if YAML parses as anything other than a dict. This handles the case where YAML could be a list, string, or other type, making the code robust and type-safe without hiding potential issues.

@khird Replaced cast() with runtime validation that checks isinstance(result, dict) and raises ValueError with clear error message if YAML parses as anything other than a dict. This handles the case where YAML could be a list, string, or other type, making the code robust and type-safe without hiding potential issues.

khird marked this conversation as resolved

src/cleveragents/templates/inline_jinja_handler.py Outdated

						
				@@ -84,3 +83,3 @@

				        # If no templates, just parse

				        if "{%" not in yaml_content and "{{" not in yaml_content:

				            return yaml.safe_load(yaml_content)

				            return cast(Dict[str, Any], yaml.safe_load(yaml_content))

khird commented

2025-10-28 20:04:45 +00:00

It's possible for yaml to represent a list rather than a dict. If there's a reason we know that can't be the case for the particular specimen in yaml_content, it would be good to document that reason. If there isn't such a reason, then mypy's correct here and you ought to handle or check that case. I'm not going to spam a bunch of identical comments about cast everywhere, but please give it a general look.

It's possible for yaml to represent a list rather than a dict. If there's a reason we know that can't be the case for the particular specimen in `yaml_content`, it would be good to document that reason. If there isn't such a reason, then mypy's correct here and you ought to handle or check that case. I'm not going to spam a bunch of identical comments about `cast` everywhere, but please give it a general look.

aditya commented

2025-10-30 13:22:41 +00:00

@khird
Replaced cast() with runtime validation that checks isinstance(result, dict) and raises ValueError with clear error message if YAML parses as anything other than a dict. This handles the case where YAML could be a list, string, or other type.

@khird Replaced cast() with runtime validation that checks isinstance(result, dict) and raises ValueError with clear error message if YAML parses as anything other than a dict. This handles the case where YAML could be a list, string, or other type.

khird marked this conversation as resolved

src/cleveragents/templates/registry.py Outdated

						
				@@ -53,2 +53,3 @@

				        if template_type == TemplateType.STREAM:

				            template = StreamTemplate(name, template_type, definition)

				        else:

				        if template_type not in [TemplateType.AGENT, TemplateType.GRAPH, TemplateType.STREAM]:

khird commented

2025-10-28 20:11:09 +00:00

I think the elif/else construction worked better here, because now anyone adding a new template type has to add an if block and an element to this list. If you'd just left it as elif blocks, there wouldn't be the possibility to forget the second part.

I think the elif/else construction worked better here, because now anyone adding a new template type has to add an if block *and* an element to this list. If you'd just left it as elif blocks, there wouldn't be the possibility to forget the second part.

aditya commented

2025-10-30 13:24:08 +00:00

@khird
You're right elif/else is better. Adding a new template type only requires one elif block, avoiding the risk of forgetting to update both a validation list and the handling logic.

@khird You're right elif/else is better. Adding a new template type only requires one elif block, avoiding the risk of forgetting to update both a validation list and the handling logic.

khird marked this conversation as resolved

aditya added 2 commits 2025-10-29 14:25:44 +00:00

fix: security hardening, cross-version compatibility, and consistency improvements 9fd86a7fdc

fix: type safety and maintainability improvements 69ebb53acd

brent.edwards reviewed 2025-10-29 18:44:17 +00:00

src/cleveragents/templates/inline_jinja_handler.py Outdated

						
				@@ -226,4 +230,3 @@

				                parsed = {"_content": parsed}

				            parsed["__templates__"] = templates

				        return parsed

brent.edwards commented

2025-10-29 18:44:17 +00:00

mypy --strict src reports:

src/cleveragents/templates/inline_jinja_handler.py:232: error: Returning Any from function declared to return "dict[str, Any]"  [no-any-return]

`mypy --strict src` reports: ``` src/cleveragents/templates/inline_jinja_handler.py:232: error: Returning Any from function declared to return "dict[str, Any]" [no-any-return] ```

aditya commented

2025-10-30 13:34:36 +00:00

@brent.edwards
Added runtime validation checks and raise ValueError with clear messages.

@brent.edwards Added runtime validation checks and raise ValueError with clear messages.

brent.edwards requested changes 2025-10-29 18:44:36 +00:00

Dismissed

brent.edwards left a comment

I feel for you. You are so close to done!

brent.edwards requested changes 2025-10-29 18:46:02 +00:00

brent.edwards left a comment

Here is the exact result from mypy --strict src:

❯ mypy --strict src
src/cleveragents/templates/inline_jinja_handler.py:232: error: Returning Any from function declared to return "dict[str, Any]"  [no-any-return]
src/cleveragents/core/application.py:481: error: "object" has no attribute "register_all_templates"  [attr-defined]
Found 2 errors in 2 files (checked 48 source files)

Here is the exact result from `mypy --strict src`: ``` ❯ mypy --strict src src/cleveragents/templates/inline_jinja_handler.py:232: error: Returning Any from function declared to return "dict[str, Any]" [no-any-return] src/cleveragents/core/application.py:481: error: "object" has no attribute "register_all_templates" [attr-defined] Found 2 errors in 2 files (checked 48 source files) ```

aditya added 1 commit 2025-10-30 09:18:50 +00:00

fix: fix mypy --strict related errors 6be9073aa0

aditya added 1 commit 2025-10-30 13:01:12 +00:00

fix: remove cast with runtime type checking; replace all Dict[..] with dict[..] 170657113f

aditya added 1 commit 2025-10-30 13:31:44 +00:00

fix: fix some minor type annotation issues cfc83dae60

aditya added 1 commit 2025-10-30 15:10:33 +00:00

fix: handle empty/comment-only YAML gracefully 271d3a76d3

khird approved these changes 2025-10-31 14:40:09 +00:00

aditya force-pushed mypy-pylint-fixed from 271d3a76d3 to 6fff3968a6

2025-11-04 13:47:34 +00:00

Compare