Studio: Don't re-prompt finished answers in the tool loop - #7505
Conversation
for more information, see https://pre-commit.ci
|
@codex review |
1 similar comment
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2e51d8774e
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4223f030d1
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| 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.
When a request first hits a pre-tool plan-without-action, _last_reprompt_text is set. If the retry then calls a tool and the next model turn repeats that same short intent (for example, “I will search the web now.” before and after the search result), _already_acted selects the fresh post-tool budget but this guard rejects the nudge as a repeat. Because _forced_tool_call_pending was cleared when the tool executed, the repeated plan is streamed/returned as the final answer instead of using the one post-tool corrective retry. Fresh evidence: this revision separated _post_tool_reprompts, but this new repeat guard still shares _last_reprompt_text across the pre- and post-tool phases.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed, and fixed in 2253126.
Reproduced on the head commit: with I will search the web now. said both before and after the search, the loop stopped after 3 generations and surrendered the repeated stall as the visible answer instead of spending the post-tool nudge.
_last_reprompt_text is now cleared right after tool_controller.record_result, so a real execution opens a clean post-tool phase. Same script now runs 4 generations, logs Re-prompt 1/1, and ends on the final answer. Regression test: test_repeat_guard_resets_after_a_tool_runs.
| r"(?:\bi\s+(?:need|have|ought)\s+to|^need\s+to|^going\s+to)" | ||
| r"\s+(?:\w+\s+){0,2}?(?:call|use|run|search|fetch|render|invoke|query)\b", |
There was a problem hiding this comment.
On a forced retry, outputs such as I should call web_search now still describe an intended tool action but include no tool call. The shared intent regex deliberately ignores I should ... to avoid false positives like I should mention, and this replacement forced-turn regex only adds need/have/ought, so that retry text falls through _should_suppress_forced_no_tool_output and is emitted as the final answer instead of being hidden like the other action-verb obligation stalls. Fresh evidence: the amended regex now handles the earlier need to cases but still omits should when it is paired with an explicit tool/action verb.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed, and fixed in 2253126. _should_suppress_forced_no_tool_output("I should call web_search now") returned False on the head commit, so that retry stall was emitted as the answer.
should is now its own alternative in _FORCED_PLAN_INTENT rather than being folded into need|have|ought, which would have demanded "I should to call". I should mention that the square is red. and You should call your bank about the charge. still pass through. Both directions are in test_forced_turn_suppression_covers_obligation_phrasing.
…...' forced stalls
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 22531264a5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| 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.
When a forced retry keeps the prior planning sentence but appends a concise answer—for example, I will now summarize the search results for you: Tokyo. after the same sentence without Tokyo—the set-based Jaccard score exceeds 0.85, so this reports a repeat. In the GGUF loop that bypasses the retry branch, and because the response still matches the intent signal, _should_suppress_forced_no_tool_output drops the entire response; the user receives the original plan rather than the newly added fact. Repeat detection should not treat added answer content as equivalent merely because a long preamble is unchanged.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in 8f384e9. Reproduced the exact example: for previous = "I will now summarize the search results for you" and that sentence plus : Tokyo., the word sets are 9 and 10 with an intersection of 9, so Jaccard is 0.9 and is_reprompt_repeat returns True. _should_suppress_forced_no_tool_output then returned True as well and the whole turn was dropped, so the appended fact was lost.
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". tool_call_parser.py gains a stricter sibling:
def is_reprompt_restatement(text: str, previous: str) -> bool:
...
return set(a.split()) <= set(b.split()) # added nothing at all_should_suppress_forced_no_tool_output now takes the nudged text as previous. An obligation-plan match still suppresses unconditionally; a bare INTENT_SIGNAL match only counts as a stall when the retry added nothing. The default previous="" keeps the standalone contract for the existing callers and tests.
This also covers the wider case in the same path: INTENT_SIGNAL fires on lead-ins that introduce a real answer ("Now I have the results. The capital is Tokyo."), which the pre-PR _FORCED_REPEAT_PLAN_SIGNAL let through. Three tests cover it, two of them end to end through the loop.
| 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", |
There was a problem hiding this comment.
must in forced retry suppression
On a forced GGUF retry, a common stall such as I must call web_search now is not matched by the shared intent regex or this obligation pattern, so _should_suppress_forced_no_tool_output returns false and the hidden retry text is emitted as the final answer. The amended alternatives now cover should, but still omit must even 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.
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.
must sits beside should rather than inside the need|have|ought ... to group, 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. and I must admit the square is red. Positives and negatives both added to test_forced_turn_suppression_covers_obligation_phrasing.
| _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.
When rag_scope causes build_rag_autoinject to run, the knowledge-base retrieval executes before the controller is created and therefore never appears in tool_controller.history. This new _already_acted check consequently treats the following model response as pre-tool and grants the full three-reprompt budget rather than the intended single post-tool nudge, allowing document requests to repeat the expensive behavior this cap is meant to prevent. The safetensors loop already tracks the same external execution with rag_autoinjected; the GGUF phase check needs to incorporate _auto as well.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in 8f384e9. The ordering is exactly as described: _auto = build_rag_autoinject(...) runs well before ToolLoopController is constructed, so a RAG-scoped request that retrieved left tool_controller.history empty and _already_acted read False, granting the full three-nudge pre-tool budget.
_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 rag_autoinjected, whereas this grants the single post-tool nudge. That matches what this PR set the post-tool cap to, and it keeps the GGUF behaviour a strict tightening rather than a new hard block. Regression test test_rag_autoinject_counts_as_a_prior_tool_execution asserts one nudge instead of three, and it fails on the previous head.
|
Went through this end to end: static and LLM malicious scan clean, no overlapping open PR, and the four tool-loop suites ( The behaviour change is right. Re-prompting a finished answer costs a full generation and re-runs tools, and the old Codex verdicts, in case they are useful to anyone reading back through the thread:
I reproduced both of the live ones and pushed 2253126 to this branch:
Both have regression tests. One non-blocking note for a follow-up: Separately, this thread made it obvious that the nudge itself is invisible in Studio's chat UI. The retried turn has its output suppressed, so the composer shows nothing at all for a whole generation and it reads as a hang. I have a small PR going up for that which puts a "Nudging tool calls" badge with a spinner in the composer while the retry runs. |
…count RAG autoinject as a prior tool run
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8f384e9811
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| # 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.
When a safetensors/MLX model repeats the same pre-tool intent after a nudge, run_safetensors_tool_loop still checks only reprompt_count and never calls this helper, so it performs all three generations that this change is intended to stop. The helper is currently consumed only by the GGUF loop; wire the same previous-response tracking and repeat check into the safetensors path so behavior does not depend on the selected backend.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
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.
run_safetensors_tool_loop now tracks last_reprompt_text alongside reprompt_count and gates the nudge on not is_reprompt_repeat(intent_text, last_reprompt_text), matching the GGUF condition.
One existing test needed updating: TestPlanWithoutActionReprompt::test_reprompt_fires_up_to_the_cap fed the identical stall MAX_ACT_REPROMPTS times, so the new guard stopped it after the first nudge. It now uses distinct stalls, which is what it meant to exercise, and test_reprompt_stops_when_the_retry_restates_the_stall covers the repeat path with two generations instead of four.
| _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", |
There was a problem hiding this comment.
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 added invoke/query cases 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.
Confirmed and fixed in 21b96b2. _should_suppress_forced_no_tool_output("The API I should invoke is foo() because it supports streaming.") returned True at the previous head, so that answer was dropped. As you say, invoke is 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/must fix 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:
r"(?:(?:^|[.!?]\s+)\s*i\s+(?:(?:need|have|ought)\s+to|should|must)"
r"|^need\s+to|^going\s+to)"
r"\s+(?:\w+\s+){0,2}?(?:call|use|run|search|fetch|render|invoke|query)\b"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_search and Understood. I should call web_search now.; now False for the example above plus The tool I need to use is documented here., and still False for I should mention that the square is red. and You should call your bank about the charge.
…guard into the safetensors loop
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 21b96b2609
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| 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.
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 Now I think the feature is not supported in version 1. triggers a nudge, Now I think the feature is supported in version 1. is both classified as a repeat and considered a restatement because its word set is a subset; _should_suppress_forced_no_tool_output then drops it, leaving the user with the original incorrect answer. The comparison needs to retain ordering and deletions rather than reducing both responses to sets.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in f161f67. Reproduced it: with previous = "Now I think the feature is not supported in version 1." and the corrected "... is supported ...", the word set is a strict subset, so is_reprompt_restatement returned True and the correction was dropped.
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 Understood. I'll search for that now. after Understood, I'll search for that now. is still a restatement. Regression test test_restatement_keeps_deletions_that_change_the_answer.
| 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.
When a model emits an imperative plan such as First, search the web for the latest release notes., this new first-person requirement no longer matches it, even though the previous first branch did. Both GGUF and safetensors therefore treat that short plan-without-action as the final response and never ask the model to make the tool call. Distinguishing First, the answer is 42 with an action cue rather than requiring I, we, or let's would retain these common plan forms.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in f161f67. INTENT_SIGNAL did not match First, search the web for the latest release notes. after the narrowing, so that plan-without-action was surrendered as the answer on both backends.
Took the action-cue route you suggested rather than loosening back to bare first:
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 First, search ..., First, check ..., First, look up ... alongside the existing first-person forms, and still does not match First, the answer is 42, First, the result is 3. or The first line is blank. The new cases are in the shared GGUF/safetensors parity test so both loops are covered.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f161f6730e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| 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.
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 CUDA version 12.4 to CUDA version 12.5 in a 15-word search plan scores about 0.89, so both tool loops treat the revised plan as a repeat and return without giving the model another chance to make the corrected call. Compare normalized token sequences, or otherwise require changed query/value tokens to make the responses distinct.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
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 is_reprompt_restatement already is, and the two helpers answer different questions: "stop nudging, it is not working" tolerates rewording, "throw the turn away" does not. Raised the bar instead:
REPROMPT_REPEAT_SIMILARITY = 0.95At 0.95 a 15-word plan needs zero differing tokens, so only filler drift and reordering survive. Verified: CUDA version 12.4 to 12.5 is no longer a repeat, while identical text, case and punctuation variants, and pure reorderings still are. Test test_reprompt_repeat_keeps_a_changed_query_token.
| _FORCED_PLAN_INTENT = re.compile( | ||
| r"(?:(?:^|[.!?]\s+)\s*i\s+(?:(?:need|have|ought)\s+to|should|must)" | ||
| r"|^need\s+to|^going\s+to)" | ||
| r"\s+(?:\w+\s+){0,2}?(?:call|use|run|search|fetch|render|invoke|query)\b", |
There was a problem hiding this comment.
On a forced GGUF retry, subjectless plans are recognized only at a line start, unlike the I ... branch which also accepts a new sentence after punctuation. Consequently, a response such as Okay. Need to call web_search now. or Understood. Going to search now. misses this pattern and also misses INTENT_SIGNAL, so _should_suppress_forced_no_tool_output exposes it as the final answer instead of suppressing the still-unfulfilled plan. Apply the (?:^|[.!?]\s+) prefix to these alternatives as well.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in f67c5b1. Okay. Need to call web_search now. and Understood. Going to search now. both returned False, so they shipped as final answers.
The sentence prefix is now hoisted out of the alternation and applies to all three branches:
r"(?:^|[.!?]\s+)\s*"
r"(?:i\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"Both examples are suppressed now, the bare Need to call web_search and Going to search now still are, and the mid-sentence prose cases from the previous round stay visible. Added to test_forced_turn_suppression_covers_obligation_phrasing.
| r"\b(?:first,?\s+(?:i|we|let['\u2019]?s|let us)\b" | ||
| r"|first,?\s+(?:call|use|run|search|fetch|render|invoke|query|check|look|find|get)\b" | ||
| r"|step \d+:?|here['\u2019]?s (?:my |the |a )?(?:plan|approach))" |
There was a problem hiding this comment.
The narrowed first alternatives no longer recognize common plan framing such as The first step is to search the web or First, analyze the attached data, even though these responses announce an action without making the enabled tool call. The previous first matcher nudged these stalls; now both backends accept them as final answers. Preserve explicit first step phrasing and the other unambiguous action forms while continuing to exclude factual openings such as First, the answer is 42.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in f67c5b1. Neither The first step is to search the web nor First, analyze the attached data matched, so both were surrendered as final answers.
Rather than keep extending the verb list, the branch now inverts the test: first has to open a sentence, and what follows must not be a determiner, which is exactly the shape of the factual openings the narrowing was aimed at.
r"(?:^|[.!?]\s+)\s*(?:the\s+)?first\s+step\b"
r"|(?:^|[.!?]\s+)\s*first,?\s+"
r"(?!(?:the|a|an|this|that|it|there|my|your|our|his|her|their)\b)\w+"Matching now: The first step is to search the web, First, analyze the attached data, First, search ..., First, check ..., First, look up ..., plus the first-person forms. Still not matching: First, the answer is 42, First, the result is 3., First, it is 42, and The first line is blank. (the sentence anchor is what keeps that one out). Cases added to the shared GGUF/safetensors parity test.
…t-step plan forms
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fbe165e3ea
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| r"|(?:^|[.!?]\s+)\s*first\s*[,:–—-]?\s+" | ||
| r"(?!(?:the|a|an|this|that|it|there|my|your|our|his|her|their)\b)\w+" |
There was a problem hiding this comment.
First matching to actual plans
When tools and plan nudging are enabled, this alternative classifies any short sentence beginning with First plus a non-determiner as intent, including completed factual answers such as “First place went to Alice,” “First class is available,” and procedural answers like “First, install the package.” Both tool loops consequently treat these finished answers as stalls and start another generation, potentially repeating visible output or prompting unnecessary tool calls. Match an actual action/intent construction rather than allowing an arbitrary \w+ after First.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in 2e3277b. The determiner exclusion fired on all three of your examples, and also on "First, install the package.", which is advice to the user rather than work for this turn.
The bare \w+ alternative is now three explicit forms: a pronoun ("First, I ...", "First, let's ..."), an explicit plan ("First, my plan is ..."), or an investigative verb from a narrow list (search/check/look/find/fetch/get/call/use/run/query/invoke/analyse/review/inspect/read/gather/examine/retrieve/browse/consult/verify/confirm/compute/calculate/determine/identify/render). "install", "add" and "open" are deliberately absent, since those name a step for the user to take.
The imperative forms accepted in earlier rounds still match ("First, search the web for the latest release notes.", "First, check the documentation.", "First, analyze the attached data", plus the colon and dash delimiters); the three new negatives are pinned in test_intent_signal_shared_by_both_backends.
|
|
||
| # Leading marks are kept unless they are quotes or brackets, so ".NET" survives; | ||
| # stripping all non-word chars would collapse "C++" and "C#" to the same token. | ||
| _REPEAT_TRAIL_PUNCT = ".,;:!?\"'`()[]{}<>‘’“”" |
There was a problem hiding this comment.
When a forced GGUF retry corrects an intent-like response using a standalone comparison operator, this punctuation set removes the correction entirely. For example, “Now I think the value is 5” and “Now I think the value is < 5” both normalize to the same text because the standalone < token becomes empty; the retry is then considered an exact restatement and _should_suppress_forced_no_tool_output discards the corrected response. Preserve semantically meaningful operators such as < and > during normalization.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Reproduced and fixed in 2e3277b. _normalize_for_repeat("Now I think the value is < 5") returned "now i think the value is 5", identical to the unbounded text, so is_reprompt_restatement was True and the corrected answer was discarded.
The normaliser now keeps the original token whenever stripping would empty it, which covers the whole class ("<", ">", "->", "..."), not just the comparison operators. Pinned in test_reprompt_repeat_keeps_standalone_operator_tokens against both is_reprompt_repeat and is_reprompt_restatement.
| return False # too short for overlap to mean anything | ||
| # Order-sensitive: a set ratio scores "cats not dogs" and "dogs not cats" as | ||
| # identical, and no threshold can tell those apart. | ||
| return difflib.SequenceMatcher(None, ta, tb).ratio() >= REPROMPT_REPEAT_SIMILARITY |
There was a problem hiding this comment.
For intent responses longer than roughly 40 tokens, changing one critical query token still produces a SequenceMatcher ratio above 0.95; a 54-token plan changing only 12.4 to 12.5, for example, is classified as a repeat. Both loops then stop nudging even though the model corrected what it intended to search for, returning the unexecuted plan instead of giving it the remaining opportunity to call the tool. The repeat test should preserve any changed substantive token rather than relying on a length-dependent aggregate ratio.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Correct, and the threshold cannot be tuned out of this: a single replaced token scores (n-1)/n, so it clears 0.95 at n >= 20 and reaches 0.981 at n = 54. Measured in the worktree: n=20 -> 0.950, n=40 -> 0.975, n=54 -> 0.981, all classified as repeats.
Fixed in 2e3277b by dropping the ratio entirely. is_reprompt_repeat now compares the ordered sequence of content words, with a small filler set (a/an/the/now/then/just/so/ok/okay/please/also/again) removed first, so any changed content token means a new attempt regardless of length, while pure rewording ("I will now summarize the findings" vs "I will summarize the findings now") still counts as a repeat. Order sensitivity is preserved by construction, so "cats not dogs" vs "dogs not cats" stays distinct without needing a threshold. REPROMPT_REPEAT_SIMILARITY and the difflib import are gone; covered by test_reprompt_repeat_keeps_a_changed_token_in_a_long_plan and test_reprompt_repeat_ignores_filler_drift.
…ntent-based
Restrict the bare "First, <word>" intent alternative to a pronoun, an explicit
plan, or an investigative verb, so ordinal prose ("First place went to Alice")
and user-facing advice ("First, install the package") no longer count as a plan
without action.
Keep punctuation-only tokens in the repeat comparison, so "the value is 5" and
"the value is < 5" stay distinct, and compare content-word sequences instead of
a similarity ratio: any ratio is length-dependent, so one corrected token in a
54-token plan still scored 0.98 and cost the model its remaining nudge.
for more information, see https://pre-commit.ci
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dcf0de499b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if _FORCED_PLAN_INTENT.search(stripped) is not None: | ||
| return True |
There was a problem hiding this comment.
When the forced retry starts with obligation phrasing but then provides an answer (for example, I should call web_search, but the answer is Tokyo.), this unconditional match suppresses the entire turn. _FINAL_ANSWER_SIGNAL does not recognize common forms such as “the answer is,” and unlike the intent branch below, this branch never checks whether the retry merely restates previous, so users can receive no final answer despite the model having completed one. Apply the same restatement/progress check here before discarding the output.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed, and this is the worst failure direction the feature has, so thank you. Reproduced in the worktree: _should_suppress_forced_no_tool_output("I should call web_search, but the answer is Tokyo.", previous) returned True, and _FINAL_ANSWER_SIGNAL matched answer: but not "the answer is", so the turn reached the user as nothing at all.
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. _FINAL_ANSWER_SIGNAL also learned "the answer is" and "to summarise".
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 test_forced_turn_suppression_covers_obligation_phrasing, alongside the existing positives, which all still suppress. 434 tests pass.
The obligation-plan branch discarded the whole turn, so a retry such as "I should call web_search, but the answer is Tokyo." reached the user as nothing at all. Suppress the plan only when nothing follows it: a pivot after the match keeps the output, and _FINAL_ANSWER_SIGNAL now recognises "the answer is" and "to summarise" alongside "answer:". Leaking a plan sentence is cosmetic, dropping an answer is not, so the doubtful case now resolves towards shipping the turn.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: eb5961187f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| _REPEAT_FILLER = frozenset( | ||
| {"a", "an", "the", "now", "then", "just", "so", "ok", "okay", "please", "also", "again"} | ||
| ) |
There was a problem hiding this comment.
Do not remove filler words from every position in the response: when a retry corrects a search target from Who to The Who, both token sequences become identical here. is_reprompt_repeat then terminates the tool nudge despite the changed query, so both GGUF and safetensors loops can return another plan without making the intended tool call even though retry budget remains.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in 611aa05. is_reprompt_repeat("I will search for The Who discography", "I will search for Who discography") returned True, so the corrected target ended the nudge.
Articles are out of the filler set entirely: whether "the" changes the referent is not something this comparison can judge, and the failure direction of guessing wrong is losing the retry. The set is now discourse fillers only (now/then/just/so/ok/okay/please/also/again), which is what motivated it in the first place ("I will now summarize the findings" vs "I will summarize the findings now" still compares equal). Pinned in test_reprompt_repeat_keeps_articles_that_name_a_target.
| _FINAL_ANSWER_SIGNAL = re.compile( | ||
| r"\b(?:final\s+answer|answer\s*:|here\s+is|here's|in\s+summary|result\s*:)\b", | ||
| r"\b(?:final\s+answer|answer\s*:|(?:the\s+)?answer\s+is|here\s+is|here's" | ||
| r"|in\s+summary|to\s+summari[sz]e|result\s*:)\b", |
There was a problem hiding this comment.
Refine this signal so phrases such as the answer is not in the provided context are not classified as completed answers. On a forced GGUF retry like I should call web_search because the answer is not in the provided context, this match returns before _FORCED_PLAN_INTENT is considered, causing the unfinished plan to be surfaced as the final response rather than suppressed.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Correct, and this one is mine: I added "the answer is" to _FINAL_ANSWER_SIGNAL in eb59611 for the pivot case last round, and it swallowed the negated forms with it. Reproduced: _should_suppress_forced_no_tool_output("I should call web_search because the answer is not in the provided context", previous) returned False, so the plan shipped as the final response.
Fixed in 611aa05 with a negative lookahead on that alternative (not / unavailable / unknown / unclear / missing). The pivot cases from last round still ship, and two missing-answer phrasings are pinned as positives in test_forced_turn_suppression_covers_obligation_phrasing.
| r"(?:^|[.!?]\s+)\s*(?:the\s+)?first\s+step\b" | ||
| r"|(?:^|[.!?]\s+)\s*first\s*[,:–—-]?\s+(?:my|our)\s+(?:plan|approach|step)\b" | ||
| r"|(?:^|[.!?]\s+)\s*first\s*[,:–—-]?\s+(?:i|we|let['’]?s|let us)\b" | ||
| r"|(?:^|[.!?]\s+)\s*first\s*[,:–—-]?\s+" + _ACTION_VERB + r"\b" |
There was a problem hiding this comment.
Allow common Markdown and numbered-list prefixes before this sentence-opening pattern. Responses such as - First, search the web, * First: check the documentation, and 1) First, fetch the URL matched the previous first signal but no longer match any alternative here, so both inference loops treat these ordinary formatted plans as final answers instead of nudging the model to make the described tool call.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Closing this one. The premise does not hold: I checked the previous signal at d6073ce and - First, search the web, * First: check the documentation and 1) First, fetch the URL did not match it either. The sentence anchor has always been (?:^|[.!?]\s+)\s*, and a list marker is not whitespace, so these never reached the first branch. This is a pre-existing gap, not something the narrowing introduced.
On the gap itself: this PR narrows over-nudging, and adding new nudge triggers widens it in the opposite direction. Every widening of this regex so far has produced a false positive in the following round, and the cost of the miss is a leaked plan sentence rather than a lost answer. Worth doing separately with its own tests if it shows up in practice, not here.
| if not _is_short_intent_without_action(stripped): | ||
| return False | ||
| # INTENT_SIGNAL also fires on lead-ins to a real answer ("Now I have the results. | ||
| # The capital is Tokyo."), so a bare intent match is a stall only when the retry | ||
| # adds nothing. No ``previous`` keeps the standalone "is this a stall?" contract. | ||
| return not previous or _is_reprompt_restatement(stripped, previous) |
There was a problem hiding this comment.
Do not preserve every non-identical direct-intent retry as progress. For example, after nudging Let me summarize the results, a retry of I will review the results now is still only a plan, but it is not an obligation-style _FORCED_PLAN_INTENT and is not an exact restatement, so this branch surfaces it as the final GGUF response once the post-tool cap or pre-tool budget is exhausted. The previous suppression recognized I will plans, so this can newly leave users with another unfinished promise instead of an answer.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
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 INTENT_SIGNAL fires on lead-ins that introduce a real answer ("Now I have the search results. The capital of Japan is Tokyo."). Suppressing on a bare intent match dropped those answers, which is what test_forced_turn_intent_lead_in_needs_a_restatement_to_be_dropped pins. Your P1 last round made the same point from the other side, and I agreed with it: suppression means the user gets nothing at all.
"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.
… phrasing Articles are not filler: dropping them made "search for The Who" and "search for Who" compare equal, so a corrected target ended the nudge. _FINAL_ANSWER_SIGNAL matched "the answer is not in the provided context", which announces a missing answer, so the plan behind it shipped as the final response instead of being suppressed. Negated forms are now excluded.
for more information, see https://pre-commit.ci
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4ca5e9e3e1
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| _FINAL_ANSWER_SIGNAL = re.compile( | ||
| r"\b(?:final\s+answer|answer\s*:|here\s+is|here's|in\s+summary|result\s*:)\b", | ||
| r"\b(?:final\s+answer|answer\s*:|here\s+is|here's|in\s+summary" | ||
| r"|to\s+summari[sz]e|result\s*:" |
There was a problem hiding this comment.
On a forced GGUF retry, a plan such as I need to call web_search to summarize the results matches this new to\s+summari[sz]e alternative before _FORCED_PLAN_INTENT is checked in _should_suppress_forced_no_tool_output, so the hidden retry is treated as a final answer and emitted instead of being suppressed. Require a summary-label boundary, or otherwise avoid matching purpose clauses inside tool plans, so these no-tool retries do not leak as the final response.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Correct, and mine: I added to summari[sz]e last round and it matches the purpose clause of a plan. Reproduced: "I need to call web_search to summarize the results" was not suppressed.
Fixed in 9bf66ee by dropping that alternative outright rather than anchoring it. It was speculative when I added it, in summary and answer: already cover the real completions, and a summary label at the start of a turn is not a phrasing worth another edge case. Pinned as a positive in test_forced_turn_suppression_covers_obligation_phrasing.
| if plan is not None: | ||
| # Only the plan itself is safe to drop; anything the turn pivots to after it | ||
| # is the answer the user is waiting for. | ||
| return _ANSWER_PIVOT.search(stripped[plan.end() :]) is None |
There was a problem hiding this comment.
On a forced GGUF retry, obligation plans that end with a pivot word but do not answer, such as I should call web_search, though., hit _FORCED_PLAN_INTENT and then this pivot check returns false for suppression, so the hidden no-tool retry is emitted as the final response. Require substantive answer text after the pivot marker before preserving the turn, otherwise these unfinished retry plans leak instead of being dropped.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in 9bf66ee. "I should call web_search, though." matched the pivot and shipped as the final response.
The pivot now has to carry text of its own: the pattern requires at least two more word tokens after the marker. That only ever adds suppression where there is demonstrably nothing after the pivot, so it does not weaken last round's fix. Both new negatives ("I should call web_search, though.", "I need to run the search, but") are pinned alongside the pivot cases that must still ship.
| _FORCED_PLAN_INTENT = re.compile( | ||
| r"(?:^|[.!?]\s+)\s*" | ||
| r"(?:i\s+(?:(?:need|have|ought)\s+to|should|must)|need\s+to|going\s+to|must|should)" | ||
| r"\s+(?:\w+\s+){0,2}?(?:call|use|run|search|fetch|render)\b", |
There was a problem hiding this comment.
On a forced GGUF retry, obligation stalls such as I need to look up the release notes now or Need to check the docs no longer match this narrowed forced-plan pattern because the action verb must be one of call/use/run/search/fetch/render. Since the shared intent signal also leaves need to ... phrasing alone, _should_suppress_forced_no_tool_output returns false and exposes the hidden retry as the final answer; cover the same unambiguous work verbs used by the plan nudge here too.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Closing this one. "no longer match" is not accurate: call/use/run/search/fetch/render has been the verb list on this pattern since it was introduced, and look/check were never in it, so nothing regressed here.
More importantly, the direction is the one you asked me to reverse earlier in this review. invoke and query were removed from this list because they read as technical prose far more often than as a stall, and "I should invoke foo() because it supports streaming" was being discarded. look and check behave the same way: "I need to check the documentation, it says Tokyo" has no pivot marker to save it, so widening the list here drops answers, which your own P1 two rounds ago established is the failure mode that matters.
The asymmetry is deliberate. _ACTION_VERB in the shared intent signal can afford a broad list because a false positive there only spends a nudge. This pattern discards the turn, so it stays narrow.
| _REPEAT_LEAD_PUNCT = "\"'`([{‘“" | ||
| # Wording that can drift between two attempts without the attempt changing. | ||
| # Articles stay out: "The Who" and "Who" are different search targets. | ||
| _REPEAT_FILLER = frozenset({"now", "then", "just", "so", "ok", "okay", "please", "also", "again"}) |
There was a problem hiding this comment.
Because these filler words are dropped from every position, a corrected retry can compare equal after changing the actual query target, e.g. I will search for OK Go discography after I will search for Go discography loses ok and both token sequences match. Both tool loops then stop nudging and return the unexecuted plan even though the model changed what it intended to search for; only ignore these words when they are framing, not when they are part of the target phrase.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed, and this is the second finding of the same shape after "The Who", so I removed the mechanism instead of the word. Reproduced: "I will search for OK Go discography" compared equal to "I will search for Go discography".
is_reprompt_repeat is now exact after normalisation. Every relaxation tried in this review lost a real correction: a similarity ratio is length dependent (one changed token in a 50-word plan scored 0.98), a set ignores order ("cats not dogs"), and a filler set eats the target itself. No word is reliably filler, so there is no version of that list worth keeping. Reordered filler now reads as a new attempt, which costs one nudge out of MAX_ACT_REPROMPTS and never strands a plan. Pinned in test_reprompt_repeat_keeps_filler_words_that_name_a_target.
…sitive repeats
The purpose clause in "call web_search to summarize the results" matched the
final-answer signal, so the plan shipped instead of being suppressed; that
alternative is gone. A pivot word now has to carry text of its own, since
"I should call web_search, though." answers nothing.
Repeat detection no longer ignores filler words. No word is reliably filler:
dropping them to absorb rewording also absorbed the target ("OK Go" became
"Go"). A missed repeat costs one nudge out of the cap; a false one strands the
plan unexecuted.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9bf66ee796
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| def _normalize_for_repeat(text: str) -> str: | ||
| words = [] | ||
| for word in text.lower().split(): |
There was a problem hiding this comment.
When a retry changes only a case-sensitive target, such as correcting src/Foo.py to src/foo.py on Linux, lowercasing the entire response makes the two plans compare equal. The new repeat guard then stops further nudging; in the GGUF loop the corrected retry is also suppressed, so the requested action can be stranded without executing. Normalize case only for known intent boilerplate, or otherwise retain case in arguments and identifiers.
Useful? React with 👍 / 👎.
| r"(?:^|[.!?]\s+)\s*(?:the\s+)?first\s+step\b" | ||
| r"|(?:^|[.!?]\s+)\s*first\s*[,:–—-]?\s+(?:my|our)\s+(?:plan|approach|step)\b" | ||
| r"|(?:^|[.!?]\s+)\s*first\s*[,:–—-]?\s+(?:i|we|let['’]?s|let us)\b" | ||
| r"|(?:^|[.!?]\s+)\s*first\s*[,:–—-]?\s+" + _ACTION_VERB + r"\b" |
There was a problem hiding this comment.
When the model formats an imperative plan as a Markdown item such as - First, search the web, none of these narrowed arms match because they require first immediately after the line start or sentence punctuation. The previous generic \bfirst\b arm detected this common plan form, but both tool loops now return it as the final response instead of nudging the model to perform the search; allow list markers before the sentence-opening first.
Useful? React with 👍 / 👎.
| _FINAL_ANSWER_SIGNAL = re.compile( | ||
| r"\b(?:final\s+answer|answer\s*:|here\s+is|here's|in\s+summary|result\s*:)\b", | ||
| r"\b(?:final\s+answer|answer\s*:|here\s+is|here's|in\s+summary|result\s*:" | ||
| r"|(?:the\s+)?answer\s+is(?!\s+(?:not|unavailable|unknown|unclear|missing)\b))\b", |
There was a problem hiding this comment.
When a forced retry says, for example, The answer is still unknown, so I should call web_search, the negative lookahead does not see unknown immediately after is, so this new alternative classifies the whole turn as a final answer. _should_suppress_forced_no_tool_output consequently returns before recognizing the remaining tool plan and exposes an unfinished response; the same happens with phrases such as the answer is probably unavailable, so the missing-answer check needs to account for intervening modifiers.
Useful? React with 👍 / 👎.
# Conflicts: # studio/backend/tests/test_safetensors_tool_loop.py
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 20fe68915c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| r"\b(?:final\s+answer|answer\s*:|here\s+is|here's|in\s+summary|result\s*:" | ||
| r"|(?:the\s+)?answer\s+is(?!\s+(?:not|unavailable|unknown|unclear|missing)\b))\b", |
There was a problem hiding this comment.
the answer is
When a forced retry says something like I should call web_search to learn what the answer is., this alternative matches the embedded question even though no answer was provided. _should_suppress_forced_no_tool_output consequently returns before checking _FORCED_PLAN_INTENT and exposes the unfinished tool plan as the final response; require answer content after is or exclude embedded what the answer is clauses.
Useful? React with 👍 / 👎.
| _ANSWER_PIVOT = re.compile( | ||
| r"\b(?:but|however|although|though|that\s+said|in\s+the\s+meantime|meanwhile)\b" | ||
| r"[\W_]*(?:\w+[\W_]+){1,}\w", |
There was a problem hiding this comment.
On a forced retry such as I should call web_search, but first I need to formulate the query., this pattern accepts the words after but solely because there are at least two of them. _should_suppress_forced_no_tool_output then preserves the whole turn even though the pivot contains only another unfinished plan, leaking the hidden retry as the final response; the pivot guard needs to distinguish substantive answers from continued intent phrasing.
Useful? React with 👍 / 👎.
Offering to help hands control back exactly like the existing "let me know" exemption. On a corpus of real model turns, "I'll do my best to help" and "allow me to assist" close a clarification request and never precede a tool call, but they were read as intent and re-prompted. "help you" keeps its plan reading when an action verb follows it. The new test scores the classifier against 300 turns captured from three local GGUF models, each one a finished answer: the turn called no tool, and three regenerations behind the production nudge produced no tool call either. Over those turns, wasted nudges go from 36 (12.0%) on main to 5 (1.7%), and retries whose text would be discarded from 60 (20.2%) to 1 (0.3%). Until now these patterns were tuned on hand-written example sentences, which cannot show how often the classifier is right on real output.
|
Since these patterns have been tuned on hand-written example sentences for a dozen rounds, I wanted to know how often the classifier is actually right on real model output, so I measured it. MethodThree local GGUF models (Qwen3-0.6B, Qwen3-1.7B, Llama-3.2-1B-Instruct) driven through llama-server with the real Studio tool schemas, over 60 prompts spanning tool-requiring questions, questions needing no tool, list-formatted answers, ambiguous requests, non-English, and follow-ups after a tool had already run. 1,176 turns, of which 517 called no tool and so reached the nudge decision. Turns cut off by the token cap were dropped, since a truncation is not a stall. Each of those was then labelled by what nudging it actually does: append the assistant turn and Both trees were then scored on the same 517 turns, calling the same functions the loop calls. Results
Paired over the same turns: 52 nudged by One fix the numbers justified11 of the 14 residual false positives were the same shape: a clarification request closing with a politeness formula.
The 5 that remain are mostly the model answering "what is your plan for a question you are unsure about?", where plan language is the correct answer. I left those alone. Shipped as a test
Two things I could not establish, stated plainlyRecall is not measured here. I first tried to label genuine stalls the same behavioural way, but hand-checking the results showed the label was invalid: the nudge message is forceful enough to coerce these small models into a tool call even when the turn was already complete, so a full, correct definition of overfitting came back labelled "stall". What that measures is coercibility, not incompleteness. Reading a stratified sample of 84 turns myself, 82 were finished answers (including refusals, deflections and clarification requests), 2 were malformed output, and none was a genuine plan-without-action. So this corpus can show that the PR stops nudging finished answers, but it cannot tell you what fraction of real stalls the PR still catches. That would need a corpus where stalls are common, and with tool-calling models and tools enabled they are rare. Limits. One machine, three small quantised models, English-dominant prompts, and only the GGUF-shaped input path. The intervals above are the honest uncertainty on these 300 turns, not a claim about production. Worth flagging separately, found while tracing the call sites and not addressed here: |
for more information, see https://pre-commit.ci
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a9e34446e2
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| # request and never precede a tool call. "help you" keeps its plan reading when an | ||
| # action follows it ("I'll help you search the web"). | ||
| _HELP_OFFER = ( | ||
| r"(?:do(?:ing)?\s+my\s+best|try\s+my\s+best|be\s+(?:able|happy|glad)\s+to\b" |
There was a problem hiding this comment.
When the model says a polite plan such as I'll be happy to search the web for current GPU prices, this new help-offer exclusion fires immediately after I'll, so is_short_intent_without_action returns false even though an action verb follows. In both GGUF and safetensors tool loops that means the no-tool plan is accepted as the final answer instead of getting the nudge to call the enabled search tool; the exclusion should only cover standalone offers, or should allow these polite phrases when they are followed by a real action verb.
Useful? React with 👍 / 👎.
|
Follow-up on the recall gap I left open. The PRs that built this feature already defined the right test, so I ran theirs rather than inventing another. #4769 introduced the plan-without-action re-prompt and measured it as "Any Tool Called" on Think-Search-Code prompts, with the weather dashboard case going 1/3 to 3/3. #4783 raised the caps and listed the same style of acceptance prompts. That is the measurement my corpus could not provide, because ordinary Q&A prompts almost never stall. Method12 stall-inducing prompts from those two PRs (weather dashboard in HTML, current exchange rates, "plot the last 7 days of Bitcoin prices", "find the population of Tokyo and compute the density", and similar), 3 seeds each, on Qwen3-0.6B and Qwen3-1.7B. The loop is replayed with the nudge gate imported from each tree, so Results
Identical outcomes on every one of the 72 runs. The PR does not lose a single tool call, and it keeps the one rescue that The only behavioural difference is on two prompts where
Those are refusals, not plans. What this settles, and what it does notIt settles the question I flagged: the narrowing did not break the feature. On the prompts this feature was written for, recall is unchanged. It does not show the nudge is effective - only 1 of 7 stalls is rescued on Qwen3-0.6B, and that is true on Still available if wanted: #4700's end-to-end benchmark shape (a hard agentic query with objective ground truth, N runs per model x quant x KV config, before/after), which would measure whether any of this changes final task accuracy rather than tool-call rates. That needs live web_search and a much longer run, so I have not done it here. |
|
Ran the other two benchmarks from this feature's history: #4769's real-world prompt sets, and #4700's end-to-end shape. Sampled down for speed, same metrics. 1. Real-world prompts (#4769 used these)70 prompts sampled from
This is the one place the PR loses anything: three prompts on Qwen3-1.7B where
The nudge there bought an extra tool call on a question that was already answered. The third is arguable: the model asserted a Firefox version and offered to check, and On Qwen3-4B the two trees are identical on all 70, with 2. End to end, #4700's shape#4700's exact query and ground truth ("songs that charted #3 on the Billboard Hot 100 in 2015", 4 correct songs), 6 runs per tree, real
No difference, but this benchmark cannot discriminate here and I would not read anything into it: the nudge fires zero times on Qwen3-4B in either tree, because the model always calls a tool, so the classifier is never consulted. The 0/4 accuracy reflects model size, not this change - #4700 found 4B models managed 0.8/4 at best and needed 27B to reach 2.7/4, which is beyond what I can run here. Across all three benchmarksSame three trees, same models: 29 nudge generations on Combined with the corpus numbers from the earlier comment, the picture is consistent: this PR removes wasted work and near-eliminates the case where a user's answer is discarded, and the measurable cost is small and concentrated on turns that were already complete. |
|
Re-ran the benchmarks on current models, Real-world prompts, 70 prompts x 3 seeds = 210 runs per tree per model
Across 420 paired runs, exactly one outcome differs, on 9B, exact McNemar p = 1.000. It is a gdpval task with an attached image the model cannot see:
This supersedes the three-prompt difference I reported on Qwen3-1.7B. That was already p = 0.25, two of the three had already answered the question, and it does not reproduce on either current model. End to end, #4700's query and ground truth, 10 runs per tree
The 9B average looks like a small drop, but the runs are paired by seed and the pairing shows it is noise: Worth noting the 9B numbers land inside #4700's published range for its own 9B rows (0.0 to 1.0 avg songs), which is a decent sign the harness reproduces that benchmark rather than measuring something else. Where this leaves itOn current models there is no measurable accuracy cost. Same tool-call rate, same end-to-end accuracy within noise, and consistently fewer nudge generations spent (6 -> 3, 23 -> 19, 10 -> 8, 11 -> 7 across the four configurations). Combined with the corpus result - wasted nudges on finished answers 12.0% -> 1.7%, and discarded answers 20.2% -> 0.3% - I am satisfied this PR is a clear improvement with no regression I can find. |
2 participants
The plan-without-action guard treats ordinary final answers as stalls and re-prompts them up to 3 times, costing extra generations and repeated tool calls.
Three fixes:
INTENT_SIGNALmatched "please let me know" (a sign-off, the opposite of intent) and anything merely starting with "First,". The lookahead now excludesknow, andfirstrequires a first-person continuation, so "First, I will explore" still matches while "First, the answer is 42" does not.Once a tool has run, the nudge is capped at 1 instead of the full budget. The model still gets a chance to deliver the summary it promised, but cannot spiral into repeated searches.
If a re-prompted turn just restates the text that triggered it, the loop stops instead of spending the rest of the budget (new
is_reprompt_repeathelper).Measured on gemma-4-E2B-it-GGUF with web_search enabled, same prompt before and after: 4 searches with the answer repeated 4x in 9,820 ms, down to 1 nudge and 2 searches in 4,310 ms. A genuine stall with no tool run yet still gets the full 3 re-prompts.