fix: resolve round 2 review findings R2-1 through R2-7

R2-1 (toggle semantics): openai.js + responses.js now check admin capability (AgentCapabilities.skills) alongside ephemeral toggle. Aligns with initialize.js. R2-2 (swallowed error): primeInvokedSkills now logs updateSkillFileCodeEnvIds failures (was .catch(() => {})) R2-4 (test cast): Record<string, string> → Record<string, unknown> R2-5 (DRY regression): Extract enrichWithSkillConfigurable() into skillDeps.js. Replaces 4 identical loadAuthValues blocks. Each loadTools callback is now a one-liner. JSDoc added (R2-6). R2-7 (sequential streams): primeInvokedSkills now uses Promise.allSettled for parallel stream acquisition.
2026-08-27 12:13:30 +00:00 · 2026-04-14 20:47:47 -04:00 · 2026-04-14 20:47:47 -04:00 · ee8bd4b99f
commit ee8bd4b99f
parent 41fe114dcd
6 changed files with 101 additions and 119 deletions
--- a/api/server/controllers/agents/openai.js
+++ b/api/server/controllers/agents/openai.js
@ -1,7 +1,12 @@
 const { nanoid } = require('nanoid');
 const { logger } = require('@librechat/data-schemas');
 const { Callback, ToolEndHandler, formatAgentMessages } = require('@librechat/agents');
-const { EModelEndpoint, ResourceType, PermissionBits } = require('librechat-data-provider');
+const {
+  EModelEndpoint,
+  ResourceType,
+  PermissionBits,
+  AgentCapabilities,
+} = require('librechat-data-provider');
 const {
  writeSSE,
  createRun,
@ -29,9 +34,11 @@ const {
  agentLogHandlerObj,
 } = require('~/server/controllers/agents/callbacks');
 const { loadAgentTools, loadToolsForExecution } = require('~/server/services/ToolService');
-const { loadAuthValues } = require('~/server/services/Tools/credentials');
 const { findAccessibleResources } = require('~/server/services/PermissionService');
-const { getSkillToolDeps } = require('~/server/services/Endpoints/agents/skillDeps');
+const {
+  getSkillToolDeps,
+  enrichWithSkillConfigurable,
+} = require('~/server/services/Endpoints/agents/skillDeps');
 const db = require('~/models');

 /**
@ -209,16 +216,18 @@ const OpenAIChatCompletionController = async (req, res) => {
      model_parameters: agent.model_parameters ?? {},
    };

+    const enabledCapabilities = new Set(agentsEConfig?.capabilities);
    const ephemeralAgent = req.body?.ephemeralAgent;
-    const accessibleSkillIds =
-      ephemeralAgent?.skills === true
-        ? await findAccessibleResources({
-            userId: req.user.id,
-            role: req.user.role,
-            resourceType: ResourceType.SKILL,
-            requiredPermissions: PermissionBits.VIEW,
-          })
-        : [];
+    const skillsEnabled =
+      enabledCapabilities.has(AgentCapabilities.skills) && ephemeralAgent?.skills === true;
+    const accessibleSkillIds = skillsEnabled
+      ? await findAccessibleResources({
+          userId: req.user.id,
+          role: req.user.role,
+          resourceType: ResourceType.SKILL,
+          requiredPermissions: PermissionBits.VIEW,
+        })
+      : [];

    const codeEnvAvailable = !!(
      process.env.LIBRECHAT_CODE_API_KEY || req.config?.endpoints?.all?.codeApiKey
@ -302,26 +311,7 @@ const OpenAIChatCompletionController = async (req, res) => {
          tool_resources: primaryConfig.tool_resources,
          actionsEnabled: primaryConfig.actionsEnabled,
        });
-        let codeApiKey;
-        try {
-          const authValues = await loadAuthValues({
-            userId: req.user.id,
-            authFields: ['LIBRECHAT_CODE_API_KEY'],
-          });
-          codeApiKey = authValues.LIBRECHAT_CODE_API_KEY;
-        } catch {
-          // Code API key not configured
-        }
-
-        return {
-          ...result,
-          configurable: {
-            ...result.configurable,
-            req,
-            codeApiKey,
-            accessibleSkillIds: primaryConfig.accessibleSkillIds,
-          },
-        };
+        return enrichWithSkillConfigurable(result, req, primaryConfig.accessibleSkillIds);
      },
      toolEndCallback,
      ...getSkillToolDeps(),
--- a/api/server/controllers/agents/responses.js
+++ b/api/server/controllers/agents/responses.js
@ -2,7 +2,12 @@ const { nanoid } = require('nanoid');
 const { v4: uuidv4 } = require('uuid');
 const { logger } = require('@librechat/data-schemas');
 const { Callback, ToolEndHandler, formatAgentMessages } = require('@librechat/agents');
-const { EModelEndpoint, ResourceType, PermissionBits } = require('librechat-data-provider');
+const {
+  EModelEndpoint,
+  ResourceType,
+  PermissionBits,
+  AgentCapabilities,
+} = require('librechat-data-provider');
 const {
  createRun,
  buildToolSet,
@ -38,9 +43,11 @@ const {
  agentLogHandlerObj,
 } = require('~/server/controllers/agents/callbacks');
 const { loadAgentTools, loadToolsForExecution } = require('~/server/services/ToolService');
-const { loadAuthValues } = require('~/server/services/Tools/credentials');
 const { findAccessibleResources } = require('~/server/services/PermissionService');
-const { getSkillToolDeps } = require('~/server/services/Endpoints/agents/skillDeps');
+const {
+  getSkillToolDeps,
+  enrichWithSkillConfigurable,
+} = require('~/server/services/Endpoints/agents/skillDeps');
 const db = require('~/models');

 /** @type {import('@librechat/api').AppConfig | null} */
@ -347,16 +354,20 @@ const createResponse = async (req, res) => {
      model_parameters: agent.model_parameters ?? {},
    };

+    const enabledCapabilities = new Set(
+      appConfig?.endpoints?.[EModelEndpoint.agents]?.capabilities,
+    );
    const ephemeralAgent = req.body?.ephemeralAgent;
-    const accessibleSkillIds =
-      ephemeralAgent?.skills === true
-        ? await findAccessibleResources({
-            userId: req.user.id,
-            role: req.user.role,
-            resourceType: ResourceType.SKILL,
-            requiredPermissions: PermissionBits.VIEW,
-          })
-        : [];
+    const skillsEnabled =
+      enabledCapabilities.has(AgentCapabilities.skills) && ephemeralAgent?.skills === true;
+    const accessibleSkillIds = skillsEnabled
+      ? await findAccessibleResources({
+          userId: req.user.id,
+          role: req.user.role,
+          resourceType: ResourceType.SKILL,
+          requiredPermissions: PermissionBits.VIEW,
+        })
+      : [];

    const codeEnvAvailable = !!(
      process.env.LIBRECHAT_CODE_API_KEY || req.config?.endpoints?.all?.codeApiKey
@ -468,26 +479,7 @@ const createResponse = async (req, res) => {
            tool_resources: primaryConfig.tool_resources,
            actionsEnabled: primaryConfig.actionsEnabled,
          });
-          let codeApiKey;
-          try {
-            const authValues = await loadAuthValues({
-              userId: req.user.id,
-              authFields: ['LIBRECHAT_CODE_API_KEY'],
-            });
-            codeApiKey = authValues.LIBRECHAT_CODE_API_KEY;
-          } catch {
-            // Code API key not configured
-          }
-
-          return {
-            ...result,
-            configurable: {
-              ...result.configurable,
-              req,
-              codeApiKey,
-              accessibleSkillIds: primaryConfig.accessibleSkillIds,
-            },
-          };
+          return enrichWithSkillConfigurable(result, req, primaryConfig.accessibleSkillIds);
        },
        toolEndCallback,
        ...getSkillToolDeps(),
