🐛 fix: Tighten registerActionTools key handling and assistants test

Follow-up to d643444 addressing the second review pass on PR #12594.

**ESLint** — Two prettier errors in the spec file (multi-line arrow
function bodies that should fit on one line). Auto-fixed.

**[MINOR] operationId containing `---` → key mismatch.** The lookup
path collapsed every `actionDomainSeparator` sequence in the full tool
name, but the registration path passed `sig.name` through unchanged.
A `---` that survived into an operationId would shift the underscore
boundary at lookup and miss its own key. Fix in `registerActionTools`:
normalize `sig.name` with the same helper so registration and lookup
always agree on the canonical form. `sanitizeOperationId` strips the
characters that produce `---` in practice, so this is theoretical
hardening, not a fix for a known reproducer.

**[MINOR] Same-operationId + same-hostname silent overwrite.** Two
actions sharing both an operationId and a hostname still produced a
silent `Map.set()` overwrite (the new key is identical, so neither the
operationId nor the domain disambiguates). Added a `setKey` helper
inside `registerActionTools` that logs a `[Actions] operationId
collision: ...` warning whenever a key is already present, naming the
overwriting action_id. The silent-overwrite mode from the original bug
cannot reappear under a different disguise without surfacing in the
logs.

**[NIT] processRequiredActions test simulated a runtime crash.**
`mockCreateActionTool` returned a tool with `_call: jest.fn()`, which
resolves to `undefined`. `processRequiredActions` chains
`.then(handleToolOutput).catch(handleToolError)` directly onto that
return, so `undefined.then(...)` threw synchronously and the outer
try/catch funneled the error into `handleToolError`. Creation count
assertions still passed because `createActionTool` runs before the
crash, but the test was silently exercising the failure path. Updated
the global mock to `_call: jest.fn().mockResolvedValue('{"status":"ok"}')`
so the success path runs end-to-end. The assistants regression test
now executes in ~5ms instead of ~90ms, which corroborates that it's
no longer hitting the synchronous throw.

**[NIT] Duplicated rationale comments.** All three call sites carried
multi-line comment blocks restating why we key on the full tool name.
That rationale now lives canonically in `registerActionTools`'s JSDoc;
the inline blocks collapsed to `// See registerActionTools for the
key-shape rationale.` Net -22 lines of comments.

41/41 tests still pass; lint is clean.

api/server/services/ToolService.js

