From 39173ae37f0989363ea9f655c605c0881f7d3202 Mon Sep 17 00:00:00 2001
From: David McKay <davidmckayv@users.noreply.github.com>
Date: Thu, 20 Aug 2026 15:54:06 -0700
Subject: [PATCH] Stop the tool-call repair recursing into itself

`repairUnansweredToolCalls` took an id source named `newId` defaulting to
`newId()`. Inside the default, `newId` resolves to the parameter, not the
import, so the default calls itself until the stack goes.

It hangs exactly when the function does its job: the parameter is only invoked
on the repair branch, and the one real caller omits the argument. Every test
passed its own ids, which is why no test ran the default and why this survived.

Renamed so it cannot shadow the import, and there is now a test that calls it
the way the real caller does, with the argument left out.

Found by Guido Vizoso. Biome had said so too, as an unused import on line 2,
which I had waved through as a warning.
---
 app/src/lib/copilot/repair-history.ts |  9 +++++--
 app/tests/repair-history.test.ts      | 36 +++++++++++++++++++++++++++
 2 files changed, 43 insertions(+), 2 deletions(-)

diff --git a/app/src/lib/copilot/repair-history.ts b/app/src/lib/copilot/repair-history.ts
index 04ac615..ad8f3e6 100644
--- a/app/src/lib/copilot/repair-history.ts
+++ b/app/src/lib/copilot/repair-history.ts
@@ -22,7 +22,12 @@ function isToolResult(message: Message): message is Message & ToolResult {
  */
 export function repairUnansweredToolCalls(
   messages: ReadonlyArray<Message>,
-  newId: () => string = () => newId(),
+  /*
+   * Named so it cannot shadow the import it defaults to. A parameter called `newId` defaulting to
+   * `newId()` resolves to itself and recurses until the stack goes, and it does so only on the
+   * repair branch, which every test avoided by passing its own. Biome said so, as an unused import.
+   */
+  mintId: () => string = newId,
 ): ReadonlyArray<Message> {
   const answered = new Set<string>();
   for (const message of messages) {
@@ -47,7 +52,7 @@ export function repairUnansweredToolCalls(
       // OpenAI requires the results to follow their calls, and some providers require the order to
       // match the `tool_calls` array as well.
       repaired.push({
-        id: newId(),
+        id: mintId(),
         role: "tool",
         toolCallId: call.id,
         content: UNANSWERED,
diff --git a/app/tests/repair-history.test.ts b/app/tests/repair-history.test.ts
index c697efe..3ee972c 100644
--- a/app/tests/repair-history.test.ts
+++ b/app/tests/repair-history.test.ts
@@ -123,3 +123,39 @@ describe("repairing a history before it is sent", () => {
     expect(repairUnansweredToolCalls(messages, ids)).toBe(messages);
   });
 });
+
+/**
+ * The default id source, which every test above replaces with its own.
+ *
+ * That is the reason a self-recursive default survived review: the parameter exists so a test can
+ * make ids predictable, so no test ever ran the default, and the default only runs on the repair
+ * branch. This one calls it the way the only real caller does, with the argument omitted.
+ */
+describe("repairUnansweredToolCalls without an id source", () => {
+  test("repairs using its own ids rather than recursing", () => {
+    const messages: Message[] = [
+      { id: "a", role: "user", content: "go" },
+      {
+        id: "b",
+        role: "assistant",
+        content: "",
+        toolCalls: [
+          {
+            id: "call-1",
+            type: "function",
+            function: { name: "act", arguments: "{}" },
+          },
+        ],
+      },
+    ];
+
+    const repaired = repairUnansweredToolCalls(messages);
+
+    expect(repaired).toHaveLength(3);
+    const result = repaired[2] as Message & { toolCallId: string };
+    expect(result.role).toBe("tool");
+    expect(result.toolCallId).toBe("call-1");
+    // A real id, not an empty string and not the call's own id.
+    expect(result.id).toMatch(/^[0-9a-f-]{36}$/);
+  });
+});