Add security#150
Draft
BenediktBurger wants to merge 9 commits into
Draft
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
pyleco/coordinators/coordinator.py (2)
100-141:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
multi_socketinjection bypasses the security configuration.When a caller passes a pre-built
multi_socket,security_configis still saved onself, the authenticator is still started/warned for, andZmqNodes are still created with the same config — but the externally supplied multi-socket may have been built with a different (or no)SecurityConfig. That mismatch can produce a "CURVE-configured" coordinator whose ROUTER socket isn’t actually using CURVE, which silently downgrades security.Either drop the
multi_socketparameter, or refuse the combination (e.g. raise if bothmulti_socketand a non-defaultsecurity_configare supplied), or document loudly that the caller must wire CURVE itself.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pyleco/coordinators/coordinator.py` around lines 100 - 141, The constructor currently allows a caller-supplied multi_socket while still applying/starting the Coordinator's SecurityConfig, which can silently mismatch; update Coordinator.__init__ to prevent this by rejecting inconsistent inputs: if multi_socket is provided, either require it to expose a matching security_config attribute equal to the provided SecurityConfig() (or the passed security_config) or raise a ValueError when multi_socket is supplied together with a non-default or non-matching security_config; reference ZmqMultiSocket, SecurityConfig, SecurityMode, start_authenticator, warn_insecure_mode and self.sock/selfecurity_config to locate the logic to change. Ensure the check runs before assigning self.sock/self.security_config and before calling start_authenticator or warn_insecure_mode.
471-479:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winWrap the long line to satisfy ruff E501.
CI fails:
Line too long (120 > 100).🛠️ Suggested fix
- ZmqNode(context=self.context, security_config=self.security_config), address=address, namespace=node + ZmqNode(context=self.context, security_config=self.security_config), + address=address, + namespace=node,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pyleco/coordinators/coordinator.py` around lines 471 - 479, The add_nodes method has a line exceeding the max length causing ruff E501; break the long call in add_nodes where you instantiate ZmqNode and call self.directory.add_node_sender so it wraps before 100 chars (for example split arguments across multiple lines), e.g. split the ZmqNode(...) construction and the keyword args onto separate lines in the add_nodes function to keep each line <=100 chars while preserving the call to ZmqNode(context=self.context, security_config=self.security_config) and address=address, namespace=node passed into self.directory.add_node_sender.
🟡 Minor comments (19)
README.md-126-136 (1)
126-136:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd a language identifier to the fenced CLI options block.
Line 126 uses an unlabeled fenced block, triggering MD040. Labeling it avoids lint noise.
💡 Suggested fix
-``` +```text --security-mode NONE|CURVE security mode (default: NONE) --server-secret-key KEY server secret key (Coordinator side) --server-public-key KEY server public key (Component side) --client-secret-key KEY client secret key --client-public-key KEY client public key --data-server-public-key KEY proxy server public key (data protocol) --authorized-keys-dir DIR directory of authorized client public keys --curve-any-authenticated accept any authenticated CURVE client --config PATH path to TOML config file</details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.In
@README.mdaround lines 126 - 136, Add a language identifier to the fenced
CLI options block to satisfy MD040; update the fence that currently surrounds
the options (the block containing lines like --security-mode,
--server-secret-key, --client-public-key, --authorized-keys-dir, --config) to
use a label such astext (orconsole) instead of an unlabeled ``` so the
CLI options block is explicitly marked and the lint warning is resolved.</details> </blockquote></details> <details> <summary>tests/coordinators/test_coordinator_curve.py-86-87 (1)</summary><blockquote> `86-87`: _⚠️ Potential issue_ | _🟡 Minor_ | _⚡ Quick win_ **Unused variable `c` breaks ruff/CI — drop the assignment where the instance is not used** Three test methods assign the constructed `Coordinator` to `c` but never reference it, triggering ruff F841 in all three cases (confirmed pipeline failures). <details> <summary>🐛 Proposed fix (apply to all three affected sites)</summary> ```diff - c = Coordinator( + Coordinator( namespace="N1", ... ) ``` </details> Also applies to: 102-103, 122-123 <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/coordinators/test_coordinator_curve.py` around lines 86 - 87, Remove the unused local variable assignment to c where a Coordinator instance is constructed (e.g., "c = Coordinator(...)" in the test methods); instead instantiate the Coordinator without assigning it (or drop the statement entirely) in the three locations that currently create an unused variable named c so the tests no longer trigger ruff F841 while preserving any side effects from Coordinator.__init__ if needed. ``` </details> </blockquote></details> <details> <summary>tests/core/test_keygen.py-32-32 (1)</summary><blockquote> `32-32`: _⚠️ Potential issue_ | _🟡 Minor_ | _⚡ Quick win_ **`pytest.TempPath` does not exist — confirmed mypy failures** The correct type annotation for the `tmp_path` fixture is `pathlib.Path`. `pytest.TempPath` is not a public pytest type. <details> <summary>🐛 Proposed fix (apply to all four affected method signatures)</summary> ```diff +import pathlib ... - def test_writes_key_files(self, tmp_path: pytest.TempPath) -> None: + def test_writes_key_files(self, tmp_path: pathlib.Path) -> None: - def test_default_name_is_key(self, tmp_path: pytest.TempPath) -> None: + def test_default_name_is_key(self, tmp_path: pathlib.Path) -> None: - def test_creates_output_dir(self, tmp_path: pytest.TempPath) -> None: + def test_creates_output_dir(self, tmp_path: pathlib.Path) -> None: - def test_secret_key_file_restricted_permissions(self, tmp_path: pytest.TempPath) -> None: + def test_secret_key_file_restricted_permissions(self, tmp_path: pathlib.Path) -> None: ``` </details> Also applies to: 47-47, 57-57, 69-69 <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/core/test_keygen.py` at line 32, Replace the incorrect pytest.TempPath annotation with pathlib.Path for the tmp_path parameter in the test functions (e.g., test_writes_key_files and the three other test_* functions in this file); update the function signatures to use tmp_path: pathlib.Path, add an import for pathlib.Path at the top if missing, and remove any references to pytest.TempPath so mypy accepts the fixture type. ``` </details> </blockquote></details> <details> <summary>AGENTS.md-275-278 (1)</summary><blockquote> `275-278`: _⚠️ Potential issue_ | _🟡 Minor_ | _⚡ Quick win_ **Hardcoded personal filesystem path should be replaced with a placeholder** Line 278 embeds a developer-specific absolute path (`/home/benediktb/Repositories/pyleco`). This should be replaced with a generic placeholder so the documentation is usable by all contributors. <details> <summary>📝 Proposed fix</summary> ```diff - PYTHONPATH=/home/benediktb/Repositories/pyleco .venv-docker/bin/python3 -m pytest + PYTHONPATH=/path/to/pyleco .venv-docker/bin/python3 -m pytest ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@AGENTS.md` around lines 275 - 278, Replace the hardcoded developer path '/home/benediktb/Repositories/pyleco' in the AGENTS.md example command with a generic placeholder (for example /path/to/repo or ${REPO_ROOT}) so other contributors can reuse it; update the PYTHONPATH example line that currently reads "PYTHONPATH=/home/benediktb/Repositories/pyleco .venv-docker/bin/python3 -m pytest" to use the chosen placeholder and keep the rest of the command unchanged. ``` </details> </blockquote></details> <details> <summary>tests/core/test_keygen.py-1-17 (1)</summary><blockquote> `1-17`: _⚠️ Potential issue_ | _🟡 Minor_ | _⚡ Quick win_ **Remove three unused imports** — `os`, `MagicMock`, and `KeyPair` are not used in the test file and cause F401 linting failures. <details> <summary>Proposed fix</summary> ```diff from __future__ import annotations -import os -from unittest.mock import MagicMock, patch +from unittest.mock import patch import pytest -from pyleco.core.security import KeyPair from pyleco.utils.keygen import main ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/core/test_keygen.py` around lines 1 - 17, Remove the unused imports causing F401: delete "os", "MagicMock", and "KeyPair" from the top of tests/core/test_keygen.py so the imports only include what the test actually uses (keep "patch" from unittest.mock, "pytest", and "from pyleco.utils.keygen import main"); this removes the lint failures while leaving FAKE_PUBLIC, FAKE_SECRET, and _mock_key_pair untouched. ``` </details> </blockquote></details> <details> <summary>tests/core/test_zap.py-91-91 (1)</summary><blockquote> `91-91`: _⚠️ Potential issue_ | _🟡 Minor_ | _⚡ Quick win_ **Drop the unused `result` assignment.** ruff fails with F841. Either assert against it or just call `start_authenticator(...)` directly. <details> <summary>🛠️ Suggested fix</summary> ```diff - result = start_authenticator(ctx, cfg) + start_authenticator(ctx, cfg) ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/core/test_zap.py` at line 91, The test assigns the return value of start_authenticator(ctx, cfg) to an unused variable `result`, causing a linter F841; fix by removing the unused assignment and call start_authenticator(ctx, cfg) directly, or if the return value matters, replace the assignment with an explicit assertion on start_authenticator(...) to use the result; locate the call to start_authenticator in the test function in tests/core/test_zap.py and update it accordingly. ``` </details> </blockquote></details> <details> <summary>pyleco/coordinators/coordinator.py-522-524 (1)</summary><blockquote> `522-524`: _⚠️ Potential issue_ | _🟡 Minor_ | _⚡ Quick win_ **Wrap the long import line to satisfy ruff E501.** CI fails: `Line too long (108 > 100)`. <details> <summary>🛠️ Suggested fix</summary> ```diff - from pyleco.utils.parser import parser, parse_command_line_parameters, build_security_config_from_kwargs # noqa: F811 + from pyleco.utils.parser import ( # noqa: F811 + parser, + parse_command_line_parameters, + build_security_config_from_kwargs, + ) ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pyleco/coordinators/coordinator.py` around lines 522 - 524, The import line inside main is exceeding the maximum line length; break the long from-import into a wrapped form (e.g., use parentheses and one identifier per line) so it complies with ruff E501: adjust the import in function main that currently imports parser, parse_command_line_parameters, and build_security_config_from_kwargs from pyleco.utils.parser to a multi-line import (or separate imports) to keep lines under 100 characters while preserving the exact names. ``` </details> </blockquote></details> <details> <summary>pyleco/utils/data_publisher.py-72-82 (1)</summary><blockquote> `72-82`: _⚠️ Potential issue_ | _🟡 Minor_ | _⚡ Quick win_ **Validate CURVE key material before opening the ZMQ socket.** Lines 72–79 create `self.socket` first and only then validate `client_key_pair` / `data_server_public_key`. If either is missing in CURVE mode, the `ValueError` is raised while a real ZMQ PUB socket is already allocated, so it leaks until the partially-constructed instance is garbage-collected (and `__del__` only catches it best-effort). Move validation above socket creation, or close on error. <details> <summary>🛡️ Suggested fix</summary> ```diff context = context or zmq.Context.instance() self.security_config = security_config - self.socket: zmq.Socket = context.socket(zmq.PUB) if security_config is not None and security_config.mode == SecurityMode.CURVE: if security_config.client_key_pair is None: raise ValueError("CURVE mode requires client_key_pair for DataPublisher") if security_config.data_server_public_key is None: raise ValueError("CURVE mode requires data_server_public_key for DataPublisher") + self.socket: zmq.Socket = context.socket(zmq.PUB) configure_curve_client(self.socket, security_config.client_key_pair, security_config.data_server_public_key) else: + self.socket: zmq.Socket = context.socket(zmq.PUB) warn_insecure_mode(address=f"{host}:{port}") self.socket.connect(f"tcp://{host}:{port}") ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pyleco/utils/data_publisher.py` around lines 72 - 82, Validate CURVE key material before allocating the ZMQ socket: when SecurityMode.CURVE is set, check security_config.client_key_pair and security_config.data_server_public_key and raise ValueError if missing prior to creating self.socket; only after validation create context.socket(zmq.PUB) and call configure_curve_client(self.socket, security_config.client_key_pair, security_config.data_server_public_key); keep the existing warn_insecure_mode branch unchanged for non-CURVE paths (alternatively, if you prefer minimal change, ensure any ValueError raised after creating self.socket closes it first, but moving validation above socket creation is preferred). ``` </details> </blockquote></details> <details> <summary>tests/utils/test_curve_integration.py-106-154 (1)</summary><blockquote> `106-154`: _⚠️ Potential issue_ | _🟡 Minor_ | _⚡ Quick win_ **Remove unused locals and the unused `Message` import to fix ruff.** CI fails with F841 on `handler` (lines 106, 121), `pub` (lines 134, 148) and F401 on `from pyleco.core.message import Message` (line 154). Either drop the assignments or assert on them — e.g. that the components were constructed without raising. <details> <summary>🛠️ Suggested fix</summary> ```diff - handler = MessageHandler( - name="test", security_config=cfg, context=mock_context - ) + MessageHandler(name="test", security_config=cfg, context=mock_context) @@ - handler = MessageHandler(name="test", context=mock_context) + MessageHandler(name="test", context=mock_context) @@ - pub = DataPublisher( - full_name="test", security_config=cfg, context=mock_context - ) + DataPublisher(full_name="test", security_config=cfg, context=mock_context) @@ - pub = DataPublisher(full_name="test", context=mock_context) + DataPublisher(full_name="test", context=mock_context) @@ def test_curve_server(self) -> None: - from pyleco.core.message import Message - class FakeSocket: ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/utils/test_curve_integration.py` around lines 106 - 154, Remove the unused local assignments and the unused import to satisfy ruff: either drop the "handler =" and "pub =" assignments and construct the objects directly (MessageHandler(...), DataPublisher(...)) or keep the assignments but add a lightweight assertion like "assert handler" / "assert pub" to show they are used; also remove the unused "from pyleco.core.message import Message" import (or use Message in an assertion) so F401 is resolved. Ensure the changes apply to the tests that instantiate MessageHandler and DataPublisher and to the unused Message import. ``` </details> </blockquote></details> <details> <summary>pyleco/core/curve.py-15-26 (1)</summary><blockquote> `15-26`: _⚠️ Potential issue_ | _🟡 Minor_ | _⚡ Quick win_ **Add `zmq.Socket` annotation on `socket` parameters.** mypy fails with `[no-untyped-def]` on both helpers because the `socket` parameter is untyped. Annotating with `zmq.Socket` also documents the contract—these functions mutate ZMQ socket attributes—and aligns with the standard type annotations used throughout the codebase. <details> <summary>🛠️ Suggested fix</summary> ```diff +import zmq + from pyleco.core.security import KeyPair @@ -def configure_curve_server(socket, server_key_pair: KeyPair) -> None: +def configure_curve_server(socket: zmq.Socket, server_key_pair: KeyPair) -> None: socket.curve_server = True socket.curve_secretkey = server_key_pair.secret_key.encode() @@ -def configure_curve_client( - socket, client_key_pair: KeyPair, server_public_key: str -) -> None: +def configure_curve_client( + socket: zmq.Socket, client_key_pair: KeyPair, server_public_key: str +) -> None: ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pyleco/core/curve.py` around lines 15 - 26, Annotate the untyped socket parameters with zmq.Socket in both helpers: change the signatures of configure_curve_server and configure_curve_client to accept socket: zmq.Socket, and add the necessary import for zmq (or the zmq.Socket type) at the top of the module so mypy recognizes the type; keep the internal mutations (setting curve_server, curve_secretkey, curve_serverkey, curve_publickey) unchanged. ``` </details> </blockquote></details> <details> <summary>pyleco/core/zap.py-35-63 (1)</summary><blockquote> `35-63`: _⚠️ Potential issue_ | _🟡 Minor_ | _⚡ Quick win_ **Add type annotations to function parameters and return types.** The mypy configuration enforces `disallow_untyped_defs = true`, so `start_authenticator` and `stop_authenticator` must have complete type annotations. Add `context: zmq.Context` parameter type and `ThreadAuthenticator` return type to `start_authenticator`, and `authenticator: ThreadAuthenticator` parameter type to `stop_authenticator`. Since zmq imports are intentionally lazy (inside functions), use a `TYPE_CHECKING` block at the top of the file to provide type information without affecting runtime behavior: <details> <summary>Suggested fix</summary> ```diff from __future__ import annotations import logging +from typing import TYPE_CHECKING, Dict, Optional -from typing import Dict, Optional from pyleco.core.security import SecurityConfig, SecurityMode, load_authorized_keys +if TYPE_CHECKING: + import zmq + from zmq.auth.thread import ThreadAuthenticator + __all__ = [ "start_authenticator", "stop_authenticator", ] ``` Then update the function signatures: ```diff -def start_authenticator(context, security_config: SecurityConfig): +def start_authenticator( + context: zmq.Context, security_config: SecurityConfig +) -> ThreadAuthenticator: ``` ```diff -def stop_authenticator(authenticator) -> None: +def stop_authenticator(authenticator: ThreadAuthenticator) -> None: ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pyleco/core/zap.py` around lines 35 - 63, Add static type hints by importing TYPE_CHECKING and, inside a TYPE_CHECKING block, import zmq and ThreadAuthenticator (from zmq.auth.thread); then annotate start_authenticator as start_authenticator(context: "zmq.Context", security_config: SecurityConfig) -> "ThreadAuthenticator" and annotate stop_authenticator as stop_authenticator(authenticator: "ThreadAuthenticator") -> None, using quoted forward references so the runtime lazy imports inside the functions remain unchanged. ``` </details> </blockquote></details> <details> <summary>tests/core/test_security.py-122-122 (1)</summary><blockquote> `122-122`: _⚠️ Potential issue_ | _🟡 Minor_ | _⚡ Quick win_ **`pytest.TempPath` does not exist — replace with `pathlib.Path` throughout.** All `tmp_path` fixture parameters typed as `pytest.TempPath` trigger mypy `name-defined` errors. pytest's `tmp_path` fixture yields `pathlib.Path`. <details> <summary>🔧 Proposed fix</summary> Add at top of file: ```diff +import pathlib ``` Then replace every occurrence: ```diff - def test_from_directory(self, tmp_path: pytest.TempPath) -> None: + def test_from_directory(self, tmp_path: pathlib.Path) -> None: ``` (Apply the same change to all 10 affected methods.) </details> Also applies to: 136-136, 160-160, 169-169, 210-210, 223-223, 235-235, 244-244, 255-255, 264-264 <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/core/test_security.py` at line 122, Replace the incorrect type annotation pytest.TempPath with pathlib.Path across the test file: add "from pathlib import Path" to the imports and update all tmp_path parameters (e.g., in test_from_directory and the other test_* functions reported) to use Path instead of pytest.TempPath so the tmp_path fixture is typed correctly and mypy errors are resolved. ``` </details> </blockquote></details> <details> <summary>tests/core/test_security.py-3-4 (1)</summary><blockquote> `3-4`: _⚠️ Potential issue_ | _🟡 Minor_ | _⚡ Quick win_ **Remove unused imports and the unused `original` variable — causing multiple ruff pipeline failures.** - `os` (line 3) and `tempfile` (line 4) are never referenced. - `import zmq` inside `test_returns_keypair` (line 92) is only used for the availability check; ruff recommends `importlib.util.find_spec` for this pattern. - `import importlib` (line 101) is never used — `import pyleco.core.security as sec` uses the normal import machinery. - `original` (line 104) is assigned but never read or restored. <details> <summary>🔧 Proposed fix</summary> ```diff from __future__ import annotations -import os -import tempfile - import pytest ``` ```diff def test_returns_keypair(self) -> None: - try: - import zmq - except ImportError: - pytest.skip("zmq not installed") + if importlib.util.find_spec("zmq") is None: + pytest.skip("zmq not installed") ``` ```diff def test_import_error_without_zmq(self) -> None: - import importlib import pyleco.core.security as sec - - original = sec.__dict__.get("zmq") import builtins ``` And add at the top: ```diff +import importlib.util ``` </details> Also applies to: 92-92, 101-101, 104-104 <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/core/test_security.py` around lines 3 - 4, Remove the unused top-level imports and variables causing ruff failures: delete the unused imports "os" and "tempfile" at the top, replace the in-test availability check in test_returns_keypair by using importlib.util.find_spec instead of importing zmq, remove the unused "import importlib" since the test imports pyleco.core.security as sec directly, and delete the unused "original" variable (and any assignment/restoration of it) so no unused variable remains; look for these symbols in the file (test_returns_keypair, the zmq import, the importlib import, and the original variable) and update them accordingly. ``` </details> </blockquote></details> <details> <summary>tests/integration_tests/test_network_topology.py-201-201 (1)</summary><blockquote> `201-201`: _⚠️ Potential issue_ | _🟡 Minor_ | _⚡ Quick win_ **Remove assignments to unused variables `remote_comp` and `e` — causing ruff F841 linting failures.** <details> <summary>🔧 Proposed fix</summary> Line 201 — also note that `sign_in()` is never called, so the component won't actually register with the coordinator (the `with` block only opens the socket): ```diff - with Communicator(name="RemoteComponent", port=comm._ports[2]) as remote_comp: + with Communicator(name="RemoteComponent", port=comm._ports[2]) as remote_comp: # noqa: F841 + remote_comp.sign_in() sleep(TALKING_TIME) ``` Or, if the intent is only to open a socket without registering: ```diff - with Communicator(name="RemoteComponent", port=comm._ports[2]) as remote_comp: + with Communicator(name="RemoteComponent", port=comm._ports[2]): sleep(TALKING_TIME) ``` Line 384: ```diff - except Exception as e: + except Exception: ``` </details> Also applies to: 384-384 <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/integration_tests/test_network_topology.py` at line 201, The test creates a Communicator instance but assigns it to an unused variable remote_comp and catches an unused exception variable e, causing ruff F841; either remove the assignments (use with Communicator(name="RemoteComponent", port=comm._ports[2]): and except: without binding) or replace them with underscores (with Communicator(... ) as _, except Exception as _). Also, if the test intends the component to register with the coordinator, call the instance's sign_in() method (i.e., call sign_in() on the Communicator created by Communicator/name RemoteComponent) so the component actually registers rather than merely opening the socket. Ensure changes reference Communicator, remote_comp, e, and sign_in(). ``` </details> </blockquote></details> <details> <summary>tests/integration_tests/test_curve_security.py-3-3 (1)</summary><blockquote> `3-3`: _⚠️ Potential issue_ | _🟡 Minor_ | _⚡ Quick win_ **Remove unused imports `logging` and `KeyPair` — causing linting pipeline failures.** Neither `logging` nor `KeyPair` is referenced anywhere in this file. <details> <summary>🔧 Proposed fix</summary> ```diff -import logging import socket import threading ``` ```diff -from pyleco.core.security import KeyPair, SecurityConfig, SecurityMode, generate_key_pair +from pyleco.core.security import SecurityConfig, SecurityMode, generate_key_pair ``` </details> Also applies to: 13-13 <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/integration_tests/test_curve_security.py` at line 3, Remove the unused imports "logging" and "KeyPair" from the test module's import section (the top-level import statements that currently import logging and KeyPair), and also remove any duplicate unused import of KeyPair further down in the same file; simply delete those import entries so the test file no longer imports unused symbols and linting will pass. ``` </details> </blockquote></details> <details> <summary>tests/core/test_parser_security.py-5-5 (1)</summary><blockquote> `5-5`: _⚠️ Potential issue_ | _🟡 Minor_ | _⚡ Quick win_ **Remove unused `SecurityConfig` import — causes ruff F401 linting failure.** `SecurityConfig` is imported but never referenced in this file. <details> <summary>🔧 Proposed fix</summary> ```diff -from pyleco.core.security import SecurityConfig, SecurityMode +from pyleco.core.security import SecurityMode ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/core/test_parser_security.py` at line 5, Remove the unused import SecurityConfig from the import line that currently reads "from pyleco.core.security import SecurityConfig, SecurityMode": keep only SecurityMode (i.e., import SecurityMode and drop SecurityConfig) so the unused-symbol lint (ruff F401) is resolved; update the import statement where SecurityConfig is referenced to remove that symbol. ``` </details> </blockquote></details> <details> <summary>pyleco/coordinators/proxy_server.py-121-122 (1)</summary><blockquote> `121-122`: _⚠️ Potential issue_ | _🟡 Minor_ | _⚡ Quick win_ **Remove unused variable `exc` — ruff F841 linting failure.** `log.exception` captures the current exception automatically; `exc` is never referenced. <details> <summary>🔧 Proposed fix</summary> ```diff - except Exception as exc: - log.exception("Some other exception on proxy happened.") + except Exception: + log.exception("Some other exception on proxy happened.") ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pyleco/coordinators/proxy_server.py` around lines 121 - 122, The except block in proxy_server.py currently declares an unused variable `exc` (except Exception as exc) causing a lint F841; change the handler to `except Exception:` (remove `exc`) so `log.exception("Some other exception on proxy happened.")` continues to capture the current exception automatically, leaving the rest of the surrounding logic (the `log.exception` call) unchanged. ``` </details> </blockquote></details> <details> <summary>tests/core/test_parser_security.py-149-149 (1)</summary><blockquote> `149-149`: _⚠️ Potential issue_ | _🟡 Minor_ | _⚡ Quick win_ **`pytest.TempPath` does not exist — use `pathlib.Path` for `tmp_path` fixture parameters.** pytest's `tmp_path` fixture yields a `pathlib.Path`, not `pytest.TempPath`. This causes a mypy `name-defined` error breaking the Static Type Checking CI job. <details> <summary>🔧 Proposed fix</summary> ```diff - def test_removes_config_key(self, tmp_path: pytest.TempPath) -> None: + def test_removes_config_key(self, tmp_path: pathlib.Path) -> None: ``` Also add at the top of the file: ```diff +import pathlib ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/core/test_parser_security.py` at line 149, The test function test_removes_config_key is annotated with the non-existent type pytest.TempPath which breaks type checking; change the parameter type of tmp_path to pathlib.Path and add an import for pathlib.Path at the top of the test module (or import pathlib and reference pathlib.Path) so the tmp_path fixture is correctly typed and mypy passes. ``` </details> </blockquote></details> <details> <summary>pyleco/core/security.py-3-4 (1)</summary><blockquote> `3-4`: _⚠️ Potential issue_ | _🟡 Minor_ | _⚡ Quick win_ **Remove unused imports `os` and `field` — causing ruff F401 linting pipeline failures.** `os` is never called, and `field` from `dataclasses` is never used (all `SecurityConfig` defaults use plain Python literals). <details> <summary>🔧 Proposed fix</summary> ```diff -import os -from dataclasses import dataclass, field +from dataclasses import dataclass from enum import Enum ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pyleco/core/security.py` around lines 3 - 4, Remove the unused imports causing ruff F401 by deleting "os" and "field" from the imports at top of the module; keep only the needed "dataclass" import so SecurityConfig (and any other dataclasses in this file) use plain Python literals for defaults without importing dataclasses.field or os. Ensure no other code in this module references os or field before removing them. ``` </details> </blockquote></details> </blockquote></details> <details> <summary>🧹 Nitpick comments (4)</summary><blockquote> <details> <summary>tests/utils/test_curve_integration.py (1)</summary><blockquote> `60-65`: _💤 Low value_ **Autouse `mock_zmq` fixture is a no-op when `zmq` is already imported.** `_setup_zmq_mock` only injects mocks for modules **not** in `sys.modules`, so as soon as any earlier test (or import) brings in real `pyzmq`, this fixture silently does nothing — the tests then accidentally exercise real ZMQ. Since the actual assertions rely on `patch("pyleco.utils.X.configure_curve_client", ...)`, the fixture isn’t protecting anything; either drop it or unconditionally replace the entries (saving/restoring the originals). <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/utils/test_curve_integration.py` around lines 60 - 65, The autouse fixture mock_zmq is ineffective when pyzmq is already imported because _setup_zmq_mock only injects when modules are absent; update mock_zmq to unconditionally replace the target entries (save originals from sys.modules/attributes, insert mocks) and restore them in _teardown_zmq_mock, or remove the fixture entirely and rely on explicit patching (e.g. patch("pyleco.utils.X.configure_curve_client")) in tests; ensure you reference and modify the same symbols used by tests (_setup_zmq_mock, _teardown_zmq_mock, mock_zmq, and any sys.modules keys) so real ZMQ is never accidentally used. ``` </details> </blockquote></details> <details> <summary>tests/core/test_zap.py (1)</summary><blockquote> `99-117`: _⚡ Quick win_ **Add coverage that the authenticator is stopped on the missing-keys ValueError.** `start_authenticator` calls `authenticator.start()` before validating the key set, so if no keys are configured and `curve_any_authenticated` is false the spawned auth thread leaks. This test currently only asserts the `ValueError`; please also assert `mock_auth.stop.assert_called_once()` once the implementation is fixed (see related comment on `pyleco/core/zap.py`). <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/core/test_zap.py` around lines 99 - 117, The test must also verify that the spawned authenticator is stopped when start_authenticator raises the "authorized_keys" ValueError; update test_no_keys_raises_value_error to, after asserting pytest.raises(ValueError, match="authorized_keys") for start_authenticator(ctx, cfg), assert that the mocked ThreadAuthenticator instance's stop method was called exactly once (mock_auth.stop.assert_called_once()); ensure you reference the mocked authenticator returned by mock_zmq_auth.ThreadAuthenticator (mock_auth) so the test validates authenticator.start() does not leak by confirming mock_auth.stop() was invoked on error. ``` </details> </blockquote></details> <details> <summary>pyleco/core/curve.py (1)</summary><blockquote> `28-39`: _⚡ Quick win_ **Brittle host parsing in `warn_insecure_mode`.** `address.rsplit(":", 1)[0]` only handles a `host:port` shape, but several callers (e.g. coordinator/data publisher) build addresses from `gethostname()` or user-supplied strings that may include `tcp://` prefixes, bracketed IPv6 (`[::1]:1234`), or no port at all. A bare `::1` becomes `::` after rsplit, so loopback detection silently misses IPv6 and protocol-prefixed strings, producing false "insecure" warnings (or hiding real ones). Consider parsing with `urllib.parse.urlsplit` (after prefixing `tcp://` if missing) and using `ipaddress.ip_address(...).is_loopback` to also catch the rest of `127.0.0.0/8` and `::1`. <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pyleco/core/curve.py` around lines 28 - 39, The host-extraction in warn_insecure_mode is brittle (uses address.rsplit) and misparses protocol prefixes, bracketed IPv6, or bare IPv6 addresses; replace the ad-hoc parse by normalizing the string (ensure a scheme like "tcp://" is present if missing), parse with urllib.parse.urlsplit to extract netloc/path, strip surrounding "[]" from IPv6, then validate using ipaddress.ip_address(...).is_loopback (also catch IPv4 loopback CIDR like 127.0.0.0/8) to decide whether to warn; update warn_insecure_mode to use urlsplit + ipaddress and handle ValueError from ip_address to treat unknown hosts as non-loopback. ``` </details> </blockquote></details> <details> <summary>tests/integration_tests/test_network_topology.py (1)</summary><blockquote> `78-133`: _⚡ Quick win_ **Coordinator threads all start simultaneously — the inter-creation sleeps are no-ops.** Threads are only appended to `threads` during lines 80–127; the `for thread in threads: thread.start()` at lines 129–131 starts all four coordinators at exactly the same moment. The `sleep(TALKING_TIME)` calls between `threads.append(...)` lines have no effect on startup ordering, so N2, N3, and N4 race to connect to their dependencies before those coordinators have bound. ZMQ's reconnect behavior usually absorbs this in practice, but the fixture design is misleading and could produce flaky timing failures in slow CI environments. <details> <summary>💡 Suggested fix: start each coordinator thread before creating the next one</summary> ```diff - threads = [] - - threads.append( - threading.Thread(target=start_coordinator, - kwargs=dict(namespace="N1", port=ports[0], stop_event=stop_events[0])) - ) - sleep(TALKING_TIME) - threads.append( - threading.Thread(target=start_coordinator, - kwargs=dict(namespace="N2", port=ports[1], - coordinators=[f"localhost:{ports[0]}"], - stop_event=stop_events[1])) - ) - # ... same pattern for N3, N4 ... - for thread in threads: - thread.daemon = True - thread.start() + threads = [] + for kwargs, event in [ + (dict(namespace="N1", port=ports[0], stop_event=stop_events[0]), None), + (dict(namespace="N2", port=ports[1], coordinators=[f"localhost:{ports[0]}"], + stop_event=stop_events[1]), None), + (dict(namespace="N3", port=ports[2], coordinators=[f"localhost:{ports[1]}"], + stop_event=stop_events[2]), None), + (dict(namespace="N4", port=ports[3], coordinators=[f"localhost:{ports[0]}"], + stop_event=stop_events[3]), None), + ]: + t = threading.Thread(target=start_coordinator, kwargs=kwargs) + t.daemon = True + t.start() + threads.append(t) + sleep(TALKING_TIME) ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/integration_tests/test_network_topology.py` around lines 78 - 133, The threads are appended but all started together, so the inter-append sleep calls do nothing; change the pattern to start each coordinator thread immediately after creating it to ensure a coordinator binds before the next tries to connect. For each threading.Thread(...) that calls start_coordinator (namespaces "N1"/"N2"/"N3"/"N4"), set thread.daemon = True and call thread.start() right after appending (and then sleep(TALKING_TIME) if you still want pacing) instead of waiting to start them all later; keep using the same stop_events and ports/namespace kwargs so the rest of the test logic is unchanged. ``` </details> </blockquote></details> </blockquote></details> <details> <summary>🤖 Prompt for all review comments with AI agents</summary>Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.Inline comments:
In@pyleco/coordinators/proxy_server.py:
- Around line 84-87: security_config.server_key_pair is Optional but currently
passed unguarded to configure_curve_server, causing mypy errors and a potential
AttributeError; update the CURVE branch in the proxy server so you only call
configure_curve_server(s, ...) and configure_curve_server(p, ...) when
security_config.server_key_pair is not None (mirror the guard used in
ExtendedMessageHandler), and still call start_authenticator(context,
security_config) as before; reference symbols: security_config,
SecurityMode.CURVE, server_key_pair, configure_curve_server,
start_authenticator, and ExtendedMessageHandler.In
@pyleco/core/__init__.py:
- Line 41: The import statement "from . import security, curve, zap, config"
should be moved to the module top-level import block to satisfy Ruff E402;
locate that statement in pyleco/core/init.py and place it with the other top
imports (ensuring isort order and grouping), then run black/isort/ruff to verify
the import is at file top and lint errors are gone.In
@pyleco/core/zap.py:
- Around line 35-56: The start_authenticator function currently starts
ThreadAuthenticator before validating CURVE configuration, which leaks the ZAP
thread if neither curve_any_authenticated nor authorized keys exist; fix by
validating/obtaining authorized_keys (call load_authorized_keys and check
security_config.curve_any_authenticated) before instantiating/starting
ThreadAuthenticator, and only create/start the ThreadAuthenticator after
validation, or if you must start it earlier ensure you call authenticator.stop()
in the error path (catch the ValueError and stop/cleanup the
ThreadAuthenticator) and re-raise; refer to start_authenticator,
ThreadAuthenticator, load_authorized_keys, and _CredentialsProvider to locate
the code to change.In
@pyleco/utils/communicator.py:
- Around line 98-102: The CURVE branch calls configure_curve_client with
SecurityConfig fields that are Optional, causing runtime TypeError and mypy
failures; update the check in Communicator (where self.security_config is
inspected) to ensure self.security_config.client_key_pair and
self.security_config.server_public_key are not None before calling
configure_curve_client, and if they are missing either raise a clear ValueError
or log an explicit error and avoid calling configure_curve_client (apply the
identical change in pyleco/utils/message_handler.py around the same CURVE
branch), referencing SecurityMode.CURVE, client_key_pair, server_public_key,
configure_curve_client, and warn_insecure_mode to locate the code to change.In
@pyleco/utils/coordinator_utils.py:
- Around line 94-97: Null checks are missing before passing Optional keypairs
into configure_curve_server/configure_curve_client; update the call sites (the
block creating self._sock where configure_curve_server(self._sock,
security_config.server_key_pair) is invoked, and the block where
configure_curve_client is called with self._security_config.client_key_pair and
self._security_config.server_public_key) to mirror ExtendedMessageHandler:
validate that the keypair(s) and server_public_key are not None and raise a
ValueError with a clear message if they are, then call
configure_curve_server/configure_curve_client with the now-non-None values so
mypy and runtime AttributeError risks are eliminated.In
@pyleco/utils/keygen.py:
- Around line 20-23: The code incorrectly unpacks a non-iterable KeyPair
returned from generate_key_pair() causing a TypeError; change the unpacking to
capture the KeyPair object (e.g., key_pair = generate_key_pair()) and then
reference its attributes (key_pair.public_key and key_pair.secret_key) when
printing or using them so mypy and runtime agree with the KeyPair type.In
@tests/core/test_config.py:
- Around line 1-83: Replace the incorrect pytest.TempPath type annotations with
pathlib.Path and add "from pathlib import Path" to the imports; update all
method signatures that use tmp_path (e.g.,
TestFindConfigFile.test_returns_none_when_no_files_exist,
test_returns_first_found_file, test_returns_second_if_first_missing,
test_default_search_paths_with_no_files, test_expands_user_home and
TestLoadConfig.test_load_specific_path,
test_returns_empty_dict_when_no_file_found, test_toml_content_parsed_correctly,
test_load_from_search_paths, test_load_first_from_search_paths) to use tmp_path:
Path so mypy no longer complains about the non-exported pytest.TempPath name.
Outside diff comments:
In@pyleco/coordinators/coordinator.py:
- Around line 100-141: The constructor currently allows a caller-supplied
multi_socket while still applying/starting the Coordinator's SecurityConfig,
which can silently mismatch; update Coordinator.init to prevent this by
rejecting inconsistent inputs: if multi_socket is provided, either require it to
expose a matching security_config attribute equal to the provided
SecurityConfig() (or the passed security_config) or raise a ValueError when
multi_socket is supplied together with a non-default or non-matching
security_config; reference ZmqMultiSocket, SecurityConfig, SecurityMode,
start_authenticator, warn_insecure_mode and self.sock/selfecurity_config to
locate the logic to change. Ensure the check runs before assigning
self.sock/self.security_config and before calling start_authenticator or
warn_insecure_mode.- Around line 471-479: The add_nodes method has a line exceeding the max length
causing ruff E501; break the long call in add_nodes where you instantiate
ZmqNode and call self.directory.add_node_sender so it wraps before 100 chars
(for example split arguments across multiple lines), e.g. split the ZmqNode(...)
construction and the keyword args onto separate lines in the add_nodes function
to keep each line <=100 chars while preserving the call to
ZmqNode(context=self.context, security_config=self.security_config) and
address=address, namespace=node passed into self.directory.add_node_sender.
Minor comments:
In@AGENTS.md:
- Around line 275-278: Replace the hardcoded developer path
'/home/benediktb/Repositories/pyleco' in the AGENTS.md example command with a
generic placeholder (for example /path/to/repo or ${REPO_ROOT}) so other
contributors can reuse it; update the PYTHONPATH example line that currently
reads "PYTHONPATH=/home/benediktb/Repositories/pyleco .venv-docker/bin/python3
-m pytest" to use the chosen placeholder and keep the rest of the command
unchanged.In
@pyleco/coordinators/coordinator.py:
- Around line 522-524: The import line inside main is exceeding the maximum line
length; break the long from-import into a wrapped form (e.g., use parentheses
and one identifier per line) so it complies with ruff E501: adjust the import in
function main that currently imports parser, parse_command_line_parameters, and
build_security_config_from_kwargs from pyleco.utils.parser to a multi-line
import (or separate imports) to keep lines under 100 characters while preserving
the exact names.In
@pyleco/coordinators/proxy_server.py:
- Around line 121-122: The except block in proxy_server.py currently declares an
unused variableexc(except Exception as exc) causing a lint F841; change the
handler toexcept Exception:(removeexc) solog.exception("Some other exception on proxy happened.")continues to capture the current exception
automatically, leaving the rest of the surrounding logic (thelog.exception
call) unchanged.In
@pyleco/core/curve.py:
- Around line 15-26: Annotate the untyped socket parameters with zmq.Socket in
both helpers: change the signatures of configure_curve_server and
configure_curve_client to accept socket: zmq.Socket, and add the necessary
import for zmq (or the zmq.Socket type) at the top of the module so mypy
recognizes the type; keep the internal mutations (setting curve_server,
curve_secretkey, curve_serverkey, curve_publickey) unchanged.In
@pyleco/core/security.py:
- Around line 3-4: Remove the unused imports causing ruff F401 by deleting "os"
and "field" from the imports at top of the module; keep only the needed
"dataclass" import so SecurityConfig (and any other dataclasses in this file)
use plain Python literals for defaults without importing dataclasses.field or
os. Ensure no other code in this module references os or field before removing
them.In
@pyleco/core/zap.py:
- Around line 35-63: Add static type hints by importing TYPE_CHECKING and,
inside a TYPE_CHECKING block, import zmq and ThreadAuthenticator (from
zmq.auth.thread); then annotate start_authenticator as
start_authenticator(context: "zmq.Context", security_config: SecurityConfig) ->
"ThreadAuthenticator" and annotate stop_authenticator as
stop_authenticator(authenticator: "ThreadAuthenticator") -> None, using quoted
forward references so the runtime lazy imports inside the functions remain
unchanged.In
@pyleco/utils/data_publisher.py:
- Around line 72-82: Validate CURVE key material before allocating the ZMQ
socket: when SecurityMode.CURVE is set, check security_config.client_key_pair
and security_config.data_server_public_key and raise ValueError if missing prior
to creating self.socket; only after validation create context.socket(zmq.PUB)
and call configure_curve_client(self.socket, security_config.client_key_pair,
security_config.data_server_public_key); keep the existing warn_insecure_mode
branch unchanged for non-CURVE paths (alternatively, if you prefer minimal
change, ensure any ValueError raised after creating self.socket closes it first,
but moving validation above socket creation is preferred).In
@README.md:
- Around line 126-136: Add a language identifier to the fenced CLI options block
to satisfy MD040; update the fence that currently surrounds the options (the
block containing lines like --security-mode, --server-secret-key,
--client-public-key, --authorized-keys-dir, --config) to use a label such as
text (orconsole) instead of an unlabeled ``` so the CLI options block is
explicitly marked and the lint warning is resolved.In
@tests/coordinators/test_coordinator_curve.py:
- Around line 86-87: Remove the unused local variable assignment to c where a
Coordinator instance is constructed (e.g., "c = Coordinator(...)" in the test
methods); instead instantiate the Coordinator without assigning it (or drop the
statement entirely) in the three locations that currently create an unused
variable named c so the tests no longer trigger ruff F841 while preserving any
side effects from Coordinator.init if needed.In
@tests/core/test_keygen.py:
- Line 32: Replace the incorrect pytest.TempPath annotation with pathlib.Path
for the tmp_path parameter in the test functions (e.g., test_writes_key_files
and the three other test_* functions in this file); update the function
signatures to use tmp_path: pathlib.Path, add an import for pathlib.Path at the
top if missing, and remove any references to pytest.TempPath so mypy accepts the
fixture type.- Around line 1-17: Remove the unused imports causing F401: delete "os",
"MagicMock", and "KeyPair" from the top of tests/core/test_keygen.py so the
imports only include what the test actually uses (keep "patch" from
unittest.mock, "pytest", and "from pyleco.utils.keygen import main"); this
removes the lint failures while leaving FAKE_PUBLIC, FAKE_SECRET, and
_mock_key_pair untouched.In
@tests/core/test_parser_security.py:
- Line 5: Remove the unused import SecurityConfig from the import line that
currently reads "from pyleco.core.security import SecurityConfig, SecurityMode":
keep only SecurityMode (i.e., import SecurityMode and drop SecurityConfig) so
the unused-symbol lint (ruff F401) is resolved; update the import statement
where SecurityConfig is referenced to remove that symbol.- Line 149: The test function test_removes_config_key is annotated with the
non-existent type pytest.TempPath which breaks type checking; change the
parameter type of tmp_path to pathlib.Path and add an import for pathlib.Path at
the top of the test module (or import pathlib and reference pathlib.Path) so the
tmp_path fixture is correctly typed and mypy passes.In
@tests/core/test_security.py:
- Line 122: Replace the incorrect type annotation pytest.TempPath with
pathlib.Path across the test file: add "from pathlib import Path" to the imports
and update all tmp_path parameters (e.g., in test_from_directory and the other
test_* functions reported) to use Path instead of pytest.TempPath so the
tmp_path fixture is typed correctly and mypy errors are resolved.- Around line 3-4: Remove the unused top-level imports and variables causing
ruff failures: delete the unused imports "os" and "tempfile" at the top, replace
the in-test availability check in test_returns_keypair by using
importlib.util.find_spec instead of importing zmq, remove the unused "import
importlib" since the test imports pyleco.core.security as sec directly, and
delete the unused "original" variable (and any assignment/restoration of it) so
no unused variable remains; look for these symbols in the file
(test_returns_keypair, the zmq import, the importlib import, and the original
variable) and update them accordingly.In
@tests/core/test_zap.py:
- Line 91: The test assigns the return value of start_authenticator(ctx, cfg) to
an unused variableresult, causing a linter F841; fix by removing the unused
assignment and call start_authenticator(ctx, cfg) directly, or if the return
value matters, replace the assignment with an explicit assertion on
start_authenticator(...) to use the result; locate the call to
start_authenticator in the test function in tests/core/test_zap.py and update it
accordingly.In
@tests/integration_tests/test_curve_security.py:
- Line 3: Remove the unused imports "logging" and "KeyPair" from the test
module's import section (the top-level import statements that currently import
logging and KeyPair), and also remove any duplicate unused import of KeyPair
further down in the same file; simply delete those import entries so the test
file no longer imports unused symbols and linting will pass.In
@tests/integration_tests/test_network_topology.py:
- Line 201: The test creates a Communicator instance but assigns it to an unused
variable remote_comp and catches an unused exception variable e, causing ruff
F841; either remove the assignments (use with
Communicator(name="RemoteComponent", port=comm._ports[2]): and except: without
binding) or replace them with underscores (with Communicator(... ) as _, except
Exception as _). Also, if the test intends the component to register with the
coordinator, call the instance's sign_in() method (i.e., call sign_in() on the
Communicator created by Communicator/name RemoteComponent) so the component
actually registers rather than merely opening the socket. Ensure changes
reference Communicator, remote_comp, e, and sign_in().In
@tests/utils/test_curve_integration.py:
- Around line 106-154: Remove the unused local assignments and the unused import
to satisfy ruff: either drop the "handler =" and "pub =" assignments and
construct the objects directly (MessageHandler(...), DataPublisher(...)) or keep
the assignments but add a lightweight assertion like "assert handler" / "assert
pub" to show they are used; also remove the unused "from pyleco.core.message
import Message" import (or use Message in an assertion) so F401 is resolved.
Ensure the changes apply to the tests that instantiate MessageHandler and
DataPublisher and to the unused Message import.
Nitpick comments:
In@pyleco/core/curve.py:
- Around line 28-39: The host-extraction in warn_insecure_mode is brittle (uses
address.rsplit) and misparses protocol prefixes, bracketed IPv6, or bare IPv6
addresses; replace the ad-hoc parse by normalizing the string (ensure a scheme
like "tcp://" is present if missing), parse with urllib.parse.urlsplit to
extract netloc/path, strip surrounding "[]" from IPv6, then validate using
ipaddress.ip_address(...).is_loopback (also catch IPv4 loopback CIDR like
127.0.0.0/8) to decide whether to warn; update warn_insecure_mode to use
urlsplit + ipaddress and handle ValueError from ip_address to treat unknown
hosts as non-loopback.In
@tests/core/test_zap.py:
- Around line 99-117: The test must also verify that the spawned authenticator
is stopped when start_authenticator raises the "authorized_keys" ValueError;
update test_no_keys_raises_value_error to, after asserting
pytest.raises(ValueError, match="authorized_keys") for start_authenticator(ctx,
cfg), assert that the mocked ThreadAuthenticator instance's stop method was
called exactly once (mock_auth.stop.assert_called_once()); ensure you reference
the mocked authenticator returned by mock_zmq_auth.ThreadAuthenticator
(mock_auth) so the test validates authenticator.start() does not leak by
confirming mock_auth.stop() was invoked on error.In
@tests/integration_tests/test_network_topology.py:
- Around line 78-133: The threads are appended but all started together, so the
inter-append sleep calls do nothing; change the pattern to start each
coordinator thread immediately after creating it to ensure a coordinator binds
before the next tries to connect. For each threading.Thread(...) that calls
start_coordinator (namespaces "N1"/"N2"/"N3"/"N4"), set thread.daemon = True and
call thread.start() right after appending (and then sleep(TALKING_TIME) if you
still want pacing) instead of waiting to start them all later; keep using the
same stop_events and ports/namespace kwargs so the rest of the test logic is
unchanged.In
@tests/utils/test_curve_integration.py:
- Around line 60-65: The autouse fixture mock_zmq is ineffective when pyzmq is
already imported because _setup_zmq_mock only injects when modules are absent;
update mock_zmq to unconditionally replace the target entries (save originals
from sys.modules/attributes, insert mocks) and restore them in
_teardown_zmq_mock, or remove the fixture entirely and rely on explicit patching
(e.g. patch("pyleco.utils.X.configure_curve_client")) in tests; ensure you
reference and modify the same symbols used by tests (_setup_zmq_mock,
_teardown_zmq_mock, mock_zmq, and any sys.modules keys) so real ZMQ is never
accidentally used.</details> <details> <summary>🪄 Autofix (Beta)</summary> Fix all unresolved CodeRabbit comments on this PR: - [ ] <!-- {"checkboxId": "4b0d0e0a-96d7-4f10-b296-3a18ea78f0b9"} --> Push a commit to this branch (recommended) - [ ] <!-- {"checkboxId": "ff5b1114-7d8c-49e6-8ac1-43f82af23a33"} --> Create a new PR with the fixes </details> --- <details> <summary>ℹ️ Review info</summary> <details> <summary>⚙️ Run configuration</summary> **Configuration used**: Organization UI **Review profile**: CHILL **Plan**: Pro **Run ID**: `429ef2d2-cd6f-42a3-a32e-6e068cffa36b` </details> <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between 921320dfcdea967b9ea33e81e07e8fbc95f3a56a and a5c637cb9ca917084cd917f2d0a681e79874d6aa. </details> <details> <summary>📒 Files selected for processing (32)</summary> * `AGENTS.md` * `README.md` * `pyleco/actors/actor.py` * `pyleco/coordinators/coordinator.py` * `pyleco/coordinators/proxy_server.py` * `pyleco/core/__init__.py` * `pyleco/core/config.py` * `pyleco/core/curve.py` * `pyleco/core/security.py` * `pyleco/core/zap.py` * `pyleco/test.py` * `pyleco/utils/communicator.py` * `pyleco/utils/coordinator_utils.py` * `pyleco/utils/data_publisher.py` * `pyleco/utils/extended_message_handler.py` * `pyleco/utils/keygen.py` * `pyleco/utils/listener.py` * `pyleco/utils/message_handler.py` * `pyleco/utils/parser.py` * `pyleco/utils/pipe_handler.py` * `pyproject.toml` * `security.md` * `tests/coordinators/test_coordinator_curve.py` * `tests/core/test_config.py` * `tests/core/test_curve.py` * `tests/core/test_keygen.py` * `tests/core/test_parser_security.py` * `tests/core/test_security.py` * `tests/core/test_zap.py` * `tests/integration_tests/test_curve_security.py` * `tests/integration_tests/test_network_topology.py` * `tests/utils/test_curve_integration.py` </details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
Comment on lines
+84
to
+87
| if security_config is not None and security_config.mode == SecurityMode.CURVE: | ||
| configure_curve_server(s, security_config.server_key_pair) | ||
| configure_curve_server(p, security_config.server_key_pair) | ||
| auth = start_authenticator(context, security_config) |
There was a problem hiding this comment.
server_key_pair is Optional[KeyPair] but passed directly to configure_curve_server — mypy errors and runtime AttributeError risk.
In CURVE mode, server_key_pair may still be None (e.g., a client-only SecurityConfig). Add an explicit guard matching the pattern in ExtendedMessageHandler.
🔧 Proposed fix
if security_config is not None and security_config.mode == SecurityMode.CURVE:
+ if security_config.server_key_pair is None:
+ raise ValueError(
+ "Local proxy CURVE mode requires server_key_pair in SecurityConfig"
+ )
configure_curve_server(s, security_config.server_key_pair)
configure_curve_server(p, security_config.server_key_pair)
auth = start_authenticator(context, security_config)🧰 Tools
🪛 GitHub Actions: PyLECO CI / 23_Static Type Checking.txt
[error] 85-85: mypy: Argument 2 to "configure_curve_server" has incompatible type "KeyPair | None"; expected "KeyPair" [arg-type]
[error] 86-86: mypy: Argument 2 to "configure_curve_server" has incompatible type "KeyPair | None"; expected "KeyPair" [arg-type]
🪛 GitHub Actions: PyLECO CI / Static Type Checking
[error] 85-85: mypy error: Argument 2 to "configure_curve_server" has incompatible type "KeyPair | None"; expected "KeyPair" [arg-type]
[error] 86-86: mypy error: Argument 2 to "configure_curve_server" has incompatible type "KeyPair | None"; expected "KeyPair" [arg-type]
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pyleco/coordinators/proxy_server.py` around lines 84 - 87,
security_config.server_key_pair is Optional but currently passed unguarded to
configure_curve_server, causing mypy errors and a potential AttributeError;
update the CURVE branch in the proxy server so you only call
configure_curve_server(s, ...) and configure_curve_server(p, ...) when
security_config.server_key_pair is not None (mirror the guard used in
ExtendedMessageHandler), and still call start_authenticator(context,
security_config) as before; reference symbols: security_config,
SecurityMode.CURVE, server_key_pair, configure_curve_server,
start_authenticator, and ExtendedMessageHandler.
Comment on lines
+35
to
+56
| def start_authenticator(context, security_config: SecurityConfig): | ||
| from zmq.auth import CURVE_ALLOW_ANY | ||
| from zmq.auth.thread import ThreadAuthenticator | ||
|
|
||
| if security_config.mode != SecurityMode.CURVE: | ||
| raise ValueError("start_authenticator requires SecurityMode.CURVE") | ||
|
|
||
| authenticator = ThreadAuthenticator(context) | ||
| authenticator.start() | ||
| if security_config.curve_any_authenticated: | ||
| authenticator.configure_curve(domain="*", location=CURVE_ALLOW_ANY) | ||
| else: | ||
| authorized_keys = load_authorized_keys(security_config) | ||
| if authorized_keys: | ||
| provider = _CredentialsProvider(authorized_keys=authorized_keys) | ||
| authenticator.configure_curve_callback(domain="*", credentials_provider=provider) | ||
| else: | ||
| raise ValueError( | ||
| "CURVE mode requires either authorized_keys_dir, authorized_keys, " | ||
| "or curve_any_authenticated=True. No authorized keys were found." | ||
| ) | ||
| return authenticator |
There was a problem hiding this comment.
Auth thread leaks when CURVE mode has no authorized keys.
ThreadAuthenticator(...).start() runs before the authorized_keys/curve_any_authenticated validation. When neither is configured, the ValueError on line 52 propagates while the spawned ZAP thread keeps running until the process (or context) is torn down. Validate first, or clean up on the error path.
🛠️ Suggested fix
if security_config.mode != SecurityMode.CURVE:
raise ValueError("start_authenticator requires SecurityMode.CURVE")
+ if not security_config.curve_any_authenticated:
+ authorized_keys = load_authorized_keys(security_config)
+ if not authorized_keys:
+ raise ValueError(
+ "CURVE mode requires either authorized_keys_dir, authorized_keys, "
+ "or curve_any_authenticated=True. No authorized keys were found."
+ )
+ else:
+ authorized_keys = None
+
authenticator = ThreadAuthenticator(context)
authenticator.start()
- if security_config.curve_any_authenticated:
- authenticator.configure_curve(domain="*", location=CURVE_ALLOW_ANY)
- else:
- authorized_keys = load_authorized_keys(security_config)
- if authorized_keys:
- provider = _CredentialsProvider(authorized_keys=authorized_keys)
- authenticator.configure_curve_callback(domain="*", credentials_provider=provider)
- else:
- raise ValueError(
- "CURVE mode requires either authorized_keys_dir, authorized_keys, "
- "or curve_any_authenticated=True. No authorized keys were found."
- )
- return authenticator
+ try:
+ if security_config.curve_any_authenticated:
+ authenticator.configure_curve(domain="*", location=CURVE_ALLOW_ANY)
+ else:
+ provider = _CredentialsProvider(authorized_keys=authorized_keys)
+ authenticator.configure_curve_callback(domain="*", credentials_provider=provider)
+ except Exception:
+ stop_authenticator(authenticator)
+ raise
+ return authenticator🧰 Tools
🪛 GitHub Actions: PyLECO CI / 23_Static Type Checking.txt
[error] 35-35: mypy: Function is missing a return type annotation [no-untyped-def]
[error] 35-35: mypy: Function is missing a type annotation for one or more parameters [no-untyped-def]
🪛 GitHub Actions: PyLECO CI / Static Type Checking
[error] 35-35: mypy error: Function is missing a return type annotation [no-untyped-def]
[error] 35-35: mypy error: Function is missing a type annotation for one or more parameters [no-untyped-def]
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pyleco/core/zap.py` around lines 35 - 56, The start_authenticator function
currently starts ThreadAuthenticator before validating CURVE configuration,
which leaks the ZAP thread if neither curve_any_authenticated nor authorized
keys exist; fix by validating/obtaining authorized_keys (call
load_authorized_keys and check security_config.curve_any_authenticated) before
instantiating/starting ThreadAuthenticator, and only create/start the
ThreadAuthenticator after validation, or if you must start it earlier ensure you
call authenticator.stop() in the error path (catch the ValueError and
stop/cleanup the ThreadAuthenticator) and re-raise; refer to
start_authenticator, ThreadAuthenticator, load_authorized_keys, and
_CredentialsProvider to locate the code to change.
Comment on lines
+98
to
+102
| if self.security_config is not None and self.security_config.mode == SecurityMode.CURVE: | ||
| configure_curve_client(self.socket, self.security_config.client_key_pair, | ||
| self.security_config.server_public_key) | ||
| if self.security_config is None or self.security_config.mode == SecurityMode.NONE: | ||
| warn_insecure_mode(address=f"{self.host}:{self.port}") |
There was a problem hiding this comment.
configure_curve_client receives potentially-None key arguments — confirmed mypy failures block CI
The guard on line 98 confirms CURVE mode but not that client_key_pair and server_public_key are non-None. Both are Optional fields on SecurityConfig, so a caller who sets mode=CURVE but omits keys will pass the guard and hit a runtime TypeError inside configure_curve_client.
🐛 Proposed fix — validate key material before delegating
if self.security_config is not None and self.security_config.mode == SecurityMode.CURVE:
+ if self.security_config.client_key_pair is None or \
+ self.security_config.server_public_key is None:
+ raise ValueError(
+ "SecurityConfig with mode=CURVE requires client_key_pair and server_public_key"
+ )
configure_curve_client(self.socket, self.security_config.client_key_pair,
self.security_config.server_public_key)The same fix is needed in pyleco/utils/message_handler.py lines 133–134 (identical pattern, identical mypy failures).
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if self.security_config is not None and self.security_config.mode == SecurityMode.CURVE: | |
| configure_curve_client(self.socket, self.security_config.client_key_pair, | |
| self.security_config.server_public_key) | |
| if self.security_config is None or self.security_config.mode == SecurityMode.NONE: | |
| warn_insecure_mode(address=f"{self.host}:{self.port}") | |
| if self.security_config is not None and self.security_config.mode == SecurityMode.CURVE: | |
| if self.security_config.client_key_pair is None or \ | |
| self.security_config.server_public_key is None: | |
| raise ValueError( | |
| "SecurityConfig with mode=CURVE requires client_key_pair and server_public_key" | |
| ) | |
| configure_curve_client(self.socket, self.security_config.client_key_pair, | |
| self.security_config.server_public_key) | |
| if self.security_config is None or self.security_config.mode == SecurityMode.NONE: | |
| warn_insecure_mode(address=f"{self.host}:{self.port}") |
🧰 Tools
🪛 GitHub Actions: PyLECO CI / 23_Static Type Checking.txt
[error] 99-99: mypy: Argument 2 to "configure_curve_client" has incompatible type "KeyPair | None"; expected "KeyPair" [arg-type]
[error] 100-100: mypy: Argument 3 to "configure_curve_client" has incompatible type "str | None"; expected "str" [arg-type]
🪛 GitHub Actions: PyLECO CI / Static Type Checking
[error] 99-99: mypy error: Argument 2 to "configure_curve_client" has incompatible type "KeyPair | None"; expected "KeyPair" [arg-type]
[error] 100-100: mypy error: Argument 3 to "configure_curve_client" has incompatible type "str | None"; expected "str" [arg-type]
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pyleco/utils/communicator.py` around lines 98 - 102, The CURVE branch calls
configure_curve_client with SecurityConfig fields that are Optional, causing
runtime TypeError and mypy failures; update the check in Communicator (where
self.security_config is inspected) to ensure
self.security_config.client_key_pair and self.security_config.server_public_key
are not None before calling configure_curve_client, and if they are missing
either raise a clear ValueError or log an explicit error and avoid calling
configure_curve_client (apply the identical change in
pyleco/utils/message_handler.py around the same CURVE branch), referencing
SecurityMode.CURVE, client_key_pair, server_public_key, configure_curve_client,
and warn_insecure_mode to locate the code to change.
Comment on lines
94
to
97
| self._sock: zmq.Socket = context.socket(zmq.ROUTER) | ||
| if security_config is not None and security_config.mode == SecurityMode.CURVE: | ||
| configure_curve_server(self._sock, security_config.server_key_pair) | ||
| super().__init__(*args, **kwargs) |
There was a problem hiding this comment.
Missing null guards before configure_curve_server / configure_curve_client — mypy errors and runtime AttributeError risk.
Both call sites pass Optional fields directly to non-optional parameters:
security_config.server_key_pair(KeyPair | None) →configure_curve_serverexpectsKeyPair(line 96)self._security_config.client_key_pairand.server_public_key(bothOptional) →configure_curve_clientexpects non-None(lines 206–207)
The same situation is handled correctly in ExtendedMessageHandler (explicit ValueError before calling configure_curve_client). Apply the same pattern here.
🔧 Proposed fix
if security_config is not None and security_config.mode == SecurityMode.CURVE:
+ if security_config.server_key_pair is None:
+ raise ValueError("CURVE server mode requires server_key_pair in SecurityConfig")
configure_curve_server(self._sock, security_config.server_key_pair) if self._security_config is not None and self._security_config.mode == SecurityMode.CURVE:
+ if self._security_config.client_key_pair is None:
+ raise ValueError("CURVE client mode requires client_key_pair in SecurityConfig")
+ if self._security_config.server_public_key is None:
+ raise ValueError("CURVE client mode requires server_public_key in SecurityConfig")
configure_curve_client(
self._dealer,
self._security_config.client_key_pair,
self._security_config.server_public_key,
)Also applies to: 203-209
🧰 Tools
🪛 GitHub Actions: PyLECO CI / 23_Static Type Checking.txt
[error] 96-96: mypy: Argument 2 to "configure_curve_server" has incompatible type "KeyPair | None"; expected "KeyPair" [arg-type]
🪛 GitHub Actions: PyLECO CI / Static Type Checking
[error] 96-96: mypy error: Argument 2 to "configure_curve_server" has incompatible type "KeyPair | None"; expected "KeyPair" [arg-type]
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pyleco/utils/coordinator_utils.py` around lines 94 - 97, Null checks are
missing before passing Optional keypairs into
configure_curve_server/configure_curve_client; update the call sites (the block
creating self._sock where configure_curve_server(self._sock,
security_config.server_key_pair) is invoked, and the block where
configure_curve_client is called with self._security_config.client_key_pair and
self._security_config.server_public_key) to mirror ExtendedMessageHandler:
validate that the keypair(s) and server_public_key are not None and raise a
ValueError with a clear message if they are, then call
configure_curve_server/configure_curve_client with the now-non-None values so
mypy and runtime AttributeError risks are eliminated.
Comment on lines
+1
to
+83
| from __future__ import annotations | ||
|
|
||
| from unittest.mock import patch | ||
|
|
||
| import pytest | ||
|
|
||
| from pyleco.core.config import find_config_file, load_config | ||
|
|
||
|
|
||
| class TestFindConfigFile: | ||
| def test_returns_none_when_no_files_exist(self, tmp_path: pytest.TempPath) -> None: | ||
| search = [str(tmp_path / "a.toml"), str(tmp_path / "b.toml")] | ||
| assert find_config_file(search_paths=search) is None | ||
|
|
||
| def test_returns_first_found_file(self, tmp_path: pytest.TempPath) -> None: | ||
| f1 = tmp_path / "a.toml" | ||
| f2 = tmp_path / "b.toml" | ||
| f1.write_text("[test]\nkey = 1\n") | ||
| f2.write_text("[test]\nkey = 2\n") | ||
| search = [str(f1), str(f2)] | ||
| result = find_config_file(search_paths=search) | ||
| assert result == str(f1) | ||
|
|
||
| def test_returns_second_if_first_missing(self, tmp_path: pytest.TempPath) -> None: | ||
| f2 = tmp_path / "b.toml" | ||
| f2.write_text("[test]\nkey = 1\n") | ||
| search = [str(tmp_path / "a.toml"), str(f2)] | ||
| result = find_config_file(search_paths=search) | ||
| assert result == str(f2) | ||
|
|
||
| def test_default_search_paths_with_no_files(self, tmp_path: pytest.TempPath) -> None: | ||
| with patch("pyleco.core.config._DEFAULT_SEARCH_PATHS", []): | ||
| assert find_config_file() is None | ||
|
|
||
| def test_expands_user_home(self, tmp_path: pytest.TempPath) -> None: | ||
| f = tmp_path / "pyleco.toml" | ||
| f.write_text("[test]\n") | ||
| with patch("pyleco.core.config._DEFAULT_SEARCH_PATHS", [str(f)]): | ||
| result = find_config_file() | ||
| assert result == str(f) | ||
|
|
||
|
|
||
| class TestLoadConfig: | ||
| def test_load_specific_path(self, tmp_path: pytest.TempPath) -> None: | ||
| toml_file = tmp_path / "pyleco.toml" | ||
| toml_file.write_text('[security]\nmode = "CURVE"\n') | ||
| result = load_config(path=str(toml_file)) | ||
| assert result == {"security": {"mode": "CURVE"}} | ||
|
|
||
| def test_returns_empty_dict_when_no_file_found(self, tmp_path: pytest.TempPath) -> None: | ||
| search = [str(tmp_path / "missing.toml")] | ||
| result = load_config(search_paths=search) | ||
| assert result == {} | ||
|
|
||
| def test_toml_content_parsed_correctly(self, tmp_path: pytest.TempPath) -> None: | ||
| toml_file = tmp_path / "pyleco.toml" | ||
| toml_file.write_text( | ||
| '[security]\nmode = "CURVE"\n' | ||
| 'server_public_key = "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"\n' | ||
| 'server_secret_key = "bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb"\n' | ||
| '[security.authorized_keys]\n' | ||
| '"N1.Actor1" = "cccccccccccccccccccccccccccccccccccccccc"\n' | ||
| ) | ||
| result = load_config(path=str(toml_file)) | ||
| assert result["security"]["mode"] == "CURVE" | ||
| assert result["security"]["server_public_key"] == "a" * 40 | ||
| assert result["security"]["authorized_keys"]["N1.Actor1"] == "c" * 40 | ||
|
|
||
| def test_load_from_search_paths(self, tmp_path: pytest.TempPath) -> None: | ||
| toml_file = tmp_path / "pyleco.toml" | ||
| toml_file.write_text('[security]\nmode = "NONE"\n') | ||
| search = [str(toml_file)] | ||
| result = load_config(search_paths=search) | ||
| assert result == {"security": {"mode": "NONE"}} | ||
|
|
||
| def test_load_first_from_search_paths(self, tmp_path: pytest.TempPath) -> None: | ||
| f1 = tmp_path / "a.toml" | ||
| f2 = tmp_path / "b.toml" | ||
| f1.write_text('[section]\nkey = "first"\n') | ||
| f2.write_text('[section]\nkey = "second"\n') | ||
| search = [str(f1), str(f2)] | ||
| result = load_config(search_paths=search) | ||
| assert result["section"]["key"] == "first" |
There was a problem hiding this comment.
Replace pytest.TempPath with pathlib.Path to fix mypy.
pytest.TempPath is not an exported name; the tmp_path fixture is typed as pathlib.Path. This is the cause of all 10 mypy name-defined failures in the static-typing job.
🛠️ Suggested fix
from __future__ import annotations
+from pathlib import Path
from unittest.mock import patch
import pytest
@@
- def test_returns_none_when_no_files_exist(self, tmp_path: pytest.TempPath) -> None:
+ def test_returns_none_when_no_files_exist(self, tmp_path: Path) -> None:Apply the same pytest.TempPath → Path rename to all ten method signatures (lines 11, 15, 24, 31, 35, 44, 50, 55, 69, 76).
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| from __future__ import annotations | |
| from unittest.mock import patch | |
| import pytest | |
| from pyleco.core.config import find_config_file, load_config | |
| class TestFindConfigFile: | |
| def test_returns_none_when_no_files_exist(self, tmp_path: pytest.TempPath) -> None: | |
| search = [str(tmp_path / "a.toml"), str(tmp_path / "b.toml")] | |
| assert find_config_file(search_paths=search) is None | |
| def test_returns_first_found_file(self, tmp_path: pytest.TempPath) -> None: | |
| f1 = tmp_path / "a.toml" | |
| f2 = tmp_path / "b.toml" | |
| f1.write_text("[test]\nkey = 1\n") | |
| f2.write_text("[test]\nkey = 2\n") | |
| search = [str(f1), str(f2)] | |
| result = find_config_file(search_paths=search) | |
| assert result == str(f1) | |
| def test_returns_second_if_first_missing(self, tmp_path: pytest.TempPath) -> None: | |
| f2 = tmp_path / "b.toml" | |
| f2.write_text("[test]\nkey = 1\n") | |
| search = [str(tmp_path / "a.toml"), str(f2)] | |
| result = find_config_file(search_paths=search) | |
| assert result == str(f2) | |
| def test_default_search_paths_with_no_files(self, tmp_path: pytest.TempPath) -> None: | |
| with patch("pyleco.core.config._DEFAULT_SEARCH_PATHS", []): | |
| assert find_config_file() is None | |
| def test_expands_user_home(self, tmp_path: pytest.TempPath) -> None: | |
| f = tmp_path / "pyleco.toml" | |
| f.write_text("[test]\n") | |
| with patch("pyleco.core.config._DEFAULT_SEARCH_PATHS", [str(f)]): | |
| result = find_config_file() | |
| assert result == str(f) | |
| class TestLoadConfig: | |
| def test_load_specific_path(self, tmp_path: pytest.TempPath) -> None: | |
| toml_file = tmp_path / "pyleco.toml" | |
| toml_file.write_text('[security]\nmode = "CURVE"\n') | |
| result = load_config(path=str(toml_file)) | |
| assert result == {"security": {"mode": "CURVE"}} | |
| def test_returns_empty_dict_when_no_file_found(self, tmp_path: pytest.TempPath) -> None: | |
| search = [str(tmp_path / "missing.toml")] | |
| result = load_config(search_paths=search) | |
| assert result == {} | |
| def test_toml_content_parsed_correctly(self, tmp_path: pytest.TempPath) -> None: | |
| toml_file = tmp_path / "pyleco.toml" | |
| toml_file.write_text( | |
| '[security]\nmode = "CURVE"\n' | |
| 'server_public_key = "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"\n' | |
| 'server_secret_key = "bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb"\n' | |
| '[security.authorized_keys]\n' | |
| '"N1.Actor1" = "cccccccccccccccccccccccccccccccccccccccc"\n' | |
| ) | |
| result = load_config(path=str(toml_file)) | |
| assert result["security"]["mode"] == "CURVE" | |
| assert result["security"]["server_public_key"] == "a" * 40 | |
| assert result["security"]["authorized_keys"]["N1.Actor1"] == "c" * 40 | |
| def test_load_from_search_paths(self, tmp_path: pytest.TempPath) -> None: | |
| toml_file = tmp_path / "pyleco.toml" | |
| toml_file.write_text('[security]\nmode = "NONE"\n') | |
| search = [str(toml_file)] | |
| result = load_config(search_paths=search) | |
| assert result == {"security": {"mode": "NONE"}} | |
| def test_load_first_from_search_paths(self, tmp_path: pytest.TempPath) -> None: | |
| f1 = tmp_path / "a.toml" | |
| f2 = tmp_path / "b.toml" | |
| f1.write_text('[section]\nkey = "first"\n') | |
| f2.write_text('[section]\nkey = "second"\n') | |
| search = [str(f1), str(f2)] | |
| result = load_config(search_paths=search) | |
| assert result["section"]["key"] == "first" | |
| from __future__ import annotations | |
| from pathlib import Path | |
| from unittest.mock import patch | |
| import pytest | |
| from pyleco.core.config import find_config_file, load_config | |
| class TestFindConfigFile: | |
| def test_returns_none_when_no_files_exist(self, tmp_path: Path) -> None: | |
| search = [str(tmp_path / "a.toml"), str(tmp_path / "b.toml")] | |
| assert find_config_file(search_paths=search) is None | |
| def test_returns_first_found_file(self, tmp_path: Path) -> None: | |
| f1 = tmp_path / "a.toml" | |
| f2 = tmp_path / "b.toml" | |
| f1.write_text("[test]\nkey = 1\n") | |
| f2.write_text("[test]\nkey = 2\n") | |
| search = [str(f1), str(f2)] | |
| result = find_config_file(search_paths=search) | |
| assert result == str(f1) | |
| def test_returns_second_if_first_missing(self, tmp_path: Path) -> None: | |
| f2 = tmp_path / "b.toml" | |
| f2.write_text("[test]\nkey = 1\n") | |
| search = [str(tmp_path / "a.toml"), str(f2)] | |
| result = find_config_file(search_paths=search) | |
| assert result == str(f2) | |
| def test_default_search_paths_with_no_files(self, tmp_path: Path) -> None: | |
| with patch("pyleco.core.config._DEFAULT_SEARCH_PATHS", []): | |
| assert find_config_file() is None | |
| def test_expands_user_home(self, tmp_path: Path) -> None: | |
| f = tmp_path / "pyleco.toml" | |
| f.write_text("[test]\n") | |
| with patch("pyleco.core.config._DEFAULT_SEARCH_PATHS", [str(f)]): | |
| result = find_config_file() | |
| assert result == str(f) | |
| class TestLoadConfig: | |
| def test_load_specific_path(self, tmp_path: Path) -> None: | |
| toml_file = tmp_path / "pyleco.toml" | |
| toml_file.write_text('[security]\nmode = "CURVE"\n') | |
| result = load_config(path=str(toml_file)) | |
| assert result == {"security": {"mode": "CURVE"}} | |
| def test_returns_empty_dict_when_no_file_found(self, tmp_path: Path) -> None: | |
| search = [str(tmp_path / "missing.toml")] | |
| result = load_config(search_paths=search) | |
| assert result == {} | |
| def test_toml_content_parsed_correctly(self, tmp_path: Path) -> None: | |
| toml_file = tmp_path / "pyleco.toml" | |
| toml_file.write_text( | |
| '[security]\nmode = "CURVE"\n' | |
| 'server_public_key = "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"\n' | |
| 'server_secret_key = "bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb"\n' | |
| '[security.authorized_keys]\n' | |
| '"N1.Actor1" = "cccccccccccccccccccccccccccccccccccccccc"\n' | |
| ) | |
| result = load_config(path=str(toml_file)) | |
| assert result["security"]["mode"] == "CURVE" | |
| assert result["security"]["server_public_key"] == "a" * 40 | |
| assert result["security"]["authorized_keys"]["N1.Actor1"] == "c" * 40 | |
| def test_load_from_search_paths(self, tmp_path: Path) -> None: | |
| toml_file = tmp_path / "pyleco.toml" | |
| toml_file.write_text('[security]\nmode = "NONE"\n') | |
| search = [str(toml_file)] | |
| result = load_config(search_paths=search) | |
| assert result == {"security": {"mode": "NONE"}} | |
| def test_load_first_from_search_paths(self, tmp_path: Path) -> None: | |
| f1 = tmp_path / "a.toml" | |
| f2 = tmp_path / "b.toml" | |
| f1.write_text('[section]\nkey = "first"\n') | |
| f2.write_text('[section]\nkey = "second"\n') | |
| search = [str(f1), str(f2)] | |
| result = load_config(search_paths=search) | |
| assert result["section"]["key"] == "first" |
🧰 Tools
🪛 GitHub Actions: PyLECO CI / 23_Static Type Checking.txt
[error] 11-11: mypy: Name "pytest.TempPath" is not defined [name-defined]
[error] 15-15: mypy: Name "pytest.TempPath" is not defined [name-defined]
[error] 24-24: mypy: Name "pytest.TempPath" is not defined [name-defined]
[error] 31-31: mypy: Name "pytest.TempPath" is not defined [name-defined]
[error] 35-35: mypy: Name "pytest.TempPath" is not defined [name-defined]
[error] 44-44: mypy: Name "pytest.TempPath" is not defined [name-defined]
[error] 50-50: mypy: Name "pytest.TempPath" is not defined [name-defined]
[error] 55-55: mypy: Name "pytest.TempPath" is not defined [name-defined]
[error] 69-69: mypy: Name "pytest.TempPath" is not defined [name-defined]
[error] 76-76: mypy: Name "pytest.TempPath" is not defined [name-defined]
🪛 GitHub Actions: PyLECO CI / Static Type Checking
[error] 11-11: mypy error: Name "pytest.TempPath" is not defined [name-defined]
[error] 15-15: mypy error: Name "pytest.TempPath" is not defined [name-defined]
[error] 24-24: mypy error: Name "pytest.TempPath" is not defined [name-defined]
[error] 31-31: mypy error: Name "pytest.TempPath" is not defined [name-defined]
[error] 35-35: mypy error: Name "pytest.TempPath" is not defined [name-defined]
[error] 44-44: mypy error: Name "pytest.TempPath" is not defined [name-defined]
[error] 50-50: mypy error: Name "pytest.TempPath" is not defined [name-defined]
[error] 55-55: mypy error: Name "pytest.TempPath" is not defined [name-defined]
[error] 69-69: mypy error: Name "pytest.TempPath" is not defined [name-defined]
[error] 76-76: mypy error: Name "pytest.TempPath" is not defined [name-defined]
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/core/test_config.py` around lines 1 - 83, Replace the incorrect
pytest.TempPath type annotations with pathlib.Path and add "from pathlib import
Path" to the imports; update all method signatures that use tmp_path (e.g.,
TestFindConfigFile.test_returns_none_when_no_files_exist,
test_returns_first_found_file, test_returns_second_if_first_missing,
test_default_search_paths_with_no_files, test_expands_user_home and
TestLoadConfig.test_load_specific_path,
test_returns_empty_dict_when_no_file_found, test_toml_content_parsed_correctly,
test_load_from_search_paths, test_load_first_from_search_paths) to use tmp_path:
Path so mypy no longer complains about the non-exported pytest.TempPath name.
- security.py: SecurityMode enum, KeyPair, SecurityConfig dataclasses, generate_key_pair(), load_authorized_keys(), load_security_config() - config.py: TOML config file loading with search path fallback - curve.py: configure_curve_server/client socket helpers, warn_insecure_mode() for non-loopback NONE mode - zap.py: ZAP authenticator management (start/stop) with key directory, inline config, or any-authenticated mode - core/__init__.py: export SecurityMode
- Coordinator: accept SecurityConfig, start ZAP authenticator, apply CURVE to ROUTER socket and inter-coordinator DEALER sockets - ProxyServer: accept SecurityConfig, apply CURVE to XPUB/XSUB sockets, start ZAP authenticator for local proxy, client mode for remote proxy - Communicator: accept SecurityConfig, apply CURVE to DEALER socket - MessageHandler: accept SecurityConfig, apply CURVE to DEALER socket - ExtendedMessageHandler: accept SecurityConfig, apply CURVE to SUB socket - DataPublisher: accept SecurityConfig, apply CURVE to PUB socket - Listener/PipeHandler: pass SecurityConfig through to handler classes - Actor: pass SecurityConfig through to parent MessageHandler - coordinator_utils: SecurityConfig for ZmqMultiSocket and ZmqNode
- parser.py: add --security-mode, --server-secret-key, --server-public-key, --client-secret-key, --client-public-key, --data-server-public-key, --authorized-keys-dir, --curve-any-authenticated, --config arguments; add build_security_config_from_kwargs() helper - keygen.py: pyleco-keygen CLI for Curve25519 key pair generation with optional --output-dir and --name for file output - pyproject.toml: add 'curve' extra (tomli for Python < 3.11), add pyleco-keygen entry point
- test.py: add CURVE socket option support to FakeSocket - test_security.py: SecurityMode, KeyPair, SecurityConfig, validation, key loading, config layering (TOML + CLI override) - test_config.py: TOML file loading and search path resolution - test_curve.py: socket configuration helpers and insecure mode warning - test_zap.py: ZAP authenticator start/stop with different key configs - test_keygen.py: key generation CLI - test_parser_security.py: security CLI argument parsing - test_coordinator_curve.py: Coordinator CURVE mode tests - test_curve_integration.py: component-side CURVE integration tests - test_curve_security.py: end-to-end CURVE handshake integration test - test_network_topology.py: multi-coordinator CURVE network test
BenediktBurger
force-pushed
the
add-security
branch
from
May 28, 2026 14:40
a5c637c to
36da329
Compare
…, Full) Replace SecurityMode enum and monolithic SecurityConfig dataclass with three typed dataclasses: ServerSecurityConfig, ClientSecurityConfig, and FullSecurityConfig. SecurityConfig is now a union type alias. - Remove SecurityMode enum; mode is inferred from which keys are present - load_security_config returns SecurityConfig | None (None replaces mode=NONE) - Remove validate() method; type system enforces validity - Replace mode == SecurityMode.CURVE checks with isinstance checks - Remove --security-mode CLI argument - Update all consumers and tests to use new types
- Wrap zmq import in generate_key_pair() with try/except to re-raise ImportError with 'zmq is required' message, enabling proper test mocking - Move zmq imports in zap.py to TYPE_CHECKING/lazy pattern so test mocks can intercept them before module-level import fails - Skip two known flaky caplog timing tests: - test_subscribe_single_again (test_extended_message_handler) - test_log_message (test_message_handler)
BenediktBurger
force-pushed
the
add-security
branch
from
May 28, 2026 16:46
36da329 to
d6676e2
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #150 +/- ##
==========================================
- Coverage 91.15% 90.43% -0.72%
==========================================
Files 39 44 +5
Lines 3154 3523 +369
Branches 323 403 +80
==========================================
+ Hits 2875 3186 +311
- Misses 237 271 +34
- Partials 42 66 +24
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Based on the security suggestion for the LECO protocol. The
security.mdfile is from that suggestion:ToDos:
security.mdand replace link to that file with a link to the protocol definition.