implementing mcp invocation approvals

[blublinsky]

Description

Implementing MCP approvals for streaming query only

Type of change

• Refactor
• New feature
• Bug fix
• CVE fix
• Optimization
• Documentation Update
• Configuration Update
• Bump-up dependent library
• Bump-up library or tool used for development (does not change the final image)
• CI configuration change
• Konflux configuration change

Related Tickets & Documents

• Related Issue #
  OLS-2659
• Closes #
  OLS-2659

Checklist before requesting a review

• I have performed a self-review of my code.
• PR has passed all pre-merge test jobs.
• If it is a core feature, I have added thorough tests.

Testing

• Please provide detailed steps to perform tests related to this code change.
• How were the fix/results from this change verified? Please provide relevant screenshots or results.

[openshift-ci]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign blublinsky for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[onmete]

This seems like multiple PRs bundled together
PR 1 docs_summarizer.iterate_with_tools method refactoring
PR 2 Tool error retry improvement (is retriable), timeout and "do not retry" messages
PR 3 Batch-to-streaming rewrite of tool execution
PR 4 Approval feature
PR 5 Adding approval endpoints (changes not in this PR)

Regarding the approval design - is the approval gate in the right layer? Currently, it lives inside the tool execution engine (tools.py), where each worker independently checks policy, emits events, and blocks waiting for a decision. But the orchestrator (_process_tool_calls_for_round) already makes other pre-execution decisions at this level (token budget checks, duplicate tool filtering). Approval is the same kind of gate. Proposed flow:

_process_tool_calls_for_round():
  1. Resolve tool calls → ToolCallDefinitions
  2. For each definition, check need_validation()            <- POLICY
  3. For tools needing approval:
     a. yield StreamedChunk(type="approval_required", ...)     <- EMIT
     b. await get_approval_decision()                          <- WAIT
     c. If rejected, add synthetic ToolMessage, skip execution
  4. Pass only approved tools to execute_tool_calls_stream()  <- EXECUTE
  5. Process results

In other words, just collect all tools needing approval, emit them together, wait for all decisions (or timeout and exit).
The contract is that the user must approve or reject every tool before the round proceeds.

[blublinsky]

Regarding the approval design - is the approval gate in the right layer? Currently, it lives inside the tool execution engine (tools.py), where each worker independently checks policy, emits events, and blocks waiting for a decision. But the orchestrator (_process_tool_calls_for_round) already makes other pre-execution decisions at this level (token budget checks, duplicate tool filtering). Approval is the same kind of gate. Proposed flow:

_process_tool_calls_for_round():
  1. Resolve tool calls → ToolCallDefinitions
  2. For each definition, check need_validation()            <- POLICY
  3. For tools needing approval:
     a. yield StreamedChunk(type="approval_required", ...)     <- EMIT
     b. await get_approval_decision()                          <- WAIT
     c. If rejected, add synthetic ToolMessage, skip execution
  4. Pass only approved tools to execute_tool_calls_stream()  <- EXECUTE
  5. Process results

In other words, just collect all tools needing approval, emit them together, wait for all decisions (or timeout and exit). The contract is that the user must approve or reject every tool before the round proceeds.

Why. What is implemented is cleaner and faster.
We execute all tools in parallel and emit an approval request asap or process the tool. Because I am using AsyncGenerator, the whole code is very straightforward.
I do not see a reason for collecting approval requests; it is simpler and faster.

What you are suggesting will require an extra loop to collect approval requests. That will make the overall logic slower and more complex

[onmete]

Regarding the approval design - is the approval gate in the right layer? Currently, it lives inside the tool execution engine (tools.py), where each worker independently checks policy, emits events, and blocks waiting for a decision. But the orchestrator (_process_tool_calls_for_round) already makes other pre-execution decisions at this level (token budget checks, duplicate tool filtering). Approval is the same kind of gate. Proposed flow:

_process_tool_calls_for_round():
  1. Resolve tool calls → ToolCallDefinitions
  2. For each definition, check need_validation()            <- POLICY
  3. For tools needing approval:
     a. yield StreamedChunk(type="approval_required", ...)     <- EMIT
     b. await get_approval_decision()                          <- WAIT
     c. If rejected, add synthetic ToolMessage, skip execution
  4. Pass only approved tools to execute_tool_calls_stream()  <- EXECUTE
  5. Process results

In other words, just collect all tools needing approval, emit them together, wait for all decisions (or timeout and exit). The contract is that the user must approve or reject every tool before the round proceeds.

Why. What is implemented is cleaner and faster. We execute all tools in parallel and emit an approval request asap or process the tool. Because I am using AsyncGenerator, the whole code is very straightforward. I do not see a reason for collecting approval requests; it is simpler and faster.

What you are suggesting will require an extra loop to collect approval requests. That will make the overall logic slower and more complex

Faster = this executes tools without approval need right away. That is the only speed difference. The turn is blocked either way on user approvals. It can be done in docsummarizer layer too. It is also using polling and not asyncio.Event.wait(), which is debatable if it is really faster.

Cleaner = approval logic lives in the tool execution engine, which was stateless (tool in, result out). It now reads config.tools_approval and config.pending_approval_store, generates UUIDs, and polls for decisions. Also streaming_ols.py now has to inspect item.data.get("event") != "approval_required" to filter approval events from stored tool calls - that's approval semantics leaking into a layer that shouldn't need to know about it. Can we at least isolate the config dependency?

[blublinsky]

Faster = this executes tools without approval need right away. That is the only speed difference. The turn is blocked either way on user approvals. It can be done in docsummarizer layer too. It is also using polling and not asyncio.Event.wait(), which is debatable if it is really faster.

It also delivers approval requests faster and in parallel. Doing this in doc-summarizer seems quite wrong - approval is part of tools invocation, thats why it is done the way it is

[blublinsky]

Cleaner = approval logic lives in the tool execution engine, which was stateless (tool in, result out). It now reads config.tools_approval and config.pending_approval_store, generates UUIDs, and polls for decisions. Also streaming_ols.py now has to inspect item.data.get("event") != "approval_required" to filter approval events from stored tool calls - that's approval semantics leaking into a layer that shouldn't need to know about it. Can we at least isolate the config dependency?

This is a different issue. Fixed most of it by introducing an additional type. It's cleaner because approval is done where it belongs - in the tools invocation. Except for yield, the code looks similar to sequential code, which is quite simple. Tools execution is still stateless, its just longer.

[blublinsky]

/test "ci/prow/verify"

[blublinsky]

/test verify

[blublinsky]

/retest

[blublinsky]

/retest

[blublinsky]

/retest

[blublinsky]

/retest

[openshift-ci]

@blublinsky: all tests passed!

Full PR test history. Your PR dashboard.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.