-
Notifications
You must be signed in to change notification settings - Fork 315
feat(runtime): add auto-stop for max iterations in non-interactive mode #2208
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
tdabasinskas
wants to merge
1
commit into
docker:main
Choose a base branch
from
cogvel:fix/auto-stop-max-iterations
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔴 HIGH SEVERITY: Auto-stop logic incorrectly triggers in interactive sessions with tools approved
The auto-stop logic checks only
sess.ToolsApprovedto determine non-interactive mode, but this flag is set in two distinct scenarios:ToolsApprovedis set totruefor non-interactive MCP tool callsToolsApprovedis set totruewhen users select "ConfirmationApproveSession" to approve all toolsThe Problem:
In interactive TUI sessions where users have approved tools, this auto-stop will prevent them from manually resuming after max iterations. The stream exits with
return, making it impossible for users to continue the conversation interactively as they would expect in TUI mode.Recommendation:
The condition should distinguish between truly non-interactive contexts (MCP server, API mode) and interactive TUI sessions with tools approved. Consider:
sess.NonInteractiveflag, orsess.ToolsApproved && !r.isInteractiveMode(), orThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, sounds valid. This flag indeed can also be set mid-session when a user selects "approve all tools" in the TUI, not just in non-interactive modes.
However, I think the practical impact is limited:
In the CLI/TUI, the
MaxIterationsReachedEventis emitted before this early return. The outer runner receives it and would attempt rt.Resume(), but the loop has already exited. The result is the stream closes cleanly - the user just can't extend iterations. This is a behavior change but not a hang or crash.The current behavior without this patch is that MCP/A2A/eval sessions block forever on
r.resumeChansince there's no consumer to callrt.Resume(). I believe this is the actual bug.I guess to properly distinguish non-interactive contexts, we'd need something like a
sess.NonInteractiveruntime option set only in MCP server, A2A, and evaluation modes.@derekmisler, what are your thoughts on this? Should I update the code to introduce
sess.NonInteractiveand use it instead?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great analysis from both sides here. The bot's review is valid —
sess.ToolsApprovedisn't a reliable proxy for non-interactive mode since it gets set totruein two distinct paths:pkg/cli/runner.go:168when a user presses "A" (approve all)So yes, a dedicated
sess.NonInteractiveflag is the right call. It should be set only in the truly non-interactive session creation paths:pkg/mcp/server.go(MCP server)pkg/a2a/adapter.go(A2A adapter)pkg/evaluation/save.go(eval framework)I agree the practical impact today is limited — an interactive user who hit max iterations would just lose the ability to extend by 10 more iterations rather than crash — but it's worth fixing properly since that extend-and-continue flow is the designed UX for interactive sessions.
@tdabasinskas would you mind updating the PR with the
NonInteractiveapproach? Should be pretty contained — new field + option on Session, set it in those three places, and swap the check in loop.go. Thanks for the contribution!