Repository navigation
Feat/chat summary compact phase - #1525
Merged
qdaxb merged 12 commits intoJun 23, 2026
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughThis PR introduces LLM-based summary compaction as a primary context-reduction strategy, propagates ChangesSummary Compaction, Termination Reason & Compaction UI
Thinking Block Deduplication Fix
Sequence Diagram(s)sequenceDiagram
participant ChatService
participant UnifiedContextGuard
participant SummaryCompactor
participant LLM
participant ContextMetricsTracker
participant Socket
participant StreamingState
participant Emitter
ChatService->>UnifiedContextGuard: __call__(state, view)
UnifiedContextGuard->>UnifiedContextGuard: _current_used_input_tokens >= trigger_limit?
UnifiedContextGuard->>ContextMetricsTracker: emit_status(phase=started, context_compaction={status:started})
Socket-->>Socket: chat:status_updated {context_compaction:{status:started}}
UnifiedContextGuard->>SummaryCompactor: compact(messages, preserve_initial_context)
SummaryCompactor->>LLM: ainvoke(system+prior+compact prompt)
LLM-->>SummaryCompactor: summary_text
SummaryCompactor-->>UnifiedContextGuard: SummaryCompactResult
UnifiedContextGuard->>ContextMetricsTracker: emit_status(phase=completed, context_compaction={status:completed})
Socket-->>Socket: chat:status_updated {context_compaction:{status:completed}}
ChatService->>StreamingState: state.context_compactions = guard.context_compactions
ChatService->>StreamingState: state.termination_reason = agent._last_termination_reason
StreamingState->>Emitter: finalize() → done(context_compactions, termination_reason)
sequenceDiagram
participant Socket
participant useChatStatusIndicator
participant ChatArea
participant MessagesArea
participant MessageBubble
Socket->>useChatStatusIndicator: onChatStatusUpdated(context_compaction={status:started})
useChatStatusIndicator->>useChatStatusIndicator: compactingTaskIds[taskId]=true
ChatArea->>useChatStatusIndicator: isCompacting=true
ChatArea->>ChatArea: compactingWaitMessage=t(chat_status.compacting)
ChatArea->>MessagesArea: waitingMessage=compactingWaitMessage
MessagesArea->>MessageBubble: waitingMessage
MessageBubble->>MessageBubble: StreamingWaitIndicator(message=waitingMessage)
Socket->>useChatStatusIndicator: onChatChunk/onChatMessage/onChatDone
useChatStatusIndicator->>useChatStatusIndicator: clearCompacting(taskId)
ChatArea->>MessagesArea: waitingMessage=undefined
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed due to a network error. 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/app/services/execution/emitters/websocket.py (1)
266-274: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winPersist
context_compactionin the cached status snapshot as well.
_emit_status_updated()emitscontext_compactionlive, but the cached snapshot omits it. Any reconnect/recovery flow using cached status can lose compacting state.🐛 Proposed fix
await session_manager.save_context_metrics( event.subtask_id, { "task_id": event.task_id, "subtask_id": event.subtask_id, "phase": phase, "context_metrics": context_metrics, + "context_compaction": ( + context_compaction + if isinstance(context_compaction, dict) + else None + ), }, )🤖 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 `@backend/app/services/execution/emitters/websocket.py` around lines 266 - 274, The context_compaction state is being emitted live but is not included in the cached status snapshot saved by session_manager.save_context_metrics(). Add context_compaction to the dictionary argument passed to session_manager.save_context_metrics() alongside the existing fields like task_id, subtask_id, phase, and context_metrics to ensure the compacting state persists in the cache for recovery flows.
🧹 Nitpick comments (11)
chat_shell/tests/test_context_guard.py (1)
1036-1042: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert compaction strategy type alongside status in runtime-event checks.
These assertions only validate
status; addingtype == "summary_compact"tightens the contract and avoids false positives if other compaction event types are emitted later.Suggested test tightening
- compaction_statuses = [ - call.kwargs["context_compaction"]["status"] + compaction_events = [ + call.kwargs["context_compaction"] for call in emitter.status_updated.await_args_list if call.kwargs.get("context_compaction") is not None ] - assert compaction_statuses == ["started", "completed"] + assert [event["type"] for event in compaction_events] == [ + "summary_compact", + "summary_compact", + ] + assert [event["status"] for event in compaction_events] == [ + "started", + "completed", + ]- compaction_statuses = [ - call.kwargs["context_compaction"]["status"] + compaction_events = [ + call.kwargs["context_compaction"] for call in emitter.status_updated.await_args_list if call.kwargs.get("context_compaction") is not None ] - assert compaction_statuses == ["started", "fallback"] + assert [event["type"] for event in compaction_events] == [ + "summary_compact", + "summary_compact", + ] + assert [event["status"] for event in compaction_events] == [ + "started", + "fallback", + ]Also applies to: 1069-1074
🤖 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 `@chat_shell/tests/test_context_guard.py` around lines 1036 - 1042, The compaction status assertions only validate the status field but should also verify that the compaction type is "summary_compact" to avoid false positives from other compaction event types. Add an additional filter condition in the list comprehension that extracts compaction_statuses to check that call.kwargs["context_compaction"]["type"] == "summary_compact", and apply this same tightening to the second similar assertion block mentioned in the comment (around lines 1069-1074) to ensure both test validations check both the type and status fields.chat_shell/tests/test_truncation_detection.py (1)
104-104: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winComplete the new test signature/docs for guideline compliance.
test_get_current_result_includes_context_compactionsis missing an explicit-> Noneannotation and a docstring. As per coding guidelines,**/*.pymust "Use type hints for all function parameters and return values" and "include docstrings for all public functions and classes".🤖 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 `@chat_shell/tests/test_truncation_detection.py` at line 104, The function test_get_current_result_includes_context_compactions is missing required type hints and documentation. Add an explicit `-> None` return type annotation to the function signature and include a docstring that describes what the test verifies. This ensures compliance with the coding guidelines that require type hints for all function return values and docstrings for all public functions.Source: Coding guidelines
chat_shell/tests/test_session.py (1)
142-144: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a return type annotation to the new async test.
test_finalize_includes_messages_chain_and_context_compactionsshould declare-> Nonefor consistency with the Python typing rule. As per coding guidelines,**/*.pymust "Use type hints for all function parameters and return values".🤖 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 `@chat_shell/tests/test_session.py` around lines 142 - 144, Add a return type annotation of `-> None` to the `test_finalize_includes_messages_chain_and_context_compactions` method signature to comply with the Python typing requirement that all function parameters and return values must have type hints. This should be added after the method parameters and before the colon in the method definition.Source: Coding guidelines
chat_shell/tests/test_knowledge_injection_strategy_clean.py (1)
261-261: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd an explicit return type annotation on the new test method.
test_reserved_output_tokens_align_with_context_governance_budgetshould use-> Nonefor guideline compliance and consistency with typed test signatures. As per coding guidelines,**/*.pymust "Use type hints for all function parameters and return values".🤖 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 `@chat_shell/tests/test_knowledge_injection_strategy_clean.py` at line 261, The test method `test_reserved_output_tokens_align_with_context_governance_budget` is missing an explicit return type annotation. Add `-> None` to the method signature after the closing parenthesis of the method parameters to comply with the coding guideline that requires type hints for all function return values.Source: Coding guidelines
chat_shell/tests/test_new_messages_from_state.py (1)
242-243: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAnnotate the new test methods with
-> None.Both newly added test methods in
TestLimitRecoveryMessagesChainare missing explicit return type annotations. As per coding guidelines,**/*.pymust "Use type hints for all function parameters and return values".Also applies to: 257-258
🤖 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 `@chat_shell/tests/test_new_messages_from_state.py` around lines 242 - 243, Add explicit return type annotations to the two newly added test methods in the TestLimitRecoveryMessagesChain class. Both test_falls_back_to_limit_notice_when_collected_state_missing and the test method around line 257-258 are missing `-> None` return type annotations in their method signatures. Update each method definition to include `-> None` after the parameter list and before the colon to comply with the coding guidelines requiring type hints for all function return values.Source: Coding guidelines
backend/tests/services/chat/storage/test_session_blocks.py (1)
270-270: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd missing docstrings and
-> Noneannotations to the new async tests.The four new async test functions are missing explicit return type annotations and docstrings; aligning these keeps test style consistent with repo Python standards. As per coding guidelines,
**/*.pymust "Use type hints for all function parameters and return values" and "include docstrings for all public functions and classes".Also applies to: 308-308, 336-336, 388-388
🤖 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 `@backend/tests/services/chat/storage/test_session_blocks.py` at line 270, The four new async test functions at lines 270, 308, 336, and 388 are missing both return type annotations and docstrings. Add `-> None` return type annotation to each of these async test function signatures (test_finalize_and_get_blocks_marks_unresolved_preview_tool_blocks_error, and the three other async test functions) and add descriptive docstrings to each function body that explain what the test is validating, following the existing docstring style used in the test file for consistency with Python coding standards.Source: Coding guidelines
chat_shell/tests/test_serialize_messages_chain.py (1)
208-209: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd
-> Nonereturn annotations for the newly added tests.
test_summary_compacted_markers_are_persistedandtest_detects_summary_compacted_live_stateshould be explicitly annotated. As per coding guidelines,**/*.pymust "Use type hints for all function parameters and return values".Also applies to: 231-232
🤖 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 `@chat_shell/tests/test_serialize_messages_chain.py` around lines 208 - 209, Add `-> None` return type annotations to the two test methods `test_summary_compacted_markers_are_persisted` and `test_detects_summary_compacted_live_state` in the test class. Both methods are missing explicit return type hints which are required by the coding guidelines for all functions. Update the method definitions to include `-> None` after the parameter list and before the colon.Source: Coding guidelines
chat_shell/chat_shell/compression/summary_compactor.py (1)
148-203: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTrace the new compaction model-call path.
compact()/_generate_summary()are the new LLM-backed rewrite path; add the shared async tracing decorator or span events so compaction latency and failures are correlated with the turn. As per coding guidelines, “Use@trace_asyncor@trace_syncdecorators from shared.telemetry.decorators for tracing async and sync functions respectively; use add_span_event() and set_span_attribute() for adding details to spans.”🤖 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 `@chat_shell/chat_shell/compression/summary_compactor.py` around lines 148 - 203, The async methods compact() and _generate_summary() lack tracing instrumentation for the new LLM-backed rewrite path. Import the `@trace_async` decorator from shared.telemetry.decorators and apply it to both the compact() method and _generate_summary() method to enable tracing of compaction latency and failures. If additional context is needed, use add_span_event() and set_span_attribute() from the telemetry module to annotate the spans with relevant details.Source: Coding guidelines
chat_shell/chat_shell/guard/context_guard.py (1)
707-827: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftSplit and trace the summary-compaction stage.
_apply_summary_compaction_passnow handles event construction, metrics snapshots, fallback records, rewrite generation, and logging in one ~120-line function. Extract small helpers for event/record/update construction and add tracing around the async stage. As per coding guidelines, “Functions should not exceed 50 lines of code (preferred maximum)” and “Use@trace_asyncor@trace_syncdecorators from shared.telemetry.decorators for tracing async and sync functions respectively.”🤖 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 `@chat_shell/chat_shell/guard/context_guard.py` around lines 707 - 827, The `_apply_summary_compaction_pass` function exceeds 120 lines and combines multiple responsibilities including event/record construction, metrics snapshots, fallback handling, and logging. Refactor by extracting helper functions for distinct tasks: one for constructing the initial ContextCompactionEvent when the compaction starts, one for handling the exception case (determining failure_reason, appending to _context_compactions, and emitting fallback status), one for constructing the replacement_view and updates from the successful result, and one for constructing the final ContextCompactionEvent and logging when compaction completes successfully. Keep each helper function under 50 lines and add the `@trace_async` decorator to the main `_apply_summary_compaction_pass` function and `@trace_sync` decorators to the helper functions for proper tracing as per coding guidelines.Source: Coding guidelines
chat_shell/chat_shell/agents/graph_builder.py (1)
295-311: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd tracing decorator to the new async usage-callback helper.
_invoke_model_usage_callback()is a new async execution path but currently untraced, which weakens observability for provider-usage baseline failures.♻️ Proposed change
+@trace_async( + span_name="agent_builder.invoke_model_usage_callback", + tracer_name="chat_shell.agents", +) async def _invoke_model_usage_callback( callback: Callable[..., Any], messages: list[dict[str, Any]], *, input_tokens: int, ) -> None:As per coding guidelines, "
**/*.py: ... Use@trace_asyncor@trace_syncdecorators from shared.telemetry.decorators for tracing async and sync functions respectively ...".🤖 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 `@chat_shell/chat_shell/agents/graph_builder.py` around lines 295 - 311, The `_invoke_model_usage_callback()` async function is missing the required tracing decorator for observability. Add the `@trace_async` decorator from shared.telemetry.decorators to the top of the `_invoke_model_usage_callback()` function definition to enable tracing of this async execution path and improve observability for provider-usage baseline failures.Source: Coding guidelines
backend/app/api/ws/chat_namespace.py (1)
91-97: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd tracing to this new async failure-finalization path.
This helper persists terminal failed state for background AI triggers, but it is not decorated, so this failure path will be missing its own span.
As per coding guidelines, “Use
@trace_asyncor@trace_syncdecorators from shared.telemetry.decorators for tracing async and sync functions respectively; use add_span_event() and set_span_attribute() for adding details to spans.”🤖 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 `@backend/app/api/ws/chat_namespace.py` around lines 91 - 97, The _finalize_failed_ai_trigger async function is missing a tracing decorator which means this failure-finalization path will not generate its own span. Add the `@trace_async` decorator from shared.telemetry.decorators above the function definition to enable tracing for this background AI trigger failure path, following the established coding guidelines for async functions.Source: Coding guidelines
🤖 Prompt for all review comments with 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.
Inline comments:
In `@backend/app/services/chat/trigger/lifecycle.py`:
- Around line 421-433: The termination_reason is correctly normalized from
runtime_result with a fallback to existing_result before being passed to
finalize_and_get_blocks, but when final_result is built later in the code, it
likely uses the unnormalized runtime_result value instead of the normalized
termination_reason variable. To fix this inconsistency, ensure that the
final_result dictionary is constructed using the normalized termination_reason
variable that was already computed and used for finalize_and_get_blocks, so that
the persisted result contains the same valid termination_reason value as the
finalized blocks.
In `@chat_shell/chat_shell/compression/config.py`:
- Around line 82-87: The target_limit method currently returns min(base_limit,
self.trigger_limit) when auto_compact_token_limit is not None, which can result
in the target equaling the trigger_limit. Since the metrics check uses >=
trigger_limit to determine an over-trigger condition, a successful compaction
landing on its target would immediately re-trigger compaction. Fix this by
ensuring the returned value is strictly less than self.trigger_limit when
applying the auto_compact_token_limit constraint, such as subtracting 1 or using
a similar adjustment to keep the target below the trigger threshold.
In `@chat_shell/chat_shell/compression/summary_compactor.py`:
- Around line 196-203: The `_generate_summary` method does not validate whether
the LLM response is empty, which would result in losing conversation history by
returning a blank summary. After calling `_extract_text(result).strip()`, add a
validation check to ensure the returned summary string is not empty. If it is
empty, raise `SummaryCompactNotApplicable` instead of returning the empty
string, so the system falls back to appropriate error handling instead of
silently replacing history with blank content.
In `@chat_shell/chat_shell/guard/context_guard.py`:
- Around line 1011-1012: The _is_over_trigger method uses a strict greater-than
comparison (>) while calculate_context_metrics uses a greater-than-or-equal-to
comparison (>=) for the trigger boundary, causing inconsistency at exactly the
trigger limit. Change the comparison operator in the return statement of
_is_over_trigger from > to >= so that both methods consistently trigger at the
same boundary condition when used_input_tokens reaches or exceeds trigger_limit.
- Around line 780-789: Add a new static helper method _ensure_message_ids to the
class that iterates through a list of messages and assigns a UUID string ID to
any message that lacks an ID attribute. Then, before the existing code that
creates the replacement_view and builds the removal updates list, call this
helper on both original_messages and result.replacement_history to ensure all
messages have stable IDs assigned. This ensures that RemoveMessage operations
are created for all original messages and that summary_message_id is properly
captured with a valid ID.
In `@chat_shell/chat_shell/services/chat_service.py`:
- Around line 320-323: The condition that checks
request.enable_tool_output_guard allows stale user preferences to override the
always-on tool guard requirement, meaning users who previously disabled this
setting will continue to bypass tool-output compaction. Remove the
request.enable_tool_output_guard check from this conditional statement so that
only the deployment-level kill switch settings.DISABLE_TOOL_OUTPUT_GUARD gates
whether the tool output guard runs, or alternatively remove the preference read
entirely from the upstream TaskRequestBuilder._is_tool_output_guard_enabled()
method to prevent stale settings from being propagated to this decision point.
In `@frontend/src/__tests__/features/tasks/hooks/useChatStatusIndicator.test.tsx`:
- Around line 16-22: The registeredHandlers object contains duplicate property
declarations for each handler (onChatChunk, onChatMessage, onBlockCreated,
onBlockUpdated, onChatDone, onChatError, and onChatCancelled), which causes
TypeScript duplicate property errors. Remove all duplicate handler declarations
in the registeredHandlers object, keeping only a single declaration for each
handler with the appropriate parameter types for task_id (either optional or
required as needed for each handler).
In `@frontend/src/features/tasks/components/input/ChatToolbarStatus.tsx`:
- Around line 271-332: The icon-only status trigger buttons at lines 277 and 304
lack accessible names, causing screen readers to announce them as unnamed
controls. Add an aria-label attribute to both Button components that contain
only the Coins icon (the first one with variant="ghost" size="icon" and the
second one with variant="ghost" and gap-1.5 className) to provide screen readers
with descriptive button names, using the same tooltip translation key
common:chat_status.tooltip or similar accessible description.
---
Outside diff comments:
In `@backend/app/services/execution/emitters/websocket.py`:
- Around line 266-274: The context_compaction state is being emitted live but is
not included in the cached status snapshot saved by
session_manager.save_context_metrics(). Add context_compaction to the dictionary
argument passed to session_manager.save_context_metrics() alongside the existing
fields like task_id, subtask_id, phase, and context_metrics to ensure the
compacting state persists in the cache for recovery flows.
---
Nitpick comments:
In `@backend/app/api/ws/chat_namespace.py`:
- Around line 91-97: The _finalize_failed_ai_trigger async function is missing a
tracing decorator which means this failure-finalization path will not generate
its own span. Add the `@trace_async` decorator from shared.telemetry.decorators
above the function definition to enable tracing for this background AI trigger
failure path, following the established coding guidelines for async functions.
In `@backend/tests/services/chat/storage/test_session_blocks.py`:
- Line 270: The four new async test functions at lines 270, 308, 336, and 388
are missing both return type annotations and docstrings. Add `-> None` return
type annotation to each of these async test function signatures
(test_finalize_and_get_blocks_marks_unresolved_preview_tool_blocks_error, and
the three other async test functions) and add descriptive docstrings to each
function body that explain what the test is validating, following the existing
docstring style used in the test file for consistency with Python coding
standards.
In `@chat_shell/chat_shell/agents/graph_builder.py`:
- Around line 295-311: The `_invoke_model_usage_callback()` async function is
missing the required tracing decorator for observability. Add the `@trace_async`
decorator from shared.telemetry.decorators to the top of the
`_invoke_model_usage_callback()` function definition to enable tracing of this
async execution path and improve observability for provider-usage baseline
failures.
In `@chat_shell/chat_shell/compression/summary_compactor.py`:
- Around line 148-203: The async methods compact() and _generate_summary() lack
tracing instrumentation for the new LLM-backed rewrite path. Import the
`@trace_async` decorator from shared.telemetry.decorators and apply it to both the
compact() method and _generate_summary() method to enable tracing of compaction
latency and failures. If additional context is needed, use add_span_event() and
set_span_attribute() from the telemetry module to annotate the spans with
relevant details.
In `@chat_shell/chat_shell/guard/context_guard.py`:
- Around line 707-827: The `_apply_summary_compaction_pass` function exceeds 120
lines and combines multiple responsibilities including event/record
construction, metrics snapshots, fallback handling, and logging. Refactor by
extracting helper functions for distinct tasks: one for constructing the initial
ContextCompactionEvent when the compaction starts, one for handling the
exception case (determining failure_reason, appending to _context_compactions,
and emitting fallback status), one for constructing the replacement_view and
updates from the successful result, and one for constructing the final
ContextCompactionEvent and logging when compaction completes successfully. Keep
each helper function under 50 lines and add the `@trace_async` decorator to the
main `_apply_summary_compaction_pass` function and `@trace_sync` decorators to the
helper functions for proper tracing as per coding guidelines.
In `@chat_shell/tests/test_context_guard.py`:
- Around line 1036-1042: The compaction status assertions only validate the
status field but should also verify that the compaction type is
"summary_compact" to avoid false positives from other compaction event types.
Add an additional filter condition in the list comprehension that extracts
compaction_statuses to check that call.kwargs["context_compaction"]["type"] ==
"summary_compact", and apply this same tightening to the second similar
assertion block mentioned in the comment (around lines 1069-1074) to ensure both
test validations check both the type and status fields.
In `@chat_shell/tests/test_knowledge_injection_strategy_clean.py`:
- Line 261: The test method
`test_reserved_output_tokens_align_with_context_governance_budget` is missing an
explicit return type annotation. Add `-> None` to the method signature after the
closing parenthesis of the method parameters to comply with the coding guideline
that requires type hints for all function return values.
In `@chat_shell/tests/test_new_messages_from_state.py`:
- Around line 242-243: Add explicit return type annotations to the two newly
added test methods in the TestLimitRecoveryMessagesChain class. Both
test_falls_back_to_limit_notice_when_collected_state_missing and the test method
around line 257-258 are missing `-> None` return type annotations in their
method signatures. Update each method definition to include `-> None` after the
parameter list and before the colon to comply with the coding guidelines
requiring type hints for all function return values.
In `@chat_shell/tests/test_serialize_messages_chain.py`:
- Around line 208-209: Add `-> None` return type annotations to the two test
methods `test_summary_compacted_markers_are_persisted` and
`test_detects_summary_compacted_live_state` in the test class. Both methods are
missing explicit return type hints which are required by the coding guidelines
for all functions. Update the method definitions to include `-> None` after the
parameter list and before the colon.
In `@chat_shell/tests/test_session.py`:
- Around line 142-144: Add a return type annotation of `-> None` to the
`test_finalize_includes_messages_chain_and_context_compactions` method signature
to comply with the Python typing requirement that all function parameters and
return values must have type hints. This should be added after the method
parameters and before the colon in the method definition.
In `@chat_shell/tests/test_truncation_detection.py`:
- Line 104: The function test_get_current_result_includes_context_compactions is
missing required type hints and documentation. Add an explicit `-> None` return
type annotation to the function signature and include a docstring that describes
what the test verifies. This ensures compliance with the coding guidelines that
require type hints for all function return values and docstrings for all public
functions.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: b62dfbdc-0940-4add-8724-8f465b342685
📒 Files selected for processing (60)
backend/app/api/ws/chat_namespace.pybackend/app/core/config.pybackend/app/services/chat/storage/session.pybackend/app/services/chat/trigger/lifecycle.pybackend/app/services/chat/webpage_ws_chat_emitter.pybackend/app/services/execution/dispatcher.pybackend/app/services/execution/emitters/status_updating.pybackend/app/services/execution/emitters/websocket.pybackend/app/services/execution/inprocess_executor.pybackend/app/services/execution/request_builder.pybackend/tests/api/ws/test_chat_namespace_task_join.pybackend/tests/core/test_config.pybackend/tests/services/chat/storage/test_session_blocks.pybackend/tests/services/execution/test_emitters.pybackend/tests/services/execution/test_extract_completed_result.pybackend/tests/services/execution/test_request_builder_user_mcp.pybackend/tests/services/execution/test_responses_api_event_parser.pybackend/tests/services/execution/test_status_updating_emitter.pychat_shell/chat_shell/agent.pychat_shell/chat_shell/agents/graph_builder.pychat_shell/chat_shell/compression/config.pychat_shell/chat_shell/compression/context_metrics.pychat_shell/chat_shell/compression/summary_compactor.pychat_shell/chat_shell/core/config.pychat_shell/chat_shell/guard/context_guard.pychat_shell/chat_shell/services/chat_service.pychat_shell/chat_shell/services/streaming/core.pychat_shell/chat_shell/tools/builtin/knowledge_base.pychat_shell/tests/test_compression.pychat_shell/tests/test_context_guard.pychat_shell/tests/test_context_metrics.pychat_shell/tests/test_graph_builder_usage_metrics.pychat_shell/tests/test_knowledge_injection_strategy.pychat_shell/tests/test_knowledge_injection_strategy_clean.pychat_shell/tests/test_new_messages_from_state.pychat_shell/tests/test_serialize_messages_chain.pychat_shell/tests/test_session.pychat_shell/tests/test_summary_compactor.pychat_shell/tests/test_truncation_detection.pyfrontend/src/__tests__/features/tasks/components/chat/ChatArea.pipeline-next-step.test.tsxfrontend/src/__tests__/features/tasks/components/chat/ChatArea.queue-message-handler.test.tsxfrontend/src/__tests__/features/tasks/components/input/ChatToolbarStatus.test.tsxfrontend/src/__tests__/features/tasks/hooks/useChatStatusIndicator.test.tsxfrontend/src/features/settings/components/NotificationSettings.tsxfrontend/src/features/tasks/components/chat/ChatArea.tsxfrontend/src/features/tasks/components/input/ChatToolbarStatus.tsxfrontend/src/features/tasks/components/message/MessageBubble.tsxfrontend/src/features/tasks/components/message/MessagesArea.tsxfrontend/src/features/tasks/components/message/thinking/MixedContentView.tsxfrontend/src/features/tasks/hooks/useChatStatusIndicator.tsfrontend/src/i18n/locales/en/common.jsonfrontend/src/i18n/locales/zh-CN/common.jsonfrontend/src/types/api.tsfrontend/src/types/socket.tspackages/chat-core/src/task-state/TaskStateMachine.blockMerging.tspackages/chat-core/src/task-state/TaskStateMachine.test.tsshared/models/execution.pyshared/models/openai_converter.pyshared/models/responses_api.pyshared/models/responses_api_emitter.py
💤 Files with no reviewable changes (3)
- backend/tests/core/test_config.py
- backend/app/core/config.py
- frontend/src/features/settings/components/NotificationSettings.tsx
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
chat_shell/chat_shell/services/chat_service.py (1)
349-352: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the summary compaction ratio into a named constant.
0.5in the newrecent_user_token_limitcalculation is a magic number; promote it to a module-level constant so tuning stays explicit and centralized.♻️ Proposed change
+SUMMARY_RECENT_USER_TARGET_RATIO = 0.5 + ... summary_compactor = SummaryCompactor( llm=summary_llm, token_counter=guard_counter, recent_user_token_limit=min( DEFAULT_RECENT_USER_TOKEN_LIMIT, - max(1, int(context_config.target_limit * 0.5)), + max( + 1, + int( + context_config.target_limit + * SUMMARY_RECENT_USER_TARGET_RATIO + ), + ), ), max_compact_input_tokens=context_config.available_tokens, )As per coding guidelines, "Extract magic numbers to named constants".
🤖 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 `@chat_shell/chat_shell/services/chat_service.py` around lines 349 - 352, The magic number 0.5 used in the recent_user_token_limit calculation needs to be extracted to a module-level constant. Define a new constant at the top of the chat_service.py module with a descriptive name that reflects its purpose as the summary compaction ratio, then replace the hardcoded 0.5 value in the recent_user_token_limit calculation with this named constant to make the tuning explicit and centralized.Source: Coding guidelines
🤖 Prompt for all review comments with 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.
Nitpick comments:
In `@chat_shell/chat_shell/services/chat_service.py`:
- Around line 349-352: The magic number 0.5 used in the recent_user_token_limit
calculation needs to be extracted to a module-level constant. Define a new
constant at the top of the chat_service.py module with a descriptive name that
reflects its purpose as the summary compaction ratio, then replace the hardcoded
0.5 value in the recent_user_token_limit calculation with this named constant to
make the tuning explicit and centralized.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f36a6669-d27c-4c6d-9818-a7546d58d1bb
📒 Files selected for processing (15)
backend/app/services/chat/trigger/lifecycle.pybackend/app/services/execution/emitters/websocket.pybackend/app/services/execution/request_builder.pybackend/tests/api/ws/test_chat_namespace_task_join.pybackend/tests/services/chat/trigger/test_lifecycle_completed_result.pybackend/tests/services/execution/test_emitters.pybackend/tests/services/execution/test_request_builder_user_mcp.pychat_shell/chat_shell/compression/config.pychat_shell/chat_shell/compression/summary_compactor.pychat_shell/chat_shell/guard/context_guard.pychat_shell/chat_shell/services/chat_service.pychat_shell/tests/test_compression.pychat_shell/tests/test_context_guard.pychat_shell/tests/test_summary_compactor.pyfrontend/src/features/tasks/components/input/ChatToolbarStatus.tsx
🚧 Files skipped from review as they are similar to previous changes (10)
- backend/app/services/chat/trigger/lifecycle.py
- backend/tests/services/execution/test_emitters.py
- backend/app/services/execution/emitters/websocket.py
- chat_shell/tests/test_summary_compactor.py
- frontend/src/features/tasks/components/input/ChatToolbarStatus.tsx
- chat_shell/tests/test_compression.py
- chat_shell/tests/test_context_guard.py
- chat_shell/chat_shell/compression/config.py
- chat_shell/chat_shell/compression/summary_compactor.py
- chat_shell/chat_shell/guard/context_guard.py
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
frontend/src/contexts/SocketContext.tsx (1)
230-245: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueConsider clearing cached status on disconnect.
latestChatStatusRefis not cleared when the socket disconnects (unlikejoinedTasksRefat line 281). This means handlers registered after reconnection but before rejoining tasks will receive potentially stale cached status.Example scenario:
- Join task, cache status
{phase: 'running'}- Disconnect
- Reconnect
- Register handlers → receives stale
{phase: 'running'}- Rejoin task → receives fresh
{phase: 'done'}The window is brief and fresh status arrives after rejoin, but worth considering whether to clear the cache on disconnect:
if (!state.isConnected) { joinedTasksRef.current.clear() latestChatStatusRef.current = {} }🤖 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 `@frontend/src/contexts/SocketContext.tsx` around lines 230 - 245, The latestChatStatusRef cache is not being cleared when the socket disconnects, which can cause handlers to receive stale cached chat status after reconnection but before rejoining tasks. Find where joinedTasksRef is cleared on disconnect (around line 281 in the SocketProvider component) and add a statement to also clear latestChatStatusRef by setting it to an empty object at that same location. This ensures both the joined tasks set and the cached chat status are synchronized and reset together on disconnect.frontend/src/__tests__/contexts/SocketContext.reconnect.test.tsx (1)
44-53: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMock socket limitation: only supports one handler per event.
The mock implementation stores only one handler per event in
socketHandlersMap. Real Socket.IO allows multiple handlers per event. If future tests need to register multiple handlers for the same event, this mock will need enhancement to track an array of handlers per event.Current tests appear to work with this limitation.
🤖 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 `@frontend/src/__tests__/contexts/SocketContext.reconnect.test.tsx` around lines 44 - 53, The mock socket implementation uses a Map (socketHandlers) that stores only a single handler per event, whereas real Socket.IO supports multiple handlers for the same event. If future tests need to register multiple handlers for the same event, enhance the socketHandlers to store an array of handlers per event instead of a single handler, and update the off method to properly add handlers to the array in the on method and remove specific handlers from the array in the off method rather than deleting the entire event entry. This will align the mock behavior more closely with actual Socket.IO functionality.
🤖 Prompt for all review comments with 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.
Nitpick comments:
In `@frontend/src/__tests__/contexts/SocketContext.reconnect.test.tsx`:
- Around line 44-53: The mock socket implementation uses a Map (socketHandlers)
that stores only a single handler per event, whereas real Socket.IO supports
multiple handlers for the same event. If future tests need to register multiple
handlers for the same event, enhance the socketHandlers to store an array of
handlers per event instead of a single handler, and update the off method to
properly add handlers to the array in the on method and remove specific handlers
from the array in the off method rather than deleting the entire event entry.
This will align the mock behavior more closely with actual Socket.IO
functionality.
In `@frontend/src/contexts/SocketContext.tsx`:
- Around line 230-245: The latestChatStatusRef cache is not being cleared when
the socket disconnects, which can cause handlers to receive stale cached chat
status after reconnection but before rejoining tasks. Find where joinedTasksRef
is cleared on disconnect (around line 281 in the SocketProvider component) and
add a statement to also clear latestChatStatusRef by setting it to an empty
object at that same location. This ensures both the joined tasks set and the
cached chat status are synchronized and reset together on disconnect.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 248e7b1d-034f-4dbd-8188-6cccf4bae8e6
📒 Files selected for processing (6)
backend/app/api/ws/chat_namespace.pybackend/app/api/ws/events.pybackend/tests/api/ws/test_chat_namespace_task_join.pyfrontend/src/__tests__/contexts/SocketContext.reconnect.test.tsxfrontend/src/contexts/SocketContext.tsxfrontend/src/types/socket.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- backend/tests/api/ws/test_chat_namespace_task_join.py
- backend/app/api/ws/chat_namespace.py
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
2 participants
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.
Summary by CodeRabbit
New Features
termination_reasonsupport to chat status/completion payloads (reflected in UI/state).Bug Fixes
Improvements