From d44bef6696b3a4b1b16a6f871722feaf36d50db6 Mon Sep 17 00:00:00 2001
From: Danny Avila <danny@librechat.ai>
Date: Mon, 4 May 2026 10:31:40 -0400
Subject: [PATCH] =?UTF-8?q?=F0=9F=94=91=20fix:=20Carry=20tool=5Fcall=5Fid?=
 =?UTF-8?q?=20On=20ToolReviewConfig=20(HITL)?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

`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
---
 packages/api/src/agents/hitl/policy.spec.ts | 23 ++++++++++++++++++++-
 packages/api/src/agents/hitl/policy.ts      |  9 ++++++++
 packages/data-provider/src/types/agents.ts  | 11 +++++++++-
 3 files changed, 41 insertions(+), 2 deletions(-)

diff --git a/packages/api/src/agents/hitl/policy.spec.ts b/packages/api/src/agents/hitl/policy.spec.ts
index 593e2a02f1..35c80b95a2 100644
--- a/packages/api/src/agents/hitl/policy.spec.ts
+++ b/packages/api/src/agents/hitl/policy.spec.ts
@@ -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', () => {
diff --git a/packages/api/src/agents/hitl/policy.ts b/packages/api/src/agents/hitl/policy.ts
index e1ad02794a..0f9c5eebf8 100644
--- a/packages/api/src/agents/hitl/policy.ts
+++ b/packages/api/src/agents/hitl/policy.ts
@@ -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,
     })),
   };
diff --git a/packages/data-provider/src/types/agents.ts b/packages/data-provider/src/types/agents.ts
index 0fcdad033e..c6c0fe5731 100644
--- a/packages/data-provider/src/types/agents.ts
+++ b/packages/data-provider/src/types/agents.ts
@@ -312,9 +312,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[];
   }