@@ -91,6 +91,13 @@ const normalizeActionToolName = (toolName) => toolName.replace(domainSeparatorRe
  * Indexing on the full tool name instead of the encoded domain alone is
  * what makes multi-action agents work when two actions share a hostname:
  * the operationId disambiguates them, so neither overwrites the other.
+ *
+ * Two actions that additionally share the same operationId still
+ * collide (nothing in the key distinguishes them). That case is
+ * pathological — `sanitizeOperationId` plus OpenAPI's own uniqueness
+ * requirement make it very unlikely — but when it does happen we log
+ * a warning so the silent-overwrite mode from the original bug cannot
+ * reappear under a different disguise.
  */
 const registerActionTools = ({
   toolToAction,
@@ -99,11 +106,27 @@ const registerActionTools = ({
   legacyNormalized,
   makeEntry,
 }) => {
+  const setKey = (key, entry) => {
+    if (toolToAction.has(key)) {
+      logger.warn(
+        `[Actions] operationId collision: "${key}" already registered; ` +
+          `action "${entry.action?.action_id}" overwrites the previous entry. ` +
+          `Two actions share both the operationId and the encoded hostname.`,
+      );
+    }
+    toolToAction.set(key, entry);
+  };
+
   for (const sig of functionSignatures) {
     const entry = makeEntry(sig);
-    toolToAction.set(`${sig.name}${actionDelimiter}${normalizedDomain}`, entry);
+    // Normalize the operationId portion of the key as well — the lookup
+    // path collapses every `actionDomainSeparator` sequence in the full
+    // tool name, so a `---` that survived into `sig.name` would shift the
+    // underscore boundary and miss an otherwise-matching key.
+    const normalizedName = normalizeActionToolName(sig.name);
+    setKey(`${normalizedName}${actionDelimiter}${normalizedDomain}`, entry);
     if (legacyNormalized !== normalizedDomain) {
-      toolToAction.set(`${sig.name}${actionDelimiter}${legacyNormalized}`, entry);
+      setKey(`${normalizedName}${actionDelimiter}${legacyNormalized}`, entry);
     }
   }
 };
@@ -308,12 +331,7 @@ async function processRequiredActions(client, requiredActions) {
             assistant_id: client.req.body.assistant_id,
           })) ?? [];
 
-        // Index every fully-qualified tool name to its owning action set.
-        // See loadAgentTools / loadActionToolsForExecution for the rationale:
-        // keying on the full tool name (operationId + delimiter + encoded
-        // domain) instead of the encoded domain alone is what allows two
-        // actions sharing a hostname to coexist on the same assistant
-        // without one silently shadowing the other.
+        // See registerActionTools for the key-shape rationale.
         const toolToAction = new Map();
 
         for (const action of actionSets) {
@@ -1034,10 +1052,7 @@ async function loadAgentTools({
     };
   }
 
-  // See loadActionToolsForExecution below for the rationale: indexing
-  // by full tool name (operationId + delimiter + encoded domain) instead
-  // of by domain alone is what makes multi-action agents work when two
-  // actions share a hostname.
+  // See registerActionTools for the key-shape rationale.
   const toolToAction = new Map();
 
   for (const action of actionSets) {
@@ -1349,21 +1364,7 @@ async function loadActionToolsForExecution({
     return loadedActionTools;
   }
 
-  /**
-   * Map every fully-qualified tool name to its owning action and the
-   * specific request builder / zod schema / signature it should resolve to.
-   *
-   * Indexing on the full tool name (`<operationId>_action_<encoded-domain>`)
-   * instead of the encoded domain alone is what makes multi-action agents
-   * work: two actions that share a hostname now occupy distinct slots
-   * because the operationId disambiguates them. The previous shape stored
-   * one entry per encoded domain, so the second action of a pair would
-   * overwrite the first and its tools would silently disappear.
-   *
-   * Both the new and the legacy domain encodings are registered for each
-   * function so agents whose stored tool names predate the current
-   * encoding still resolve correctly.
-   */
+  // See registerActionTools for the key-shape rationale.
   const toolToAction = new Map();
   const allowedDomains = appConfig?.actions?.allowedDomains;
 

api/server/services/__tests__/ToolService.spec.js

@@ -610,7 +610,11 @@ describe('ToolService - Action Capability Gating', () => {
         // assertions can verify each tool was wired to the correct action's
         // builder, not its sibling's.
         _builder: requestBuilder,
-        _call: jest.fn(),
+        // Resolve instead of returning undefined — processRequiredActions
+        // chains `.then(handleToolOutput)` directly onto this call, which
+        // would throw synchronously on an undefined return and mask the
+        // test as a simulated runtime crash.
+        _call: jest.fn().mockResolvedValue('{"status":"ok"}'),
         schema: {},
         description: '',
       }));
@@ -734,9 +738,7 @@ describe('ToolService - Action Capability Gating', () => {
       // zodSchema, name, and description for assistants API"), so key
       // resolution assertions off the request builder path instead.
       expect(mockCreateActionTool).toHaveBeenCalledTimes(2);
-      const builderPaths = mockCreateActionTool.mock.calls.map(
-        (c) => c[0].requestBuilder?.path,
-      );
+      const builderPaths = mockCreateActionTool.mock.calls.map((c) => c[0].requestBuilder?.path);
       expect(builderPaths).toEqual(expect.arrayContaining(['/echo', '/items']));
       // Each call must carry a distinct builder — guards against the bug
       // where the surviving action's builders got routed to every tool.
@@ -790,9 +792,7 @@ describe('ToolService - Action Capability Gating', () => {
       });
 
       expect(mockCreateActionTool).toHaveBeenCalledTimes(2);
-      const callsByName = new Map(
-        mockCreateActionTool.mock.calls.map((c) => [c[0].name, c[0]]),
-      );
+      const callsByName = new Map(mockCreateActionTool.mock.calls.map((c) => [c[0].name, c[0]]));
       expect(callsByName.has(rawNameA)).toBe(true);
       expect(callsByName.has(rawNameB)).toBe(true);
       expect(callsByName.get(rawNameA).requestBuilder.path).toBe('/echo');