Parse new LCORE tool_call format#150
Conversation
WalkthroughThe streaming parser adds support for Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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)
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 |
|
@asamal4 w/o this change i was not getting tool_call data in the eval results when using a current LCORE deployment |
![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
🤖 Fix all issues with AI agents
In `@src/lightspeed_evaluation/core/api/streaming_parser.py`:
- Around line 206-219: The parser incorrectly treats an empty new-format args
dict as missing because it uses "or" when assigning tool_name and arguments;
change the logic to prefer the "name"/"args" keys when present by checking key
presence on event_data (e.g., use "if 'name' in event_data: tool_name =
event_data['name'] else: tool_name = event_data.get('tool_name')" and similarly
for "args"/"arguments") and then treat arguments as missing only if neither key
is present (i.e., arguments is None and neither "args" nor "arguments" keys
exist). Update the code paths around event_data, tool_name, and arguments in
streaming_parser.py so empty dicts for args are accepted as valid.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/unit/core/api/test_streaming_parser.py (1)
3-22:⚠️ Potential issue | 🟠 MajorUse pytest-mock types instead of
unittest.mock.
Line 4 importsMockfromunittest.mock, which violates the tests/**/*.py guideline. Replace withAnyfor type hints and remove the unittest.mock import entirely.🔧 Suggested changes
-from typing import Generator -from unittest.mock import Mock +from typing import Any, Generator ... -@pytest.fixture -def mock_response(mocker: MockerFixture) -> Generator[Mock, None, None]: +@pytest.fixture +def mock_response(mocker: MockerFixture) -> Generator[Any, None, None]:Update all test function parameters from
mock_response: Mocktomock_response: Any(lines 27, 45, 61, 71, 81, 91, 107, 122, 138, 157, 319, 333, 349, 367, 381, 396, 411).As per coding guidelines:
tests/**/*.py: Use pytest mocking with mocker fixture instead of unittest.mock; Import mocking utilities from pytest (mocker fixture) rather than from unittest.mock.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tests/unit/core/api/test_streaming_parser.py (2)
3-22:⚠️ Potential issue | 🟠 MajorRemove the local
mock_responsefixture and drop theunittest.mockimport to comply with testing guidelines.The local fixture shadows the shared one from
tests/unit/core/api/conftest.pyand theunittest.mock.Mockimport violates the requirement to use pytest mocking only. Update type annotations fromMocktoAnyand reuse the shared fixture.Proposed changes
-from typing import Generator -from unittest.mock import Mock +from typing import Any -import pytest -from pytest_mock import MockerFixture +import pytest from lightspeed_evaluation.core.api.streaming_parser import ( parse_streaming_response, _parse_sse_line, _parse_tool_call, _format_tool_sequences, ) - -@pytest.fixture -def mock_response(mocker: MockerFixture) -> Generator[Mock, None, None]: - """Create a mock streaming response.""" - response = mocker.Mock() - yield response - class TestParseStreamingResponse: - def test_parse_complete_response(self, mock_response: Mock) -> None: + def test_parse_complete_response(self, mock_response: Any) -> None:
274-280:⚠️ Potential issue | 🟠 MajorAdd
Anytype annotation and import to the invalid-structure test.The test passes a string to
_parse_tool_call()which expectsdict[str, Any]. AnnotatetokenasAnyto make the call type-safe, and addAnyto the typing imports.🛠️ Proposed fix
-from typing import Generator +from typing import Any, Generator- token = "not a dict" + token: Any = "not a dict"
🤖 Fix all issues with AI agents
In `@tests/unit/core/api/test_streaming_parser.py`:
- Around line 45-50: The long JSON string literals in the test cases (e.g., the
lines list inside test_parse_response_with_tool_calls) are exceeding line-length
limits; update those literals by splitting them using implicit string
concatenation or building the JSON via helper variables (e.g., split the 'data:
{"event": ...}' strings across multiple adjacent quoted segments or construct
the dict then json.dumps it) to keep each source line under the linter limit;
apply the same fix to the other affected test blocks referenced (around lines
122-127, 138-167, 396-401, 411-417) so all long 'data: {...}' literals are
wrapped/split similarly.
- Around line 210-211: The test unpacks result into event and data but never
uses data, triggering Pylint W0612; change the unpack to use a throwaway name
(e.g., event, _ = result) or explicitly assert something about data if intended
to be checked, updating the line that currently reads "event, data = result" and
keeping the existing assertion "assert event == \"\"" and referencing the
variables result, event, and data in the test function.
| event, data = result | ||
| assert event == "" # Default empty string |
There was a problem hiding this comment.
Avoid unused data binding.
Pylint W0612 — use _ or assert on the value.
🛠️ Proposed fix
- event, data = result
+ event, _ = result📝 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.
| event, data = result | |
| assert event == "" # Default empty string | |
| event, _ = result | |
| assert event == "" # Default empty string |
🧰 Tools
🪛 GitHub Actions: Python linter
[error] 210-210: W0612: Unused variable 'data' (unused-variable)
🤖 Prompt for AI Agents
In `@tests/unit/core/api/test_streaming_parser.py` around lines 210 - 211, The
test unpacks result into event and data but never uses data, triggering Pylint
W0612; change the unpack to use a throwaway name (e.g., event, _ = result) or
explicitly assert something about data if intended to be checked, updating the
line that currently reads "event, data = result" and keeping the existing
assertion "assert event == \"\"" and referencing the variables result, event,
and data in the test function.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@tests/unit/core/api/test_streaming_parser.py`:
- Around line 3-5: Remove the import "from unittest.mock import Mock" and
replace every type annotation that uses Mock in the test functions with
typing.Any (e.g., change parameters annotated as Mock to Any) so the pytest-mock
"mocker" fixture is used without referencing unittest.mock; update all affected
test function signatures (the ones noted in the review) to use Any and ensure
"from typing import Any" remains as the only import for typing.
| from typing import Any | ||
| from unittest.mock import Mock | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n tests/unit/core/api/test_streaming_parser.py | head -30Repository: lightspeed-core/lightspeed-evaluation
Length of output: 1212
🏁 Script executed:
grep -n "def test_" tests/unit/core/api/test_streaming_parser.py | head -20Repository: lightspeed-core/lightspeed-evaluation
Length of output: 1501
🏁 Script executed:
find tests -name "conftest.py" -exec cat -n {} \;Repository: lightspeed-core/lightspeed-evaluation
Length of output: 27967
🏁 Script executed:
sed -n '133,137p' tests/unit/core/api/test_streaming_parser.py
sed -n '156,160p' tests/unit/core/api/test_streaming_parser.pyRepository: lightspeed-core/lightspeed-evaluation
Length of output: 495
Remove unittest.mock import; use pytest-mock fixture typing instead.
The test file imports Mock from unittest.mock at line 4, but tests use the mocker fixture (which creates mocks via pytest-mock). Replace all Mock type hints with Any and remove the unittest.mock import.
🔧 Example adjustment (apply to all test signatures)
-from unittest.mock import Mock
+from typing import Any
...
- def test_parse_complete_response(self, mock_response: Mock) -> None:
+ def test_parse_complete_response(self, mock_response: Any) -> None:As per coding guidelines: Import mocking utilities from pytest (mocker fixture) rather than from unittest.mock.
Applies to all affected test methods: lines 19, 37, 54, 64, 74, 84, 100, 115, 133, 156.
🤖 Prompt for AI Agents
In `@tests/unit/core/api/test_streaming_parser.py` around lines 3 - 5, Remove the
import "from unittest.mock import Mock" and replace every type annotation that
uses Mock in the test functions with typing.Any (e.g., change parameters
annotated as Mock to Any) so the pytest-mock "mocker" fixture is used without
referencing unittest.mock; update all affected test function signatures (the
ones noted in the review) to use Any and ensure "from typing import Any" remains
as the only import for typing.
There was a problem hiding this comment.
@asamal4, sounds good! Would you like me to create a follow-up issue to track this refactoring task?
🧠 Learnings used
Learnt from: CR
Repo: lightspeed-core/lightspeed-evaluation PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-12T11:01:11.398Z
Learning: Applies to tests/**/*.py : Import mocking utilities from pytest (mocker fixture) rather than from unittest.mock
Learnt from: CR
Repo: lightspeed-core/lightspeed-evaluation PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-12T11:01:11.398Z
Learning: Applies to tests/**/*.py : Use pytest mocking with mocker fixture instead of unittest.mock
Learnt from: CR
Repo: lightspeed-core/lightspeed-evaluation PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-12T11:01:11.398Z
Learning: Applies to tests/**/*.py : Mock LLM calls in tests using pytest mocker fixture
Learnt from: asamal4
Repo: lightspeed-core/lightspeed-evaluation PR: 47
File: src/lightspeed_evaluation/core/output/generator.py:140-145
Timestamp: 2025-09-11T12:47:06.747Z
Learning: User asamal4 prefers that non-critical comments are sent when actual code changes are pushed, not on unrelated commits.
|
@asamal4 i think this is ready now |
2 participants
Description
Adds support for the current LCORE tool_call response format, e.g:
Type of change
Tools used to create PR
Identify any AI code assistants used in this PR (for transparency and review context)
Related Tickets & Documents
Checklist before requesting a review
Testing
Verified by running evals against a current LCORE deployment (v0.4+)
Summary by CodeRabbit