-
-
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 6 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,8 @@ | |
| 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_reprompt_restatement as _is_reprompt_restatement, | ||
| is_short_intent_without_action as _is_short_intent_without_action, | ||
| reprompt_to_act_message as _reprompt_to_act_message, | ||
| ) | ||
|
|
@@ -339,8 +341,15 @@ 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 must | ||
| # call ..."), paired with an action verb: the bare topic words this replaced ate | ||
| # real answers. Two shapes, not one list: the semi-modals take a "to" | ||
| # ("I need to call"), the plain modals take a bare infinitive ("I must call") -- | ||
| # folding "should"/"must" 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|must)|^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( | ||
|
|
@@ -436,14 +445,27 @@ def _held_rehearsal_tail_len(text: str, active_tools: list[dict]) -> int: | |
| return len(tail) if tail and _is_rehearsal_prefix(tail, active_tools) else 0 | ||
|
|
||
|
|
||
| def _should_suppress_forced_no_tool_output(text: str) -> bool: | ||
| """Suppress only repeated forced-turn planning text, not final answers.""" | ||
| def _should_suppress_forced_no_tool_output(text: str, previous: str = "") -> bool: | ||
| """Suppress only repeated forced-turn planning text, not final answers. | ||
|
|
||
| ``previous`` is the stall text that triggered the nudge, so a retry that | ||
| moved on can be told from one that just said the same thing again. | ||
| """ | ||
| stripped = text.strip() | ||
| if not stripped or len(stripped) >= _REPROMPT_MAX_CHARS: | ||
| return False | ||
| if _FINAL_ANSWER_SIGNAL.search(stripped): | ||
| return False | ||
| return _FORCED_REPEAT_PLAN_SIGNAL.search(stripped) is not None | ||
| if _FORCED_PLAN_INTENT.search(stripped) is not None: | ||
| return True | ||
|
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 the forced retry starts with obligation phrasing but then provides an 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 this is the worst failure direction the feature has, so thank you. Reproduced in the worktree: Fixed in eb59611. I did not gate this branch on the restatement check, because an obligation plan is a stall on its own even when it differs from the nudged text, and that behaviour was added for an earlier finding ("I need to use render_html now" leaking on forced retries). Instead the branch now suppresses the plan only when nothing follows it: if a pivot ("but", "however", "although", "though", "that said", "meanwhile") appears after the match, the output ships. The trade is deliberate and asymmetric: "I should call web_search but first let me think" will now leak its plan sentence, which is cosmetic, whereas the case you found lost a completed answer. The doubtful case resolves towards shipping the turn. Four new negatives are pinned in |
||
| if not _is_short_intent_without_action(stripped): | ||
| return False | ||
| # INTENT_SIGNAL also fires on lead-ins that introduce a real answer ("Now I | ||
| # have the results. The capital is Tokyo."), which _FORCED_REPEAT_PLAN_SIGNAL | ||
| # used to let through. Dropping those loses the answer outright, so a bare | ||
| # intent match only counts as a stall once the retry adds nothing to what we | ||
| # nudged; no ``previous`` keeps the standalone "is this a stall?" contract. | ||
| return not previous or _is_reprompt_restatement(stripped, previous) | ||
|
Comment on lines
+498
to
+503
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.
Do not preserve every non-identical direct-intent retry as progress. For example, after nudging 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. Closing this one: it reverses a finding from an earlier round of this same review, and the direction it asks for is the more damaging one. The restatement gate on this branch exists because "I will review the results now" and "Now I have the results. Tokyo is the capital." are not separable by pattern, which is why the branch asks whether the retry moved past the nudged text instead. Obligation plans keep their unconditional path; direct-intent plans do not, deliberately. A leaked plan sentence is cosmetic; a dropped answer is not. |
||
|
|
||
|
|
||
| # ── Pre-compiled patterns for GGUF shard detection ─────────── | ||
|
|
@@ -11241,14 +11263,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 +11902,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 +11915,35 @@ 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. | ||
| # RAG autoinject retrieves before the controller exists, so it | ||
| # never lands in history; without _auto a doc-grounded turn | ||
| # would still be read as pre-tool and get the full budget. | ||
| # Mirrors the safetensors loop's rag_autoinjected. | ||
| _already_acted = bool(_auto) or 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 | ||
| # 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)" | ||
| ) | ||
|
|
@@ -11935,7 +11978,10 @@ def _tool_succeeded(tool_name: str) -> bool: | |
|
|
||
| if _forced_tool_call_pending: | ||
| _forced_tool_call_pending = False | ||
| if not _should_suppress_forced_no_tool_output(_stripped): | ||
| if not _should_suppress_forced_no_tool_output( | ||
| _stripped, | ||
| _last_reprompt_text, | ||
| ): | ||
| if cumulative_display: | ||
| forced_visible_text = _strip_tool_markup( | ||
| cumulative_display, | ||
|
|
@@ -12252,6 +12298,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,42 @@ 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. |
||
|
|
||
|
|
||
| # Stricter sibling of ``is_reprompt_repeat``: nothing at all was added. Near-repeat | ||
| # is the right test for "stop nudging, it isn't working", but not for "throw the | ||
| # turn away" -- a retry that restates the plan and appends the answer ("... for | ||
| # you: Tokyo.") clears the similarity bar, and dropping it loses the answer. | ||
| def is_reprompt_restatement(text: str, previous: str) -> bool: | ||
| if not previous: | ||
| return False | ||
| a, b = _normalize_for_repeat(text), _normalize_for_repeat(previous) | ||
| if not a or not b: | ||
| return False | ||
| return set(a.split()) <= set(b.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 forced GGUF retry removes a word that changes the answer, this set-containment check treats it as adding nothing and suppresses the corrected response. For example, after 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. Reproduced it: with The helper now compares the normalized token sequences directly rather than their sets, so both ordering and deletions survive: a, b = _normalize_for_repeat(text), _normalize_for_repeat(previous)
return bool(a) and a == bThat reduces it to "the retry said the same thing again, word for word", which is the only case where discarding the turn is safe. Case, punctuation and whitespace still normalize away, so |
||
|
|
||
|
|
||
| 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.
On a forced retry, this regex suppresses any matching response before comparing it with the stalled response, which can discard a completed answer such as “The API I should invoke is
foo()because it supports streaming.” The newly addedinvoke/querycases make this a regression from the previous suppression pattern: when the answer lacks one of_FINAL_ANSWER_SIGNAL's narrow labels, the loop returns without emitting it. Treat these phrases as stalls only when the retry actually restates the prior plan, or otherwise distinguish an announced action from explanatory prose.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 21b96b2.
_should_suppress_forced_no_tool_output("The API I should invoke isfoo()because it supports streaming.")returned True at the previous head, so that answer was dropped. As you say,invokeis new here, so it is a regression rather than a pre-existing gap.I did not take the restatement gate for this branch, because on a forced retry there is always a previous stall, and a model that answers one stall with a different stall ("I must call web_search now" after "I will search the web") would then leak, undoing the
should/mustfix from the last round.Anchored to sentence starts instead. An announced action is its own sentence; the same words mid-sentence are prose that happens to name a tool:
Checked both directions: still True for
I must call web_search now,I should call web_search now,I need to use render_html now,Need to call web_searchandUnderstood. I should call web_search now.; now False for the example above plusThe tool I need to use is documented here., and still False forI should mention that the square is red.andYou should call your bank about the charge.