LCORE-1607: specific rules to ignore types#1465
Conversation
WalkthroughThis pull request refactors type-checking suppressions across multiple files from generic Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
tests/e2e/features/steps/feedback.py (5)
57-57:⚠️ Potential issue | 🟡 MinorIncomplete refactoring: Update to specific type-ignore code.
This decorator still uses a generic
# type: ignorecomment, while other decorators in this file (lines 21, 29, 37, 95, 101, 143) were updated to use# type: ignore[reportCallIssue]. For consistency with the PR objectives and the rest of this file, this should also be updated.♻️ Proposed fix for consistency
-@when("I submit the following feedback for the conversation created before") # type: ignore +@when("I submit the following feedback for the conversation created before") # type: ignore[reportCallIssue] def submit_feedback_valid_conversation(context: Context) -> None:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/features/steps/feedback.py` at line 57, The decorator `@when`("I submit the following feedback for the conversation created before") uses a generic "# type: ignore" that was not updated; replace it with the specific "# type: ignore[reportCallIssue]" to match the rest of the file (see other decorators updated at lines near the top) so the type-ignore is explicit and consistent with functions handling step definitions in this module.
74-74:⚠️ Potential issue | 🟡 MinorIncomplete refactoring: Update to specific type-ignore code.
This decorator still uses a generic
# type: ignorecomment. For consistency with the updated decorators in this file (lines 21, 29, 37, 95, 101, 143), this should be updated to# type: ignore[reportCallIssue].♻️ Proposed fix for consistency
-@when("I submit the following feedback without specifying conversation ID") # type: ignore +@when("I submit the following feedback without specifying conversation ID") # type: ignore[reportCallIssue] def submit_feedback_without_conversation(context: Context) -> None:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/features/steps/feedback.py` at line 74, Update the decorator on the step definition that reads `@when`("I submit the following feedback without specifying conversation ID") # type: ignore to use the specific ignore code; replace the trailing comment with # type: ignore[reportCallIssue] so it matches the other decorators in this file (e.g., those at lines using # type: ignore[reportCallIssue]) and keeps consistent typing annotations for this step definition.
66-66:⚠️ Potential issue | 🟡 MinorIncomplete refactoring: Update to specific type-ignore code.
This decorator still uses a generic
# type: ignorecomment. For consistency with the updated decorators in this file (lines 21, 29, 37, 95, 101, 143), this should be updated to# type: ignore[reportCallIssue].♻️ Proposed fix for consistency
-@when('I submit the following feedback for nonexisting conversation "{conversation_id}"') # type: ignore +@when('I submit the following feedback for nonexisting conversation "{conversation_id}"') # type: ignore[reportCallIssue] def submit_feedback_nonexisting_conversation(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/features/steps/feedback.py` at line 66, Update the decorator comment on the `@when`('I submit the following feedback for nonexisting conversation "{conversation_id}"') line to use the specific ignore code instead of the generic one: replace the trailing "# type: ignore" with "# type: ignore[reportCallIssue]" so it matches the other decorators in this file (e.g., the ones at lines with similar `@when/`@given decorators).
7-7:⚠️ Potential issue | 🟡 MinorRemove duplicate type-ignore comments.
The
# pyright: ignore[reportAttributeAccessIssue]comment is duplicated three times on line 7. This appears to be a copy-paste error.🧹 Proposed fix
-from behave import ( # pyright: ignore[reportAttributeAccessIssue] # pyright: ignore[reportAttributeAccessIssue] # pyright: ignore[reportAttributeAccessIssue] +from behave import ( # pyright: ignore[reportAttributeAccessIssue] given, step, when, )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/features/steps/feedback.py` at line 7, The import line in tests/e2e/features/steps/feedback.py contains three identical pyright ignore comments; remove the duplicates so only a single "# pyright: ignore[reportAttributeAccessIssue]" remains after the "from behave import (" import to avoid redundant comments while preserving the intended type-ignore; ensure spacing and punctuation around the remaining comment matches project style.
107-107:⚠️ Potential issue | 🟡 MinorIncomplete refactoring: Update to specific type-ignore code.
This decorator still uses a generic
# type: ignorecomment. For consistency with the updated decorators in this file (lines 21, 29, 37, 95, 101, 143), this should be updated to# type: ignore[reportCallIssue].♻️ Proposed fix for consistency
-@given('A new conversation is initialized with user_id "{user_id}"') # type: ignore +@given('A new conversation is initialized with user_id "{user_id}"') # type: ignore[reportCallIssue] def initialize_conversation_with_user_id(context: Context, user_id: str) -> None:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/features/steps/feedback.py` at line 107, The decorator on the step definition `@given`('A new conversation is initialized with user_id "{user_id}"') still uses a generic "# type: ignore"; update it to the specific ignore code used elsewhere ("# type: ignore[reportCallIssue]") for consistency with the other decorators (see other `@given/`@when/@then decorators in this file), by replacing the trailing comment on that decorator line with "# type: ignore[reportCallIssue]".
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/client.py`:
- Line 175: The method currently calls self._lsc.copy(...) and suppresses a type
error; instead add a runtime guard that enforces "service mode only" by checking
the runtime type of self._lsc and rejecting library mode (e.g., if
isinstance(self._lsc, AsyncLlamaStackAsLibraryClient): raise
RuntimeError("...service mode only...")), then narrow the type using typing.cast
before calling copy (remove the # type: ignore[arg-type]); reference self._lsc
and AsyncLlamaStackAsLibraryClient to locate where to add the guard and cast,
and raise a clear error when a library client is detected.
---
Outside diff comments:
In `@tests/e2e/features/steps/feedback.py`:
- Line 57: The decorator `@when`("I submit the following feedback for the
conversation created before") uses a generic "# type: ignore" that was not
updated; replace it with the specific "# type: ignore[reportCallIssue]" to match
the rest of the file (see other decorators updated at lines near the top) so the
type-ignore is explicit and consistent with functions handling step definitions
in this module.
- Line 74: Update the decorator on the step definition that reads `@when`("I
submit the following feedback without specifying conversation ID") # type:
ignore to use the specific ignore code; replace the trailing comment with #
type: ignore[reportCallIssue] so it matches the other decorators in this file
(e.g., those at lines using # type: ignore[reportCallIssue]) and keeps
consistent typing annotations for this step definition.
- Line 66: Update the decorator comment on the `@when`('I submit the following
feedback for nonexisting conversation "{conversation_id}"') line to use the
specific ignore code instead of the generic one: replace the trailing "# type:
ignore" with "# type: ignore[reportCallIssue]" so it matches the other
decorators in this file (e.g., the ones at lines with similar `@when/`@given
decorators).
- Line 7: The import line in tests/e2e/features/steps/feedback.py contains three
identical pyright ignore comments; remove the duplicates so only a single "#
pyright: ignore[reportAttributeAccessIssue]" remains after the "from behave
import (" import to avoid redundant comments while preserving the intended
type-ignore; ensure spacing and punctuation around the remaining comment matches
project style.
- Line 107: The decorator on the step definition `@given`('A new conversation is
initialized with user_id "{user_id}"') still uses a generic "# type: ignore";
update it to the specific ignore code used elsewhere ("# type:
ignore[reportCallIssue]") for consistency with the other decorators (see other
`@given/`@when/@then decorators in this file), by replacing the trailing comment
on that decorator line with "# type: ignore[reportCallIssue]".
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: c376e954-426a-4b0c-a817-7734134079c7
📒 Files selected for processing (8)
src/client.pytests/e2e/features/steps/auth.pytests/e2e/features/steps/common.pytests/e2e/features/steps/conversation.pytests/e2e/features/steps/feedback.pytests/integration/endpoints/test_info_integration.pytests/integration/endpoints/test_model_list.pytests/integration/endpoints/test_query_integration.py
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: build-pr
- GitHub Check: Konflux kflux-prd-rh02 / lightspeed-stack-on-pull-request
- GitHub Check: E2E Tests for Lightspeed Evaluation job
- GitHub Check: E2E: server mode / ci
- GitHub Check: E2E: library mode / ci
🧰 Additional context used
📓 Path-based instructions (2)
tests/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
tests/**/*.py: Use pytest for all unit and integration tests; do not use unittest
Usepytest.mark.asynciomarker for async unit tests
Files:
tests/e2e/features/steps/common.pytests/integration/endpoints/test_model_list.pytests/integration/endpoints/test_info_integration.pytests/e2e/features/steps/auth.pytests/integration/endpoints/test_query_integration.pytests/e2e/features/steps/conversation.pytests/e2e/features/steps/feedback.py
src/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
src/**/*.py: Use absolute imports for internal modules (e.g.,from authentication import get_auth_dependency)
Usefrom llama_stack_client import AsyncLlamaStackClientfor Llama Stack imports
Checkconstants.pyfor shared constants before defining new ones
All modules start with descriptive docstrings explaining purpose
Uselogger = get_logger(__name__)fromlog.pyfor module logging
Type aliases defined at module level for clarity
All functions require docstrings with brief descriptions
Use complete type annotations for function parameters and return types
Use union types with modern syntax:str | intinstead ofUnion[str, int]
UseOptional[Type]for optional types in type annotations
Use snake_case with descriptive, action-oriented names for functions (get_, validate_, check_)
Avoid in-place parameter modification anti-patterns: return new data structures instead of modifying parameters
Useasync deffor I/O operations and external API calls
HandleAPIConnectionErrorfrom Llama Stack in error handling
Uselogger.debug()for detailed diagnostic information
Uselogger.info()for general information about program execution
Uselogger.warning()for unexpected events or potential problems
Uselogger.error()for serious problems that prevented function execution
All classes require descriptive docstrings explaining purpose
Use PascalCase for class names with descriptive names and standard suffixes:Configuration,Error/Exception,Resolver,Interface
Use complete type annotations for all class attributes; avoid usingAny
Follow Google Python docstring conventions for all modules, classes, and functions
IncludeParameters:,Returns:,Raises:sections in function docstrings as needed
Files:
src/client.py
🧠 Learnings (1)
📚 Learning: 2026-04-05T12:19:36.009Z
Learnt from: CR
Repo: lightspeed-core/lightspeed-stack PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-04-05T12:19:36.009Z
Learning: Applies to src/**/*.py : Handle `APIConnectionError` from Llama Stack in error handling
Applied to files:
tests/integration/endpoints/test_model_list.pytests/integration/endpoints/test_info_integration.pytests/integration/endpoints/test_query_integration.py
🔇 Additional comments (9)
tests/integration/endpoints/test_query_integration.py (1)
134-135: Scoped ignore and explicit expected value are a good refactor.This keeps the assertion behavior intact while making the suppression more targeted and readable.
tests/integration/endpoints/test_model_list.py (1)
210-211: Looks good: assertion refactor is clear and targeted.Using a named expected value plus a specific type-ignore rule improves maintainability without changing behavior.
tests/integration/endpoints/test_info_integration.py (1)
126-127: Approved: consistent, behavior-preserving cleanup.The local expected message and narrowed ignore rule are both sensible here.
tests/e2e/features/steps/common.py (2)
35-35: LGTM: Type suppression specificity improved.The change from generic
# type: ignoreto# type: ignore[reportCallIssue]improves type-checking precision by narrowing the suppression scope.
1-89: Verify framework choice aligns with coding guidelines.This file uses the Behave framework for BDD-style e2e tests. As per coding guidelines,
tests/**/*.pyshould use pytest. While these are e2e tests (which may fall outside the "unit and integration tests" scope mentioned in the guideline), the path pattern is broad enough to include this file.Please confirm whether the guideline applies to e2e/BDD tests, or if Behave is an approved exception for behavior-driven scenarios in the
tests/e2e/directory.tests/e2e/features/steps/conversation.py (1)
416-416: LGTM: Type suppression specificity improved.The change from generic
# type: ignoreto# type: ignore[reportCallIssue]improves type-checking precision.tests/e2e/features/steps/auth.py (1)
40-40: LGTM: Type suppression specificity improved.The change from generic
# type: ignoreto# type: ignore[reportCallIssue]improves type-checking precision.tests/e2e/features/steps/feedback.py (2)
21-21: LGTM: Type suppression specificity improved.The changes from generic
# type: ignoreto# type: ignore[reportCallIssue]improve type-checking precision.Also applies to: 29-29, 37-37
95-95: LGTM: Type suppression specificity improved.The changes from generic
# type: ignoreto# type: ignore[reportCallIssue]improve type-checking precision.Also applies to: 101-101, 143-143
| } | ||
|
|
||
| self._lsc = self._lsc.copy(set_default_headers=updated_headers) # type: ignore | ||
| self._lsc = self._lsc.copy(set_default_headers=updated_headers) # type: ignore[arg-type] |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
🌐 Web query:
What is the current signature of AsyncLlamaStackClient.copy(...)inllama_stack_client, and does it accept set_default_headers: dict[str, str]?
💡 Result:
In llama_stack_client, AsyncLlamaStackClient.copy(...) is currently:
def copy(
self,
*,
api_key: str | None = None,
base_url: str | httpx.URL | None = None,
timeout: float | Timeout | None | NotGiven = not_given,
http_client: httpx.AsyncClient | None = None,
max_retries: int | NotGiven = not_given,
default_headers: Mapping[str, str] | None = None,
set_default_headers: Mapping[str, str] | None = None,
default_query: Mapping[str, object] | None = None,
set_default_query: Mapping[str, object] | None = None,
_extra_kwargs: Mapping[str, Any] = {},
) -> Self
``` ([github.com](https://github.com/llamastack/llama-stack-client-python/blob/main/src/llama_stack_client/_client.py))
Yes— it accepts `set_default_headers`, typed as `Mapping[str, str] | None`, so a `dict[str, str]` is valid. ([github.com](https://github.com/llamastack/llama-stack-client-python/blob/main/src/llama_stack_client/_client.py))
Citations:
- 1: https://github.com/llamastack/llama-stack-client-python/blob/main/src/llama_stack_client/_client.py
- 2: https://github.com/llamastack/llama-stack-client-python/blob/main/src/llama_stack_client/_client.py
---
🏁 Script executed:
```shell
# First, examine the context around line 175 in src/client.py
head -200 src/client.py | tail -50Repository: lightspeed-core/lightspeed-stack
Length of output: 1819
🏁 Script executed:
# Get the full class context and _lsc declaration
rg -B 30 "self._lsc = self._lsc.copy\(set_default_headers" src/client.py -A 5Repository: lightspeed-core/lightspeed-stack
Length of output: 1132
🏁 Script executed:
# Check for AsyncLlamaStackAsLibraryClient definition and copy method
rg "AsyncLlamaStackAsLibraryClient" src/ -A 10 -B 2Repository: lightspeed-core/lightspeed-stack
Length of output: 4490
🏁 Script executed:
# Check if is_library_client property exists
rg "is_library_client" src/client.py -B 2 -A 2Repository: lightspeed-core/lightspeed-stack
Length of output: 250
🏁 Script executed:
# Check _lsc declaration type annotation
rg "_lsc:" src/client.py -B 2 -A 2Repository: lightspeed-core/lightspeed-stack
Length of output: 706
🏁 Script executed:
# Find where AsyncLlamaStackAsLibraryClient is imported from and examine its definition
rg "AsyncLlamaStackAsLibraryClient" -A 20 | grep -E "(def copy|class AsyncLlamaStackAsLibraryClient)" -A 5Repository: lightspeed-core/lightspeed-stack
Length of output: 58
🏁 Script executed:
# Check if llama_stack.core.library_client module defines copy() for library client
fd -e py | xargs grep -l "AsyncLlamaStackAsLibraryClient" | head -5Repository: lightspeed-core/lightspeed-stack
Length of output: 109
🏁 Script executed:
# Look at the imports more closely - where does it come from?
rg "from llama_stack" src/client.pyRepository: lightspeed-core/lightspeed-stack
Length of output: 302
🏁 Script executed:
# Check if AsyncLlamaStackAsLibraryClient is defined locally in the repo
find src -name "*.py" -type f | xargs grep -l "class AsyncLlamaStackAsLibraryClient"Repository: lightspeed-core/lightspeed-stack
Length of output: 60
🏁 Script executed:
# Get full context of the method signature to confirm return type
rg -B 5 "def update_provider_data" src/client.pyRepository: lightspeed-core/lightspeed-stack
Length of output: 295
🏁 Script executed:
# Verify the class structure of AsyncLlamaStackClientHolder to see all method signatures
rg "class AsyncLlamaStackClientHolder" src/client.py -A 100 | head -120Repository: lightspeed-core/lightspeed-stack
Length of output: 4153
Enforce service-mode-only contract with a runtime guard instead of type suppression.
The method's docstring promises "For use with service mode only," but there's no runtime check, allowing it to be called with a library client. The # type: ignore[arg-type] suppresses the type mismatch between _lsc's declared type and the AsyncLlamaStackAsLibraryClient it can hold at runtime. Add an explicit guard to reject library mode and narrow the type with cast().
Proposed fix
+from typing import cast
@@
def update_provider_data(self, updates: dict[str, str]) -> AsyncLlamaStackClient:
@@
if not self._lsc:
raise RuntimeError(
"AsyncLlamaStackClient has not been initialised. Ensure 'load(..)' has been called."
)
+ if self.is_library_client:
+ raise RuntimeError(
+ "update_provider_data is only supported for service client mode."
+ )
@@
- self._lsc = self._lsc.copy(set_default_headers=updated_headers) # type: ignore[arg-type]
+ service_client = cast(AsyncLlamaStackClient, self._lsc)
+ self._lsc = service_client.copy(set_default_headers=updated_headers)
return self._lsc🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/client.py` at line 175, The method currently calls self._lsc.copy(...)
and suppresses a type error; instead add a runtime guard that enforces "service
mode only" by checking the runtime type of self._lsc and rejecting library mode
(e.g., if isinstance(self._lsc, AsyncLlamaStackAsLibraryClient): raise
RuntimeError("...service mode only...")), then narrow the type using typing.cast
before calling copy (remove the # type: ignore[arg-type]); reference self._lsc
and AsyncLlamaStackAsLibraryClient to locate where to add the guard and cast,
and raise a clear error when a library client is detected.
1 participant
Description
LCORE-1607: specific rules to ignore types
Type of change
Tools used to create PR
Related Tickets & Documents
Summary by CodeRabbit
Release Notes
Tests
Chores