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] ```

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] ```

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] ```

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] ```

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] ```

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

					
				@ -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.

src/cleveragents/langgraph/nodes.py

					
				@ -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.

src/cleveragents/reactive/config_parser.py

					
				@ -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.

src/cleveragents/reactive/config_parser.py

					
				@ -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.

src/cleveragents/reactive/stream_router.py

					
				@ -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?

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.

src/cleveragents/templates/enhanced_registry.py

					
				@ -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.

src/cleveragents/templates/inline_jinja_handler.py

					
				@ -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.

src/cleveragents/templates/registry.py

					
				@ -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.

This pull request can be merged automatically.

You are not authorized to merge this pull request.

View command line instructions

Checkout

From your project repository, check out a new branch and test the changes.

git fetch -u origin mypy-pylint-fixed:mypy-pylint-fixed

git switch mypy-pylint-fixed

Merge

Merge the changes and update on Forgejo.

Warning: The "Autodetect manual merge" setting is not enabled for this repository, you will have to mark this pull request as manually merged afterwards.

git switch master

git merge --no-ff mypy-pylint-fixed

git switch mypy-pylint-fixed

git rebase master

git switch master

git merge --ff-only mypy-pylint-fixed

git switch mypy-pylint-fixed

git rebase master

git switch master

git merge --no-ff mypy-pylint-fixed

git switch master

git merge --squash mypy-pylint-fixed