fix(opencode): end the turn when a permission is rejected with feedback#31861
Open
remorses wants to merge 2 commits into
Open
fix(opencode): end the turn when a permission is rejected with feedback#31861remorses wants to merge 2 commits into
remorses wants to merge 2 commits into
Conversation
A permission reject sent with a message produces PermissionV1.CorrectedError, but failToolCall only blocked the loop on RejectedError. So a reject carrying feedback ignored experimental.continue_loop_on_deny semantics entirely: the model got the feedback as a tool error, retried the same call, and re-asked the permission in a tight loop. Plugins that auto-reject via the v1 SDK hit this because they reply with an explanatory message. The fix adds CorrectedError to the blocked check so messaged rejects follow the same semantics as plain rejects: end the turn by default, continue when continue_loop_on_deny: true. The feedback stays on the errored tool part so the model reads it and works around the denial. Static ruleset denies (DeniedError) still do not block. Regression test: auto-reject with feedback, continue_loop_on_deny: false, 6 queued identical denied retries. Without the fix: 7 LLM calls. With the fix: 1 LLM call. Closes anomalyco#31108 Session: ses_14a04959cffeCy41V2N7hW0NWd
Contributor
|
The following comment was made by an LLM, it may be inaccurate: Potential Related PR Found:
This PR appears to address a related but distinct issue: it handles the "doom loop" caused by unhandled permission rejections, whereas PR #31861 specifically fixes the case where a permission is rejected with feedback/message (producing |
Session: ses_149fdc524ffewxqK8fnP6Fr039
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
1 participant
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.
Issue for this PR
Fixes #31108
Fixes #30726
Supersedes #31216 which was closed because it didn't follow the PR template.
Type of change
What does this PR do?
failToolCallinprocessor.tsonly checked forPermissionV1.RejectedErrorwhen deciding whether to block the loop. A rejection sent with a message (e.g. from a plugin's permission-timeout auto-reject) producesPermissionV1.CorrectedErrorinstead, which was not in the check. The model received the feedback as a normal tool error, retried the same call, got rejected again, and looped.The fix adds
CorrectedErrorto theinstanceofcheck alongsideRejectedError, so messaged rejects follow the samecontinue_loop_on_denysemantics as plain rejects.DeniedError(static ruleset denies) intentionally stays out since those are routine and the model should adapt.How did you verify your code works?
Regression test that auto-rejects a permission ask with feedback, with
continue_loop_on_deny: false. Without the fix the test gets 7 LLM calls (the model retries the denied call in a tight loop). With the fix it gets 1 LLM call (the turn ends immediately). Ran the fulltest/session/prompt.test.tssuite andbun typecheckinpackages/opencode.Screenshots / recordings
N/A (loop-behavior fix, covered by the regression test)
Checklist