-
Notifications
You must be signed in to change notification settings - Fork 1
Add bounds to streaming tool call buffer #651
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: dev
Are you sure you want to change the base?
Conversation
WalkthroughAdds per-stream buffer cap enforcement to ToolCallRepairProcessor and exposes max_buffer_bytes configuration via ToolCallRepairService. Processor computes and caches a cap, enforces it per stream (UTF-8 byte-sized, trims oldest on overflow, special cases 0 and negative), and integrates it into processing. Tests cover overflow and zero-cap behaviors. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant Stream as Stream Source
participant Processor as ToolCallRepairProcessor
participant Service as ToolCallRepairService
participant Repair as Repair Logic
Note over Processor,Service: Initialization
Service->>Service: Normalize max_buffer_bytes (int/default/clamp)
Processor->>Service: Read max_buffer_bytes
Processor->>Processor: Cache resolved buffer cap
Client->>Stream: Send chunk(s)
loop For each chunk
Stream->>Processor: on_chunk(data)
Processor->>Processor: Append to per-stream buffer
Processor->>Processor: Enforce cap (UTF-8 size, trim oldest if exceeded)
alt Cap == 0
Processor->>Processor: Drop buffer and warn
else Cap < 0
Processor->>Processor: Allow unlimited
end
Processor->>Repair: Attempt tool_call repair with current buffer
Repair-->>Processor: Result/No-op
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/core/services/streaming/tool_call_repair_processor.py (1)
141-142: Remove unreachable negative cap check.Lines 141-142 check for
cap < 0, but this condition is unreachable because_resolve_buffer_cap(line 121-126) already normalizes negative values to 0 during initialization.Apply this diff to remove the dead code:
- if cap < 0: - return buffer - if not buffer:
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/core/services/streaming/tool_call_repair_processor.py(3 hunks)src/core/services/tool_call_repair_service.py(1 hunks)tests/unit/core/services/test_tool_call_repair.py(2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Avoid using emojis in Python code comments or docstrings
Follow PEP 8 and use type hints for all functions
Import order must be: standard library, third-party, then local imports, separated by blank lines
Naming conventions: snake_case for variables and functions; PascalCase for classes
Error handling: use specific exceptions and include meaningful error messages
Prefer f-strings for string formatting
Files:
src/core/services/tool_call_repair_service.pysrc/core/services/streaming/tool_call_repair_processor.pytests/unit/core/services/test_tool_call_repair.py
src/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
src/**/*.py: Catch specific exceptions; avoid broad except Exception blocks
If a broad exception must be caught, log with exc_info=True and re-raise a specific custom exception
Use the most specific exception class from src.core.common.exceptions that accurately describes the error
Provide clear, helpful error messages and include relevant details in the details dictionary of custom exceptions
Files:
src/core/services/tool_call_repair_service.pysrc/core/services/streaming/tool_call_repair_processor.py
🧬 Code graph analysis (2)
src/core/services/streaming/tool_call_repair_processor.py (1)
src/core/interfaces/tool_call_repair_service_interface.py (1)
IToolCallRepairService(7-21)
tests/unit/core/services/test_tool_call_repair.py (3)
src/core/domain/streaming_content.py (1)
StreamingContent(10-158)src/core/services/tool_call_repair_service.py (2)
ToolCallRepairService(12-218)max_buffer_bytes(45-48)src/core/services/streaming/tool_call_repair_processor.py (2)
ToolCallRepairProcessor(18-160)process(29-105)
⏰ 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). (1)
- GitHub Check: test (3.10)
🔇 Additional comments (7)
src/core/services/tool_call_repair_service.py (1)
38-48: LGTM! Clean buffer cap implementation.The initialization and property exposure are well-implemented:
- Defensive casting with sensible defaults (64 KiB)
- Proper handling of negative values (clamped to 0)
- Clear documentation
tests/unit/core/services/test_tool_call_repair.py (2)
6-6: Good refactoring.Moving the
StreamingContentimport to module level follows Python best practices and improves readability.
154-195: Excellent test coverage for buffer cap enforcement.These tests comprehensively verify the new buffering behavior:
- Truncation when the cap is exceeded
- Buffer dropping when cap is zero
- Proper interaction with the repair service
The tests validate both the stored buffer state and the data passed to the repair logic, ensuring end-to-end correctness.
src/core/services/streaming/tool_call_repair_processor.py (4)
27-27: Good initialization.Caching the buffer cap at initialization time is efficient and follows the principle of computing configuration once.
40-40: Correct enforcement placement.Enforcing the cap immediately after buffer accumulation ensures that the buffer never exceeds the configured limit before being processed.
107-127: Robust buffer cap resolution.The defensive implementation handles invalid inputs gracefully:
- Safe attribute access with default fallback
- Type validation with fallback to default
- Normalization of negative values to zero with warning
This ensures the processor always has a valid cap regardless of the service implementation.
129-160: Solid enforcement logic with correct truncation strategy.The implementation correctly:
- Drops buffers entirely when cap is zero (lines 133-139)
- Measures buffer size in UTF-8 bytes for accuracy (lines 147-148)
- Keeps the newest data by truncating from the end (line 152), which is appropriate for streaming scenarios
- Handles UTF-8 decoding errors gracefully (line 160)
- Provides clear warning logs with relevant details (lines 154-159)
The truncation strategy (keeping the last
capbytes) ensures the most recent streaming data is retained, which aligns well with the goal of detecting tool calls in ongoing streams.
Summary
ToolCallRepairServiceTesting
https://chatgpt.com/codex/tasks/task_e_68ec26d70c8083338f26e9e0caaa409e
Summary by CodeRabbit