feat(tools): FunctionTool require_confirmation — HITL approval (Part 7) - #594
feat(tools): FunctionTool require_confirmation — HITL approval (Part 7)#594kalenkevich wants to merge 3 commits into
Conversation
AmaadMartin
left a comment
There was a problem hiding this comment.
Verified against the head tree at c7aac5b. The gate itself is correct for an LlmAgent turn: it runs after schema validation so the predicate sees validated args, it fails closed, denial returns a clean {error: 'This tool call is rejected.'} without executing, and the default is opt-in (requireConfirmation ?? false) with no false convenience knob — which matches Python and is the right default for a generic function wrapper. Two things I would want resolved before this lands: requireConfirmation is not enforced end-to-end for a workflow ToolNode (the request is dropped and the node returns an error string as ordinary output — details inline), and the new plain-text approval fallback lets a single ok approve every pending confirmation in the session, on any surface, not just the CLI.
| /** | ||
| * Whether this tool requires user confirmation before it runs. A boolean, or | ||
| * a predicate over the (validated) call arguments and tool context returning | ||
| * a boolean. When confirmation is required the tool pauses the run (HITL): | ||
| * the framework emits an `adk_request_confirmation` interrupt, and the tool | ||
| * only executes once the user approves. Mirrors Python's | ||
| * `FunctionTool(require_confirmation=...)`. | ||
| */ | ||
| requireConfirmation?: | ||
| | boolean | ||
| | (( | ||
| input: ToolExecuteArgument<TParameters>, | ||
| tool_context?: Context, | ||
| ) => boolean | Promise<boolean>); |
There was a problem hiding this comment.
Not a nit. This doc comment is accurate for an LlmAgent turn but not for a workflow ToolNode, and someone reading it will believe a requireConfirmation tool is gated everywhere.
I checked the head tree (c7aac5b). The interrupt is only ever produced from core/src/agents/functions.ts, which builds the adk_request_confirmation call out of functionResponseEvent.actions.requestedToolConfirmations (functions.ts:131-168) after handleFunctionCallList injects toolConfirmation into the tool Context (functions.ts:324-333). core/src/workflow/nodes/tool_node.ts:40-51 does not go through that path at all:
const toolContext = new Context({
invocationContext: ctx.invocationContext,
functionCallId: randomUUID(),
});
const args = coerceToolArgs(input);
const response = await this.tool.runAsync({args, toolContext});
const stateDelta =
Object.keys(toolContext.actions.stateDelta).length > 0
? {...toolContext.actions.stateDelta}
: undefined;So for node(new FunctionTool({..., requireConfirmation: true})):
checkConfirmationfires,toolContext.requestConfirmation(...)writes intotoolContext.actions.requestedToolConfirmations, andrunAsyncreturns{error: 'This tool call requires confirmation, please approve or reject.'}.ToolNodecopies onlystateDeltaoff the actions —requestedToolConfirmationsis dropped — and emits nolongRunningToolIds, which is the signal the workflow HITL path uses to suspend (workflow/utils/hitl_utils.ts:60and:148).- The node therefore succeeds, and
{error: ...}becomes the node output that downstream nodes consume as ordinary data. - Nothing can ever approve it: the request never reaches the session as an interrupt,
functionCallIdis a freshrandomUUID()per invocation so no id could match across a resume, and the resume processor bails atrequest_confirmation_llm_request_processor.ts:42(if (!isLlmAgent(agent)) return;).
It does fail closed — the tool body does not execute — so this is not "runs unapproved". But a requireConfirmation tool is silently unusable inside a workflow, and the failure presents as a successful node returning an error string rather than a pause. Either route ToolNode through handleFunctionCallList (which also restores the plugin/agent before- and after-tool callbacks it currently skips), or have it translate toolContext.actions.requestedToolConfirmations into a longRunningToolIds interrupt event the way createRequestInputEvent does. At minimum, scope this doc comment to the LLM-agent path and say ToolNode is not covered yet.
| if (Object.keys(requestConfirmationFunctionResponses).length === 0) { | ||
| const fallback = mapPlainTextConfirmation(events); | ||
| Object.assign(requestConfirmationFunctionResponses, fallback.responses); | ||
| if (fallback.turnIndex >= 0) { | ||
| confirmationEventIndex = fallback.turnIndex; | ||
| } | ||
| } |
There was a problem hiding this comment.
Not a nit. This fallback is unconditional — it runs on every LlmAgent invocation on every surface, not only an interactive adk run.
if (Object.keys(requestConfirmationFunctionResponses).length === 0) {
const fallback = mapPlainTextConfirmation(events);
Object.assign(requestConfirmationFunctionResponses, fallback.responses);The structured path above it requires a FunctionResponse explicitly addressed to a specific adk_request_confirmation id, and that binding is the whole reason an approval can't be produced by accident. The fallback drops the binding: on a web/API deployment the user's next ordinary chat message is silently reinterpreted as a security decision on a pending tool gate (see the comment on line 275 for what it decides).
If this is for the CLI, make it opt-in — a flag the CLI runner sets — rather than the default behaviour for every embedder of RequestConfirmationLlmRequestProcessor.
| const confirmed = AFFIRMATIVE.has(text.trim().toLowerCase()); | ||
| const responses: Record<string, ToolConfirmation> = {}; | ||
| for (const id of pendingIds) { | ||
| responses[id] = new ToolConfirmation({confirmed}); | ||
| } |
There was a problem hiding this comment.
Not a nit. One word approves every pending confirmation in the session, and anything not in the word list is recorded as an explicit rejection.
const confirmed = AFFIRMATIVE.has(text.trim().toLowerCase());
const responses: Record<string, ToolConfirmation> = {};
for (const id of pendingIds) {
responses[id] = new ToolConfirmation({confirmed});
}Two concrete failures:
pendingIdsis every unansweredadk_request_confirmationin the whole event history (lines 239-249), including stale ones from turns the user abandoned. A singleokapproves all of them, and each is then really executed byhandleFunctionCallListat line 186. The user approved the one thing in front of them, not N things they no longer remember.- The scan takes the most recent user turn whatever it is (lines 257-270). A user replying
what does that tool do?, or answering an unrelatedadk_request_input, has their pending call silently rejected with nothing telling them so; a typo (ues) is a rejection too.
If the fallback stays, bind it to the single most recent pending id, require the plain-text turn to immediately follow the confirmation request, and treat unrecognized text as no decision (leave it pending) instead of a denial. Only the deny side is currently fail-safe; the approve side is not.
| | boolean | ||
| | (( | ||
| input: ToolExecuteArgument<TParameters>, | ||
| tool_context?: Context, |
There was a problem hiding this comment.
Nit. tool_context is snake_case in a public type signature — everywhere else in this file uses toolContext, including checkConfirmation's own parameter at line 214. This is the name TypeDoc and editor completion show to anyone writing a predicate.
| ((
input: ToolExecuteArgument<TParameters>,
tool_context?: Context,
) => boolean | Promise<boolean>); | ((
input: ToolExecuteArgument<TParameters>,
toolContext?: Context,
) => boolean | Promise<boolean>);The same union is repeated verbatim as the field type at lines 129-134. Naming it once fixes both and gives the predicate a documented type:
export type RequireConfirmation<TParameters extends ToolInputParameters> =
| boolean
| ((
input: ToolExecuteArgument<TParameters>,
toolContext?: Context,
) => boolean | Promise<boolean>);If you add it, export it from core/src/common.ts next to ToolOptions so the docs build resolves it.
| const confirmationResult = this.checkConfirmation( | ||
| validatedArgs as ToolExecuteArgument<TParameters>, | ||
| req.toolContext, | ||
| ); | ||
| const pending = await confirmationResult; |
There was a problem hiding this comment.
Nit. The promise is stored and awaited on the very next line for no reason.
const confirmationResult = this.checkConfirmation(
validatedArgs as ToolExecuteArgument<TParameters>,
req.toolContext,
);
const pending = await confirmationResult; const pending = await this.checkConfirmation(
validatedArgs as ToolExecuteArgument<TParameters>,
req.toolContext,
);The three-line comment above it also restates what checkConfirmation's own doc comment already says. Running the gate after parameters.parse is right, though — the predicate sees validated args, not raw model output.
| it('runs the tool once the call is confirmed', async () => { | ||
| const {tool, didRun} = makeTool(); | ||
| const ctx = makeContext({ | ||
| functionCallId: 'fc-1', | ||
| toolConfirmation: new ToolConfirmation({confirmed: true}), | ||
| }); | ||
|
|
||
| const result = await tool.runAsync({ | ||
| args: {path: '/tmp/x'}, | ||
| toolContext: ctx, | ||
| }); | ||
|
|
||
| expect(result).toBe('deleted'); | ||
| expect(didRun()).toBe(true); | ||
| }); |
There was a problem hiding this comment.
Not a nit. These tests hand-construct the post-approval state instead of exercising the round trip.
const ctx = makeContext({
functionCallId: 'fc-1',
toolConfirmation: new ToolConfirmation({confirmed: true}),
});Nothing in this file runs RequestConfirmationLlmRequestProcessor, so the 90 lines this PR adds to that file — the plain-text fallback — have no test at all. The PR body's "the request/resume round-trip is covered by the test" does not hold: the two ends of the gate are asserted independently and never connected. The test worth adding is one that drives a session event list through the processor and asserts the original tool is re-invoked with the right toolConfirmation, since that is the step where an id mismatch on resume would show up.
| }); | ||
| const invocationContext = new InvocationContext({ | ||
| invocationId: 'inv-1', | ||
| agent: {name: 'a', runAsync: async function* () {}} as never, |
There was a problem hiding this comment.
Nit. as never silences exactly the check the agent parameter's type exists to provide.
agent: {name: 'a', runAsync: async function* () {}} as never,Use a real agent instance (or the repo's test factory) so this fixture breaks when InvocationContext's contract changes — same objection as as any. If other test files already do this, a shared makeInvocationContext helper would fix them all at once rather than just this one.
c7aac5b to
e3e3da5
Compare
… approval) Part 7/9 of the feature/workflows split. - tools/function_tool: a `requireConfirmation` option so a FunctionTool pauses for human approval before executing. - agents/processors/request_confirmation_llm_request_processor: handles the confirmation request/resume round-trip for such tools. This tool-approval HITL is independent of the workflow engine (it works for any FunctionTool), so it is a small, self-contained slice. Tests: tools/function_tool_confirmation_test (5). Full core suite green (2481), docs:check + tsc clean.
Addresses the security/API review on FunctionTool require_confirmation: - The plain-text confirmation fallback no longer runs on every LlmAgent invocation. It is now opt-in via a new `RunConfig.plainTextToolConfirmation` flag (default off), which the interactive `adk run` CLI sets — so on a web/API surface an ordinary chat message is never silently reinterpreted as a tool-gate decision. The structured FunctionResponse path is unchanged. - Harden the fallback itself: resolve only the SINGLE most-recent pending confirmation (never a broadcast across every unanswered gate), require the reply to IMMEDIATELY follow the request (no intervening user turn), and treat unrecognized text as NO decision — the gate stays pending instead of being silently denied (only explicit negatives deny). - Extract a `RequireConfirmation<TParameters>` type with a `toolContext` (not snake_case `tool_context`) parameter, reuse it for both the option and the field, and export it from common.ts. - Correct the `requireConfirmation` doc: the HITL gate is enforced on the LlmAgent path; a workflow ToolNode does not yet route through it (it returns the "requires confirmation" error as node output rather than pausing). - Inline the redundant `await` in runAsync and drop the stale comment.
- Add end-to-end tests that drive a session event list back through RequestConfirmationLlmRequestProcessor with a real LlmAgent + real FunctionTool (no mocks) and assert the original tool is actually re-invoked with the right decision — the step where an id mismatch on resume would show up, and the first coverage of the plain-text fallback: opt-in gating, single-gate binding, unrecognized-text-stays-pending, and no cross-gate broadcast. - Replace the `agent: ... as never` fixture with a real LlmAgent instance so it breaks if InvocationContext's contract changes.
e3e3da5 to
34ed5af
Compare
Please ensure you have read the contribution guide before creating a pull request.
Link to Issue or Description of Change
Problem: A
FunctionToolmay perform sensitive actions that should pause for human approval before executing.Solution — Part 7 of 8. Stacked on Part 6.
Included:
tools/function_tool.ts— arequireConfirmationoption so aFunctionToolpauses for human approval before running.agents/processors/request_confirmation_llm_request_processor.ts— handles the confirmation request/resume round-trip for such tools.This tool-approval HITL is independent of the workflow engine (it works for any
FunctionTool), so it's a small, self-contained slice and could equally targetmaindirectly.Testing Plan
Tests:
tools/function_tool_confirmation_test(5). Full core suite 2481 green; docs:check + tsc clean.Manual E2E: N/A (unit-level; the request/resume round-trip is covered by the test).
Checklist
Additional context
Stacked split — merge in order (…Part 6 → Part 7 → Part 8). Diff: 3 files, +317.