diff --git a/tests/tools/test_clarify_tool.py b/tests/tools/test_clarify_tool.py index 3820acc2e6cbc..de721981dfd0b 100644 --- a/tests/tools/test_clarify_tool.py +++ b/tests/tools/test_clarify_tool.py @@ -62,7 +62,7 @@ class TestClarifyToolChoicesValidation: return "answer" clarify_tool("Pick", choices=[1, 2, 3], callback=mock_callback) # type: ignore - assert choices_received == ["1", "2", "3"] + assert choices_received == ["1 (Recommended)", "2", "3"] class TestClarifyToolCallbackHandling: @@ -129,7 +129,7 @@ class TestClarifyDictChoices: callback=cb, )) # type: ignore assert seen == [ - "Tight, covers all 3 points", + "Tight, covers all 3 points (Recommended)", "Loose layout", "A plain string choice", ] @@ -222,6 +222,78 @@ class TestClarifyToolMultiSelect: assert len(choices_passed) == MAX_CHOICES +class TestClarifyRecommendedLabel: + """The first choice is the agent's pick and is labelled as such. + + The schema tells the model to order choices best-first, so the tool tags + element 0 with "(Recommended)" at the one platform-agnostic entry point — + CLI, TUI, desktop, and messaging adapters all inherit the same label. The + label is presentation only: it never appears in the answer the agent reads. + """ + + def test_first_choice_is_labelled(self): + seen = [] + + def cb(question, choices): + seen.extend(choices or []) + return choices[1] + + clarify_tool("Pick", choices=["Rebase", "Merge"], callback=cb) + assert seen == ["Rebase (Recommended)", "Merge"] + + def test_answer_strips_the_label(self): + """Picking the recommended option returns the bare option text.""" + def cb(question, choices): + return choices[0] + + result = json.loads(clarify_tool("Pick", choices=["Rebase", "Merge"], callback=cb)) + assert result["user_response"] == "Rebase" + assert result["choices_offered"] == ["Rebase", "Merge"] + + def test_multi_select_answers_strip_the_label(self): + def cb(question, choices, multi_select=False): + return ", ".join(choices[:2]) + + result = json.loads(clarify_tool( + "Pick some", + choices=["Rebase", "Merge", "Squash"], + multi_select=True, + callback=cb, + )) + assert result["user_response"] == ["Rebase", "Merge"] + + def test_single_choice_is_not_labelled(self): + """One option isn't a recommendation — there's nothing to prefer it over.""" + seen = [] + + def cb(question, choices): + seen.extend(choices or []) + return choices[0] + + clarify_tool("Confirm", choices=["Ship it"], callback=cb) + assert seen == ["Ship it"] + + def test_label_is_not_doubled(self): + """A model that wrote its own label doesn't get a second one.""" + seen = [] + + def cb(question, choices): + seen.extend(choices or []) + return choices[0] + + clarify_tool("Pick", choices=["Rebase (recommended)", "Merge"], callback=cb) + assert seen == ["Rebase (recommended)", "Merge"] + + def test_open_ended_unaffected(self): + def cb(question, choices): + assert choices is None + return "whatever" + + result = json.loads(clarify_tool("Thoughts?", callback=cb)) + assert result["choices_offered"] is None + assert result["user_response"] == "whatever" + + class TestInvokeCallbackDispatch: """_invoke_callback uses signature inspection, never a TypeError retry.""" diff --git a/tools/clarify_gateway.py b/tools/clarify_gateway.py index 76bb3b4d09d30..c88ef2f45dcd3 100644 --- a/tools/clarify_gateway.py +++ b/tools/clarify_gateway.py @@ -201,6 +201,19 @@ def get_pending_for_session( return None +def _label_matches(text: str, choice: object) -> bool: + """Case-insensitive label match that ignores the '(Recommended)' suffix. + + The first choice reaches adapters already decorated (see + ``tools.clarify_tool.mark_recommended``), so a user who types the option + text as the agent worded it — without the label — must still resolve the + prompt. + """ + from tools.clarify_tool import strip_recommended + + return strip_recommended(text).casefold() == strip_recommended(str(choice)).casefold() + + def _coerce_text_response(entry: _ClarifyEntry, response: str) -> Optional[str]: """Map typed choice replies to canonical choice text, otherwise keep or reject custom text. @@ -250,7 +263,7 @@ def _coerce_text_response(entry: _ClarifyEntry, response: str) -> Optional[str]: # Try exact choice label match (always valid for multi-choice) for choice in entry.choices: - if text.casefold() == str(choice).strip().casefold(): + if _label_matches(text, choice): return str(choice).strip() # For text fallback or awaiting_text mode, accept custom text @@ -300,7 +313,7 @@ def _coerce_multi_select_text(entry: _ClarifyEntry, text: str) -> Optional[str]: # Exact label match (case-insensitive) matched = None for choice in choices: - if token.casefold() == str(choice).strip().casefold(): + if _label_matches(token, choice): matched = str(choice).strip() break if matched is None: diff --git a/tools/clarify_tool.py b/tools/clarify_tool.py index 98bc9e4b42f46..d4bc8abeb13e6 100644 --- a/tools/clarify_tool.py +++ b/tools/clarify_tool.py @@ -22,6 +22,11 @@ from typing import List, Optional, Callable # A 5th "Other (type your answer)" option is always appended by the UI. MAX_CHOICES = 4 +# Suffix appended to the first choice so the user can see, at a glance, which +# option the agent actually recommends. Applied here rather than per-surface so +# CLI, TUI, desktop, and messaging adapters all render the same label. +RECOMMENDED_LABEL = "(Recommended)" + def _flatten_choice(c) -> str: """Coerce a single choice into its user-facing display string. @@ -56,6 +61,42 @@ def _flatten_choice(c) -> str: return str(c).strip() +def mark_recommended(choices: List[str]) -> List[str]: + """Label the first choice as the agent's recommendation. + + The schema tells the model to order ``choices`` best-first, so element 0 is + always the option it would pick itself. Tagging it here — the one + platform-agnostic entry point — means every surface (CLI panel, TUI, + desktop card, Telegram buttons) reads the same way without four copies of + the same string concatenation, and the label can never drift between them. + + Idempotent: a model that writes its own "(recommended)" into the choice is + left alone rather than getting the suffix twice. A lone choice isn't a + recommendation — there's nothing to prefer it over — so single-choice lists + pass through untouched. + """ + if len(choices) < 2: + return choices + first = str(choices[0]).strip() + if first != strip_recommended(first): + return choices + return [f"{first} {RECOMMENDED_LABEL}"] + list(choices[1:]) + + +def strip_recommended(text: str) -> str: + """Remove the recommendation label from a resolved answer. + + The user picks the decorated string, but the agent asked about the bare + option — returning "Rebase onto main (Recommended)" as ``user_response`` + would leak presentation into the answer the model reasons about and into + anything it echoes back. + """ + stripped = str(text).strip() + if stripped.casefold().endswith(RECOMMENDED_LABEL.casefold()): + return stripped[: -len(RECOMMENDED_LABEL)].strip() + return stripped + + def _invoke_callback(callback, question, choices, multi_select): """Invoke the platform callback, passing multi_select if supported. @@ -159,19 +200,26 @@ def clarify_tool( if callback is None: return tool_error("Clarify tool is not available in this execution context.") + # The first choice is the agent's pick (the schema says order best-first), + # so it reaches every surface carrying the "(Recommended)" label. The bare + # list is what goes back to the agent — the label is presentation only. + offered = choices + if choices is not None: + choices = mark_recommended(choices) + try: raw_response = _invoke_callback(callback, question, choices, multi_select) except Exception as exc: return tool_error(f"Failed to get user input: {exc}") if multi_select and choices is not None: - user_response = _parse_multi_select_response(raw_response) + user_response = [strip_recommended(r) for r in _parse_multi_select_response(raw_response)] else: - user_response = str(raw_response).strip() + user_response = strip_recommended(raw_response) return json.dumps({ "question": question, - "choices_offered": choices, + "choices_offered": offered, "user_response": user_response, }, ensure_ascii=False) @@ -191,7 +239,8 @@ CLARIFY_SCHEMA = { "Ask the user a question when you need clarification, feedback, or a " "decision before proceeding. Supports three modes:\n\n" "1. **Single-select multiple choice** — provide up to 4 choices. The user picks one " - "or types their own answer via a 5th 'Other' option.\n" + "or types their own answer via a 5th 'Other' option. List the choice you recommend " + "FIRST: the UI labels it '(Recommended)' and highlights it by default.\n" "2. **Multi-select multiple choice** — set multi_select=true. The user can select " "multiple options via checkboxes. user_response will be a list of selected choices.\n" "3. **Open-ended** — omit choices entirely. The user types a free-form " @@ -229,6 +278,10 @@ CLARIFY_SCHEMA = { "description": ( "REQUIRED whenever you are presenting selectable options: " "each distinct option is its own array element (up to 4). " + "ORDER MATTERS: put the option you actually recommend " + "FIRST — the UI labels it '(Recommended)' and pre-selects " + "it, so a list ordered arbitrarily recommends the wrong " + "thing to the user. Do not write '(Recommended)' yourself. " "The UI renders these as pickable rows and auto-appends an " "'Other (type your answer)' option. Omit this parameter " "entirely ONLY for a genuinely open-ended free-text question."