@ -654,26 +646,7 @@ const createResponse = async (req, res) => {
            tool_resources: primaryConfig.tool_resources,
            actionsEnabled: primaryConfig.actionsEnabled,
          });
-          let codeApiKey;
-          try {
-            const authValues = await loadAuthValues({
-              userId: req.user.id,
-              authFields: ['LIBRECHAT_CODE_API_KEY'],
-            });
-            codeApiKey = authValues.LIBRECHAT_CODE_API_KEY;
-          } catch {
-            // Code API key not configured
-          }
-
-          return {
-            ...result,
-            configurable: {
-              ...result.configurable,
-              req,
-              codeApiKey,
-              accessibleSkillIds: primaryConfig.accessibleSkillIds,
-            },
-          };
+          return enrichWithSkillConfigurable(result, req, primaryConfig.accessibleSkillIds);
        },
        toolEndCallback,
        ...getSkillToolDeps(),
--- a/api/server/services/Endpoints/agents/initialize.js
+++ b/api/server/services/Endpoints/agents/initialize.js
@ -26,7 +26,7 @@ const {
 const { loadAgentTools, loadToolsForExecution } = require('~/server/services/ToolService');
 const { loadAuthValues } = require('~/server/services/Tools/credentials');
 const { filterFilesByAgentAccess } = require('~/server/services/Files/permissions');
-const { getSkillToolDeps } = require('./skillDeps');
+const { getSkillToolDeps, enrichWithSkillConfigurable } = require('./skillDeps');
 const { getModelsConfig } = require('~/server/controllers/ModelController');
 const { checkPermission, findAccessibleResources } = require('~/server/services/PermissionService');
 const AgentClient = require('~/server/controllers/agents/client');
@ -143,28 +143,7 @@ const initializeClient = async ({ req, res, signal, endpointOption }) => {
      });

      logger.debug(`[ON_TOOL_EXECUTE] loaded ${result.loadedTools?.length ?? 0} tools`);
