Conversation
There was a problem hiding this comment.
Pull request overview
This PR focuses on resolving typing issues across nats-core, improving type narrowing for protocol parsing and JWT auth handling, and tightening type correctness in tests and tooling.
Changes:
- Update protocol message handling to use
isinstance-based narrowing (Info/Err) instead ofopchecks. - Adjust client callback and handler typing (notably error callbacks and
_handle_info), plus refine JWT auth type narrowing. - Fix minor typing issues in tests/bench tool and expand CI type-check coverage to include
nats-core.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
nats-core/tools/bench.py |
Removes an unnecessary type: ignore for sub.messages iteration. |
nats-core/tests/test_protocol.py |
Uses isinstance(msg, Err) for safer type narrowing in protocol parsing tests. |
nats-core/tests/test_examples.py |
Adds assertions/guard rails to satisfy typing and ensure CompletedProcess is always returned or an error is raised. |
nats-core/tests/test_client.py |
Adds a narrowing assertion for msg.reply in JWT request/reply test. |
nats-core/src/nats/client/__init__.py |
Improves protocol union narrowing, broadens error callback type, updates _handle_info typing, and refines JWT auth narrowing/validation. |
.github/workflows/check.yml |
Runs ty type checking for both nats-core and nats-server. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| elif isinstance(jwt, tuple) and isinstance(jwt[0], Path): | ||
| # Separate files | ||
| jwt_file, seed_file = jwt | ||
| jwt_file: Path = jwt[0] | ||
| seed_file: Path = jwt[1] # type: ignore[assignment] | ||
| jwt_content = jwt_file.read_bytes().strip() | ||
| seed_bytes = seed_file.read_bytes().strip() |
There was a problem hiding this comment.
In _setup_jwt_auth, the elif isinstance(jwt, tuple) and isinstance(jwt[0], Path): branch does not verify that jwt[1] is also a Path, so the code needs a type: ignore and can still blow up at runtime if the tuple shape is wrong. Consider checking tuple length and isinstance(jwt[1], Path) in the condition and then assign without type: ignore.
| jwt_str, seed_str = jwt[0], jwt[1] | ||
| assert isinstance(jwt_str, str) | ||
| assert isinstance(seed_str, str) |
There was a problem hiding this comment.
In the tuple-of-strings JWT branch, indexing (jwt[0], jwt[1]) plus assert isinstance(...) is weaker than the previous unpacking: it won’t validate tuple length (can raise IndexError) and assert can be stripped with python -O, removing the runtime type checks. Prefer explicit validation (e.g., length/type checks that raise TypeError) and/or unpacking to keep errors deterministic and user-friendly.
| jwt_str, seed_str = jwt[0], jwt[1] | |
| assert isinstance(jwt_str, str) | |
| assert isinstance(seed_str, str) | |
| if len(jwt) != 2: | |
| msg = f"jwt tuple must contain exactly 2 elements (jwt, seed), got {len(jwt)}" | |
| raise TypeError(msg) | |
| jwt_str, seed_str = jwt | |
| if not isinstance(jwt_str, str) or not isinstance(seed_str, str): | |
| msg = ( | |
| "jwt tuple elements must be strings: " | |
| f"got types ({type(jwt_str).__name__}, {type(seed_str).__name__})" | |
| ) | |
| raise TypeError(msg) |
Exception | str_handle_infoparameter type to useProtocolServerInfoisinstancenarrowing for protocol message unions_setup_jwt_authtype: ignorecomments