fix: check require_approval field instead of mcp_server in ApprovalFilter isinstance check#5288
Open
jaideepr97 wants to merge 12 commits intollamastack:mainfrom
Open
fix: check require_approval field instead of mcp_server in ApprovalFilter isinstance check#5288jaideepr97 wants to merge 12 commits intollamastack:mainfrom
jaideepr97 wants to merge 12 commits intollamastack:mainfrom
Conversation
jaideepr97
requested review from
ashwinb,
bbrowning,
cdoern,
ehhuang,
franciscojavierarceo,
leseb,
mattf and
raghotham
as code owners
meta-cla
bot
added
the
CLA Signed
Contributor
|
This pull request has merge conflicts that must be resolved before it can be merged. @jaideepr97 please rebase it. https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/syncing-a-fork |
…lter isinstance check The _approval_required() method checked isinstance(mcp_server, ApprovalFilter) but mcp_server is an OpenAIResponseInputToolMCP, so the check was always False. This made the ApprovalFilter.always and .never lists dead code — every tool defaulted to requiring approval regardless of filter configuration. Fix by checking isinstance(mcp_server.require_approval, ApprovalFilter) and accessing .always/.never on the require_approval field. Closes llamastack#5287 Signed-off-by: Jaideep Rao <jrao@redhat.com> Made-with: Cursor
jaideepr97
force-pushed
the
fix/approval-filter-isinstance-bug
branch
from
a0aea1a to
d181118
Compare
Update test imports to use the new module paths after upstream renamed providers/inline/agents/ to providers/inline/responses/. Signed-off-by: Jaideep Rao <jrao@redhat.com> Made-with: Cursor
Signed-off-by: Jaideep Rao <jrao@redhat.com>
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
2 participants
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.
Summary
_approval_required()to checkisinstance(mcp_server.require_approval, ApprovalFilter)instead ofisinstance(mcp_server, ApprovalFilter).always/.neveronmcp_server.require_approvalinstead ofmcp_serverrequire_approvalmodes ("always","never", andApprovalFilterwith both lists)The previous code checked
isinstance(mcp_server, ApprovalFilter), butmcp_serveris anOpenAIResponseInputToolMCP, so the check was alwaysFalse. TheApprovalFilter.alwaysand.neverlists were dead code — every tool defaulted to requiring approval regardless of configuration.Closes #5287
Test plan
test_approval_filter_bug.py(7 tests covering all branches)tests/unit/providers/inline/agents/builtin/responses/suite passes (8/8)Made with Cursor