From 84ff3a5106e9ac6e992b7bcdce855a798881b447 Mon Sep 17 00:00:00 2001 From: Randoku Date: Tue, 14 Jul 2026 00:40:55 +0800 Subject: [PATCH] fix(agent): skip background reviews outside direct chats Gate automatic memory and skill review at the shared scheduler boundary while preserving CLI/direct contexts and external memory synchronization. --- agent/background_review.py | 10 +++ run_agent.py | 9 ++- tests/run_agent/test_background_review.py | 64 +++++++++++++++++++ .../test_background_review_cache_parity.py | 1 + ...t_background_review_toolset_restriction.py | 1 + .../test_codex_app_server_integration.py | 30 +++++++++ 6 files changed, 114 insertions(+), 1 deletion(-) diff --git a/agent/background_review.py b/agent/background_review.py index bf78f679236db..c32ac57ab9dfc 100644 --- a/agent/background_review.py +++ b/agent/background_review.py @@ -28,6 +28,16 @@ logger = logging.getLogger(__name__) +# Empty preserves CLI agents; aliases support non-gateway direct-chat callers. +_DIRECT_CHAT_TYPES = frozenset({"", "dm", "direct", "private"}) + + +def background_review_allowed(chat_type: Any) -> bool: + """Return whether automatic owner-scoped background review is safe.""" + normalized = "" if chat_type is None else str(chat_type).strip().lower() + return normalized in _DIRECT_CHAT_TYPES + + # --------------------------------------------------------------------------- # Background-review aux-model selector + routed digest. # diff --git a/run_agent.py b/run_agent.py index fe378f396ae9d..dc0101c30293b 100644 --- a/run_agent.py +++ b/run_agent.py @@ -1611,7 +1611,14 @@ def _spawn_background_review( here so existing tests that patch ``run_agent.threading.Thread`` keep working. """ - from agent.background_review import spawn_background_review_thread + from agent.background_review import ( + background_review_allowed, + spawn_background_review_thread, + ) + + if not background_review_allowed(getattr(self, "_chat_type", False)): + return + from tools.thread_context import propagate_context_to_thread target, _prompt = spawn_background_review_thread( self, diff --git a/tests/run_agent/test_background_review.py b/tests/run_agent/test_background_review.py index 1198f4abe7f83..0b414e4440787 100644 --- a/tests/run_agent/test_background_review.py +++ b/tests/run_agent/test_background_review.py @@ -2,6 +2,11 @@ from __future__ import annotations +from unittest.mock import Mock + +import pytest + +import agent.background_review as background_review import run_agent as run_agent_module from run_agent import AIAgent @@ -29,6 +34,7 @@ def _bare_agent() -> AIAgent: agent.background_review_callback = None agent.status_callback = None agent._safe_print = lambda *_args, **_kwargs: None + agent._chat_type = None return agent @@ -40,6 +46,64 @@ def start(self): self._target() +@pytest.mark.parametrize( + ("chat_type", "expected"), + [ + (None, True), + ("", True), + (False, False), + (0, False), + ("dm", True), + (" DM ", True), + ("direct", True), + ("private", True), + ("group", False), + ("channel", False), + ("forum", False), + ("thread", False), + ("room", False), + ("webhook", False), + ("unknown", False), + ], +) +def test_background_review_allowed_for_direct_chats_only(chat_type, expected): + assert background_review.background_review_allowed(chat_type) is expected + + +@pytest.mark.parametrize( + "chat_type", + ["group", "channel", "forum", "thread", "room", "webhook", "unknown"], +) +def test_background_review_does_not_start_for_multi_user_chat(monkeypatch, chat_type): + thread = Mock() + monkeypatch.setattr(run_agent_module.threading, "Thread", thread) + + agent = _bare_agent() + agent._chat_type = chat_type + AIAgent._spawn_background_review( + agent, + messages_snapshot=[{"role": "user", "content": "hello"}], + review_memory=True, + ) + + thread.assert_not_called() + + +def test_background_review_does_not_start_without_chat_type(monkeypatch): + thread = Mock() + monkeypatch.setattr(run_agent_module.threading, "Thread", thread) + + agent = _bare_agent() + del agent._chat_type + AIAgent._spawn_background_review( + agent, + messages_snapshot=[{"role": "user", "content": "hello"}], + review_memory=True, + ) + + thread.assert_not_called() + + def test_background_review_shuts_down_memory_provider_before_close(monkeypatch): events = [] diff --git a/tests/run_agent/test_background_review_cache_parity.py b/tests/run_agent/test_background_review_cache_parity.py index 58a2dfa4812f4..9ce5aeece30c7 100644 --- a/tests/run_agent/test_background_review_cache_parity.py +++ b/tests/run_agent/test_background_review_cache_parity.py @@ -21,6 +21,7 @@ def _make_agent_stub(agent_cls): agent.platform = "test" agent.provider = "openai" agent.session_id = "sess-123" + agent._chat_type = None agent.quiet_mode = True agent._memory_store = None agent._memory_enabled = True diff --git a/tests/run_agent/test_background_review_toolset_restriction.py b/tests/run_agent/test_background_review_toolset_restriction.py index f8d6559da5f5d..8812a44bb1827 100644 --- a/tests/run_agent/test_background_review_toolset_restriction.py +++ b/tests/run_agent/test_background_review_toolset_restriction.py @@ -23,6 +23,7 @@ def _make_agent_stub(agent_cls): agent.platform = "test" agent.provider = "openai" agent.session_id = "sess-123" + agent._chat_type = None agent.quiet_mode = True agent._memory_store = None agent._memory_enabled = True diff --git a/tests/run_agent/test_codex_app_server_integration.py b/tests/run_agent/test_codex_app_server_integration.py index c88d45c483af1..fb0acf2d17515 100644 --- a/tests/run_agent/test_codex_app_server_integration.py +++ b/tests/run_agent/test_codex_app_server_integration.py @@ -304,6 +304,36 @@ def fake_run_turn(self, user_input: str, **kwargs): # Counter should be reset after the review fires assert agent._iters_since_skill == 0 + def test_group_review_guard_preserves_external_memory_sync(self, monkeypatch): + def fake_run_turn(self, user_input: str, **kwargs): + return TurnResult( + final_text="done", + projected_messages=[{"role": "assistant", "content": "done"}], + tool_iterations=10, + turn_id="t1", + thread_id="th1", + ) + + monkeypatch.setattr(CodexAppServerSession, "run_turn", fake_run_turn) + monkeypatch.setattr( + CodexAppServerSession, "ensure_started", lambda self: "th1" + ) + + agent = _make_codex_agent(chat_type="group") + agent._skill_nudge_interval = 10 + agent._iters_since_skill = 0 + agent.valid_tool_names = set(getattr(agent, "valid_tool_names", set())) + agent.valid_tool_names.add("skill_manage") + agent._memory_manager = MagicMock() + agent._memory_manager.build_system_prompt.return_value = "" + + with patch.object(run_agent.threading, "Thread") as thread: + result = agent.run_conversation("do tool work") + + thread.assert_not_called() + agent._memory_manager.sync_all.assert_called_once() + assert agent._memory_manager.sync_all.call_args.kwargs["messages"] == result["messages"] + def test_background_review_signature_never_breaks(self, fake_session): """Even when no trigger fires, the helper must never call _spawn_background_review with the wrong signature. Run a turn,