Skip to content

fix(agents): carry request-input resume inputs in their own type - #822

Open
ScottMansfield wants to merge 2 commits into
mainfrom
fix/tool-resume-payload
Open

fix(agents): carry request-input resume inputs in their own type#822
ScottMansfield wants to merge 2 commits into
mainfrom
fix/tool-resume-payload

Conversation

@ScottMansfield

Copy link
Copy Markdown
Member

Fixes #773.

What

ToolConfirmation means one thing everywhere else in the tree: a human looked at an action and approved it. The request-input resume path used it as a transport instead, building an approval nobody gave so the resume inputs had something to ride in:

toolConfirmationDict[id] = new ToolConfirmation({confirmed: true, payload: resumeInputs});

That object lands on toolContext.toolConfirmation, the field the confirmation gate reads (function_tool.ts, security_plugin.ts) — so the one fabricated confirmation in the codebase was, at the point of use, indistinguishable from a real one.

This PR does the type split the issue asked for rather than the interim guard:

  • ToolResumePayload (new, core/src/tools/tool_resume_payload.ts) — carries inputs keyed by interrupt id, with no confirmed field to set.
  • handleFunctionCallList / handleFunctionCallsAsync take their own toolResumePayloadDict, alongside but separate from toolConfirmationDict.
  • Context.toolResumePayload — its own field, so the two never share a slot.
  • NodeTool reads resume inputs from that field. This also retires the typeof payload === 'object' narrowing that the untyped unknown payload required.
  • RequestInputLlmRequestProcessor builds resume payloads instead of confirmations.

toolConfirmation now has a single writer path again — a real approval that survived the binding checks from #771.

On exploitability

Nothing here is exploitable today, and this PR does not claim otherwise. I went looking, and the issue's assessment holds:

  • A NodeTool does not override checkRequireConfirmation, so FunctionTool's gate never engages for one.
  • SecurityPlugin gates on policy rather than on the tool, so it can arm a gate for a node tool — but its state machine cannot currently reach the combination that would matter. A node tool it blocks never runs, so it never pauses for input; and once a real approval releases it, the state recorded for that call is the ToolConfirmation object rather than 'CONFIRM', so the gate is not re-armed on the resume turn. (That last part depends on fix: preserve EventActions when a long-running tool returns no response #571, which landed two days ago and stopped the actions-only state delta from being dropped for a silent long-running tool.)

So the safety is a coincidence of two unrelated facts, neither enforced by a type nor asserted by a test — which is the case the issue makes for fixing it.

Tests

Two added to core/test/agents/processors/request_input_llm_request_processor_test.ts:

  1. The resumed call sees toolConfirmation === undefined while the resume inputs still arrive.
  2. An armed SecurityPlugin gate holds against a request-input resume — the node does not run.

Both fail against the previous code, the second by executing the node. The gate fixture in (2) is seeded rather than played out, for the reachability reason above; the test comment says so explicitly so a future reader doesn't mistake it for a reproduction.

Verification

On 3aaf49b: ts:check, eslint, prettier --check, docs:check, 3776 unit tests, 243 integration tests — all pass. Pre-fix revert confirmed to fail exactly the two new tests.

Note for reviewers

@Varun-S10 is assigned on #773 — happy to hand this over or close it if you have work in flight.


🤖 Generated with CloudCode (session ses_e5fc4ecd6b3ffeh8xOEvMAgBFq)

`ToolConfirmation` means one thing everywhere else in the tree: a human
looked at an action and approved it. The request-input resume path used it
as a transport instead, building `{confirmed: true, payload: resumeInputs}`
for every pending node-tool call so the answers had something to ride in.
That object is installed on `toolContext.toolConfirmation`, which is the
field the confirmation gate reads — so the one fabricated confirmation in
the codebase was, at the point of use, indistinguishable from one a person
granted.

Nothing is exploitable today, and this does not claim otherwise. A NodeTool
does not override `checkRequireConfirmation`, so FunctionTool's gate never
engages for one; SecurityPlugin gates on policy rather than on the tool, but
its state machine cannot currently reach the combination that would matter —
a node tool it blocks never runs, so it never pauses for input, and once a
real approval releases it the recorded state is the ToolConfirmation rather
than 'CONFIRM'. The safety is a coincidence of two unrelated facts, neither
enforced by a type nor asserted by a test, which is what #773 asked to fix.

Split the types. `ToolResumePayload` carries the inputs and has no
`confirmed` field to set, travels in its own `toolResumePayloadDict`
parameter on `handleFunctionCallList`, and lands on its own
`Context.toolResumePayload`. NodeTool reads that field, which also retires
the `typeof payload === 'object'` narrowing the untyped `unknown` payload
needed. `toolConfirmation` now has a single writer path again — a real
approval that survived the binding checks from #771 — so the gate cannot be
satisfied by anything else, and a node tool that starts gating fails closed
instead of silently becoming a bypass.

Tests: the resumed call is asserted to see no confirmation at all while the
inputs still arrive, and an armed SecurityPlugin gate is asserted to hold
against a resume. Both fail against the previous code, the second by running
the node. The gate fixture is seeded rather than played out, for the
reachability reason above, and says so.

Fixes #773

@AmaadMartin AmaadMartin left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved. The type split is sound: ToolResumePayload carries resume inputs in its own field, and NodeTool reads toolResumePayload?.inputs ?? {} (node_tool.ts:161), so the resumed value is unchanged while the fabricated ToolConfirmation is gone. I confirmed both new tests fail against the old code: test 2 runs the node when a confirmed:true object is fabricated, and holds when it is not (security_plugin.ts:134), and its seeded state key node-call-1 matches the pending call id. No any, suppression, or instanceof is added, and the new public type is exported so the docs check passes. CI was green at review time.

adk-python already carries this concept and never had the conflation
this branch fixes: `Context.__init__` takes `tool_confirmation` and
`resume_inputs` as separate parameters (`agents/context.py`), and
nothing in its `src/` fabricates a confirmed `ToolConfirmation` to move
data. So the split here is a parity fix, and it should land in the
shape upstream already uses rather than inventing a second one.

Renames the concept to match and drops the wrapper object:

  ToolResumePayload {inputs}  ->  ResumeInputs = Record<string, unknown>
  Context.toolResumePayload?  ->  Context.resumeInputs
  toolResumePayloadDict       ->  resumeInputsDict

`resumeInputs` now defaults to `{}` rather than being absent, matching
`self._resume_inputs = resume_inputs or {}`. The constructor option
stays optional, so nothing that builds a Context has to change, and
NodeTool loses its `?.inputs ?? {}` dance because the field can no
longer be missing.

The named type is kept — a TypeScript addition upstream has no need
for — but as an alias over the bare mapping, so it hosts the
"this is not an approval" contract without adding a level of nesting
that adk-python does not have.

@AmaadMartin AmaadMartin left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-reviewed at 5f05cf0 after the rename commit; my earlier approval predated it. The split is sound: ResumeInputs travels in its own resumeInputsDict and Context.resumeInputs, so a resume no longer builds a fabricated ToolConfirmation and an armed confirmation gate fails closed. I verified the adk-python parity (agents/context.py resume_inputs, separate from tool_confirmation, default {}), the common.ts export, the removal of the old file and helper, and that both new tests fail against the previous code. All CI checks pass at review time.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix(agents): request-input resume fabricates a ToolConfirmation to carry resume inputs

3 participants