feat: 集成 ChatAgent 到主流程并修复 CI 测试 (#183, #184) - #185
Conversation
- Run tests on Python 3.10, 3.11, 3.12 - Execute on PR and push to main - Generate coverage reports - Upload to Codecov
- core/agent/ 完整实现（ChatAgent/PlanAgent/ToolAgent，331 行） - core/main.py 新增 ChatLLMAdapter，将 ChatLLM 适配为 ChatAgent provider 接口 - 补齐 ChatAgent 38 个占位测试为真实测试 - 新增 test_chat_agent_integration.py 集成测试
📝 WalkthroughWalkthroughThe PR integrates stateless ChangesChatAgent runtime
TaleCore ChatAgent integration
Adapter and cache behavior
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant TaleCore
participant ChatAgent
participant ChatLLMAdapter
participant ChatLLM
participant SessionManager
TaleCore->>SessionManager: load persisted history
TaleCore->>ChatAgent: generate(history, session_id, timeout)
ChatAgent->>ChatLLMAdapter: chat(messages)
ChatLLMAdapter->>ChatLLM: forward system header and messages
ChatLLM-->>ChatLLMAdapter: response
ChatLLMAdapter-->>ChatAgent: response
ChatAgent-->>TaleCore: staged response
TaleCore->>SessionManager: persist or clear snapshot
Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Nitpick comments (7)
tests/test_memory_leak_fix.py (1)
145-154: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd an access-order LRU eviction case.
These tests insert keys in order only. A FIFO cache would retain the same
300..499keys and pass both tests. The tests do not verify theBoundedCache.__getitem__access refresh.
tests/test_memory_leak_fix.py#L145-L154: Insert 200 groups, read the oldest group, insert one new group, and assert that the read group remains while the next-oldest group is evicted.tests/test_memory_leak_fix.py#L282-L288: Add the same access-order case for_name_to_id, or parameterize one shared cache-LRU test.🤖 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 `@tests/test_memory_leak_fix.py` around lines 145 - 154, Extend the cache tests to verify access-order LRU behavior: in tests/test_memory_leak_fix.py lines 145-154, insert 200 groups, access the oldest group through BoundedCache.__getitem__, insert one additional group, and assert the accessed group remains while the next-oldest is evicted. Add the equivalent scenario for _name_to_id at tests/test_memory_leak_fix.py lines 282-288, or parameterize a shared test covering both caches.core/agent/base.py (1)
19-39: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAlign the declared return type with the real contract.
generateis annotated-> str. The production providerChatLLMAdapter.chatreturnsNoneon failure (core/main.py lines 83-106), andcore/main.py_call_chatllmexplicitly handlesreply is None. DeclareOptional[str]so callers see the nullable contract, or document that implementations must convertNoneto"".♻️ Proposed signature change
async def generate( self, messages: List[Dict], session_id: str, timeout: Optional[float] = 60.0 - ) -> str: + ) -> Optional[str]:🤖 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 `@core/agent/base.py` around lines 19 - 39, Update the abstract generate method’s return annotation in the base agent class from str to Optional[str], preserving the existing nullable failure contract used by ChatLLMAdapter.chat and _call_chatllm.tests/unit/agent/test_chat_agent_concurrency.py (1)
132-133: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winWall-clock upper bounds are too tight for shared CI runners. Both test files assert an upper bound on elapsed wall-clock time with 0.3s to 0.5s of slack. The lower bounds are deterministic, because
asyncio.sleepguarantees them. The upper bounds depend on runner load, so the new GitHub Actions matrix on Python 3.10, 3.11, and 3.12 can fail these tests without any code defect. The shared root cause is one benchmark pattern copied across both files.
tests/unit/agent/test_chat_agent_concurrency.py#L132-L133: widen the<2.5upper bound, or move the timing assertion behind a marker that the default CI job skips.tests/unit/agent/test_chat_agent_performance.py#L62-L62: widen the<0.8upper bound for the 3-user benchmark, or apply the same marker.tests/unit/agent/test_chat_agent_performance.py#L127-L128: widen the<1.3upper bound for the 5-user semaphore benchmark, or apply the same marker.tests/unit/agent/test_chat_agent_performance.py#L217-L218: widen the<2.5upper bound for the 10-user stress benchmark, or apply the same marker.Keep every lower bound. The lower bounds are what prove concurrency and semaphore behavior.
🤖 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 `@tests/unit/agent/test_chat_agent_concurrency.py` around lines 132 - 133, Widen the wall-clock upper bounds while preserving every lower-bound assertion: update tests/unit/agent/test_chat_agent_concurrency.py lines 132-133, and the corresponding 3-user, 5-user semaphore, and 10-user stress benchmarks in tests/unit/agent/test_chat_agent_performance.py lines 62, 127-128, and 217-218. Alternatively, apply one consistent marker to these timing assertions so the default CI job skips them; do not remove or weaken the lower bounds.core/agent/chat_agent.py (1)
91-97: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winReplace
asyncio.get_event_loop()withasyncio.get_running_loop()in the three agent sync-provider paths. Each site runs this inside an already-running coroutine, so useget_running_loop()and keep the rest of the executor dispatch unchanged.
core/agent/chat_agent.py#L93core/agent/plan_agent.py#L64core/agent/tool_agent.py#L66If the sync-provider dispatch is extracted later, apply the fix in that shared helper.
🤖 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 `@core/agent/chat_agent.py` around lines 91 - 97, Replace asyncio.get_event_loop() with asyncio.get_running_loop() in the sync-provider executor paths of core/agent/chat_agent.py lines 91-97, core/agent/plan_agent.py lines 63-68, and core/agent/tool_agent.py lines 65-70; keep the existing executor dispatch unchanged. If these paths are later consolidated into a shared helper, apply the change there instead.tests/unit/agent/test_chat_agent_locks.py (1)
107-107: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReplace
asyncio.Semaphore._valueassertions with behavioral checks.
_valueis private and may vary across supported Python versions. Assert the semaphore state by draining permits withasyncio.wait_for(sem.acquire(), timeout=...)and confirming the next acquire blocks. Keep this change for lines 107, 168, 177, and 191.🤖 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 `@tests/unit/agent/test_chat_agent_locks.py` at line 107, Replace the private _value assertions in the affected semaphore tests, including the assertions around agent._global_semaphore at lines 107, 168, 177, and 191, with behavioral checks that acquire all expected permits using asyncio.wait_for and a short timeout, then verify the next acquire times out or otherwise blocks. Preserve each test’s expected semaphore capacity while avoiding direct access to asyncio.Semaphore._value.tests/unit/test_chat_agent_integration.py (1)
344-347: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winStrengthen the history assertion.
ziptruncates to the shorter sequence. If a thirdusermessage leaks into the prompt, the assertion still passes. Assert the count first.♻️ Proposed refactor
user_msgs = [m for m in second if m.get("role") == "user"] - assert [m["content"].endswith(t) for m, t in zip(user_msgs, ["第一轮消息", "第二轮消息"])] == [True, True], ( - "持久化模式应从 SessionManager 记忆读取历史" - ) + assert len(user_msgs) == 2, f"应只含记忆中的一条 + 本轮一条，实际 {len(user_msgs)} 条" + assert user_msgs[0]["content"].endswith("第一轮消息") + assert user_msgs[1]["content"].endswith("第二轮消息")🤖 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 `@tests/unit/test_chat_agent_integration.py` around lines 344 - 347, Strengthen the history assertion in the test by first asserting that user_msgs contains exactly two messages, then retain the existing content checks for the first and second messages. Avoid relying on zip alone, so extra user messages cause the test to fail.tests/unit/test_concurrency_lock.py (1)
342-359: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThese wall-clock assertions can flake on shared CI runners.
max_active >= 3combined withmax_active <= 3requires the scheduler to fill the semaphore exactly.0.5 < total_time < 1.5requires stable timing. The PR adds a GitHub Actions workflow across three Python versions, so these tests will run on shared runners where a slow wave pushestotal_timeabove 1.5s. Consider widening the upper bounds, or replacing the timing checks with event-based synchronization (for example, a barrier that releases only after three calls are active).The same concern applies to Line 277 and Line 490.
🤖 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 `@tests/unit/test_concurrency_lock.py` around lines 342 - 359, Update the concurrency tests around the max_active and total_time assertions, including the analogous checks near the other referenced cases, to avoid relying on exact scheduler saturation or tight wall-clock limits. Use event-based synchronization where possible so three calls must become active before proceeding, and widen remaining timing bounds enough for shared CI runners while preserving verification of semaphore limiting and per-session parallelism.
🤖 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 `@core/agent/chat_agent.py`:
- Around line 91-101: Update the synchronous path in ChatAgent’s executor call
to use a dedicated ThreadPoolExecutor instead of the shared default executor,
isolating timed-out chat_method work from other run_in_executor users. Ensure
the executor is owned and cleaned up appropriately, and add a comment
documenting that asyncio timeout cancellation cannot stop an already-running
worker thread; retain the provider-level transport timeout requirement where
applicable.
- Around line 38-41: Bound the _session_locks registry while preserving locks
that are held or have waiters. Replace the plain dict with an OrderedDict-based
registry, update the lock lookup path in ChatAgent to refresh entries and evict
only least-recently-used locks whose lock is neither locked nor awaited, and
enforce a bounded size consistent with the existing cache policy. Keep
synchronization through _lock_manager_mutex so lookup, insertion, and eviction
remain race-safe.
In `@core/main.py`:
- Around line 1612-1625: Propagate the active session ID through the follow-up
tool-round path: update _handle_respond_message to pass sid into
_resolve_follow_up, then through _call_chatllm_with_timeout into _call_chatllm.
Ensure _call_chatllm_with_timeout forwards sid explicitly so the ChatAgent
branch uses it instead of self.chat.current_sid, preserving session history and
snapshots when persistence is disabled.
- Around line 337-345: Update CoreManager._init_chat_agent() to retain the model
returned by provider_manager.resolve("main_llm") and bind it when constructing
ChatAgent. Update ChatAgent.generate to forward the bound model to provider.chat
alongside messages, preserving the existing fallback behavior for unresolved
providers and initialization failures.
- Around line 366-372: Update the reload condition around _init_chat_agent so it
compares the existing ChatAgent provider adapter’s chatllm attribute with
self.chat, rather than comparing _provider directly to self.chat. Preserve
rebuilding when chat_agent is absent or the wrapped ChatLLM instance differs,
while avoiding recreation when the same ChatLLM remains configured.
- Around line 1520-1540: Update _get_session_messages to check the session’s
enabled state before calling session_manager.get_memory(sid), matching the
session.enabled guard used by _persist_snapshot. Skip persisted history for
disabled sessions while preserving snapshot handling and existing error
behavior.
In `@tests/test_memory_leak_fix.py`:
- Around line 507-513: Replace the shallow sys.getsizeof-based ratios in the
memory-growth assertions with a check that measures transitive retained entries,
including the buffer’s cache, timestamps, message lists, and the name map’s
nickname-related structures; alternatively remove the size ratios and explicitly
limit the test to verifying bounded key counts. Update the assertions and
failure messages around ratio_buffer and ratio_name_map accordingly.
In `@tests/unit/agent/test_chat_agent_locks.py`:
- Around line 53-58: Rename test_session_locks_are_reentrant to state that
session locks are not reentrant, preserving its existing assertions and
docstring.
- Line 95: Rename the ambiguous loop variable l in the assertion within the lock
test to a descriptive name, and update its reference in the all expression while
preserving the existing identity check against locks[0].
In `@tests/unit/agent/test_chat_agent_timeout.py`:
- Around line 198-209: Update the timeout test around agent.generate so it
validates the 60-second default rather than duplicating the explicit-timeout
case: remove the timeout override and make the simulated call exceed the
default, or revise the comment to accurately describe the custom timeout
behavior. Keep the existing asyncio.TimeoutError assertion and use
test_timeout_parameter_contract for the separate structural default check.
In `@tests/unit/test_chat_agent_integration.py`:
- Around line 31-46: Make the lightweight module stubs scoped to this test
module rather than leaving them in process-wide sys.modules: move the
_stub_modules injection into a module-scoped autouse fixture and remove only the
entries it added during teardown, or remove the stubbing entirely. Ensure later
test modules can import the real numpy and bs4 packages.
In `@tests/unit/test_concurrency_lock.py`:
- Around line 339-340: Remove the unnecessary f-string prefixes from the two
constant print messages near the Semaphore timing output and the corresponding
message near line 415. Keep the string contents and print behavior unchanged so
Ruff F541 is resolved.
---
Nitpick comments:
In `@core/agent/base.py`:
- Around line 19-39: Update the abstract generate method’s return annotation in
the base agent class from str to Optional[str], preserving the existing nullable
failure contract used by ChatLLMAdapter.chat and _call_chatllm.
In `@core/agent/chat_agent.py`:
- Around line 91-97: Replace asyncio.get_event_loop() with
asyncio.get_running_loop() in the sync-provider executor paths of
core/agent/chat_agent.py lines 91-97, core/agent/plan_agent.py lines 63-68, and
core/agent/tool_agent.py lines 65-70; keep the existing executor dispatch
unchanged. If these paths are later consolidated into a shared helper, apply the
change there instead.
In `@tests/test_memory_leak_fix.py`:
- Around line 145-154: Extend the cache tests to verify access-order LRU
behavior: in tests/test_memory_leak_fix.py lines 145-154, insert 200 groups,
access the oldest group through BoundedCache.__getitem__, insert one additional
group, and assert the accessed group remains while the next-oldest is evicted.
Add the equivalent scenario for _name_to_id at tests/test_memory_leak_fix.py
lines 282-288, or parameterize a shared test covering both caches.
In `@tests/unit/agent/test_chat_agent_concurrency.py`:
- Around line 132-133: Widen the wall-clock upper bounds while preserving every
lower-bound assertion: update tests/unit/agent/test_chat_agent_concurrency.py
lines 132-133, and the corresponding 3-user, 5-user semaphore, and 10-user
stress benchmarks in tests/unit/agent/test_chat_agent_performance.py lines 62,
127-128, and 217-218. Alternatively, apply one consistent marker to these timing
assertions so the default CI job skips them; do not remove or weaken the lower
bounds.
In `@tests/unit/agent/test_chat_agent_locks.py`:
- Line 107: Replace the private _value assertions in the affected semaphore
tests, including the assertions around agent._global_semaphore at lines 107,
168, 177, and 191, with behavioral checks that acquire all expected permits
using asyncio.wait_for and a short timeout, then verify the next acquire times
out or otherwise blocks. Preserve each test’s expected semaphore capacity while
avoiding direct access to asyncio.Semaphore._value.
In `@tests/unit/test_chat_agent_integration.py`:
- Around line 344-347: Strengthen the history assertion in the test by first
asserting that user_msgs contains exactly two messages, then retain the existing
content checks for the first and second messages. Avoid relying on zip alone, so
extra user messages cause the test to fail.
In `@tests/unit/test_concurrency_lock.py`:
- Around line 342-359: Update the concurrency tests around the max_active and
total_time assertions, including the analogous checks near the other referenced
cases, to avoid relying on exact scheduler saturation or tight wall-clock
limits. Use event-based synchronization where possible so three calls must
become active before proceeding, and widen remaining timing bounds enough for
shared CI runners while preserving verification of semaphore limiting and
per-session parallelism.
🪄 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 Plus
Run ID: 656245d2-3bfe-4959-8a41-cac0b893f188
📒 Files selected for processing (18)
core/adapter/src/qq/adapter.pycore/adapter/src/websocket/adapter.pycore/agent/__init__.pycore/agent/base.pycore/agent/chat_agent.pycore/agent/plan_agent.pycore/agent/tool_agent.pycore/main.pytests/test_adapter_error_logging.pytests/test_memory_leak_fix.pytests/unit/agent/test_chat_agent_basic.pytests/unit/agent/test_chat_agent_concurrency.pytests/unit/agent/test_chat_agent_locks.pytests/unit/agent/test_chat_agent_performance.pytests/unit/agent/test_chat_agent_timeout.pytests/unit/agent/test_llm_agent_base.pytests/unit/test_chat_agent_integration.pytests/unit/test_concurrency_lock.py
修复 PR #185 CodeRabbit 审查发现的 12 条评论： core/main.py: - 修复 _on_config_reloaded 的 ChatAgent 重建检查：_provider 是 ChatLLMAdapter 而非 self.chat，改为比较 adapter 包装的 ChatLLM， 避免每次 config_reloaded 都重建 agent、丢失锁/semaphore 状态 - 禁用的会话不加载历史（_get_session_messages），与旧路径 set_session(load_history=session_enabled) 语义一致 - 工具轮次贯通 sid：_call_chatllm_with_timeout/_resolve_follow_up 增加 sid 参数，无持久化模式下工具轮次不再落到空 sid， 快照读写（上下文连续）恢复正常 - provider 直连回退路径绑定 model（functools.partial）， BaseProvider.chat 的 model 是必填参数 - context window 图片下载/VLM 调用改用专用 executor， 避免锁内阻塞任务占用默认线程池；run() 退出时释放线程池 core/agent/chat_agent.py: - _session_locks 改为 LRU 有界（max_sessions=1000），持有中的锁 绝不淘汰（推迟到空闲） - 同步 provider 改用专用 ThreadPoolExecutor（4 线程）， 超时不中断已运行线程的局限在注释中说明 tests/: - sys.modules 的 numpy/bs4 stub 移入 session 级 autouse fixture 并在结束恢复原状，消除进程级全局污染（CI 顺序失败根因） - 重命名 test_session_locks_are_not_reentrant（断言验证非重入） - 修复 Ruff E741 歧义变量名、F541 多余 f 前缀 - 补充 LRU 淘汰与持有锁保护测试；timeout 默认 60s 契约断言
core/__init__.py 的 from .main import main 把 core.main 属性遮蔽为函数，
patch("core.main.calculate_split_interval") 在 CI 收集顺序下会解析到
函数对象而非模块而抛 AttributeError；改用 importlib 取真实模块对象
patch.object（同 test_memory_leak_fix.py 的修复）。
# Conflicts: # core/agent/chat_agent.py # tests/unit/agent/test_chat_agent_locks.py # tests/unit/agent/test_chat_agent_timeout.py
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 (4)
core/main.py (2)
1603-1605: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winClear snapshots when a session is disabled.
Line 1605 returns before removing
_chat_snapshots[sid]._get_session_messages()then appends that snapshot on later requests. Disabled sessions therefore retain and resend prior user and assistant content to the LLM.Proposed fix
session = self.session_manager.get_session(sid) if session is not None and not session.enabled: - return # 禁用的会话不持久化新记忆 + self._chat_snapshots.pop(sid, None) + return🤖 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 `@core/main.py` around lines 1603 - 1605, Update the disabled-session branch in the session persistence flow around get_session and _chat_snapshots so it removes _chat_snapshots[sid] before returning. Preserve the existing behavior of skipping new-memory persistence while ensuring later _get_session_messages() calls cannot reuse stale snapshot content.
1643-1669: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winMake the provider fallback executable.
Line 1643 returns before the
ChatAgentbranch whenChatLLMinitialization fails. This makes the direct-provider fallback from_init_chat_agent()unreachable.Also create
sidwhenself.chat_agentexists, and callself.chat.set_session(...)only whenself.chatexists. Otherwise platform fallback calls either fail onself.chat.current_sidor lose per-session isolation.Proposed fix
- if self.chat is None: + if self.chat is None and self.chat_agent is None: logger.error("ChatLLM 未初始化") return "[系统错误] ChatLLM 未初始化，请检查 services.yaml 配置" ... if not sid: - sid = self.chat.current_sid or "" + sid = (self.chat.current_sid if self.chat is not None else "") or ""🤖 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 `@core/main.py` around lines 1643 - 1669, Update the initialization guard before the ChatAgent branch so a missing self.chat does not return when self.chat_agent is available, allowing the direct-provider fallback from _init_chat_agent() to execute. In the self.chat_agent path, derive sid without assuming self.chat exists, and guard self.chat.set_session(...) so it is called only when self.chat is present, preserving per-session isolation for both initialized and fallback providers.core/agent/chat_agent.py (1)
59-90: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winEviction only checks the oldest entry, and the LRU test cannot detect the gap. The root cause is in
_get_session_lock: it inspects onlynext(iter(self._session_locks))and gives up entirely if that entry is locked, instead of scanning for any evictable entry. Under bursty traffic, a single long-running session at the head of the ordering can let the registry exceedmax_sessionseven while many newer, already-completed sessions sit free behind it. The companion test cannot catch this because it never holds a lock during eviction.
core/agent/chat_agent.py#L59-L90: scan past a busy head for the first free, non-current candidate before giving up, instead of breaking on the first locked entry (see proposed diff in the per-file comment above).tests/unit/agent/test_chat_agent_locks.py#L98-L120: extendtest_session_locks_lru_boundedwith a case where the oldest lock stays held while later locks are free, and assert those free entries are still evicted once the bound-check runs.🤖 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 `@core/agent/chat_agent.py` around lines 59 - 90, The _get_session_lock eviction loop must scan past a locked oldest entry and remove the first free, non-current lock until the registry meets _max_sessions, rather than stopping at the first busy lock. Update tests/unit/agent/test_chat_agent_locks.py lines 98-120 in test_session_locks_lru_bounded to keep the oldest lock held while later locks remain free, then assert those free entries are evicted when the bound is exceeded.tests/unit/test_concurrency_lock.py (1)
568-584: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winDo not skip overlap checks based on message text.
ProcessedMessageobjects created with the same text produce identicalcurrent_message()keys. If two sessions send identical text, this loop drops that cross-session pair before checking its lock overlap.Compare unique event instances or session IDs instead, and exclude only the same event.
🤖 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 `@tests/unit/test_concurrency_lock.py` around lines 568 - 584, Update the overlap-check loop to distinguish events by their unique event instances or session IDs rather than comparing msg_i and msg_j message text. Remove the msg_i == msg_j skip based on current message content, and exclude only when the two event instances represent the same event so identical messages from different sessions still undergo overlap checks.
🧹 Nitpick comments (3)
tests/unit/agent/test_chat_agent_locks.py (1)
98-120: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtend LRU coverage to a busy-head scenario.
Every lock touched in this test is released before the next
_get_session_lockcall, solocked()is alwaysFalseduring eviction. The test does not cover the case where the oldest entry stays held while newer, already-free entries accumulate behind it. Add a case with a held oldest lock and multiple free later entries, and assert that the free entries still get evicted once eviction resumes (or that the bound is not silently exceeded forever). This complementstest_session_locks_held_lock_never_evicted, which only checks the single-oldest-locked case.🤖 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 `@tests/unit/agent/test_chat_agent_locks.py` around lines 98 - 120, Add a busy-head scenario to test_session_locks_lru_bounded: retain the oldest session lock in a held state while creating multiple newer, released sessions, then release the oldest lock and verify eviction resumes by removing free entries while keeping _session_locks within max_sessions. Ensure the assertions specifically cover that free later locks are evicted rather than allowing the dictionary to remain over capacity indefinitely.tests/unit/test_concurrency_lock.py (2)
392-435: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAssert the stateless call payload.
recorded_callsstores thesidandmessagesarguments, but the test checks only the extracted user text. The final assertion checksrecorded_sidsfromset_session(), which is a different path. A regression that drops or misroutessidor snapshotmessagescan still pass.Assert the sid and relevant message snapshot for each
recorded_callsentry.🤖 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 `@tests/unit/test_concurrency_lock.py` around lines 392 - 435, Extend the assertions in the stateless session test around recorded_calls to validate each chat invocation’s sid and messages snapshot, not just the extracted user text. Use the expected per-session sids and corresponding message contents for the group_stateless_a and group_stateless_b calls, while retaining the existing recorded_sids assertion for set_session().
347-364: 🩺 Stability & Availability | 🔵 Trivial | 🏗️ Heavy liftMake concurrency assertions scheduler-independent.
The
max_active >= 3assertion and fixed duration ranges depend on CI scheduling and wall-clock load. A correct run on a busy worker can fail these bounds.Use events or barriers to hold calls deterministically, assert semaphore occupancy directly, and keep only a generous watchdog for deadlock detection. Move strict performance thresholds to a benchmark if they are required.
Also applies to: 490-498
🤖 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 `@tests/unit/test_concurrency_lock.py` around lines 347 - 364, Update the concurrency test around the max_active and total_time assertions, including the corresponding assertions near the later task batch, to avoid scheduler-dependent parallelism and duration thresholds. Use synchronization events or barriers to hold calls deterministically, assert Semaphore(3) occupancy directly, and retain only a generous timeout as a deadlock watchdog; move strict performance validation out of this unit test.
🤖 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 `@tests/unit/test_chat_agent_integration.py`:
- Around line 46-62: The _stub_heavy_deps fixture leaves stub modules in
sys.modules across the entire test session and runs too late to affect top-level
imports. Change it to module scope and defer imports that require numpy or bs4
until after fixture setup, or remove the stubs and rely on real dependencies;
ensure sys.modules is restored after each test module.
In `@tests/unit/test_concurrency_lock.py`:
- Line 342: Update the diagnostic print statement in the concurrency test to use
f-string interpolation so it outputs the measured max_active value instead of
the literal placeholder.
---
Outside diff comments:
In `@core/agent/chat_agent.py`:
- Around line 59-90: The _get_session_lock eviction loop must scan past a locked
oldest entry and remove the first free, non-current lock until the registry
meets _max_sessions, rather than stopping at the first busy lock. Update
tests/unit/agent/test_chat_agent_locks.py lines 98-120 in
test_session_locks_lru_bounded to keep the oldest lock held while later locks
remain free, then assert those free entries are evicted when the bound is
exceeded.
In `@core/main.py`:
- Around line 1603-1605: Update the disabled-session branch in the session
persistence flow around get_session and _chat_snapshots so it removes
_chat_snapshots[sid] before returning. Preserve the existing behavior of
skipping new-memory persistence while ensuring later _get_session_messages()
calls cannot reuse stale snapshot content.
- Around line 1643-1669: Update the initialization guard before the ChatAgent
branch so a missing self.chat does not return when self.chat_agent is available,
allowing the direct-provider fallback from _init_chat_agent() to execute. In the
self.chat_agent path, derive sid without assuming self.chat exists, and guard
self.chat.set_session(...) so it is called only when self.chat is present,
preserving per-session isolation for both initialized and fallback providers.
In `@tests/unit/test_concurrency_lock.py`:
- Around line 568-584: Update the overlap-check loop to distinguish events by
their unique event instances or session IDs rather than comparing msg_i and
msg_j message text. Remove the msg_i == msg_j skip based on current message
content, and exclude only when the two event instances represent the same event
so identical messages from different sessions still undergo overlap checks.
---
Nitpick comments:
In `@tests/unit/agent/test_chat_agent_locks.py`:
- Around line 98-120: Add a busy-head scenario to
test_session_locks_lru_bounded: retain the oldest session lock in a held state
while creating multiple newer, released sessions, then release the oldest lock
and verify eviction resumes by removing free entries while keeping
_session_locks within max_sessions. Ensure the assertions specifically cover
that free later locks are evicted rather than allowing the dictionary to remain
over capacity indefinitely.
In `@tests/unit/test_concurrency_lock.py`:
- Around line 392-435: Extend the assertions in the stateless session test
around recorded_calls to validate each chat invocation’s sid and messages
snapshot, not just the extracted user text. Use the expected per-session sids
and corresponding message contents for the group_stateless_a and
group_stateless_b calls, while retaining the existing recorded_sids assertion
for set_session().
- Around line 347-364: Update the concurrency test around the max_active and
total_time assertions, including the corresponding assertions near the later
task batch, to avoid scheduler-dependent parallelism and duration thresholds.
Use synchronization events or barriers to hold calls deterministically, assert
Semaphore(3) occupancy directly, and retain only a generous timeout as a
deadlock watchdog; move strict performance validation out of this unit test.
🪄 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 Plus
Run ID: ae10e6df-3196-4eb2-a043-3b9484cb4f83
📒 Files selected for processing (8)
.github/workflows/test.ymlcore/agent/chat_agent.pycore/main.pytests/test_memory_leak_fix.pytests/unit/agent/test_chat_agent_locks.pytests/unit/agent/test_chat_agent_timeout.pytests/unit/test_chat_agent_integration.pytests/unit/test_concurrency_lock.py
| @pytest.fixture(autouse=True, scope="session") | ||
| def _stub_heavy_deps(): | ||
| """session 级：注入 numpy/bs4 轻量 stub，结束后恢复 sys.modules 原状。""" | ||
| saved = {} | ||
| for name in _STUB_MODULES: | ||
| if name in sys.modules: | ||
| saved[name] = sys.modules[name] | ||
| try: | ||
| for name in _STUB_MODULES: | ||
| sys.modules.setdefault(name, type(sys)(name)) | ||
| yield | ||
| finally: | ||
| for name in _STUB_MODULES: | ||
| if name in saved: | ||
| sys.modules[name] = saved[name] | ||
| else: | ||
| sys.modules.pop(name, None) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major
Fix the dependency-stub fixture lifecycle.
scope="session" runs the finally block only at pytest session teardown. The empty numpy and bs4 modules can remain in process-wide sys.modules while later tests run. This repeats the previous cross-module pollution issue.
Fixture setup also occurs after top-level imports, so it cannot stub dependencies needed during collection. Use module scope with deferred dependent imports, or remove the stubs and use the real dependencies.
🧰 Tools
🪛 Ruff (0.16.0)
[warning] 48-48: Docstring contains ambiguous ： (FULLWIDTH COLON). Did you mean : (COLON)?
(RUF002)
[warning] 48-48: Docstring contains ambiguous ， (FULLWIDTH COMMA). Did you mean , (COMMA)?
(RUF002)
🤖 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 `@tests/unit/test_chat_agent_integration.py` around lines 46 - 62, The
_stub_heavy_deps fixture leaves stub modules in sys.modules across the entire
test session and runs too late to affect top-level imports. Change it to module
scope and defer imports that require numpy or bs4 until after fixture setup, or
remove the stubs and rely on real dependencies; ensure sys.modules is restored
after each test module.
| print(f"\n=== Semaphore Test Results ===") | ||
| print(f"Max concurrent executions: {max_active}") | ||
| print("\n=== Semaphore Test Results ===") | ||
| print("Max concurrent executions: {max_active}") |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Restore the f-string in the concurrency diagnostic.
Line 342 prints the literal {max_active} instead of the measured value.
Proposed fix
- print("Max concurrent executions: {max_active}")
+ print(f"Max concurrent executions: {max_active}")📝 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.
| print("Max concurrent executions: {max_active}") | |
| print(f"Max concurrent executions: {max_active}") |
🤖 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 `@tests/unit/test_concurrency_lock.py` at line 342, Update the diagnostic print
statement in the concurrency test to use f-string interpolation so it outputs
the measured max_active value instead of the literal placeholder.
1 participant
feat: 集成 ChatAgent 到主流程并修复 CI 测试 (#183, #184)
Summary by CodeRabbit
New Features
Bug Fixes
Tests