Skip to content

Rejecting a single call after always_approve is ignored, so the tool still runs #4429

Closed

Description

@chiruu12

Sticky and per-call approval decisions are stored in the same two fields but in different shapes. always_approve and always_reject write a bool into _ApprovalRecord.approved / .rejected, while a per-call decision appends the call ID to a list in those same fields. Once a sticky decision has been made, a later per-call decision on the same tool is either ignored on read or dropped on write.

from agents import Agent
from agents.items import ToolApprovalItem
from agents.run_context import RunContextWrapper

agent = Agent(name="a")

def item(call_id):
    return ToolApprovalItem(
        agent=agent,
        raw_item={"type": "function_call", "name": "do_thing", "call_id": call_id, "arguments": "{}"},
        tool_name="do_thing",
    )

ctx = RunContextWrapper(context=None)
ctx.approve_tool(item("c1"), always_approve=True)
ctx.reject_tool(item("c2"), rejection_message="denied by user")
print(ctx.get_approval_status("do_thing", "c2"))
True

The record holds both decisions at once:

approved=True  rejected=['c2']  rejection_messages={'c2': 'denied by user'}

get_approval_status returns at if approval_entry.approved is True: return True before it reaches rejected_ids, so the rejection and its message are written to state that nothing reads. tool_execution.py treats True as approved and only consults the on_approval hook when the status is None, so nothing re-asks and the tool runs.

The other direction fails earlier. After always_reject, approved holds the bool False, so the writer's isinstance(target, list) guard is false and the call ID is never appended:

ctx = RunContextWrapper(context=None)
ctx.reject_tool(item("d1"), always_reject=True)
ctx.approve_tool(item("d2"))
print(ctx.get_approval_status("do_thing", "d2"))
False

Here the approval never reaches state at all.

This matters because rejecting one call from the interruption list after approving the tool for the session is a normal human in the loop flow, and the current behaviour runs the tool anyway.

I do not think this is the "approval takes precedence" rule the comment above that branch refers to. That rule is about approved and rejected conflicting at the same granularity, which is what tests/test_run_state.py covers. Elsewhere the semantics are latest decision wins, and tests/test_run_context_wrapper.py asserts that always_approve followed by always_reject gives False. No test covers a per-call decision after a sticky one in either direction.

A fix would check the per-call list before the sticky bool, and stop a sticky write from replacing the list form, while keeping those two existing tests green.

Verified on 1816d2e.

I would like to work on this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Type

    No type

    Projects

    No projects

      Milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions