From d38bb3f077351a4fe3cd23cc426ddb759c580da5 Mon Sep 17 00:00:00 2001 From: Daniel Han Date: Sun, 24 May 2026 15:08:18 +0000 Subject: [PATCH] Studio: revert Plan: / "Here is the plan" intent and narrow plan verbs Reviewer round 9 (5 of 10 reviewers) flagged that the new bare ``Plan:`` / ``Approach:`` / ``Here is the plan`` intent branches reintroduced the original "wipe a complete answer" failure for realistic final answers whose topic happens to contain a tool-action word. Triggers for prompts like "Create a lesson plan for teaching search skills" when the model answers: Plan: 1. Search skills: students learn query keywords. 2. Source evaluation: compare domains. 3. Reflection: write what worked. ``_INTENT_SIGNAL`` matched the new ``Plan:`` lookahead because ``search`` appears within 120 chars, then ``_PLAN_LIST_FRAMING`` disqualified the numbered list, and the synthetic STOP turn wiped a valid answer. Revert the additions in ``_INTENT_SIGNAL``: * Drop ``Plan:`` / ``Approach:`` (newline + action-verb lookahead). * Drop ``Here is the plan`` / ``Here are my steps`` (action-verb lookahead). Plan stalls phrased with explicit first-person intent ("I'll search...", "First, I'll fetch...", "Let me look up...") are still caught by the existing intent patterns and ``_PLAN_LIST_FRAMING``. Also narrow the plan-list action-verb whitelist to tool-specific verbs (``search`` / ``look up`` / ``fetch`` / ``browse`` / ``web search`` / ``call (a) tool`` / ``run python`` / ``execute python``). Broad verbs like ``use`` / ``compare`` / ``check`` / ``find`` / ``think`` / ``respond`` / ``answer`` / ``analyse`` / ``explore`` / ``outline`` / ``reason`` are removed because real answer lists use them ("1. Use BFS", "1. Compare versions"). Finally, fix the test module's ``loggers`` / ``structlog`` stub injection to only fire when the real module is missing AND to set ``__path__ = []`` on the stub. Previously the bare ``ModuleType`` could poison ``sys.modules`` for any later test that imports a real submodule (``from loggers.handlers import ...``). Net behavioural change vs the previous commit: stricter on what counts as a plan stall, never wipes a final answer titled ``Plan:`` / ``My plan:`` / ``Here is the plan you asked for``. --- studio/backend/core/inference/llama_cpp.py | 54 +++----- .../tests/test_llama_cpp_reprompt_guard.py | 127 ++++++------------ 2 files changed, 58 insertions(+), 123 deletions(-) diff --git a/studio/backend/core/inference/llama_cpp.py b/studio/backend/core/inference/llama_cpp.py index 5c44c2062b..74a6e3529a 100644 --- a/studio/backend/core/inference/llama_cpp.py +++ b/studio/backend/core/inference/llama_cpp.py @@ -52,18 +52,18 @@ logger = get_logger(__name__) # ── Pre-compiled patterns for plan-without-action re-prompt ── -# Re-prompt-action verbs. Used both as a nearby-verb lookahead for the -# new ``Plan:`` / ``Here is the plan`` intents (so prose final answers -# such as ``Plan:\n1. Warm-up\n2. Group practice`` or "Here is the plan -# you asked for" do not wipe) and as the plan-list disqualifier verb -# set in _PLAN_LIST_FRAMING below. Conservative on purpose: ambiguous -# verbs like ``write``, ``create``, ``make``, ``build``, ``do``, -# ``handle`` are deliberately excluded because real answer lists use -# them ("1. Write a poem", "1. Create directory"). -_REPROMPT_ACTION_VERBS = ( - r"search|look up|call|use|fetch|browse|run|execute|" - r"check|find|open|verify|compare|summari[sz]e|think|respond|" - r"answer|analy[sz]e|explore|outline|gather|query|reason" +# Tool-action verbs used by _PLAN_LIST_FRAMING to distinguish plan-only +# numbered lists ("1. search the docs", "1. fetch the data") from +# answer numbered lists ("1. Apple", "1. Use BFS", "1. Write a poem"). +# Kept intentionally narrow: only verbs that strongly imply an actual +# tool invocation. Broad verbs like ``use``, ``compare``, ``check``, +# ``find``, ``write``, ``create``, ``make``, ``build``, ``think``, +# ``respond``, ``answer``, ``analyse``, ``explore`` are deliberately +# excluded because real answer lists use them ("1. Use BFS", +# "1. Compare versions", "1. Write a poem"). +_TOOL_ACTION_VERBS = ( + r"search|look up|fetch|browse|web[ _-]?search|" + r"call (?:a |the )?tool|run (?:python|the code)|execute (?:python|the code)" ) # Forward-looking intent signals that indicate the model is @@ -79,20 +79,6 @@ _INTENT_SIGNAL = re.compile( # Step/plan framing: "First ...", "Step 1:", "Here's my plan". r"\b(?:first\b|step \d+:?|here['\u2019]?s (?:my |the |a )?(?:plan|approach))" r"|" - # "Here is the plan" / "Here are my steps" framings. Require an - # action verb within 120 chars so prose answers like "Here is the - # plan you asked for" do not match. - r"\bhere (?:is|are) (?:my |the |a )?(?:plan|approach|steps)\b" - rf"(?=[\s\S]{{0,120}}\b(?:{_REPROMPT_ACTION_VERBS})\b)" - r"|" - # Bare "Plan:" / "Approach:" (optionally preceded by a determiner - # like "My" / "The" / "Our") anchored to start of line AND followed - # by a newline AND followed within 120 chars by an action verb so - # final answers shaped like "Plan:\n1. Warm-up\n2. Group practice" - # or "My plan:\n1. Breakfast\n2. Lunch" do NOT wipe. - r"(?:^|\r?\n)[ \t]*(?:(?:my|the|our|a|this|that)\s+)?(?:plan|approach):[ \t]*(?=\r?\n)" - rf"(?=[\s\S]{{0,120}}\b(?:{_REPROMPT_ACTION_VERBS})\b)" - r"|" # "Now I" / "Next I" patterns r"\b(?:now i|next i)\b" r")" @@ -139,22 +125,18 @@ _NUMBERED_LIST_ARTIFACT = re.compile( ) # Markers that a numbered list is a plan (still re-promptable), not a -# final answer. The intent alternatives mirror _INTENT_SIGNAL above so -# every recognised intent phrase can disqualify a numbered list. The -# apostrophe in ``i['’]ll`` is required (no ``?``) so the regex does not -# accidentally match the word "ill". Both branches require an action -# verb nearby so plan-style answer headers ("Plan:\n1. Warm-up\n2. -# Group practice") are NOT treated as plans and stay artifacts. +# final answer. Only fires when an intent phrase from _INTENT_SIGNAL is +# already followed within 80 chars by a narrow tool-action verb. The +# apostrophe in ``i['’]ll`` is required (no ``?``) so the regex does +# not accidentally match the word "ill". Without a tool-action verb the +# numbered list is treated as a completed answer artifact. _PLAN_LIST_FRAMING = re.compile( r"\b(?:here['’]?s (?:my |the |a )?(?:plan|approach)|" - r"here (?:is|are) (?:my |the |a )?(?:plan|approach|steps)|" r"step \d+|first|" r"i['’](?:ll|m going to|m gonna)|i am (?:going to|gonna)|" r"i will|i shall|let me|allow me|now i|next i)\b" r"[\s\S]{0,80}" - rf"\b(?:{_REPROMPT_ACTION_VERBS})\b" - r"|(?:^|\r?\n)[ \t]*(?:(?:my|the|our|a|this|that)\s+)?(?:plan|approach):" - rf"[\s\S]{{0,120}}\b(?:{_REPROMPT_ACTION_VERBS})\b", + rf"\b(?:{_TOOL_ACTION_VERBS})\b", re.IGNORECASE, ) diff --git a/studio/backend/tests/test_llama_cpp_reprompt_guard.py b/studio/backend/tests/test_llama_cpp_reprompt_guard.py index 7f531c1115..492ee1ecc7 100644 --- a/studio/backend/tests/test_llama_cpp_reprompt_guard.py +++ b/studio/backend/tests/test_llama_cpp_reprompt_guard.py @@ -38,13 +38,26 @@ _BACKEND_DIR = str(Path(__file__).resolve().parent.parent) if _BACKEND_DIR not in sys.path: sys.path.insert(0, _BACKEND_DIR) -_loggers_stub = _types.ModuleType("loggers") -_loggers_stub.get_logger = lambda name: __import__("logging").getLogger(name) -sys.modules.setdefault("loggers", _loggers_stub) +# Inject minimal stand-ins ONLY when the real modules are unavailable. +# Using ``setdefault`` with a non-package ``ModuleType`` would otherwise +# poison ``sys.modules`` for any later test that does +# ``from loggers.handlers import ...`` (Python would raise "loggers is +# not a package" because the stub has no ``__path__``). +try: # noqa: E402 + import loggers # type: ignore # real backend package +except ModuleNotFoundError: + _loggers_stub = _types.ModuleType("loggers") + _loggers_stub.__path__ = [] # type: ignore[attr-defined] + _loggers_stub.get_logger = lambda name: __import__("logging").getLogger(name) + sys.modules["loggers"] = _loggers_stub -_structlog_stub = _types.ModuleType("structlog") -_structlog_stub.get_logger = lambda *a, **k: __import__("logging").getLogger("stub") -sys.modules.setdefault("structlog", _structlog_stub) +try: # noqa: E402 + import structlog # type: ignore +except ModuleNotFoundError: + _structlog_stub = _types.ModuleType("structlog") + _structlog_stub.__path__ = [] # type: ignore[attr-defined] + _structlog_stub.get_logger = lambda *a, **k: __import__("logging").getLogger("stub") + sys.modules["structlog"] = _structlog_stub from core.inference.llama_cpp import ( # noqa: E402 _HAS_ANSWER_ARTIFACT, @@ -220,16 +233,16 @@ def test_numbered_list_without_plan_framing_is_artifact(): def test_numbered_list_with_plan_framing_is_NOT_artifact(): - """A numbered list paired with explicit plan framing must NOT count - as a completed artifact. The list IS the plan, not the answer.""" + """A numbered list paired with explicit plan framing (intent phrase + followed by a narrow tool-action verb such as ``search`` / ``fetch`` + / ``browse``) must NOT count as a completed artifact. The list IS + the plan, not the answer. Broad verbs like ``compare`` / ``use`` / + ``verify`` are intentionally NOT plan framing because real answer + lists use them.""" samples = [ - # "Here's my plan" / "plan:" / "approach:". - "Here's my plan:\n1. Search the web\n2. Summarise the result.", - "Here is the plan:\n1. Look up the date.\n2. Compare versions.", - "My approach:\n1. Search\n2. Verify\n3. Answer.", - # Intent phrase + tool-action verb in close proximity. + "Here's my plan:\n1. Search the web\n2. then summarise.", "First, I'll do these:\n1. search for the song list\n2. cross-check the chart", - "Let me look up the values:\n1. fetch the data\n2. compare to baseline", + "Let me look up the values: fetch the data first.", ] for s in samples: assert _PLAN_LIST_FRAMING.search(s), s @@ -334,32 +347,8 @@ def test_reprompts_on_incomplete_html_intent(): assert _would_reprompt(content) -def test_reprompts_on_plan_colon_intent(): - """Bare ``Plan:`` / ``Approach:`` followed by a newline at the start - of a structured reply is now an intent signal so the plan stall - re-prompts. Inline ``Plan: `` (no newline) is NOT an intent - signal because that shape is common in marketing / product answers - such as ``Plan: Pro is $10/month``.""" - samples = [ - "Plan:\n1. search the docs\n2. summarise", - "Approach:\n1. fetch the data\n2. compare", - ] - for s in samples: - assert _INTENT_SIGNAL.search(s), s - assert _would_reprompt(s), s -def test_reprompts_on_plan_with_extended_action_verbs(): - """The plan-framing verb whitelist also covers think / respond / - answer / analy[sz]e / explore / outline / gather / query / reason - so plan stalls phrased with those verbs still re-prompt.""" - samples = [ - "Here is what I will do:\n1. think it through\n2. respond clearly", - "First, let me reason about this:\n1. weigh options\n2. answer concisely", - "Now I will analyse this:\n1. break it down\n2. summarise findings", - ] - for s in samples: - assert _would_reprompt(s), s def test_plan_framing_requires_apostrophe_in_ill(): @@ -398,10 +387,10 @@ def test_reprompts_on_all_intent_form_numbered_action_plans(): def test_no_reprompt_on_plan_titled_final_answer_without_actions(): """A final answer naturally titled ``Plan:`` / ``My plan:`` / - ``Approach:`` whose numbered items are content (not action verbs) - must NOT wipe. The action-verb lookahead on the Plan: intent - branch is what filters lesson plans, meal plans, dinner plans, - and similar from being misclassified as tool-action stalls.""" + ``Approach:`` must NOT wipe. Bare ``Plan:`` / ``Approach:`` is + deliberately NOT an intent signal in _INTENT_SIGNAL because it + too often appears as a normal answer heading (lesson plan, meal + plan, business plan, project plan, ...).""" samples = [ "Plan:\n1. Warm-up: Students review fractions.\n2. Group practice.\n3. Assessment.", "My plan:\n1. Breakfast: oatmeal and fruit.\n2. Lunch: rice bowl.\n3. Dinner: lentil soup.", @@ -411,16 +400,21 @@ def test_no_reprompt_on_plan_titled_final_answer_without_actions(): assert not _would_reprompt(s), s -def test_reprompts_on_plan_titled_action_stall(): - """A ``Plan:`` / ``Approach:`` header whose items DO contain action - verbs (search / fetch / verify / ...) still re-prompts.""" +def test_no_reprompt_on_bare_plan_header_action_stall(): + """Bare ``Plan:`` / ``Approach:`` headers paired with tool-action + verbs are NOT classified as plan stalls. Adding them as intent + markers caused false positives on legitimate plan answers; we + accept the smaller false negative (action plans titled only with + ``Plan:`` slip through) in exchange for not wiping valid answers. + Plan stalls that use an explicit first-person intent phrase ("I'll + search...", "First, I'll fetch...") are still caught.""" samples = [ "Plan:\n1. search the docs\n2. summarise the result", "My plan:\n1. fetch the data\n2. verify the rows", "The approach:\n1. look up the value\n2. compare versions", ] for s in samples: - assert _would_reprompt(s), s + assert not _would_reprompt(s), s def test_no_reprompt_on_here_is_the_plan_prose_answer(): @@ -436,47 +430,6 @@ def test_no_reprompt_on_here_is_the_plan_prose_answer(): assert not _would_reprompt(s), s -def test_plan_colon_intent_is_line_anchored(): - """``Plan:`` / ``Approach:`` only counts as an intent marker when it - is at the start of a line. Without this anchor, normal direct - answers containing phrases like ``lesson plan:``, ``meal plan:``, - ``migration plan:``, or ``My approach:`` would trigger the - re-prompt path and risk wiping a valid response.""" - # These mid-line "plan:" / "approach:" mentions are NOT intent signals. - # The qualifier before "plan" is a content noun ("lesson", "meal", - # "migration") rather than a generic determiner, OR the colon is - # followed by inline content instead of a newline-anchored header. - direct_answers = [ - "Here is a lesson plan:\n1. Warm-up\n2. Group practice\n3. Assessment", - "I prepared a meal plan: rice, beans, eggs.", - "Quick approach: top-down then bottom-up.", - "Your current Plan: Pro includes local chats.", - "The plan: Basic is free, Pro is $10/month, Enterprise is custom.", - "My plan: use dynamic programming with memoisation.", - "Recommended approach: use the Python SDK for uploads.", - "The migration plan: backup, run, verify all in one window.", - ] - for s in direct_answers: - assert not _INTENT_SIGNAL.search(s), s - assert not _would_reprompt(s), s - # "Plan:" / "Approach:" with optional generic determiner at the start - # of a line, followed by a newline AND followed by an action verb - # within 120 chars, IS an intent signal. The newline requirement - # filters inline product/answer text such as "The plan: Pro is - # $10/month"; the action-verb requirement filters real prose answers - # such as "Plan:\n1. Warm-up\n2. Group practice". - plan_starts = [ - "Plan:\n1. search\n2. summarise", - "Approach:\n1. fetch\n2. compare", - " Plan:\n1. think\n2. respond", # leading indent OK - "Lorem ipsum\nPlan:\n1. search\n2. fetch", # plan: on a later line - "My plan:\n1. search\n2. summarise", - "The plan:\n1. look up\n2. compare", - "Our approach:\n1. fetch\n2. verify", - ] - for s in plan_starts: - assert _INTENT_SIGNAL.search(s), s - assert _would_reprompt(s), s # ── Cross-platform line endings ────────────────────────────────────