From 4ba85d2a05cf966ff79605ab4b288fdf7990c670 Mon Sep 17 00:00:00 2001 From: "Eric A. Litman" Date: Mon, 3 Aug 2026 19:09:11 -0400 Subject: [PATCH] fix: make adversarial reviewer failures visible [refs OSWE-239] The first production adversarial review published nothing and exited "success" in 4.6s. Both finders failed with "bounded reviewer stage returned no structured response", but nothing reached the log: every spine failure is captured into state["error"] and routed to settle, which never read it. The root cause was only recoverable from the LangSmith trace. Log the spine error and the per-finder errors at settle, the single funnel every failure route reaches. In _run_stage, log the returned state's keys, its message count, and the resolved configurable's keys before raising -- a message count of 0 means the stage graph ran no nodes at all, which is what happened in production and is a different failure from a model that answered without the tool. Keys only, since the configurable carries auth values. Cover the finder sub-agent that production actually builds. Every compiled-graph test patches out both _bounded_agent and _run_stage, so create_deep_agent(..., response_format=ToolStrategy(FinderOutput)) was never constructed or invoked under test. Co-Authored-By: Claude Opus 5 --- agent/reviewer_adversarial.py | 22 ++++ tests/reviewer/test_reviewer_adversarial.py | 120 +++++++++++++++++++- 2 files changed, 141 insertions(+), 1 deletion(-) diff --git a/agent/reviewer_adversarial.py b/agent/reviewer_adversarial.py index 6e43ae555..839014da1 100644 --- a/agent/reviewer_adversarial.py +++ b/agent/reviewer_adversarial.py @@ -242,6 +242,15 @@ async def _run_stage( ) structured = result.get("structured_response") if isinstance(result, dict) else None if not isinstance(structured, BaseModel): + # An empty message list means the stage graph ran no nodes at all, which + # is a different failure from a model that answered without the tool. + logger.error( + "Bounded reviewer stage returned no structured response: " + "result_keys=%s message_count=%s configurable_keys=%s", + sorted(result) if isinstance(result, dict) else type(result).__name__, + len(result["messages"]) if isinstance(result, dict) and "messages" in result else None, + sorted(config.get("configurable") or {}), + ) raise RuntimeError("bounded reviewer stage returned no structured response") return structured @@ -682,6 +691,19 @@ async def record_publish(state: AdversarialState) -> dict[str, Any]: return {"error": f"record/publish failed: {exc}"} async def settle(state: AdversarialState, runtime: Runtime) -> dict[str, Any]: + # Every failure route lands here, and the graph still exits "success", so + # this is the only place a swallowed spine error can reach the log. + if error := state.get("error"): + finder_errors = { + item["finder"]: item["error"] + for item in state.get("finder_results", []) + if item["error"] + } + logger.error( + "Adversarial review ended without publishing: %s finder_errors=%s", + error, + finder_errors or None, + ) await settle_review_check_on_exit.aafter_agent(cast(AgentState, state), runtime) return {} diff --git a/tests/reviewer/test_reviewer_adversarial.py b/tests/reviewer/test_reviewer_adversarial.py index 946ccbb00..3946ad55c 100644 --- a/tests/reviewer/test_reviewer_adversarial.py +++ b/tests/reviewer/test_reviewer_adversarial.py @@ -12,8 +12,11 @@ import pytest from langchain.agents.middleware import AgentState from langchain_core.language_models.chat_models import BaseChatModel +from langchain_core.messages import AIMessage +from langchain_core.outputs import ChatGeneration, ChatResult from langgraph.graph.state import RunnableConfig from langgraph.runtime import Runtime +from pydantic import Field from agent.middleware import ExcludeToolsMiddleware from agent.review.trace_context import PRTraceContext @@ -30,11 +33,13 @@ RESERVED_SUBAGENT_TOOLS, PrepareAdversarialReviewerRunMiddleware, _active_finder_names, + _bounded_agent, _finder_payload, _finder_prompt, _judgment_context, _prepare_context, _render_parent_prompt, + _run_stage, get_reviewer_adversarial_agent, ) from agent.utils.agent_definitions import ( @@ -1104,9 +1109,13 @@ def test_gate_added_same_file_candidate_requires_independence() -> None: @pytest.mark.asyncio -async def test_finder_timeout_fails_closed_and_settles_terminal_check() -> None: +async def test_finder_timeout_fails_closed_and_settles_terminal_check( + caplog: pytest.LogCaptureFixture, +) -> None: from agent.review.adversarial import FinderOutput + caplog.set_level(logging.ERROR, logger="agent.reviewer_adversarial") + stages = [object() for _ in range(6)] stage_iter = iter(stages) @@ -1178,6 +1187,9 @@ async def run_stage(graph: object, *_args: object, **_kwargs: object) -> FinderO add.assert_not_awaited() publish.assert_not_awaited() settle.assert_awaited_once() + assert "Adversarial review ended without publishing" in caplog.text + assert "finder fanout incomplete or failed" in caplog.text + assert "security finder timed out" in caplog.text @pytest.mark.asyncio @@ -1462,3 +1474,109 @@ async def run_stage(graph: object, *_args: object, **_kwargs: object) -> Any: assert gate_calls == 2 add.assert_awaited_once() publish.assert_awaited_once() + + +class _StubToolCallModel(BaseChatModel): + """Tool-calling stand-in that answers with whatever tool it is told to call.""" + + response_tool: str | None + bound_tool_names: list[str] = Field(default_factory=list) + invocations: list[int] = Field(default_factory=list) + + @property + def _llm_type(self) -> str: + return "stub-tool-call" + + def bind_tools(self, tools: Any, **kwargs: Any) -> _StubToolCallModel: + self.bound_tool_names = [_stub_tool_name(tool) for tool in tools] + return self + + def _generate( + self, messages: Any, stop: Any = None, run_manager: Any = None, **kwargs: Any + ) -> ChatResult: + self.invocations.append(len(messages)) + message = AIMessage( + content="done", + tool_calls=( + [{"name": self.response_tool, "args": {"candidates": []}, "id": "stub-1"}] + if self.response_tool + else [] + ), + ) + return ChatResult(generations=[ChatGeneration(message=message)]) + + +def _stub_tool_name(tool: Any) -> str: + if isinstance(tool, dict): + nested = tool.get("function") + name = tool.get("name") or (nested.get("name") if isinstance(nested, dict) else None) + return str(name or "") + return str(getattr(tool, "name", None) or getattr(tool, "__name__", "")) + + +def _finder_specs(model: BaseChatModel) -> list[Any]: + definition = load_agent_definition("reviewer-adversarial") + return [ + spec + for spec in build_subagents(definition, model=model, reserved_tools=RESERVED_SUBAGENT_TOOLS) + if str(spec["name"]) not in {"general-purpose", "adjudicator"} + ] + + +@pytest.mark.asyncio +async def test_bounded_finder_agent_calls_its_model_and_returns_structured_output() -> None: + """Every other compiled-graph test patches out `_bounded_agent` and `_run_stage`, + so this is the only coverage of the finder sub-agent production actually builds.""" + from deepagents.backends import StateBackend + + from agent.review.adversarial import FinderOutput + + specs = _finder_specs(cast(BaseChatModel, _StubToolCallModel(response_tool="FinderOutput"))) + assert {str(spec["name"]) for spec in specs} == {"conventions", "correctness", "security"} + + for spec in specs: + model = cast(_StubToolCallModel, spec.get("model")) + graph = _bounded_agent( + model=cast(BaseChatModel, model), + response_format=FinderOutput, + backend=StateBackend(), + tools=cast(list[Any], spec.get("tools", [])), + middleware=[ + *cast(list[Any], spec.get("middleware", [])), + ExcludeToolsMiddleware(excluded=frozenset({"task"})), + ], + ) + structured = await _run_stage(graph, "review the diff", cast(RunnableConfig, {})) + + assert isinstance(structured, FinderOutput), str(spec["name"]) + assert model.invocations, f"{spec['name']} finder never called its model" + assert "FinderOutput" in model.bound_tool_names, str(spec["name"]) + + +@pytest.mark.asyncio +async def test_run_stage_logs_diagnostics_when_no_structured_response( + caplog: pytest.LogCaptureFixture, +) -> None: + from deepagents.backends import StateBackend + + from agent.review.adversarial import FinderOutput + + caplog.set_level(logging.ERROR, logger="agent.reviewer_adversarial") + graph = _bounded_agent( + model=cast(BaseChatModel, _StubToolCallModel(response_tool=None)), + response_format=FinderOutput, + backend=StateBackend(), + tools=[], + middleware=[], + ) + + with pytest.raises(RuntimeError, match="no structured response"): + await _run_stage( + graph, + "review the diff", + cast(RunnableConfig, {"configurable": {"thread_id": "t", "repo": {"owner": "o"}}}), + ) + + assert "Bounded reviewer stage returned no structured response" in caplog.text + assert "message_count=" in caplog.text + assert "configurable_keys=['repo', 'thread_id']" in caplog.text