diff --git a/lib/galaxy/agents/prompts/router.md b/lib/galaxy/agents/prompts/router.md index ef351721466..a612abd417d 100644 --- a/lib/galaxy/agents/prompts/router.md +++ b/lib/galaxy/agents/prompts/router.md @@ -54,7 +54,7 @@ Ask when: - The message names no analysis, tool, dataset, or goal ("Can you help with my data?", "What should I do next?") - A failure is reported with no error text, exit code, or tool name ("It keeps failing", "My job isn't working") - The intent could plausibly mean several different things -- a tool, a tutorial, usage help, or debugging ("I need help with variant calling") -- A follow-up's referent cannot be determined from the message itself ("Is there a better one?") +- A follow-up's referent cannot be resolved even from the recent turn you were given Do NOT ask when the current message is clear enough to route or answer on its own. A confident route or answer is always better than an unnecessary question -- over-asking is @@ -62,6 +62,12 @@ as harmful as mis-routing. When you do ask, name the options where you can ("Do tool recommendation or a tutorial?") rather than a generic "can you clarify?". You may pass 2-4 short `options` so the user can pick an answer directly. +You are given the most recent turn of the conversation (the previous message and its reply) +as context. When the current message is a follow-up -- "what about a workflow for this?", +"is there a better one?", "and for paired-end reads?" -- resolve "this" / "it" / "one" from +that prior turn and route accordingly. Do not ask what it refers to when the prior turn makes +it clear. + If the user's message is answering a clarifying question you just asked, route using that question together with their original request -- e.g. after you asked "tool recommendation or a tutorial?", a reply of "the second one" or "a tutorial" means hand off to the tutorial diff --git a/lib/galaxy/agents/router.py b/lib/galaxy/agents/router.py index 2a65cf057cd..a5a3c3ad001 100644 --- a/lib/galaxy/agents/router.py +++ b/lib/galaxy/agents/router.py @@ -51,13 +51,15 @@ class QueryRouterAgent(BaseGalaxyAgent): agent_type = AgentType.ROUTER _handoff_context: Optional[dict[str, Any]] = None - # How many recent conversation turns the router sees when making its routing - # decision. Evals show routing accuracy degrades monotonically as more history is - # fed to the router -- a tool/workflow question that routes correctly on turn 1 gets - # answered directly deep in a conversation. Routing on the current message alone - # (0 turns) recovers it; specialists still receive the full conversation_history via - # the handoff context, so nothing downstream loses information. - ROUTING_HISTORY_TURNS = 0 + # The current message drives routing, plus the most recent conversation turn(s) so an + # elliptical follow-up ("what about a workflow for this?", or the answer to a clarifying + # question -- "the second one") keeps its referent. Capped at ROUTING_HISTORY_TURNS: + # forwarding the whole deep conversation degrades routing -- it biases the model toward + # answering directly (the #22791 finding) -- but forwarding the last turn does not. It's + # the whole turn, request and reply: a bare user message with no assistant reply reads as + # a dangling/compound request and mis-routes. Specialists always receive the full + # conversation_history via the handoff context. + ROUTING_HISTORY_TURNS = 1 def _create_agent(self) -> Agent[GalaxyAgentDependencies, str]: model_name = self._get_agent_config("model", "") @@ -499,7 +501,7 @@ class QueryRouterAgent(BaseGalaxyAgent): - A failure is reported with no error text, exit code, or tool name ("It keeps failing") - The intent could plausibly mean several different things -- a tool, a tutorial, usage help, or debugging ("I need help with variant calling") - - A follow-up's referent cannot be determined from the message itself + - A follow-up's referent cannot be resolved even from the recent turn provided Do NOT use this when the current message is clear enough to route or answer on its own -- a confident route or answer is always better than an unnecessary question. @@ -523,33 +525,16 @@ class QueryRouterAgent(BaseGalaxyAgent): return [i for i, message in enumerate(full_history) if _is_turn_start(message)] def _routing_history(self, full_history: Optional[list]) -> Optional[list]: - """The history the router uses for its routing decision. - - Capped at ``ROUTING_HISTORY_TURNS`` recent turns (0 -> none). Conversation history - dilutes the routing signal and biases the model toward answering directly, so the - router routes on the current message while specialists still get the full history. - """ + """The most recent ``ROUTING_HISTORY_TURNS`` turn(s) of ``full_history``, or None when + capped to 0 or there's no history. See ``ROUTING_HISTORY_TURNS`` for the rationale.""" if not full_history or self.ROUTING_HISTORY_TURNS <= 0: return None - - turn_starts = self._turn_start_indices(full_history) - if len(turn_starts) <= self.ROUTING_HISTORY_TURNS: - return full_history - return full_history[turn_starts[-self.ROUTING_HISTORY_TURNS] :] - - def _clarification_routing_history(self, full_history: Optional[list]) -> list: - """The last conversation turn (original request + the clarifying question we asked). - - A narrow exception to history-withholding: when the user is answering a clarification, - the router needs that turn to route an elliptical answer like "the second one" -- on - its own it has no referent. Specialists still receive the full history via the handoff. - """ - if not full_history: - return [] turn_starts = self._turn_start_indices(full_history) if not turn_starts: - return full_history - return full_history[turn_starts[-1] :] + return None + if len(turn_starts) > self.ROUTING_HISTORY_TURNS: + return full_history[turn_starts[-self.ROUTING_HISTORY_TURNS] :] + return full_history async def process(self, query: str, context: Optional[dict[str, Any]] = None) -> AgentResponse: validation_error = self._validate_query(query) @@ -558,22 +543,16 @@ class QueryRouterAgent(BaseGalaxyAgent): try: full_history = self._extract_message_history(context) - # Route on the current message; deep history degrades the routing decision. - # Specialists still receive the full conversation_history via _handoff_context. - message_history: Optional[list] - if context and context.get("responding_to_clarification") and full_history: - # The previous turn asked a clarifying question -- route the answer using - # that one turn so an elliptical reply ("the second one") has a referent. - message_history = self._clarification_routing_history(full_history) - log.info(f"Router: answering a clarification, routing on the last turn ({len(message_history)} msgs)") + # Route on the current message plus the most recent turn(s) (see ROUTING_HISTORY_TURNS). + # The responding_to_clarification context flag is intentionally not consulted: forwarding + # the last turn handles clarification answers and ordinary follow-ups the same way. + message_history = self._routing_history(full_history) + if message_history: + log.info(f"Router: routing on the current message plus the last turn ({len(message_history)} msgs)") + elif full_history: + log.info(f"Router: routing on the current message, withholding {len(full_history)} history messages") else: - message_history = self._routing_history(full_history) - if full_history and not message_history: - log.info(f"Router: routing on current message, withholding {len(full_history)} history messages") - elif message_history: - log.info(f"Router: routing on {len(message_history)} recent messages") - else: - log.info("Router: processing query with no conversation history") + log.info("Router: processing query with no conversation history") previous_handoff_context = self._handoff_context self._handoff_context = context.copy() if context else {} diff --git a/test/evals/datasets/__init__.py b/test/evals/datasets/__init__.py index 8a5a528dfbe..d1ad4b3125a 100644 --- a/test/evals/datasets/__init__.py +++ b/test/evals/datasets/__init__.py @@ -13,6 +13,7 @@ from .routing_depth import ( build_history, routing_depth_dataset, ) +from .routing_followup import routing_followup_dataset from .staining_quantification import staining_quantification_dataset from .tool_recommendation import tool_recommendation_dataset @@ -28,6 +29,7 @@ __all__ = [ "routing_clarification_followup_dataset", "routing_dataset", "routing_depth_dataset", + "routing_followup_dataset", "staining_quantification_dataset", "tool_recommendation_dataset", ] diff --git a/test/evals/datasets/routing_followup.py b/test/evals/datasets/routing_followup.py new file mode 100644 index 00000000000..bebfb6e961e --- /dev/null +++ b/test/evals/datasets/routing_followup.py @@ -0,0 +1,63 @@ +"""Routing followup dataset: route a follow-up to a NORMAL (non-clarification) answer. + +The router routes on the current message and forwards the prior user turn(s) so an elliptical +follow-up keeps its referent. ``routing_clarification_followup`` covers the case where the +previous assistant turn was a clarifying question; this dataset covers the adjacent case the +clarification carve-out missed: a follow-up to an ordinary answer -- "what about a workflow +for this?", "is there a tutorial for that?" -- whose "this"/"that"/"it" points at the prior +*user* message, not the assistant's prose. + +Each case reconstructs the prior turn (the user's request + a normal assistant answer) and +provides the follow-up as the current message. Scored by HandoffMatch on the router's chosen +``agent_type`` against the gold specialist. Run with the fix OFF (``ROUTING_HISTORY_TURNS = 0``) +vs ON to quantify its value: without it the referent is lost and the router asks for +clarification (agent_type "clarification") instead of routing. + +Scenarios are generated data in ``routing_followup_scenarios.json``. +""" + +import json +from pathlib import Path +from typing import ( + Any, + Optional, +) + +from pydantic_evals import ( + Case, + Dataset, +) + +_SCENARIOS_PATH = Path(__file__).parent / "routing_followup_scenarios.json" + + +def _load_scenarios() -> list[dict[str, Any]]: + return json.loads(_SCENARIOS_PATH.read_text()) + + +def routing_followup_dataset( + only: Optional[list[str]] = None, +) -> Dataset[dict[str, Any], str, dict[str, Any]]: + """Build the followup Dataset. + + Case input is ``{"original_query", "assistant_answer", "followup"}``; the task + reconstructs the prior turn into conversation_history and routes the follow-up. + """ + cases: list[Case[dict[str, Any], str, dict[str, Any]]] = [] + for scenario in _load_scenarios(): + cases.append( + Case( + name=scenario["name"], + inputs={ + "original_query": scenario["original_query"], + "assistant_answer": scenario["assistant_answer"], + "followup": scenario["followup"], + }, + expected_output=scenario["expected"], + metadata={"followup_kind": scenario.get("followup_kind", ""), "requires_galaxy": False}, + ) + ) + if only: + wanted = set(only) + cases = [c for c in cases if c.name in wanted] + return Dataset(name="routing_followup", cases=cases) diff --git a/test/evals/datasets/routing_followup_scenarios.json b/test/evals/datasets/routing_followup_scenarios.json new file mode 100644 index 00000000000..df098f94324 --- /dev/null +++ b/test/evals/datasets/routing_followup_scenarios.json @@ -0,0 +1,50 @@ +[ + { + "name": "staining_tutorial_then_workflow", + "original_query": "Is there a tutorial for quantifying histological staining?", + "assistant_answer": "Yes -- the GTN has a tutorial on quantitative analysis of histological staining using color deconvolution that walks through the full process with example data.", + "followup": "What about a workflow for this?", + "expected": "tool_recommendation", + "followup_kind": "referential" + }, + { + "name": "trim_tool_then_tutorial", + "original_query": "What tool can I use to trim adapters from my reads?", + "assistant_answer": "Trimmomatic and Cutadapt both trim adapter sequences from sequencing reads.", + "followup": "Is there a tutorial for that?", + "expected": "gtn_training", + "followup_kind": "referential" + }, + { + "name": "rnaseq_then_workflow", + "original_query": "How do I do RNA-seq differential expression in Galaxy?", + "assistant_answer": "You align the reads, count features, and run a differential expression test such as DESeq2.", + "followup": "Is there a ready-made workflow for it?", + "expected": "tool_recommendation", + "followup_kind": "referential" + }, + { + "name": "variant_tutorial_better_one", + "original_query": "Can you recommend a tutorial for variant calling?", + "assistant_answer": "The GTN variant calling tutorial walks through mapping, calling, and filtering variants.", + "followup": "Is there a better one for bacterial genomes?", + "expected": "gtn_training", + "followup_kind": "referential" + }, + { + "name": "longread_tool_then_workflow", + "original_query": "Which tool aligns long reads?", + "assistant_answer": "Minimap2 is commonly used to align long reads against a reference.", + "followup": "And a full workflow that uses it?", + "expected": "tool_recommendation", + "followup_kind": "referential" + }, + { + "name": "assembly_then_tutorial", + "original_query": "What can I use to assemble a bacterial genome?", + "assistant_answer": "Unicycler and SPAdes are widely used for bacterial genome assembly.", + "followup": "How do I learn the whole process end to end?", + "expected": "gtn_training", + "followup_kind": "named" + } +] diff --git a/test/evals/specs.py b/test/evals/specs.py index 2f9ef6f62fd..4a3f289dfe8 100644 --- a/test/evals/specs.py +++ b/test/evals/specs.py @@ -30,6 +30,7 @@ from .datasets import ( routing_clarification_followup_dataset, routing_dataset, routing_depth_dataset, + routing_followup_dataset, staining_quantification_dataset, tool_recommendation_dataset, ) @@ -49,6 +50,7 @@ from .tasks import ( make_orchestrator_plan_task, make_router_clarification_task, make_router_content_task, + make_router_followup_task, make_router_inspect_task, make_router_multiturn_task, make_router_task, @@ -183,6 +185,47 @@ def build_routing_clarification_followup_nofix( return _build_routing_clarification_followup(deps, False, only, usage_buffer) +def _build_routing_followup( + deps: GalaxyAgentDependencies, + route_followup: bool, + only: Optional[list[str]], + usage_buffer: Optional[list[dict[str, int]]], +) -> BuiltDataset: + dataset = routing_followup_dataset(only=only) + dataset.add_evaluator(HandoffMatch()) + return BuiltDataset( + dataset=dataset, + task=make_router_followup_task(deps, route_followup=route_followup, usage_buffer=usage_buffer), + primary_score="HandoffMatch", + ) + + +def build_routing_followup( + deps: GalaxyAgentDependencies, + judge_model: Optional[Model] = None, + only: Optional[list[str]] = None, + include_galaxy_required: bool = False, + usage_buffer: Optional[list[dict[str, int]]] = None, +) -> BuiltDataset: + """Route a follow-up to a normal answer WITH the fix (the shipped behavior): the router + sees the prior user turn, so "what about a workflow for this?" routes to the right + specialist instead of asking what "this" means.""" + return _build_routing_followup(deps, True, only, usage_buffer) + + +def build_routing_followup_nofix( + deps: GalaxyAgentDependencies, + judge_model: Optional[Model] = None, + only: Optional[list[str]] = None, + include_galaxy_required: bool = False, + usage_buffer: Optional[list[dict[str, int]]] = None, +) -> BuiltDataset: + """A/B control: route the same follow-ups WITHOUT the fix (prior user turn withheld). The + elliptical follow-up has no referent, so this should score well below the fixed variant -- + that gap is the fix's value.""" + return _build_routing_followup(deps, False, only, usage_buffer) + + def build_error_analysis( deps: GalaxyAgentDependencies, judge_model: Optional[Model] = None, @@ -323,6 +366,8 @@ SPECS: dict[str, Callable[..., BuiltDataset]] = { "routing_ambiguous": build_routing_ambiguous, "routing_clarification_followup": build_routing_clarification_followup, "routing_clarification_followup_nofix": build_routing_clarification_followup_nofix, + "routing_followup": build_routing_followup, + "routing_followup_nofix": build_routing_followup_nofix, "error_analysis": build_error_analysis, "tool_recommendation": build_tool_recommendation, "custom_tool": build_custom_tool, diff --git a/test/evals/tasks.py b/test/evals/tasks.py index 110aed7ddd9..818e215ce07 100644 --- a/test/evals/tasks.py +++ b/test/evals/tasks.py @@ -257,6 +257,39 @@ def make_router_clarification_task( return router_clarification_task +def make_router_followup_task( + deps: GalaxyAgentDependencies, + route_followup: bool = True, + usage_buffer: UsageBuffer = None, +) -> Callable[[dict], Awaitable[str]]: + """Build an async callable for the followup dataset. + + The case input is ``{"original_query", "assistant_answer", "followup"}``. Reconstructs the + prior turn (the user's request + a normal assistant answer) as conversation_history and + routes the elliptical ``followup``. With ``route_followup=True`` (the shipped default) the + router forwards the prior turn so "this"/"that" has a referent; with ``False`` it sets + ``ROUTING_HISTORY_TURNS = 0`` to withhold it -- the A/B that quantifies the fix's value. + Returns the router's chosen agent_type. + """ + + async def router_followup_task(case_input: dict) -> str: + history = [ + {"role": "user", "content": case_input["original_query"]}, + {"role": "assistant", "content": case_input["assistant_answer"]}, + ] + router = QueryRouterAgent(deps) + if not route_followup: + router.ROUTING_HISTORY_TURNS = 0 + response = await router.process( + case_input["followup"], + context={"conversation_history": history}, + ) + _record_response_usage(usage_buffer, response) + return response.agent_type + + return router_followup_task + + def make_router_content_task( deps: GalaxyAgentDependencies, context: Optional[dict] = None, diff --git a/test/unit/app/test_agents.py b/test/unit/app/test_agents.py index 343171fdc7f..632770d4c7a 100644 --- a/test/unit/app/test_agents.py +++ b/test/unit/app/test_agents.py @@ -629,32 +629,63 @@ class TestAgentUnitMocked: ) @pytest.mark.asyncio - async def test_router_withholds_history_for_routing(self): - """Router routes on the current message, withholding conversation history from the - model. Feeding history dilutes the routing signal (evals show it degrades routing - monotonically), so ``message_history`` is not forwarded; specialists still get the - full history via the handoff context.""" + async def test_router_forwards_prior_turn_for_followup(self): + """A follow-up to a normal answer ("what about a workflow for this?") needs the prior + turn for its referent, so the router forwards the whole last turn -- the prior request + AND its assistant reply. (A bare user message with no reply reads as a dangling request + and mis-routes.) Deep history is still withheld; specialists get it via the handoff.""" router = QueryRouterAgent(self.deps) history: list[ModelMessage] = [ - ModelRequest(parts=[UserPromptPart(content="What histories do I have?")]), - ModelResponse(parts=[TextPart(content="You have 3.")]), + ModelRequest(parts=[UserPromptPart(content="Is there a tutorial for quantifying histological staining?")]), + ModelResponse(parts=[TextPart(content="Yes -- here is a GTN tutorial on color deconvolution.")]), ] with mock.patch.object(router, "_run_with_retry") as mock_run: mock_result = mock.Mock(spec=["output"]) - mock_result.output = "Following up: here is more detail." + mock_result.output = "Routed." mock_run.return_value = mock_result await router.process( - "Tell me more about the second one", + "What about a workflow for this?", context={"conversation_history": history}, ) mock_run.assert_called_once() args, kwargs = mock_run.call_args - # Routing decision is made on the current message, not the accumulated history. - assert kwargs["message_history"] is None - assert args[0] == "Tell me more about the second one" + assert args[0] == "What about a workflow for this?" + forwarded = kwargs["message_history"] + assert forwarded is not None + # The whole prior turn rides along -- request and reply both. + contents = [part.content for message in forwarded for part in message.parts] + assert any("histological staining" in content for content in contents) + assert any("color deconvolution" in content for content in contents) + + @pytest.mark.asyncio + async def test_router_followup_history_capped_to_last_turn(self): + """Across a deeper conversation the router forwards only the most recent turn + (ROUTING_HISTORY_TURNS) -- both its messages, not the older turns -- enough to resolve + the referent without re-diluting the routing signal that #22791 protects.""" + router = QueryRouterAgent(self.deps) + history: list[ModelMessage] = [ + ModelRequest(parts=[UserPromptPart(content="What histories do I have?")]), + ModelResponse(parts=[TextPart(content="You have 3.")]), + ModelRequest(parts=[UserPromptPart(content="Tell me about RNA-seq alignment tools")]), + ModelResponse(parts=[TextPart(content="HISAT2 and STAR are common aligners.")]), + ] + + with mock.patch.object(router, "_run_with_retry") as mock_run: + mock_result = mock.Mock(spec=["output"]) + mock_result.output = "Routed." + mock_run.return_value = mock_result + + await router.process("Is there a workflow for that?", context={"conversation_history": history}) + + _, kwargs = mock_run.call_args + forwarded = kwargs["message_history"] + assert forwarded is not None + # Only the last turn (both messages), not the earlier "What histories" turn. + contents = [part.content for message in forwarded for part in message.parts] + assert contents == ["Tell me about RNA-seq alignment tools", "HISAT2 and STAR are common aligners."] @pytest.mark.asyncio async def test_router_injects_interface_context_into_prompt(self):