🔑 fix: Carry tool_call_id On ToolReviewConfig (HITL)

`ToolReviewConfig` was joining with `ToolApprovalRequest` by position only.
That breaks the moment a single batch contains the same tool called twice
(e.g. a model fanning out parallel `mcp:server:search` calls): the UI can't
tell which review config applies to which action request once it filters
or reorders.

Mirrors the SDK's `ToolApprovalReviewConfig` shape — `tool_call_id` is the
join key, `action_name` is retained for display only.

Also: drop a JSDoc warning on `isHITLEnabled` so a future contributor doesn't
wire `humanInTheLoop: { enabled: true }` without supplying a host
checkpointer — the SDK's `MemorySaver` fallback is process-local and
silently breaks resume across worker hops.

- `Agents.ToolReviewConfig` adds `tool_call_id: string`
- `buildToolApprovalPayload` populates `tool_call_id` per review config
- New test covers the duplicate-tool batch case (two parallel calls to
  the same tool); 27 → 28 tests
This commit is contained in:
Danny Avila 2026-05-04 10:31:40 -04:00
parent 2360f51d80
commit 711325bfef
3 changed files with 41 additions and 2 deletions

View file

@ -116,6 +116,25 @@ describe('buildToolApprovalPayload', () => {
]);
expect(payload.review_configs.map((r) => r.action_name)).toEqual(['a', 'b']);
});
test('carries tool_call_id on each review_config (join key for duplicate-tool batches)', () => {
const payload = buildToolApprovalPayload([
{ name: 'mcp:server:search', arguments: { q: 'a' }, tool_call_id: 'call_1' },
{ name: 'mcp:server:search', arguments: { q: 'b' }, tool_call_id: 'call_2' },
]);
expect(payload.review_configs).toEqual([
{
action_name: 'mcp:server:search',
tool_call_id: 'call_1',
allowed_decisions: ['approve', 'reject', 'edit'],
},
{
action_name: 'mcp:server:search',
tool_call_id: 'call_2',
allowed_decisions: ['approve', 'reject', 'edit'],
},
]);
});
});
describe('buildAskUserQuestionPayload', () => {
@ -149,7 +168,9 @@ describe('buildPendingAction', () => {
const toolApprovalPayload: Agents.ToolApprovalInterruptPayload = {
type: 'tool_approval',
action_requests: [{ name: 'shell', arguments: { command: 'ls' }, tool_call_id: 'call_abc' }],
review_configs: [{ action_name: 'shell', allowed_decisions: ['approve', 'reject'] }],
review_configs: [
{ action_name: 'shell', tool_call_id: 'call_abc', allowed_decisions: ['approve', 'reject'] },
],
};
test('wraps a tool_approval payload with job context', () => {

View file

@ -18,6 +18,14 @@ const DEFAULT_REVIEW_DECISIONS: Agents.ToolApprovalDecisionType[] = ['approve',
* checkpointer fallback and skips installing the policy hook entirely.
* Users wanting "stop asking me" should use `mode: 'bypass'` instead, which
* keeps the machinery in place but auto-approves.
*
* **Wiring caveat (Slice B):** when this returns `true` and the host passes
* `humanInTheLoop: { enabled: true }` to `Run.create`, the host MUST also
* supply `compileOptions.checkpointer` with a durable saver
* (`LibreChatCheckpointSaver`). Otherwise the SDK installs a process-local
* `MemorySaver` fallback, which silently breaks resume across worker hops in
* any multi-process deployment. Pair this predicate with the checkpointer
* assignment at the `Run.create` call site.
*/
export function isHITLEnabled(policy: TToolApprovalPolicy | undefined): boolean {
return policy?.enabled !== false;
@ -83,6 +91,7 @@ export function buildToolApprovalPayload(
})),
review_configs: toolCalls.map((tc) => ({
action_name: tc.name,
tool_call_id: tc.tool_call_id,
allowed_decisions: decisionsByToolName?.[tc.name] ?? DEFAULT_REVIEW_DECISIONS,
})),
};

View file

@ -300,9 +300,18 @@ export namespace Agents {
description?: string;
}
/** Per-tool review configuration: which decisions the user is allowed to make. */
/**
* Per-call review configuration: which decisions the user is allowed to make.
*
* `tool_call_id` (NOT `action_name`) is the join key against
* {@link ToolApprovalRequest.tool_call_id}. By-position mapping breaks the
* moment a single batch contains the same tool called twice e.g. a model
* fanning out two `mcp:server:search` calls in parallel so always join
* by `tool_call_id`. `action_name` is retained for display only.
*/
export interface ToolReviewConfig {
action_name: string;
tool_call_id: string;
allowed_decisions: ToolApprovalDecisionType[];
}