ronantakizawa/codereview-bench huggingface dataset:
Repository: cypress-io/cypress
File: packages/server/test/unit/socket_spec.js (JavaScript)

Code before:
        const state = session.getState()

        state.globalSessions = {
          global: { id: 'global' },
        }

        state.specSessions = {
          local: { id: 'local' },
        }

        this.client.emit('backend:request', 'reset:cached:test:state', ({ error }) => {
          expect(error).to.be.undefined

          expect(state).to.deep.eq({
            globalSessions: {
              'global': { id: 'global' },
            },
            specSessions: {},
          })

          done()
        })
      })
    })

    context('on(get:cached:test:state)', () => {
      it('returns cached test state', async function () {
        await new Promise((resolve) => {
          this.client.emit('backend:request', 'preserve:run:state', {
            currentId: 'test',
            currentRetry: 0,
          }, resolve)
        })

        const mockProtocolManager = {
          resetTest: sinon.stub(),
        }

        this.socket['_protocolManager'] = mockProtocolManager

        await new Promise((resolve) => {
          this.client.emit('get:cached:test:state', (runState, testState) => {
            expect(runState).to.deep.eq({
              currentId: 'test',
              currentRetry: 0,
            })

            expect(testState).to.deep.eq({
              activeSessions: {},
            })


Review comment (performance):
More of a nit but not a big deal since `context` isn't available in vitest
```suggestion
    describe('on(get:cached:test:state)', () => {
```

Code after:
        const state = session.getState()

        state.globalSessions = {
          global: { id: 'global' },
        }

        state.specSessions = {
          local: { id: 'local' },
        }

        this.client.emit('backend:request', 'reset:cached:test:state', ({ error }) => {
          expect(error).to.be.undefined

          expect(state).to.deep.eq({
            globalSessions: {
              'global': { id: 'global' },
            },
            specSessions: {},
          })

          done()
        })
      })
    })

    describe('on(get:cached:test:state)', () => {
      it('returns cached test state', async function () {
        await new Promise((resolve) => {
          this.client.emit('backend:request', 'preserve:run:state', {
            currentId: 'test',
            currentRetry: 0,
          }, resolve)
        })

        const mockProtocolManager = {
          resetTest: sinon.stub(),
        }

        this.socket['_protocolManager'] = mockProtocolManager

        await new Promise((resolve) => {
          this.client.emit('get:cached:test:state', (runState, testState) => {
            expect(runState).to.deep.eq({
              currentId: 'test',
              currentRetry: 0,
            })

            expect(testState).to.deep.eq({
              activeSessions: {},
            })
Repository: cypress-io/cypress
File: npm/mount-utils/create-rollup-entry.mjs (JavaScript)

Code before:
// CommonJS to easily share across packages
import ts from 'rollup-plugin-typescript2'
import resolve from '@rollup/plugin-node-resolve'
import commonjs from '@rollup/plugin-commonjs'
import { mergeWith } from 'lodash'
import { readFileSync } from 'fs'
import dts from 'rollup-plugin-dts'

const pkg = JSON.parse(readFileSync('./package.json'))

/** @type {(options: { formats: string[], input: string, config: {} }) => []} */
export function createEntries (options) {
  const {
    formats,
    input,
    config = {},
    dtsOptions = {},
  } = options

  const banner = `
/**
 * ${pkg.name} v${pkg.version}
 * (c) ${new Date().getFullYear()} Cypress.io
 * Released under the MIT License
 */
`

  return formats.map((format) => {
    const baseConfig = {
      input,

Review comment (suggestion):
@jennifer-shehane in the future if you want this to be tree-shakable (which is doesn't matter here since this is a build script), you would want to have `import mergeWith from "lodash/mergeWith"` so it only loads in the mergeWith module and not the whole library into the context

Code after:
// CommonJS to easily share across packages
import ts from 'rollup-plugin-typescript2'
import resolve from '@rollup/plugin-node-resolve'
import commonjs from '@rollup/plugin-commonjs'
import _ from 'lodash'
import { readFileSync } from 'fs'
import dts from 'rollup-plugin-dts'

const pkg = JSON.parse(readFileSync('./package.json'))

/** @type {(options: { formats: string[], input: string, config: {} }) => []} */
export function createEntries (options) {
  const {
    formats,
    input,
    config = {},
    dtsOptions = {},
  } = options

  const banner = `
/**
 * ${pkg.name} v${pkg.version}
 * (c) ${new Date().getFullYear()} Cypress.io
 * Released under the MIT License
 */
`

  return formats.map((format) => {
    const baseConfig = {
      input,
Repository: cypress-io/cypress
File: npm/webpack-dev-server/src/CypressCTWebpackPlugin.ts (TypeScript)

Code before:
import type { EventEmitter } from 'events'
import _ from 'lodash'
import fs, { PathLike } from 'fs-extra'
import path from 'path'

type UtimesSync = (path: PathLike, atime: string | number | Date, mtime: string | number | Date) => void

interface CypressCTWebpackPluginOptions {
  files: Cypress.Cypress['spec'][]
  projectRoot: string
  supportFile: string | false
  devServerEvents: EventEmitter
  webpack: Function
  indexHtmlFile: string
}

type CypressCTContextOptions = Omit<CypressCTWebpackPluginOptions, 'devServerEvents' | 'webpack'>

export interface CypressCTWebpackContext {
  _cypress: CypressCTContextOptions
}

/**
 * @internal
 */
type Webpack45Compilation = Compilation & {
  // TODO: Drop these additional Webpack 4 types
  inputFileSystem: {
    fileSystem: {
      utimesSync: UtimesSync
    }
  }
}

/**
 * A webpack 4/5 compatible Cypress Component Testing Plugin
 *
 * @internal
 */
export class CypressCTWebpackPlugin {
  private files: Cypress.Cypress['spec'][] = []
  private supportFile: string | false
  private compilation: Webpack45Compilation | null = null
  private webpack: Function
  private indexHtmlFile: string

  private readonly projectRoot: string
  private readonly devServerEvents: EventEmitter

  constructor (options: CypressCTWebpackPluginOptions) {
    this.files = options.files

Review comment (suggestion):
We should probably rename this no that webpack 4 isn't supported
```suggestion
type Webpack5Compilation = Compilation & {
```

Code after:
import type { EventEmitter } from 'events'
import _ from 'lodash'
import fs from 'fs-extra'
import path from 'path'

interface CypressCTWebpackPluginOptions {
  files: Cypress.Cypress['spec'][]
  projectRoot: string
  supportFile: string | false
  devServerEvents: EventEmitter
  webpack: Function
  indexHtmlFile: string
}

type CypressCTContextOptions = Omit<CypressCTWebpackPluginOptions, 'devServerEvents' | 'webpack'>

export interface CypressCTWebpackContext {
  _cypress: CypressCTContextOptions
}

/**
 * A webpack 5 compatible Cypress Component Testing Plugin
 *
 * @internal
 */
export class CypressCTWebpackPlugin {
  private files: Cypress.Cypress['spec'][] = []
  private supportFile: string | false
  private compilation: Compilation | null = null
  private webpack: Function
  private indexHtmlFile: string

  private readonly projectRoot: string
  private readonly devServerEvents: EventEmitter

  constructor (options: CypressCTWebpackPluginOptions) {
    this.files = options.files
    this.supportFile = options.supportFile
    this.projectRoot = options.projectRoot
    this.devServerEvents = options.devServerEvents
    this.webpack = options.webpack
    this.indexHtmlFile = options.indexHtmlFile
  }

  private addLoaderContext = (loaderContext: object, module: any) => {
    (loaderContext as CypressCTWebpackContext)._cypress = {
      files: this.files,
      projectRoot: this.projectRoot,
      supportFile: this.supportFile,
      indexHtmlFile: this.indexHtmlFile,
    }
Repository: cypress-io/cypress
File: packages/app/src/runner/index.ts (TypeScript)

Code before:
}

// this is how the Cypress driver knows which spec to run.
// we change name internally to be the relative path, and
// the `spec.name` property is now `spec.baseName`.
// but for backwards compatibility with the Cypress.spec API
// just assign `name` to be `baseName`.
function setSpecForDriver (spec: SpecFile) {
  return { ...spec, name: spec.baseName }
}

/**
 * Set up an E2E spec by creating a fresh AUT for the spec to evaluate under,
 * a Spec IFrame to load the spec's source code, and
 * initialize Cypress on the AUT.
 */
async function runSpecE2E (config, spec: SpecFile) {
  const $runnerRoot = getRunnerElement()

  // clear AUT, if there is one.
  empty($runnerRoot)

  // create root for new AUT
  const $container = document.createElement('div')

  $container.id = 'aut-frames-container'
  $container.classList.add('screenshot-height-container')

  $runnerRoot.append($container)

  // create new AUT
  const autIframe = getAutIframeModel()

  const { autIframe: $autIframe, autSnapshotIframes: $autSnapshotIframes } = autIframe.create()

  $autIframe.appendTo($container)

  $autSnapshotIframes.forEach((iframe) => {
    iframe.appendTo($container)
  })

  // Remove the spec bridge iframe
  document.querySelectorAll('iframe.spec-bridge-iframe').forEach((el) => {
    el.remove()
  })

  await autIframe.visitBlankPage()

  // create Spec IFrame
  const specSrc = getSpecUrl({
    browserFamily: config.browser.family,

Review comment (suggestion):
I think most of the other code uses `iframe`:
```suggestion
  $container.id = 'aut-iframes-container'
```

Code after:
}

// this is how the Cypress driver knows which spec to run.
// we change name internally to be the relative path, and
// the `spec.name` property is now `spec.baseName`.
// but for backwards compatibility with the Cypress.spec API
// just assign `name` to be `baseName`.
function setSpecForDriver (spec: SpecFile) {
  return { ...spec, name: spec.baseName }
}

/**
 * Set up an E2E spec by creating a fresh AUT for the spec to evaluate under,
 * a Spec IFrame to load the spec's source code, and
 * initialize Cypress on the AUT.
 */
async function runSpecE2E (config, spec: SpecFile) {
  const $runnerRoot = getRunnerElement()

  // clear AUT, if there is one.
  empty($runnerRoot)

  // create root for new AUT
  const $container = document.createElement('div')

  $container.id = 'aut-iframes-container'
  $container.classList.add('screenshot-height-container')

  $runnerRoot.append($container)

  // create new AUT
  const autIframe = getAutIframeModel()

  const { autIframe: $autIframe, autSnapshotIframes: $autSnapshotIframes } = autIframe.create()

  $autIframe.appendTo($container)

  $autSnapshotIframes.forEach((iframe) => {
    iframe.appendTo($container)
  })

  // Remove the spec bridge iframe
  document.querySelectorAll('iframe.spec-bridge-iframe').forEach((el) => {
    el.remove()
  })

  await autIframe.visitBlankPage()

  // create Spec IFrame
  const specSrc = getSpecUrl({
    browserFamily: config.browser.family,
Repository: cypress-io/cypress
File: packages/server/lib/cloud/api/create_instance.ts (TypeScript)

Code before:
        `/runs/${runId}/instances`,
        instanceData,
        {
          headers: {
            'x-route-version': '5',
            'x-cypress-run-id': runId,
            'x-cypress-request-attempt': `${attemptNumber}`,
          },
          timeout,
        },
      )

      return data
    } catch (err: unknown) {
      attemptNumber++

      throw err
    }
  }, {
    maxAttempts: MAX_RETRIES,
    retryDelay: exponentialBackoff(),
    shouldRetry: isRetryableCloudError,
    onRetry: (delay, err) => {
      errors.warning(
        'CLOUD_API_RESPONSE_FAILED_RETRYING', {
          delay: humanTime.long(delay),
          tries: MAX_RETRIES - attemptNumber,
          response: isAxiosError(err) ? err : err instanceof Error ? err : new Error(String(err)),
        },
      )
    },
  })()
}


Review comment (suggestion):
```suggestion
          delay: humanTime.long(delay, false),
```

Code after:
        `/runs/${runId}/instances`,
        instanceData,
        {
          headers: {
            'x-route-version': '5',
            'x-cypress-run-id': runId,
            'x-cypress-request-attempt': `${attemptNumber}`,
          },
          timeout,
        },
      )

      return data
    } catch (err: unknown) {
      attemptNumber++

      throw err
    }
  }, {
    maxAttempts: MAX_RETRIES,
    retryDelay: exponentialBackoff(),
    shouldRetry: isRetryableCloudError,
    onRetry: (delay, err) => {
      errors.warning(
        'CLOUD_API_RESPONSE_FAILED_RETRYING', {
          delay: humanTime.long(delay, false),
          tries: MAX_RETRIES - attemptNumber,
          response: isAxiosError(err) ? err : err instanceof Error ? err : new Error(String(err)),
        },
      )
    },
  })()
}
Repository: cypress-io/cypress
File: packages/proxy/lib/network-proxy.ts (TypeScript)

Code before:
    await this.http.handleSourceMapRequest(req, res)
  }

  setHttpBuffer (buffer) {
    this.http.setBuffer(buffer)
  }

  reset (options: { resetBetweenSpecs: boolean } = { resetBetweenSpecs: false }) {
    this.http.reset(options)
  }

  setProtocolManager (protocolManager) {
    this.http.setProtocolManager(protocolManager)
  }

  setPreRequestTimeout (timeout) {
    this.http.setPreRequestTimeout(timeout)
  }

  setCredentials = ({ url, resourceType, credentialStatus }: {
    url: string
    resourceType: ResourceType
    credentialStatus: RequestCredentialLevel
  }): void => {
    resourceTypeAndCredentialManager.set({ url, resourceType, credentialStatus })
  }

  clearCredentials (): void {
    resourceTypeAndCredentialManager.clear()
  }
}


Review comment (security):
```suggestion
  setCredentials ({ url, resourceType, credentialStatus }: {
    url: string
    resourceType: ResourceType
    credentialStatus: RequestCredentialLevel
  }): void {
    resourceTypeAndCredentialManager.set({ url, resourceType, credentialStatus })
  }
```

Code after:
    await this.http.handleSourceMapRequest(req, res)
  }

  setHttpBuffer (buffer) {
    this.http.setBuffer(buffer)
  }

  reset (options: { resetBetweenSpecs: boolean } = { resetBetweenSpecs: false }) {
    this.http.reset(options)
  }

  setProtocolManager (protocolManager) {
    this.http.setProtocolManager(protocolManager)
  }

  setPreRequestTimeout (timeout) {
    this.http.setPreRequestTimeout(timeout)
  }

  setCredentials ({ url, resourceType, credentialStatus }: {
    url: string
    resourceType: ResourceType
    credentialStatus: RequestCredentialLevel
  }): void {
    resourceTypeAndCredentialManager.set({ url, resourceType, credentialStatus })
  }

  clearCredentials (): void {
    resourceTypeAndCredentialManager.clear()
  }
}
Repository: danny-avila/LibreChat
File: packages/api/src/cluster/LeaderElection.ts (TypeScript)

Code before:
import { keyvRedisClient } from '~/cache/redisClients';
import { cacheConfig as con } from '~/cache/cacheConfig';
import { logger } from '@librechat/data-schemas';

/**
 * Distributed leader election implementation using Redis for coordination across multiple server instances.
 *
 * Leadership election:
 * - During bootup, every server attempts to become the leader by calling isLeader()
 * - Uses atomic Redis SET NX (set if not exists) to ensure only ONE server can claim leadership
 * - The first server to successfully set the key becomes the leader; others become followers
 * - Works with any number of servers (1 to infinite) - single server always becomes leader
 *
 * Leadership maintenance:
 * - Leader holds a key in Redis with a 60-second TTL (time-to-live)
 * - Leader refreshes this TTL every 30 seconds to maintain leadership
 * - If leader crashes, the TTL expires after 60 seconds and the key disappears
 * - On shutdown, leader deletes its key to allow immediate re-election
 * - Followers check for leadership and attempt to claim it when the key is empty
 */
export class LeaderElection {
  // We can't use Keyv namespace here because we need direct Redis access for atomic operations
  static readonly LEADER_KEY = `${con.REDIS_KEY_PREFIX}${con.GLOBAL_PREFIX_SEPARATOR}LeadingServerUUID`;

  readonly UUID: string;
  private readonly LEADER_TTL: number; // seconds
  private readonly REFRESH_INTERVAL: number; // seconds

Review comment (performance):
nit: Can we keep `cacheConfig` as is, I keep reading `con` as `connection`
We don't seem to be using it often either

Code after:
import { keyvRedisClient } from '~/cache/redisClients';
import { cacheConfig as cache } from '~/cache/cacheConfig';
import { clusterConfig as cluster } from './config';
import { logger } from '@librechat/data-schemas';

/**
 * Distributed leader election implementation using Redis for coordination across multiple server instances.
 *
 * Leadership election:
 * - During bootup, every server attempts to become the leader by calling isLeader()
 * - Uses atomic Redis SET NX (set if not exists) to ensure only ONE server can claim leadership
 * - The first server to successfully set the key becomes the leader; others become followers
 * - Works with any number of servers (1 to infinite) - single server always becomes leader
 *
 * Leadership maintenance:
 * - Leader holds a key in Redis with a 25-second lease duration
 * - Leader renews this lease every 10 seconds to maintain leadership
 * - If leader crashes, the lease eventually expires, and the key disappears
 * - On shutdown, leader deletes its key to allow immediate re-election
 * - Followers check for leadership and attempt to claim it when the key is empty
 */
export class LeaderElection {
  // We can't use Keyv namespace here because we need direct Redis access for atomic operations
  static readonly LEADER_KEY = `${cache.REDIS_KEY_PREFIX}${cache.GLOBAL_PREFIX_SEPARATOR}LeadingServerUUID`;
  private static _instance = new LeaderElection();

  readonly UUID: string = crypto.randomUUID();
Repository: danny-avila/LibreChat
File: api/server/services/Config/__tests__/getCachedTools.spec.js (JavaScript)

Code before:
    it('should generate different cache keys for same user with different servers', () => {
      const keyGithub = ToolCacheKeys.MCP_SERVER('user123', 'github');
      const keyGitlab = ToolCacheKeys.MCP_SERVER('user123', 'gitlab');

      expect(keyGithub).not.toEqual(keyGitlab);
      expect(keyGithub).toBe('tools:mcp:user123:github');
      expect(keyGitlab).toBe('tools:mcp:user123:gitlab');
    });

    it('should generate unique cache keys for each user-server combination', () => {
      const combinations = [
        ['user1', 'server1'],
        ['user1', 'server2'],
        ['user2', 'server1'],
        ['user2', 'server2'],
      ];

      const keys = combinations.map(([userId, serverName]) =>
        ToolCacheKeys.MCP_SERVER(userId, serverName),
      );

      // All keys should be unique
      const uniqueKeys = new Set(keys);
      expect(uniqueKeys.size).toBe(keys.length);
    });
  });

  describe('getMCPServerTools - User Isolation', () => {
    it('should isolate cache between different users for same server', async () => {
      const serverName = 'github';
      const userATools = { tool1: { name: 'Tool 1' } };
      const userBTools = { tool2: { name: 'Tool 2' } };

      // Mock cache to return different tools for different users
      mockCache.get.mockImplementation((key) => {
        if (key === 'tools:mcp:userA:github') {
          return userATools;
        }
        if (key === 'tools:mcp:userB:github') {
          return userBTools;
        }
        return null;
      });

      // User A should get their tools
      const resultA = await getMCPServerTools('userA', serverName);
      expect(resultA).toEqual(userATools);
      expect(mockCache.get).toHaveBeenCalledWith('tools:mcp:userA:github');

      // User B should get their own different tools
      const resultB = await getMCPServerTools('userB', serverName);

Review comment (performance):
These tests are overkill. They are all testing that with different input params for `ToolCacheKeys.MCP_SERVER`, different results are returned and they conform to a specific format. Since the test before these already shows us that the key is constructed from the input params in the `tools:mcp:<userId>:<serverName>` format, testing that having different combinations of userId/serverName resulting different keys is quite redundant. At most we only need 1 of such tests.

Code after:
const { ToolCacheKeys } = require('../getCachedTools');

describe('getCachedTools - Cache Isolation Security', () => {
  describe('ToolCacheKeys.MCP_SERVER', () => {
    it('should generate cache keys that include userId', () => {
      const key = ToolCacheKeys.MCP_SERVER('user123', 'github');
      expect(key).toBe('tools:mcp:user123:github');
    });
  });
});
Repository: danny-avila/LibreChat
File: api/server/services/Config/__tests__/getCachedTools.spec.js (JavaScript)

Code before:
        }
        return null;
      });

      // User A should get their tools
      const resultA = await getMCPServerTools('userA', serverName);
      expect(resultA).toEqual(userATools);
      expect(mockCache.get).toHaveBeenCalledWith('tools:mcp:userA:github');

      // User B should get their own different tools
      const resultB = await getMCPServerTools('userB', serverName);
      expect(resultB).toEqual(userBTools);
      expect(mockCache.get).toHaveBeenCalledWith('tools:mcp:userB:github');

      // Verify User A didn't get User B's tools
      expect(resultA).not.toEqual(resultB);
    });

    it('should return null when user has no cached tools for server', async () => {
      mockCache.get.mockResolvedValue(null);

      const result = await getMCPServerTools('userA', 'github');

      expect(result).toBeNull();
      expect(mockCache.get).toHaveBeenCalledWith('tools:mcp:userA:github');
    });

    it('should not leak tools between users even with similar userIds', async () => {
      const userIdA = 'user1';
      const userIdB = 'user10'; // Similar but different
      const serverName = 'github';

      mockCache.get.mockImplementation((key) => {
        if (key === 'tools:mcp:user1:github') {
          return { tool1: {} };
        }
        return null;
      });

      const resultA = await getMCPServerTools(userIdA, serverName);
      const resultB = await getMCPServerTools(userIdB, serverName);

      expect(resultA).toEqual({ tool1: {} });
      expect(resultB).toBeNull(); // user10 should NOT get user1's tools
    });

    it('should call getLogStores with correct cache store', async () => {
      mockCache.get.mockResolvedValue(null);

      await getMCPServerTools('user123', 'github');


Review comment (bug):
I'm not sure if this is actually testing anything:
- `expect(resultA).toEqual(userATools)`:  this testing `mockCache.get`, which is a mocked func.
- `expect(mockCache.get).toHaveBeenCalledWith('tools:mcp:userA:github')`: this is testing `ToolCacheKeys.MCP_SERVER` which we already have tests for above.


The bug is "different users share the same tools cache because `userId` is not encoded into the cache key". So IMO, a simple test on `ToolCacheKeys.MCP_SERVER` to guarantee that the userId is in the key is enough. We can trust that `Keyv.get` and `redis.get` are already properly tested to fetch you the unique value assigned to a unique key.

Code after:
const { ToolCacheKeys } = require('../getCachedTools');

describe('getCachedTools - Cache Isolation Security', () => {
  describe('ToolCacheKeys.MCP_SERVER', () => {
    it('should generate cache keys that include userId', () => {
      const key = ToolCacheKeys.MCP_SERVER('user123', 'github');
      expect(key).toBe('tools:mcp:user123:github');
    });
  });
});