fix(studio): gate external tool nudges and preserve retry context - #9125
fix(studio): gate external tool nudges and preserve retry context#9125Biotrioo wants to merge 12 commits into
Conversation
for more information, see https://pre-commit.ci
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
for more information, see https://pre-commit.ci
|
Confirmed the external loop in studio/backend/core/inference/studio_tool_loop.py re-prompts without consulting the nudge setting the frontend never sends, and that the stalled assistant turn is dropped unless a hosted tool ran. Will get this reviewed. |
mahiatlinux
left a comment
mahiatlinux
left a comment
There was a problem hiding this comment.
studio/backend/tests/test_hosted_result_replay.py:546, :575, :939 fail on this branch
nudge_enabled defaults to off (passthrough_healing.py:58, UNSLOTH_TOOL_CALL_NUDGE unset), so the new gate at studio_tool_loop.py:1005 short-circuits for every policy built without the flag. Three tests build theirs inline and never opt in:
tests/test_hosted_result_replay.py merge base: 27 passed
this branch: 3 failed, 24 passed
test_a_stalled_turn_keeps_its_hosted_result AssertionError: the stall reprompt never happened
test_a_stalled_continuation_stays_one_assistant_turn IndexError: list index out of range
test_a_stalled_turn_keeps_its_thought_signature IndexError: list index out of range
All three pass again with UNSLOTH_TOOL_CALL_NUDGE=1. .github/workflows/studio-backend-ci.yml:240 runs pytest tests/ on any studio/** change with no such variable set, so Backend CI reds on merge. The stated validation covered two files and could not have seen this.
studio/backend/core/inference/studio_tool_loop.py:1004 disables nudging for every ChatGPT subscription user
openai_codex_tool_loop.py:119 hardcodes auto_heal = False because Codex never writes tool calls as text. The new policy.auto_heal is not False condition turns that healer opt-out into a nudge kill switch, and CodexToolPolicy has no nudge_tool_calls field, so the user's Settings toggle cannot reach it either.
With the exact policy and transport shape that file builds:
merge base (auto_heal=False): provider requests = 2
this branch (auto_heal=False, nudge_tool_calls=True): provider requests = 1
The same gate covers the post-tool nudge (max_reprompts = _MAX_POST_TOOL_REPROMPTS, :1328), where the user loses the answer. web_search runs, the model stalls with "Now I will summarise the result.":
merge base: 3 turns, visible text "Now I will summarise the result.The capital of France is Paris."
this branch: 2 turns, visible text "Now I will summarise the result."
Text-form healing and the plan-without-action reprompt are unrelated mechanisms: the reprompt fires on prose with no markup to heal. Drop the condition, or add nudge_tool_calls to CodexToolPolicy and forward payload.nudge_tool_calls where the policy is built.
studio/backend/core/inference/studio_tool_loop.py:1005 gives the same request field a third meaning
nudge_tool_calls omitted, same /v1/chat/completions endpoint, three answers:
llama_cpp.py:21842(nudge_tool_calls is None or nudge_tool_calls), onsafetensors_agentic.py:1012and nudge_tool_calls, offstudio_tool_loop.py:1005nudge_enabled(...), whateverUNSLOTH_TOOL_CALL_NUDGEsays
Neither local agentic loop imports passthrough_healing, so that variable has never influenced a plan-without-action reprompt anywhere; this is the first place it does. Two consequences. unsloth run --disable-tool-call-nudging (unsloth_cli/commands/studio.py:2316) states "No effect on streaming requests or the server-side agentic loop", and unsloth start (unsloth_cli/commands/start.py:230) scopes it to "a non-streaming passthrough tool call"; both are now false. And unsloth studio, the callback the desktop app runs, never writes that variable, so the process default in the Studio backend is off and the external loop's stall recovery is gone for every caller that omits the field.
policy.nudge_tool_calls is not False, matching llama_cpp.py:21842, gets the Settings toggle honoured without inventing a new default.
studio/backend/core/inference/studio_tool_loop.py:1032 breaks role alternation on an assistant prefill
_continue_final_message (routes/inference.py:308) returns False whenever the payload flag is absent, including for a history that already ends with an assistant turn. The new append is unconditional, so the retry carries two assistant messages in a row:
messages = [user "hi", assistant "Sure, "], model stalls
merge base: ['user', 'assistant', 'user']
this branch: ['user', 'assistant', 'assistant', 'user']
_append_user_turn at :655 merges into a trailing user turn for exactly this reason, and its docstring gives it: "this conversation is rendered by the provider, and a strict server rejects that." The assistant side needs the same treatment.
studio/backend/core/inference/studio_tool_loop.py:1032 destroys the resumed partial's extra_content
append_assistant_turn replaces conversation[-1] wholesale (chat_template_helpers.py:2414), so every key on the trailing message other than content is dropped. With continue_final_message and a partial carrying a thought signature:
messages = [user "hi",
assistant "The answer is" + extra_content={"google": {"thought_signature": "SIG-PARTIAL"}}]
merge base: assistant "The answer is" extra_content preserved
this branch: assistant "The answer isI'll search for that now." extra_content None
The comment added at :1027 says a turn replayed without that field is rejected, and this path is what removes it. Before this change the prose stall appended nothing, so the partial survived. Carry the trailing message's other keys through the merge.
studio/backend/core/inference/studio_tool_loop.py:1023 replays tool markup to the provider
visible_answer = "".join(turn.text) goes into the conversation raw. The tool-call path at :1291 runs the same value through strip_tool_markup(..., final = True, enabled_tool_names = allowed_tool_names) under the comment "Markup never replays". A turn that finished at length re-releases promoted spans into turn.text (:961) and then falls into this branch, because calls is emptied for a truncated turn:
{"role": "assistant",
"content": "I'll search now. <tool_call>{\"name\": \"web_search\", \"arguments\": {\"query\": \"x\"}}</tool_call>"}The retry that exists to obtain a structured call now shows the model its own text-form markup as accepted assistant output. Strip the replayed copy the way :1291 does, keeping last_reprompt_text raw so the repeat check is unchanged.
studio/backend/tests/test_external_tool_nudge_wiring.py:20 asserts against the wrong function
src = inspect.getsource(routes_inference.openai_chat_completions)
assert src.count("nudge_tool_calls = payload.nudge_tool_calls") >= 2openai_chat_completions starts at routes/inference.py:13152. The line this PR adds is at :12797, inside _proxy_to_external_provider, a module-level async def at :12244. It is not in that source. The two matches are the pre-existing local forwards at :13989 and :15508, and the count is 2 at the merge base as well.
Deleting the route change entirely leaves all three tests in the file green. The comment above the assert describes the opposite of what it measures.
and policy.auto_heal is not False has no test at all: removing it changes no result anywhere in the backend suite.
Use inspect.getsource(routes_inference._proxy_to_external_provider), or drive the route and observe the policy.
studio/backend/tests/test_studio_tool_loop.py:732 depends on an ambient environment variable
_NUDGE_DEFAULT is read at import and nothing under studio/backend/tests/ pins it.
UNSLOTH_TOOL_CALL_NUDGE=1 pytest tests/test_studio_tool_loop.py
1 failed, 30 passed
test_a_stalled_model_is_not_nudged_by_default AssertionError: assert 2 == 1
The same gate also removed the classifier's coverage in this file. Mutating is_short_intent_without_action to return True:
merge base: 1 failed (test_a_finished_answer_is_not_nudged, assert 4 == 1)
this branch: 31 passed (mutant survives)
test_a_finished_answer_is_not_nudged at :768 calls _run(transport) with no flag, so the gate short-circuits before the classifier ever runs. Add nudge_tool_calls = True there, and pin _NUDGE_DEFAULT in the default test rather than reading the process environment.
studio/frontend/src/features/chat/chat-settings-sheet.tsx:1800 describes a different feature
When a tool call cannot be repaired, re-ask the model once so the intended tool still runs.
Neither half holds for the path this PR puts behind the toggle. The trigger is prose with no tool call to repair, and the cap is MAX_ACT_REPROMPTS = 3 (tool_call_parser.py:209), not once. studio/backend/models/inference.py:1560 still advertises the field as "Opt-in, non-streaming client-tool passthrough only", which is what generated OpenAPI documentation hands external callers.
studio/backend/tests/test_studio_tool_loop.py:734 attributes the wrong fix to #8907
"""Issue #8907: ordinary prose must not trigger a hidden retry by default."""The test then asserts that "I'll search for that now." must not be nudged. #8946 is merged (657aea945) and deliberately keeps that string nudgeable; what it fixed was the clarification turn "Let me know what you're after and I'll dig in.". #8907's own log line, model responded without calling tools, is emitted only by llama_cpp.py:21855, the local GGUF loop, which this PR does not touch.
657aea945 is not an ancestor of this branch. The gate has never run against a tree containing the classifier fix it is layered on top of.
studio/backend/core/inference/studio_tool_loop.py:1019 is dead
is_short_intent_without_action requires 0 < len(text.strip()) (tool_call_parser.py:216), so visible_answer is always non-empty when line 1019 is reached and stalled_content is always truthy. The if stalled_hosted: it replaced was meaningful; this guard and its comment never fire.
The summary describes the previous behaviour incorrectly
The retry therefore saw
user -> user
It did not. _append_user_turn merges into a trailing user turn, so at the merge base the model's stall text was dropped and the nudge was concatenated onto the user's own prompt:
[{"role": "user",
"content": "hi\n\nYou have access to enabled tools. If a tool is needed ..."}]Two stalls compound it, rewriting message one twice and invalidating the prefix cache. The :1014 hunk is the right fix for that, and it is worth describing accurately.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 80d01cb0cf
ℹ️ 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".
| merged_msg = {**conversation[-1], **assistant_msg} | ||
| merged_msg["content"] = f"{prev_text}{assistant_msg['content']}" | ||
| conversation[-1] = merged_msg |
There was a problem hiding this comment.
When continue_final_message is enabled, this assigns a new dictionary to conversation[-1] while callers retain and subsequently mutate assistant_msg. Both the GGUF loop (llama_cpp.py) and safetensors loop append additional tool calls to that original object after the first call, so a continued turn containing multiple tool calls records only the first call in the conversation while appending results for all of them; the next provider request then contains orphan tool results and can be rejected. Merge the prior metadata into assistant_msg itself, or otherwise ensure later mutations update the stored conversation entry.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Not reproducible on 80d01cb: the merged dict and assistant_msg share the same tool_calls list, and both GGUF and safetensors append to that list in place. A two-call probe leaves both calls in conversation[-1], so no orphan results are created.
|
Codex Review: Didn't find any major issues. What shall we delve into next? Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
# Conflicts: # studio/backend/models/inference.py # studio/backend/tests/test_external_confirm_gate_and_saved_keys.py # studio/backend/tests/test_hosted_result_replay.py # studio/frontend/src/features/chat/api/chat-adapter.ts
append_assistant_turn dropping a resumed turn's extra_content is independent of tool-call nudging: six of its eight call sites are tool-result replay, and the merge path is reached by the continue-a-partial-response feature. It is now unslothai#9684, so it is dropped here rather than reviewed twice.
The nudge decided on the raw stream but replayed a markup-stripped copy, so the two could disagree. A turn whose whole visible content is an unpromotable call block still reads as a stall to the classifier, since an intent phrase inside the arguments is enough, while stripping leaves nothing to replay. The assistant turn was then skipped and the nudge merged into the user's own message, which is the failure this branch exists to prevent, on the one transport class where it is easiest to reach: heals_text_tool_calls = False, i.e. Codex. Strip first, then classify and replay the same text. A stall with nothing left to continue from is no longer a stall. This also stops unparsed markup inflating the length past REPROMPT_MAX_CHARS and defeating the repeat guard through markup churn alone, and it matches the local loops, which classify prepared text via _reprompt_intent_text.
|
Pushed to this branch to unblock it. Summary of what changed and why, so nothing here is a surprise. Why it was conflicting#9126 landed first and included byte-identical copies of several hunks from this PR: the Your unique assertion on the Codex policy is kept on top of main's version: assert entered["policy"].nudge_tool_calls is False
assert entered["policy"].auto_heal is FalseSplit out to its own PRThe One more fix on topThe strip introduced a mismatch: the classifier read the raw stream while the replay used the stripped copy. A turn whose whole visible content is an unpromotable call block classifies as a stall (an intent phrase inside the arguments is enough) but strips to Fixed by stripping first and then classifying and replaying the same text, which is also what the local loops do via Measurement worth having in the recordI A/B'd the two conversation shapes on a real model (Qwen3-0.6B via llama-server, 108 generations per arm, same prompts and seeds, replaying turns where the nudge had produced a tool call): The broken shape extracts more tool calls, not fewer. It works by hiding the model's own previous answer from it, so the model does not dig in on what it already said. That is the same amnesia that makes it re-answer from scratch and stack up to four answers into one message, so the compliance and the bug are one mechanism and cannot be separated. Caveat on that number: the replayed turns are 75% confident hallucinated answers, so what it measures is anchoring. For a genuine plan-shaped stall, replaying the assistant turn is the whole point, since the nudge says "complete the action you described". I could not test that case, having found no plan-shaped stall in 924 captured turns. So this PR should be read as a correctness fix (the conversation sent to the provider stops being false, the user's own message stops being rewritten, the prefix cache survives) and not as a recovery improvement. I have not changed the PR description; worth updating it on that point. VerificationGuards run against unfixed Thanks for finding this one. The |
for more information, see https://pre-commit.ci
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
The name was bound twice in one scope, so the later copy won silently and the first tested nothing. The surviving copy pins _NUDGE_DEFAULT rather than reading the ambient one, which matters because both CLI launchers export UNSLOTH_TOOL_CALL_NUDGE=1: under that env the deleted copy failed.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a916f6a6ab
ℹ️ 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".
| replayable_answer = leading_whitespace + strip_tool_markup( | ||
| visible_answer, | ||
| final = True, | ||
| enabled_tool_names = allowed_tool_names, |
There was a problem hiding this comment.
For external models that emit a reasoning-only stall using Magistral-style [THINK]...[/THINK] markup, strip_tool_markup(..., final=True) removes the entire block, so replayable_answer is empty and is_short_intent_without_action never triggers the enabled nudge. This differs from the referenced _reprompt_intent_text, which deliberately falls back to reasoning when there is no visible answer, and causes these models to stop after planning to use a tool instead of receiving the retry; classify with the reasoning-aware fallback while keeping the separately stripped text for replay.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed. My first probe made this look like a non-issue, because the string I tried classified False before stripping as well. Shorter phrasings do diverge:
raw stripped text
True False '[THINK]I will search now.[/THINK]'
True False '[THINK]Let me look that up.[/THINK]'
True True '<think>I will search now.</think>'
So the nudge really was being dropped for reasoning-only stalls that main still nudges.
I matched _reprompt_intent_text rather than only classifying differently: it classifies and replays the same string, and for a reasoning-only stall that string is the think block itself. Classifying the reasoning while replaying the stripped text would have put back the empty-replay case this PR exists to fix.
Ordering carries the correctness here. The fallback runs after strip_tool_markup, so a turn that is only an unpromotable call block has no think span, stays empty, and is still not nudged:
reasoning-only stall nudge=True replay='[THINK]I will search now.[/THINK]'
reasoning + answer nudge=True replay='I will search now.'
markup-only nudge=False replay=''
real answer nudge=False
Both cases are now pinned by tests. The reasoning-only test fails without the change and passes with it (1 failed, 2 passed then 3 passed); the suite is 54 passed.
Fixed in 0abe73b.
A reasoning-only stall keeps its whole promise inside the think block, so stripping tool markup empties it and the nudge never fired, while the local loops still nudge via _reprompt_intent_text. The fallback runs after stripping, so a turn that is only an unpromotable call block still has no think span and is still not nudged, and the replayed turn is the text that was classified.
|
@codex review |
|
Codex Review: Didn't find any major issues. 🎉 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
4 participants
Summary
Complementary follow-up to #8946 / #8907.
#8946 fixes the false-positive classifier that nudges valid clarification turns. While tracing the later CPU-only reproducer in #8907, I found two separate bugs in the external Studio tool loop:
nudgeToolCallssetting was not forwarded on the external local-tool path, so that loop could re-prompt independently of the request setting;conversationunless a provider-hosted tool had run. The retry therefore sawuser -> userinstead ofuser -> assistant -> userand could answer the original request from scratch.This PR intentionally does not change the current Studio default (
nudgeToolCalls: true) or the plan/clarification classifier; #8946 remains the classifier fix for #8907.Changes
nudge_tool_callsfrom the frontend external local-tool request through the route intoToolLoopPolicy;Validation
python -m compileall ...git diff --checkpython -m pytest studio/backend/tests/test_studio_tool_loop.py studio/backend/tests/test_external_tool_nudge_wiring.py -q --tb=short34 passedcd studio/frontend && npm ci && npx tsc -b --pretty falseRefs #8907.
Complementary to #8946.