fix(agent): cap consecutive turns that call no tool and give no answer - #76
Open
xepozz wants to merge 2 commits into
Open
fix(agent): cap consecutive turns that call no tool and give no answer#76xepozz wants to merge 2 commits into
xepozz wants to merge 2 commits into
Conversation
…tor returns The previous version of this branch removed the node.output filter from outputs_from_yield_results, on the reasoning that output_ids come out of the child's own result envelope and are already pinned exactly. Measured against the suite, that is wrong twice over. complete() pins every routed data_id, not only the terminal one, and a yield reports one result envelope per run node -- so a mid-graph child pins the node.input row it fed to a sibling. Removing the filter folds those rows into the parent's observation. On sqlite the removal turned a green suite (993/993 at the parent commit) into 989/993: func child output collection, func control command chaining, cycle template chains and a signal burst all broke. The delegation case the removal was meant to fix is not reachable through this call site: delegation_handler's child is collected by collect_delegation_result, which reads the row by data_id with no type filter, and the flow compiler always assigns node.output to a nested child's terminal target. Adding agent.delegation to the filter would mean importing agent-node constants into dataflow core for a case no caller produces. So the production change is dropped and the invariant is pinned by tests instead: pinned output rows are returned, a pinned sibling input is not, and an envelope that pins nothing falls back to a node scan. The middle case fails without the filter. 996/996 on sqlite. The stub no longer leaks the data type between queries -- the earlier one did, so its own assertion read the result envelope back instead of the answer row and the test could not pass. Follow-up, not in this branch: check_completion has no cap on consecutive empty steps. 0.7.16 tells EMPTY_RESULT apart from NO_TOOLS_CALLED, but a model that returns nothing is still asked again until max_iterations -- three empty turns in one five-minute run measured on 0.7.13. That changes loop-termination semantics and belongs in its own change.
check_completion answers an unproductive turn with feedback and lets the loop ask again. Nothing counts those turns, so a model that keeps returning nothing is asked until max_iterations, one full round trip each time. Measured on a deployment running 0.7.13: three empty turns inside one five-minute run. The loop now carries the streak of consecutive unproductive turns and fails the node at DEFAULTS.MAX_UNPRODUCTIVE_STEPS (3) with its own lifecycle reason. Any productive turn resets the streak. check_completion already computed whether it wrote feedback; finalize_iteration was dropping that flag. It now reports a second, separate flag: tool_calling=none writes no observation at all, so a run whose every turn is unusable produced no feedback to count and would have escaped the cap. feedback_recorded still drives the durable yield; unproductive drives the cap. This narrows what a run may do: a model that ignores the nudge three times in a row and recovers on the fourth now fails instead. It does not change the outcome of a run that never recovers -- that run already failed at max_iterations, since completion needs either a usable result or a tool call. It fails sooner and says why. The streak lives in the loop, not in node metadata, so a recovered node restarts it. max_iterations still bounds the total; recovery only loosens the cap. Tests: five check_completion cases pin the flag across none/auto/any, a tool turn and a warm-up turn below min_iterations. At loop level, empty_until_limit with max_iterations = 12 now records 3 empty-result observations, not 12, and the node result carries result.error. Every one of the six fails without this change. 1002/1002 on sqlite.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What was changed
DEFAULTS.MAX_UNPRODUCTIVE_STEPS(3) with its own lifecycle reason. Any productive turn resets the streak.check_completionreports that condition as a flag of its own, separate from "I wrote a feedback observation".child_output_test.luapins which of a child's pinned data idsoutputs_from_yield_resultsreturns. No production change there — see below.Why?
The cap.
check_completionanswers an unproductive turn with feedback and lets the loop ask again. Nothing counts those turns, so a model that keeps returning nothing is asked untilmax_iterations, one full model round trip each time. Measured on a deployment running 0.7.13: three empty turns inside one five-minute run. 0.7.16 tellsEMPTY_RESULTapart fromNO_TOOLS_CALLED, which made the turns legible, but did not bound them.check_completionalready computed whether it wrote feedback;finalize_iterationdropped that flag on the floor. Reusing it alone is not enough:tool_calling = "none"writes no observation at all, so a run whose every turn is unusable produces nothing to count and escapes the cap entirely. Hence two flags —feedback_recordedstill drives the durable yield,unproductivedrives the cap.This narrows what a run may do: a model that ignores the nudge three times in a row and recovers on the fourth now fails instead. It does not change the outcome of a run that never recovers — that run already failed at
max_iterations, because completion needs either a usable result or a tool call. It fails sooner and says why.The streak lives in the loop, not in node metadata, so a recovered node restarts it.
max_iterationsstill bounds the total; recovery only loosens the cap.The child_output test, and the change that is not here. An earlier version of this branch removed the
node.outputfilter fromoutputs_from_yield_results, reasoning thatoutput_idscome out of the child's own result envelope and are therefore already pinned exactly. Measured against the suite, that is wrong twice over.complete()pins every routeddata_id, not only the terminal one, and a yield reports one result envelope per run node — so a mid-graph child pins thenode.inputrow it fed to a sibling. Removing the filter folds those rows into the parent's observation. On sqlite it turned a green suite (993/993 at the parent commit) into 989/993: func child output collection, func control-command chaining, cycle template chains and a signal burst all broke.The delegation case the removal was meant to fix is not reachable through that call site.
delegation_handler's child is collected bycollect_delegation_result, which reads the row bydata_idwith no type filter, and the flow compiler always assignsnode.outputto a nested child's terminal target (session_parent_idis set for both agent and func hosts). Addingagent.delegationto the filter would mean importing agent-node constants into dataflow core for a case no caller produces.So the production change is dropped and the invariant is pinned by tests instead.
Checklist
Full
wippy test --profile sqlite: 1002/1002.check_completion_test— five cases pin the new flag acrossnone/auto/any, a tool-calling turn, and a warm-up turn belowmin_iterations. All five fail without the change (expected true, got nil).agent_empty_result_test—empty_until_limitwithmax_iterations = 12records 3 empty-result observations, not 12, and the node result carriesresult.error. Without the cap the same test readsexpected 3, got 12. The existing limit case (max_iterations = 3) is unchanged and still green.child_output_test— pinned output rows are returned, a pinnednode.inputrow the child routed to a sibling is not, and an envelope that pins nothing falls back to a node scan. The middle case fails without the filter (expected 1, got 2).make lint: 172 entries, 0 errors, 2 pre-existing warnings.One flake seen and not caused here:
Signal Stress Tests > 10 wrong signals then 1 correctfailed on one run out of five and passed on the immediate re-run. It drives signal nodes only and touches nothing in this change.