-
-      /** Load code API key for skill file priming (shared with execute_code) */
-      let codeApiKey;
-      try {
-        const authValues = await loadAuthValues({
-          userId: req.user.id,
-          authFields: ['LIBRECHAT_CODE_API_KEY'],
-        });
-        codeApiKey = authValues.LIBRECHAT_CODE_API_KEY;
-      } catch {
-        // Code API key not configured — skill file priming will be skipped
-      }
-
-      return {
-        ...result,
-        configurable: {
-          ...result.configurable,
-          req,
-          codeApiKey,
-          accessibleSkillIds: ctx.accessibleSkillIds,
-        },
-      };
+      return enrichWithSkillConfigurable(result, req, ctx.accessibleSkillIds);
    },
    toolEndCallback,
    ...getSkillToolDeps(),
--- a/api/server/services/Endpoints/agents/skillDeps.js
+++ b/api/server/services/Endpoints/agents/skillDeps.js
@ -1,11 +1,13 @@
 const { getStrategyFunctions } = require('~/server/services/Files/strategies');
 const { batchUploadCodeEnvFiles } = require('~/server/services/Files/Code/crud');
 const { getSessionInfo, checkIfActive } = require('~/server/services/Files/Code/process');
+const { loadAuthValues } = require('~/server/services/Tools/credentials');
 const db = require('~/models');

 /**
 * Returns the skill-related properties for ToolExecuteOptions.
 * Shared across all three controller entry points (initialize.js, openai.js, responses.js).
+ * @returns {{ getSkillByName: Function, listSkillFiles: Function, getStrategyFunctions: Function, batchUploadCodeEnvFiles: Function, getSessionInfo: Function, checkIfActive: Function, updateSkillFileCodeEnvIds: Function, getSkillFileByPath: Function, updateSkillFileContent: Function }}
 */
 function getSkillToolDeps() {
  return {
@ -21,4 +23,34 @@ function getSkillToolDeps() {
  };
 }

-module.exports = { getSkillToolDeps };
+/**
+ * Augments a loadTools result with skill-specific configurable properties.
+ * Loads the code API key and merges it with accessibleSkillIds and the request object.
+ * @param {object} result - The result from loadToolsForExecution
+ * @param {object} req - The Express request object
+ * @param {Array} accessibleSkillIds - Pre-computed accessible skill IDs
+ * @returns {Promise<object>} Augmented result with skill configurable
+ */
+async function enrichWithSkillConfigurable(result, req, accessibleSkillIds) {
+  let codeApiKey;
+  try {
+    const authValues = await loadAuthValues({
+      userId: req.user.id,
+      authFields: ['LIBRECHAT_CODE_API_KEY'],
+    });
+    codeApiKey = authValues.LIBRECHAT_CODE_API_KEY;
+  } catch {
+    // Code API key not configured
+  }
+  return {
+    ...result,
+    configurable: {
+      ...result.configurable,
+      req,
+      codeApiKey,
+      accessibleSkillIds,
+    },
+  };
+}
+
+module.exports = { getSkillToolDeps, enrichWithSkillConfigurable };
--- a/packages/api/src/agents/tests/skills.test.ts
+++ b/packages/api/src/agents/tests/skills.test.ts
@ -2,7 +2,8 @@
 jest.mock('@librechat/agents', () => ({
  ...jest.requireActual('@librechat/agents'),
  Constants: {
-    ...(jest.requireActual('@librechat/agents') as { Constants: Record<string, string> }).Constants,
+    ...(jest.requireActual('@librechat/agents') as { Constants: Record<string, unknown> })
+      .Constants,
    SKILL_TOOL: 'skill',
  },
 }));
--- a/packages/api/src/agents/skillFiles.ts
+++ b/packages/api/src/agents/skillFiles.ts
@ -342,17 +342,19 @@ export async function primeInvokedSkills(
        filename: `${skill.name}/SKILL.md`,
      });

-      for (const file of files) {
-        try {
+      const streamResults = await Promise.allSettled(
+        files.map(async (file) => {
          const strategy = deps.getStrategyFunctions(file.source);
-          if (!strategy.getDownloadStream) continue;
+          if (!strategy.getDownloadStream) return null;
          const stream = await strategy.getDownloadStream(deps.req, file.filepath);
-          allFileStreams.push({ stream, filename: `${skill.name}/${file.relativePath}` });
-        } catch (err) {
-          logger.warn(
-            `[primeInvokedSkills] Failed to get stream for "${file.relativePath}":`,
-            err instanceof Error ? err.message : err,
-          );
+          return { stream, filename: `${skill.name}/${file.relativePath}` };
+        }),
+      );
+      for (const r of streamResults) {
+        if (r.status === 'fulfilled' && r.value) {
+          allFileStreams.push(r.value);
+        } else if (r.status === 'rejected') {
+          logger.warn('[primeInvokedSkills] Failed to get stream:', r.reason);
        }
      }
    }
@ -398,7 +400,12 @@ export async function primeInvokedSkills(
            })
            .filter((u) => u.skillId !== '');
          if (updates.length > 0) {
-            deps.updateSkillFileCodeEnvIds(updates).catch(() => {});
+            deps.updateSkillFileCodeEnvIds(updates).catch((err: unknown) => {
+              logger.warn(
+                '[primeInvokedSkills] Failed to persist codeEnvIdentifiers:',
+                err instanceof Error ? err.message : err,
+              );
+            });
          }
        }
      } catch (err) {