Size Change: +1.99 kB (+0.01%)

Total Size: 33.9 MB

_{compressed-size-action}

/gemini review

From /frontend-review

PR Review: feat(cli): provide manual session UUID via command line arg

Nice work on implementing manual session IDs! This is a great addition for users who need predictable session tracking.

Suggestions for Improvement:

1. Yargs Configuration (config.ts):
   Most string-based options in config.ts use nargs: 1 to ensure exactly one value is consumed. Adding nargs: 1 to the session-id option would maintain consistency and prevent edge cases where subsequent flags might be swallowed.

2. Early Input Validation:
   While ChatRecordingService performs sanitization, it would be safer and more user-friendly to validate the session-id early in parseArguments or resolveSessionId. Rejecting IDs with invalid characters (e.g., path traversal characters) with a clear error is preferable to silent sanitization.

3. Test Style & Cleanup (gemini.test.tsx & config.test.ts):

   • Imports: In gemini.test.tsx, consider moving the SessionSelector and resolveSessionId imports to the top level instead of using dynamic await import inside the it blocks, to match the existing style of the file.
   • Mock Restoration: In config.test.ts, ensure all mocks (like console.error and process.exit) are consistently restored in an afterEach block using vi.restoreAllMocks() rather than manual restoration at the end of each test, to align with the project's strict testing guidelines.

Minor Observation:

The logic in resolveSessionId correctly prevents duplicate session IDs by checking existing sessions, which is excellent for data integrity.

@devr0306 addressed all comments

Suggestions from /review-frontend

Thank you for the PR! This is a great addition that addresses #19602.

The implementation and tests are solid. I have two suggestions to improve validation consistency and performance:

1. Consolidate Session ID Validation

Currently, there are two different validation checks for the session ID:

• In config.ts, the coerce function checks for path traversal characters (/[/\\..]/).
• In gemini.tsx, resolveSessionId uses a stricter regex (/^[a-zA-Z0-9-_]+$/).

We should consolidate this by moving the stricter regex check into the coerce function in config.ts. This enforces the rule earlier in the argument parsing phase and simplifies the logic in gemini.tsx.

Suggested change in packages/cli/src/config/config.ts:

        .option('session-id', {
          type: 'string',
          nargs: 1,
          description: 'Start a new session with a manually provided UUID.',
          coerce: (value: string): string => {
            const trimmed = value.trim();
            if (!trimmed) {
              throw new Error('The --session-id option cannot be empty.');
            }
            if (!/^[a-zA-Z0-9-_]+$/.test(trimmed)) {
              throw new Error(
                'Invalid session ID "' +
                  trimmed +
                  '": Only alphanumeric characters, dashes, and underscores are allowed.',
              );
            }
            return trimmed;
          },
        })

(You can then remove the regex check from resolveSessionId in gemini.tsx).

2. Optimize Session Existence Check

In gemini.tsx, the check for an existing session ID currently reads and parses all session files:

const sessions = await sessionSelector.listSessions();
if (sessions.some((s) => s.id === sessionIdArg)) { ... }

If a user has many past sessions, this can become slow. We can optimize this by checking for the specific file directly using fs.stat or by adding an efficient sessionExists(id) method to SessionSelector.

Suggested change (adding to packages/cli/src/utils/sessionUtils.ts):

  async sessionExists(id: string): Promise<boolean> {
    const chatsDir = path.join(this.storage.getProjectTempDir(), 'chats');
    const files = await fs.readdir(chatsDir).catch(() => []);
    return files.some(file => file.startsWith(SESSION_FILE_PREFIX + id + '-'));
  }

Then update gemini.tsx to use it:

    if (await sessionSelector.sessionExists(sessionIdArg)) {
      coreEvents.emitFeedback(
        'error',
        `Error starting session: Session ID "${sessionIdArg}" already exists. Use --resume to resume it, or provide a different ID.`,
      );

Let me know if you have any questions!

addressed all