🪡 fix: Handle Missing Skill File Upsert Metadata (#13520)

2026-06-22 21:43:06 +00:00 · 2026-06-04 21:06:12 -04:00 · 2026-06-04 21:06:12 -04:00 · 40ec77e061
commit 40ec77e061
parent 44ed7864fb
4 changed files with 159 additions and 22 deletions
--- a/api/server/services/Endpoints/agents/skillDeps.js
+++ b/api/server/services/Endpoints/agents/skillDeps.js
@ -74,6 +74,11 @@ async function saveSkillFileContent({ req, skillId, relativePath, content, mimeT
      author: req.user._id ?? req.user.id,
      tenantId,
    });
+    if (!result) {
+      const error = new Error('Skill file save failed to persist metadata');
+      error.code = 'SKILL_FILE_UPSERT_NOT_FOUND';
+      throw error;
+    }
  } catch (error) {
    const { deleteFile } = getStrategyFunctions(storage.source);
    if (deleteFile) {
--- a/api/server/services/Endpoints/agents/skillDeps.spec.js
+++ b/api/server/services/Endpoints/agents/skillDeps.spec.js
@ -0,0 +1,102 @@
+const mockSaveBuffer = jest.fn();
+const mockDeleteFile = jest.fn();
+const mockGetStrategyFunctions = jest.fn();
+const mockGetFileStrategy = jest.fn();
+const mockGetStorageMetadata = jest.fn();
+const mockResolveRequestTenantId = jest.fn();
+
+jest.mock('~/server/services/Files/strategies', () => ({
+  getStrategyFunctions: (...args) => mockGetStrategyFunctions(...args),
+}));
+
+jest.mock('~/server/services/Files/Code/crud', () => ({
+  batchUploadCodeEnvFiles: jest.fn(),
+}));
+
+jest.mock('~/server/services/Files/Code/process', () => ({
+  getSessionInfo: jest.fn(),
+  checkIfActive: jest.fn(),
+  readSandboxFile: jest.fn(),
+  writeSandboxFile: jest.fn(),
+}));
+
+jest.mock('@librechat/api', () => ({
+  checkAccess: jest.fn(),
+  enrichWithSkillConfigurable: jest.fn(),
+  getStorageMetadata: (...args) => mockGetStorageMetadata(...args),
+  resolveRequestTenantId: (...args) => mockResolveRequestTenantId(...args),
+}));
+
+jest.mock('librechat-data-provider', () => ({
+  AccessRoleIds: { SKILL_OWNER: 'SKILL_OWNER' },
+  FileContext: { skill_file: 'skill_file' },
+  PermissionBits: { EDIT: 2 },
+  Permissions: { USE: 'USE', CREATE: 'CREATE' },
+  PermissionTypes: { SKILLS: 'SKILLS' },
+  PrincipalType: { USER: 'USER' },
+  ResourceType: { SKILL: 'SKILL' },
+  isEphemeralAgentId: jest.fn(() => false),
+}));
+
+jest.mock('~/server/services/PermissionService', () => ({
+  checkPermission: jest.fn(),
+  grantPermission: jest.fn(),
+}));
+
+jest.mock('~/server/utils/getFileStrategy', () => ({
+  getFileStrategy: (...args) => mockGetFileStrategy(...args),
+}));
+
+const mockDb = {
+  getSkillFileByPath: jest.fn(),
+  upsertSkillFile: jest.fn(),
+};
+
+jest.mock('~/models', () => mockDb);
+
+const { getSkillToolDeps } = require('./skillDeps');
+
+describe('skillDeps saveSkillFileContent', () => {
+  beforeEach(() => {
+    jest.clearAllMocks();
+    mockGetFileStrategy.mockReturnValue('s3');
+    mockGetStrategyFunctions.mockReturnValue({
+      saveBuffer: mockSaveBuffer,
+      deleteFile: mockDeleteFile,
+    });
+    mockSaveBuffer.mockResolvedValue('https://files.example.test/uploads/file.txt');
+    mockDeleteFile.mockResolvedValue(undefined);
+    mockGetStorageMetadata.mockReturnValue({
+      storageKey: 'uploads/file.txt',
+      storageRegion: 'us-east-2',
+    });
+    mockResolveRequestTenantId.mockReturnValue('tenant-1');
+    mockDb.getSkillFileByPath.mockResolvedValue(null);
+  });
+
+  it('cleans up the uploaded object when metadata upsert returns no row', async () => {
+    mockDb.upsertSkillFile.mockResolvedValue(null);
+
+    await expect(
+      getSkillToolDeps().saveSkillFileContent({
+        req: {
+          user: { id: 'user-1', _id: 'user-1' },
+          config: {},
+        },
+        skillId: 'skill-1',
+        relativePath: 'references/template.html',
+        content: '<html></html>',
+        mimeType: 'text/html',
+      }),
+    ).rejects.toMatchObject({ code: 'SKILL_FILE_UPSERT_NOT_FOUND' });
+
+    expect(mockDeleteFile).toHaveBeenCalledWith(
+      expect.objectContaining({ user: expect.objectContaining({ id: 'user-1' }) }),
+      {
+        filepath: 'https://files.example.test/uploads/file.txt',
+        user: 'user-1',
+        tenantId: 'tenant-1',
+      },
+    );
+  });
+});
--- a/packages/data-schemas/src/methods/skill.spec.ts
+++ b/packages/data-schemas/src/methods/skill.spec.ts
@ -1390,6 +1390,40 @@ describe('SkillFile methods', () => {
    expect(files[0].storageRegion).toBe('us-east-2');
  });

+  it('throws an explicit error when the upsert returns no saved file row', async () => {
+    const { skill } = await methods.createSkill(makeSkillInput());
+    const missingUpsert = {
+      lean: jest.fn().mockResolvedValue({
+        value: null,
+        lastErrorObject: { updatedExisting: false },
+      }),
+    } as unknown as ReturnType<typeof SkillFile.findOneAndUpdate>;
+    const findOneAndUpdateSpy = jest
+      .spyOn(SkillFile, 'findOneAndUpdate')
+      .mockReturnValueOnce(missingUpsert);
+    const bumpSpy = jest.spyOn(Skill, 'findByIdAndUpdate');
+
+    try {
+      await expect(
+        methods.upsertSkillFile({
+          skillId: skill._id,
+          relativePath: 'scripts/parse.sh',
+          file_id: 'file-1',
+          filename: 'parse.sh',
+          filepath: '/tmp/v1',
+          source: 'local',
+          mimeType: 'text/plain',
+          bytes: 10,
+          author: owner._id,
+        }),
+      ).rejects.toMatchObject({ code: 'SKILL_FILE_UPSERT_NOT_FOUND' });
+      expect(bumpSpy).not.toHaveBeenCalled();
+    } finally {
+      findOneAndUpdateSpy.mockRestore();
+      bumpSpy.mockRestore();
+    }
+  });
+
  it('clears codeEnvRef when a skill file is upserted (replacement)', async () => {
    /* A re-upload of a skill file replaces the row's contents — but the
     * cached `codeEnvRef` refers to the OLD bytes living in codeapi.
--- a/packages/data-schemas/src/methods/skill.ts
+++ b/packages/data-schemas/src/methods/skill.ts
@ -42,6 +42,13 @@ export type ValidationIssue = {
  severity?: 'error' | 'warning';
 };

+type SkillFileUpsertResult = {
+  value: (ISkillFile & { _id: Types.ObjectId }) | null;
+  lastErrorObject?: {
+    updatedExisting?: boolean;
+  };
+};
+
 /** Partition an issue list into blocking errors and non-blocking warnings. */
 export function partitionIssues(issues: ValidationIssue[]): {
  errors: ValidationIssue[];
@ -1437,12 +1444,7 @@ export function createSkillMethods(mongoose: typeof import('mongoose'), deps: Sk
    }
    const SkillFile = mongoose.models.SkillFile as Model<ISkillFileDocument>;
    const category = inferSkillFileCategory(row.relativePath);
-    // Atomic new-vs-replace detection: with `new: false, upsert: true`,
-    // `findOneAndUpdate` returns the pre-update document (or null if the doc
-    // did not exist and was just inserted). Checking the return value replaces
-    // a non-atomic `findOne` + `upsert` pair that could double-count on
-    // concurrent uploads of the same (skillId, relativePath).
-    const previous = await SkillFile.findOneAndUpdate(
+    const result = (await SkillFile.findOneAndUpdate(
      { skillId: row.skillId, relativePath: row.relativePath },
      {
        $set: {
@ -1463,23 +1465,17 @@ export function createSkillMethods(mongoose: typeof import('mongoose'), deps: Sk
        },
        $unset: { content: '', isBinary: '', codeEnvRef: '' },
      },
-      { new: false, upsert: true },
-    ).lean();
-    const delta = previous ? 0 : 1;
+      { new: true, upsert: true, includeResultMetadata: true },
+    ).lean()) as unknown as SkillFileUpsertResult;
+    const current = result.value;
+    if (!current) {
+      const error = new Error('Skill file upsert failed to read the saved file row');
+      (error as Error & { code?: string }).code = 'SKILL_FILE_UPSERT_NOT_FOUND';
+      throw error;
+    }
+    const delta = result.lastErrorObject?.updatedExisting === false ? 1 : 0;
    await bumpSkillVersionAndAdjustFileCount(row.skillId, delta);
-
-    // Fetch the current (post-upsert) document for the caller. This second
-    // round-trip is an intentional tradeoff for the TOCTOU-safe detection
-    // above: `new: false` is required to distinguish insert from replace
-    // atomically, which means `findOneAndUpdate` returns the pre-update
-    // document (null on insert). A separate `findOne` is the simplest way
-    // to return the authoritative post-upsert state. Performance impact is
-    // negligible compared to the file upload I/O this sits behind.
-    const current = await SkillFile.findOne({
-      skillId: row.skillId,
-      relativePath: row.relativePath,
-    }).lean();
-    return current as unknown as ISkillFile & { _id: Types.ObjectId };
+    return current;
  }

  async function deleteSkillFile(