diff --git a/agent/chat.py b/agent/chat.py index 45dae8a4..4798407c 100644 --- a/agent/chat.py +++ b/agent/chat.py @@ -115,8 +115,10 @@ async def _resolve_chat_model(configurable: dict) -> tuple[str, str]: async def get_chat_agent(config: RunnableConfig) -> Pregel: """Get a read-only PR chat agent. No sandbox; PR context comes via config.""" + config = config.copy() + config["configurable"] = config["configurable"].copy() + config.setdefault("recursion_limit", DEFAULT_RECURSION_LIMIT) thread_id = config["configurable"].get("thread_id") - config["recursion_limit"] = DEFAULT_RECURSION_LIMIT if thread_id is None or not graph_loaded_for_execution(config): return create_deep_agent(system_prompt="", tools=[]).with_config(config) diff --git a/agent/reviewer.py b/agent/reviewer.py index 6c2db45a..ac7285fa 100644 --- a/agent/reviewer.py +++ b/agent/reviewer.py @@ -854,10 +854,11 @@ async def _resolve_grouping_model( async def get_reviewer_agent(config: RunnableConfig) -> Pregel: """Get or create a reviewer agent with a sandbox + prepped repo.""" + config = config.copy() + config["configurable"] = config["configurable"].copy() + config.setdefault("recursion_limit", DEFAULT_RECURSION_LIMIT) thread_id = config["configurable"].get("thread_id", None) - config["recursion_limit"] = DEFAULT_RECURSION_LIMIT - if thread_id is None or not graph_loaded_for_execution(config): logger.info("No thread_id or not for execution, returning reviewer agent without sandbox") return create_deep_agent(system_prompt="", tools=[]).with_config(config) diff --git a/tests/reviewer/test_factory_config_isolation.py b/tests/reviewer/test_factory_config_isolation.py new file mode 100644 index 00000000..14fe78bf --- /dev/null +++ b/tests/reviewer/test_factory_config_isolation.py @@ -0,0 +1,122 @@ +"""Tests that get_reviewer_agent and get_chat_agent do not mutate the caller's config.""" + +from __future__ import annotations + +from unittest.mock import MagicMock, patch + +import pytest +from langgraph.graph.state import RunnableConfig + + +def _make_config(recursion_limit: int = 25) -> RunnableConfig: + return { + "configurable": {"thread_id": None}, + "recursion_limit": recursion_limit, + } + + +@pytest.mark.asyncio +async def test_get_reviewer_agent_does_not_mutate_caller_config() -> None: + """get_reviewer_agent must not overwrite the caller's recursion_limit.""" + from agent import reviewer + + config = _make_config(recursion_limit=25) + original_limit = config["recursion_limit"] + + fake_pregel = MagicMock() + fake_pregel.with_config = MagicMock(return_value=fake_pregel) + + with patch("agent.reviewer.create_deep_agent", return_value=fake_pregel): + await reviewer.get_reviewer_agent(config) + + assert config["recursion_limit"] == original_limit, ( + f"get_reviewer_agent mutated caller's recursion_limit: " + f"expected {original_limit}, got {config['recursion_limit']}" + ) + + +@pytest.mark.asyncio +async def test_get_reviewer_agent_applies_default_when_limit_unset() -> None: + """get_reviewer_agent should apply DEFAULT_RECURSION_LIMIT when the caller didn't set one.""" + from agent import reviewer + + config: RunnableConfig = {"configurable": {"thread_id": None}} + + fake_pregel = MagicMock() + fake_pregel.with_config = MagicMock(return_value=fake_pregel) + + with patch("agent.reviewer.create_deep_agent", return_value=fake_pregel): + await reviewer.get_reviewer_agent(config) + + assert "recursion_limit" not in config + + +@pytest.mark.asyncio +async def test_get_chat_agent_does_not_mutate_caller_config() -> None: + """get_chat_agent must not overwrite the caller's recursion_limit.""" + from agent import chat + + config = _make_config(recursion_limit=50) + original_limit = config["recursion_limit"] + + fake_pregel = MagicMock() + fake_pregel.with_config = MagicMock(return_value=fake_pregel) + + with patch("agent.chat.create_deep_agent", return_value=fake_pregel): + await chat.get_chat_agent(config) + + assert config["recursion_limit"] == original_limit, ( + f"get_chat_agent mutated caller's recursion_limit: " + f"expected {original_limit}, got {config['recursion_limit']}" + ) + + +@pytest.mark.asyncio +async def test_get_chat_agent_applies_default_when_limit_unset() -> None: + """get_chat_agent should apply DEFAULT_RECURSION_LIMIT when the caller didn't set one.""" + from agent import chat + + config: RunnableConfig = {"configurable": {"thread_id": None}} + + fake_pregel = MagicMock() + fake_pregel.with_config = MagicMock(return_value=fake_pregel) + + with patch("agent.chat.create_deep_agent", return_value=fake_pregel): + await chat.get_chat_agent(config) + + assert "recursion_limit" not in config + + +@pytest.mark.parametrize( + ("module_name", "factory_name"), + [("agent.reviewer", "get_reviewer_agent"), ("agent.chat", "get_chat_agent")], +) +@pytest.mark.asyncio +async def test_factory_copies_config_dicts_but_preserves_runtime_objects( + module_name: str, factory_name: str +) -> None: + """Factory config isolation must preserve callback and configurable value identities.""" + module = __import__(module_name, fromlist=[factory_name]) + factory = getattr(module, factory_name) + callback = object() + configurable_value = object() + callbacks = [callback] + config: RunnableConfig = { + "configurable": {"thread_id": None, "custom_key": configurable_value}, + "callbacks": callbacks, + } + + fake_pregel = MagicMock() + fake_pregel.with_config = MagicMock(return_value=fake_pregel) + + with patch(f"{module_name}.create_deep_agent", return_value=fake_pregel): + await factory(config) + + bound_config = fake_pregel.with_config.call_args.args[0] + assert bound_config is not config + assert bound_config["configurable"] is not config["configurable"] + assert bound_config["configurable"]["custom_key"] is configurable_value + assert bound_config["callbacks"] is callbacks + assert bound_config["callbacks"][0] is callback + assert "recursion_limit" not in config + assert config["configurable"] == {"thread_id": None, "custom_key": configurable_value} diff --git a/tests/reviewer/test_reviewer.py b/tests/reviewer/test_reviewer.py index 7a5fb3a1..1073c356 100644 --- a/tests/reviewer/test_reviewer.py +++ b/tests/reviewer/test_reviewer.py @@ -1288,13 +1288,17 @@ async def test_reviewer_populates_diff_line_set_from_github_api() -> None: patch("agent.utils.deferred_model.make_model", return_value=MagicMock()), patch("agent.reviewer.create_deep_agent", side_effect=fake_create_deep_agent), ): - await reviewer.get_reviewer_agent(config) + agent = await reviewer.get_reviewer_agent(config) mock_fetch_diff.assert_awaited_once_with( owner="acme", repo="repo", pr_number=42, token="gh-token" ) - assert config["configurable"]["diff_text"] == pr_diff - assert config["configurable"]["diff_line_set"] == {"in_diff.py": {"RIGHT": {10}, "LEFT": {1}}} + # get_reviewer_agent copies the caller's config before mutating it (it must not + # mutate the caller's dict in place), so assert against the config actually + # bound to the returned agent rather than the original input dict. + bound_configurable = agent.config["configurable"] + assert bound_configurable["diff_text"] == pr_diff + assert bound_configurable["diff_line_set"] == {"in_diff.py": {"RIGHT": {10}, "LEFT": {1}}} @pytest.mark.asyncio @@ -1353,10 +1357,14 @@ async def test_reviewer_leaves_validation_disabled_when_diff_fetch_fails() -> No patch("agent.utils.deferred_model.make_model", return_value=MagicMock()), patch("agent.reviewer.create_deep_agent", side_effect=fake_create_deep_agent), ): - await reviewer.get_reviewer_agent(config) + agent = await reviewer.get_reviewer_agent(config) - assert config["configurable"]["diff_text"] == "" - assert config["configurable"]["diff_line_set"] is None + # get_reviewer_agent copies the caller's config before mutating it (it must not + # mutate the caller's dict in place), so assert against the config actually + # bound to the returned agent rather than the original input dict. + bound_configurable = agent.config["configurable"] + assert bound_configurable["diff_text"] == "" + assert bound_configurable["diff_line_set"] is None @pytest.mark.asyncio