-
-
Notifications
You must be signed in to change notification settings - Fork 6.8k
Studio: Don't re-prompt finished answers in the tool loop #7505
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 5 commits
a1c3be8
2e51d87
6e7fcc0
4223f03
2253126
8f384e9
21b96b2
f161f67
f67c5b1
4824e33
b8a86ed
2c667f7
7be4415
c66c6de
d6073ce
fbe165e
2e3277b
dcf0de4
88784c0
eb59611
611aa05
4ca5e9e
9bf66ee
20fe689
a105c44
a9e3444
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -92,6 +92,7 @@ | |
| from core.inference.tool_call_parser import ( | ||
| MAX_ACT_REPROMPTS as _MAX_REPROMPTS, | ||
| REPROMPT_MAX_CHARS as _REPROMPT_MAX_CHARS, | ||
| is_reprompt_repeat as _is_reprompt_repeat, | ||
| is_short_intent_without_action as _is_short_intent_without_action, | ||
| reprompt_to_act_message as _reprompt_to_act_message, | ||
| ) | ||
|
|
@@ -339,8 +340,13 @@ def _finalize_reasoning_only_cumulative( | |
| # loop). Structured delta.tool_calls are grammar-bounded by llama-server; text | ||
| # parsed from content is not, so one runaway turn could fan out unbounded. | ||
| _MAX_TOOL_CALLS_PER_TURN = 8 | ||
| _FORCED_REPEAT_PLAN_SIGNAL = re.compile( | ||
| r"\b(?:i\s+will|i'll|let\s+me|going\s+to|need\s+to|call|use|run|search|fetch|render)\b", | ||
| # Obligation phrasing INTENT_SIGNAL leaves alone ("I need to call ...", "I should | ||
| # call ..."), paired with an action verb: the bare topic words this replaced ate | ||
| # real answers. "should" is its own alternative -- folding it into the | ||
| # need|have|ought group would demand "I should to call". | ||
| _FORCED_PLAN_INTENT = re.compile( | ||
| r"(?:\bi\s+(?:(?:need|have|ought)\s+to|should)|^need\s+to|^going\s+to)" | ||
| r"\s+(?:\w+\s+){0,2}?(?:call|use|run|search|fetch|render|invoke|query)\b", | ||
| re.I, | ||
| ) | ||
| _FINAL_ANSWER_SIGNAL = re.compile( | ||
|
|
@@ -443,7 +449,9 @@ def _should_suppress_forced_no_tool_output(text: str) -> bool: | |
| return False | ||
| if _FINAL_ANSWER_SIGNAL.search(stripped): | ||
| return False | ||
| return _FORCED_REPEAT_PLAN_SIGNAL.search(stripped) is not None | ||
| if _is_short_intent_without_action(stripped): | ||
| return True | ||
| return _FORCED_PLAN_INTENT.search(stripped) is not None | ||
|
|
||
|
|
||
| # ── Pre-compiled patterns for GGUF shard detection ─────────── | ||
|
|
@@ -11241,14 +11249,18 @@ def _tool_succeeded(tool_name: str) -> bool: | |
| # direct answer ("4", "Hello!") won't match. Pattern shared with the | ||
| # safetensors loop (tool_call_parser.INTENT_SIGNAL). | ||
| _reprompt_count = 0 | ||
| # Budgeted apart from _reprompt_count so a pre-tool nudge can't spend it. | ||
| _post_tool_reprompts = 0 | ||
| # Text that triggered the last nudge; if the retry restates it, stop. | ||
| _last_reprompt_text = "" | ||
| # Gates ``max_tool_iterations`` on real tool turns (not the enlarged range) so reserved | ||
| # re-prompt slots don't extend the budget. Mirrors the safetensors guard. | ||
| _tool_iters_done = 0 | ||
| _forced_tool_call_pending = False | ||
|
|
||
| # Reserve extra iterations for re-prompts so they don't consume the | ||
| # caller's tool-call budget; only when tool iterations are allowed. | ||
| _extra = _MAX_REPROMPTS if max_tool_iterations > 0 else 0 | ||
| _extra = _MAX_REPROMPTS + 1 if max_tool_iterations > 0 else 0 | ||
| for iteration in range(max_tool_iterations + _extra): | ||
| if cancel_event is not None and cancel_event.is_set(): | ||
| return | ||
|
|
@@ -11876,7 +11888,7 @@ def _tool_succeeded(tool_name: str) -> bool: | |
| # ── Re-prompt on plan-without-action ── | ||
| # If the model described its intent (forward-looking | ||
| # language) without calling a tool, nudge it to act. | ||
| # Fires at most once per request, only on short | ||
| # Fires up to _MAX_REPROMPTS times, only on short | ||
| # responses with intent signals -- "4" or "Hello!" | ||
| # won't trigger it. Use content if available, else | ||
| # fall back to reasoning text (reasoning-only stalls). | ||
|
|
@@ -11889,18 +11901,29 @@ def _tool_succeeded(tool_name: str) -> bool: | |
| r"(?i)\brender[_\s-]?html\b", | ||
| _stripped, | ||
| ) | ||
| # A stall after a tool ran still deserves a nudge, but | ||
| # each retry re-runs tools, so allow only one. | ||
| _already_acted = any(record.executed for record in tool_controller.history) | ||
| if _already_acted: | ||
| _reprompt_used, _reprompt_cap = _post_tool_reprompts, 1 | ||
| else: | ||
| _reprompt_used, _reprompt_cap = _reprompt_count, _MAX_REPROMPTS | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When Useful? React with 👍 / 👎.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Confirmed and fixed in 8f384e9. The ordering is exactly as described: _already_acted = bool(_auto) or any(
record.executed for record in tool_controller.history
)One deliberate difference from safetensors: that loop blocks the nudge outright when |
||
| # None keeps the default-on re-prompt; False disables it. | ||
| if ( | ||
| auto_heal_tool_calls | ||
| and (nudge_tool_calls is None or nudge_tool_calls) | ||
| and active_tools | ||
| and not _render_html_already_done_intent | ||
| and _reprompt_count < _MAX_REPROMPTS | ||
| and _reprompt_used < _reprompt_cap | ||
| and not _is_reprompt_repeat(_stripped, _last_reprompt_text) | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When a request first hits a pre-tool plan-without-action, Useful? React with 👍 / 👎.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Confirmed, and fixed in 2253126. Reproduced on the head commit: with
|
||
| and _is_short_intent_without_action(_stripped) | ||
| ): | ||
| _reprompt_count += 1 | ||
| if _already_acted: | ||
| _post_tool_reprompts += 1 | ||
| _last_reprompt_text = _stripped | ||
| logger.info( | ||
| f"Re-prompt {_reprompt_count}/{_MAX_REPROMPTS}: " | ||
| f"Re-prompt {_reprompt_used + 1}/{_reprompt_cap}: " | ||
| f"model responded without calling tools " | ||
| f"({len(_stripped)} chars)" | ||
| ) | ||
|
|
@@ -12252,6 +12275,11 @@ def _invoke_tool(_output_callback, _decision = decision): | |
| _kb_search_count += 1 | ||
| completion = tool_controller.record_result(decision, result) | ||
| resolved_provisional_tool_call_ids.add(decision.tool_call_id) | ||
| # A real execution opens the post-tool phase. The pre-tool stall | ||
| # text must not carry over, or "I will search the web now." said | ||
| # before and after the search reads as a repeat and swallows the | ||
| # one post-tool nudge. | ||
| _last_reprompt_text = "" | ||
| # A tool ran this turn, so it counts against the caller's budget. | ||
| _turn_executed_real_tool = True | ||
| yield completion.tool_end_event() | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -169,12 +169,15 @@ | |
| # Forward-looking intent: the model says what it *will* do, not a final answer. | ||
| INTENT_SIGNAL = re.compile( | ||
| r"(?i)(" | ||
| # Direct intent ("I'll", "Let me"); lookahead drops negated forms | ||
| # ("I will not") so a refusal does not re-prompt. | ||
| r"\b(i['\u2019](ll|m going to|m gonna)|i am (going to|gonna)|i will|i shall|let me|allow me)\b(?!\s+(?:not|never)\b)" | ||
| # Direct intent ("I'll"); lookahead drops negated forms ("I will not"). | ||
| r"\b(i['\u2019](ll|m going to|m gonna)|i am (going to|gonna)|i will|i shall)\b(?!\s+(?:not|never)\b)" | ||
| r"|" | ||
| # Step/plan framing: "First ...", "Step 1:", "Here's my plan" | ||
| r"\b(?:first\b|step \d+:?|here['\u2019]?s (?:my |the |a )?(?:plan|approach))" | ||
| # "let me know" hands control back rather than announcing an action. | ||
| r"\b(?:let me|allow me)\b(?!\s+(?:not|never|know)\b)" | ||
| r"|" | ||
| # Step/plan framing: "First, I ...", "Step 1:", "Here's my plan". "first" | ||
| # needs first person so "First, the answer is 42" isn't read as a stall. | ||
| r"\b(?:first,?\s+(?:i|we|let['\u2019]?s|let us)\b|step \d+:?|here['\u2019]?s (?:my |the |a )?(?:plan|approach))" | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When a model emits an imperative plan such as Useful? React with 👍 / 👎.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Confirmed and fixed in f161f67. Took the action-cue route you suggested rather than loosening back to bare r"\b(?:first,?\s+(?:i|we|let['’]?s|let us)\b"
r"|first,?\s+(?:call|use|run|search|fetch|render|invoke|query|check|look|find|get)\b"
r"|step \d+:?|here['’]?s (?:my |the |a )?(?:plan|approach))"Now matches |
||
| r"|" | ||
| r"\b(?:now i|next i)\b" | ||
| r")" | ||
|
|
@@ -190,6 +193,29 @@ def is_short_intent_without_action(text: str) -> bool: | |
| return 0 < len(stripped) < REPROMPT_MAX_CHARS and INTENT_SIGNAL.search(stripped) is not None | ||
|
|
||
|
|
||
| _REPEAT_WORD_RE = re.compile(r"[^\w\s]+") | ||
| REPROMPT_REPEAT_SIMILARITY = 0.85 | ||
|
|
||
|
|
||
| def _normalize_for_repeat(text: str) -> str: | ||
| return " ".join(_REPEAT_WORD_RE.sub(" ", text.lower()).split()) | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When a retry changes a punctuation-sensitive query, Useful? React with 👍 / 👎.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Confirmed and fixed in 4824e33. Both plans really did normalize to
_REPEAT_EDGE_PUNCT = ".,;:!?\"'`()[]{}<>‘’“”"
...
stripped for word in text.lower().split() if (stripped := word.strip(_REPEAT_EDGE_PUNCT))
|
||
|
|
||
|
|
||
| # A nudge that just gets the same answer back has not worked, so stop there. | ||
| def is_reprompt_repeat(text: str, previous: str) -> bool: | ||
| if not previous: | ||
| return False | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When a safetensors/MLX model repeats the same pre-tool intent after a nudge, Useful? React with 👍 / 👎.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fair, and fixed in 21b96b2. The PR added the shared helper but wired only one consumer, so the same repeated stall cost three generations on safetensors/MLX and one on GGUF.
One existing test needed updating: |
||
| a, b = _normalize_for_repeat(text), _normalize_for_repeat(previous) | ||
| if not a or not b: | ||
| return False | ||
| if a == b: | ||
| return True | ||
| wa, wb = set(a.split()), set(b.split()) | ||
| if len(wa) < 4 or len(wb) < 4: | ||
| return False # too short for overlap to mean anything | ||
| return len(wa & wb) / len(wa | wb) >= REPROMPT_REPEAT_SIMILARITY | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When a forced retry keeps the prior planning sentence but appends a concise answer—for example, Useful? React with 👍 / 👎.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Confirmed and fixed in 8f384e9. Reproduced the exact example: for Fixed at the suppression site rather than by lowering the threshold. Near-repeat is the right test for "stop nudging, it is not working", but far too loose for "throw the turn away". def is_reprompt_restatement(text: str, previous: str) -> bool:
...
return set(a.split()) <= set(b.split()) # added nothing at all
This also covers the wider case in the same path: There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When a retry changes one important term in an otherwise long plan, this set-based Jaccard score can still exceed 0.85 and prematurely stop the tool nudge. For example, changing only Useful? React with 👍 / 👎.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Confirmed and fixed in f67c5b1. Your example scores 0.867 on the old bar, so a single corrected token in a 15-word plan did read as a repeat. Kept the fuzzy comparison rather than switching to sequence equality, because that is what REPROMPT_REPEAT_SIMILARITY = 0.95At 0.95 a 15-word plan needs zero differing tokens, so only filler drift and reordering survive. Verified: There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When a retry rearranges the same words in a way that changes the requested query, this set-based comparison still returns 1.0; for example, Useful? React with 👍 / 👎.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Right, and fixed in d6073ce. Set overlap cannot see order, so no threshold helps.
return difflib.SequenceMatcher(None, ta, tb).ratio() >= REPROMPT_REPEAT_SIMILARITY
One deliberate consequence: a pure reordering that means the same thing ("Now I will summarize the results" after "I will summarize the results now") is no longer a repeat and costs one more nudge. That is the safe direction to be wrong in. |
||
|
|
||
|
|
||
| def reprompt_to_act_message(tool_hint: str) -> str: | ||
| """The user message appended when re-prompting a plan-without-action turn.""" | ||
| return ( | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mustin forced retry suppressionOn a forced GGUF retry, a common stall such as
I must call web_search nowis not matched by the shared intent regex or this obligation pattern, so_should_suppress_forced_no_tool_outputreturns false and the hidden retry text is emitted as the final answer. The amended alternatives now covershould, but still omitmusteven when it is paired with an explicit action verb; include that obligation form so these retries remain suppressed.Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Confirmed and fixed in 8f384e9.
_should_suppress_forced_no_tool_output("I must call web_search now")returned False at the previous head, so that hidden retry shipped as the final answer.mustsits besideshouldrather than inside theneed|have|ought ... togroup, since the plain modals take a bare infinitive and folding them in would demand "I should to call":r"(?:\bi\s+(?:(?:need|have|ought)\s+to|should|must)|^need\s+to|^going\s+to)"Still False for
You must call your bank about the charge.andI must admit the square is red.Positives and negatives both added totest_forced_turn_suppression_covers_obligation_phrasing.