refactor: decouple stored session deletion from ChatRecordingService (#22920)

[yuvrajangadsingh]

Summary

Decouples stored-session deletion from ChatRecordingService so the CLI's one-shot delete utility doesn't need to construct a recording service with Config.

Fixes #22920.

Details

ChatRecordingService is built to live inside the agent loop. Its constructor expects an AgentLoopContext, and recording methods rely on context.promptId, registries, and the live session id.

But session deletion only touches config.storage.getProjectTempDir(). The CLI delete utility at packages/cli/src/utils/sessions.ts:99 was calling new ChatRecordingService(config) (where config: Config) and then deleteSession, which compiles only because Config implements AgentLoopContext as a deprecated backwards-compat surface. The recording test file similarly relied on a mockConfig.config = mockConfig recursive hack to make the constructor accept a Config mock.

This PR:

• Extracts deletion into deleteStoredSession(config, sessionIdOrBasename) in packages/core/src/utils/sessionOperations.ts, along with three helpers moved from the class: deriveSessionShortId, getMatchingSessionFiles, deleteSessionFileAndArtifacts. Behavior is preserved verbatim.
• Reduces ChatRecordingService.deleteSession() to a thin wrapper that delegates to the free function. This preserves the existing instance-method consumer in useSessionBrowser (which holds a properly-constructed instance via getGeminiClient().getChatRecordingService()) and the recording test suite.
• Updates packages/cli/src/utils/sessions.ts to call deleteStoredSession(config, file) directly. No ChatRecordingService construction with Config. No reliance on the deprecated AgentLoopContext-compat path.
• Updates packages/cli/src/utils/sessions.test.ts to mock the free function instead of ChatRecordingService.

How to Validate

Local commands:

# Type check both packages
cd packages/core && npx tsc --noEmit
cd packages/cli && npx tsc --noEmit

# Unit tests covering the changed paths
cd packages/core && npx vitest run src/services/chatRecordingService.test.ts src/utils/sessionOperations.test.ts
cd packages/cli && npx vitest run src/utils/sessions.test.ts src/ui/hooks/useSessionBrowser.test.ts

Results on this branch:

• Type-check: clean on both packages
• Tests: chatRecordingService.test.ts 41/41, sessionOperations.test.ts 7/7, sessions.test.ts 17/17, useSessionBrowser.test.ts 9/9
• Lint: clean on changed files

Manual smoke test:

1. Build: npm run build
2. Run gemini sessions --list-sessions to populate
3. Run gemini sessions --delete-session <id> and verify the session file and artifacts are deleted

Pre-Merge Checklist

• Updated relevant documentation and README (no public docs reference the removed helpers)
• Added/updated tests
• Noted breaking changes (none, instance method preserved as wrapper)
• Validated on required platforms/methods:
  • MacOS
    • npm run

[gemini-code-assist]

Summary of Changes

Hello, I'm Gemini Code Assist¹! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request refactors the session deletion mechanism to improve architectural decoupling. By moving the core deletion logic out of the ChatRecordingService class and into a standalone utility function, the CLI can now perform session cleanup operations without the overhead of constructing a full agent service. This change simplifies dependencies for CLI utilities while maintaining backward compatibility for existing consumers of the service.

Highlights

• Decoupling Session Deletion: Extracted session deletion logic from ChatRecordingService into a standalone function, deleteStoredSession, to allow usage without requiring an AgentLoopContext.
• Refactored CLI Utility: Updated the CLI's one-shot delete utility to call the new deleteStoredSession function directly, removing the need to instantiate ChatRecordingService with a full Config object.
• Test Suite Migration: Updated test suites to mock the new standalone function instead of the service class, ensuring cleaner and more focused unit tests.

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature	Command	Description
Code Review	/gemini review	Performs a code review for the current pull request in its current state.
Pull Request Summary	/gemini summary	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help	/gemini help	Displays a list of available commands.

Customization

To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here.

Footnotes

1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

[gemini-code-assist]

Code Review

This pull request refactors session deletion logic by moving it from ChatRecordingService to a centralized utility, deleteStoredSession, in sessionOperations.ts, enabling session deletion without an AgentLoopContext. The CLI and its tests were updated to use this new utility. A review comment highlights that the fallback mechanism for recovering session IDs using JSON.parse on entire files will fail for JSONL formats and suggests using a streaming or line-by-line approach for better performance and compatibility.

[gemini-code-assist]

The fallback mechanism using fs.readFile and JSON.parse on the entire file content will fail for current session files which are in JSONL format (multiple JSON objects separated by newlines). While this was likely intended for legacy single-object JSON files, it may lead to unnecessary memory consumption and silent failures if a large JSONL file's first line cannot be parsed. Ensure that asynchronous file system operations (e.g., fs.promises.readFile) are used to avoid blocking the event loop. Consider using a streaming approach or a line-by-line reader if a full file scan is necessary to recover the session ID.

References

1. Use asynchronous file system operations (e.g., fs.promises.readFile) instead of synchronous ones (e.g., fs.readFileSync) to avoid blocking the event loop.

[yuvrajangadsingh]

Thanks for flagging this. The fallback path here is preserved verbatim from ChatRecordingService.deleteSessionAndArtifacts, which has lived with this behavior since cf8e2e8. This PR is a pure relocation (zero behavior change) so I kept the legacy fallback intact to avoid scope creep.

A few clarifying notes on the existing semantics:

• The fallback only runs when the first-line parse fails, which is unusual for current JSONL session files (the first 4096 bytes normally cover a complete first JSON record).
• The fs.readFile call is already async via fs/promises, not blocking. The fs import in this file resolves to node:fs/promises (see the top of the file), not the sync API.
• The fallback exists to recover the session id from legacy pretty-printed single-object JSON files written before the JSONL switch. For current files this branch is effectively unused.

I agree a streaming fallback (e.g. readline.createInterface) would be better if we want to harden this path, but I'd prefer to land that separately so this PR stays behavior-preserving and easy to verify as a pure move.

[yuvrajangadsingh]

/gemini review

[gemini-code-assist]

Code Review

This pull request refactors session deletion logic by extracting it from ChatRecordingService into a standalone utility, deleteStoredSession, located in sessionOperations.ts. This change allows the CLI to perform session deletions using only a Config object. The ChatRecordingService and CLI utilities have been updated to delegate to this new function, and the test suite has been adjusted to reflect the updated API. Feedback suggests improving code consistency by using shared type-checking helpers and hardening the directory check for the chat storage path to prevent potential runtime errors.

[yuvrajangadsingh]

/gemini review

[gemini-code-assist]

Code Review

This pull request refactors session deletion logic by moving it from the ChatRecordingService to a new standalone utility, deleteStoredSession, in sessionOperations.ts. This allows the CLI to delete sessions using only a configuration object. Common type guards were also extracted into a new typeUtils.ts file. Review feedback identified a potential path traversal vulnerability due to missing input validation in the deletion utility and suggested handling ENOENT errors to reduce log noise during concurrent deletions.

[yuvrajangadsingh]

/gemini review

[gemini-code-assist]

Code Review

This pull request refactors session deletion logic by moving it from ChatRecordingService to a standalone deleteStoredSession utility in sessionOperations.ts, enabling deletion without an AgentLoopContext. It also introduces a shared typeUtils.ts for type guards and implements a security check to protect reserved top-level directories from accidental deletion. Feedback suggests further hardening the session ID validation by explicitly checking for path separators to prevent potential path traversal vulnerabilities.

[yuvrajangadsingh]

/gemini review

[gemini-code-assist]

Code Review

This pull request refactors the session deletion logic by moving core functionality from ChatRecordingService into a standalone utility, deleteStoredSession, within sessionOperations.ts. This change allows the CLI to delete sessions using only a Config object without needing to instantiate a full ChatRecordingService. Additionally, the PR introduces a new typeUtils.ts for shared type guards and implements security enhancements in deleteSessionArtifactsAsync to prevent the accidental deletion of reserved system directories. I have no feedback to provide.

[scidomino]

This is way too generic for such a small change. use zod for type safety if you have to.

[yuvrajangadsingh]

Good call. I dropped the generic typeUtils.ts module entirely. Reverted chatRecordingService.ts to use the inline hasProperty/isStringProperty/isObjectProperty helpers it had before extraction, and inlined the single one-field check in sessionOperations.ts (isSessionIdRecord). Kept it as a plain runtime check since the only accepted shape is { sessionId: string } and pulling zod in just for that felt like more than what the PR is doing.

Pushed in 47c6c59. Type check + tests pass.

[yuvrajangadsingh]

/gemini review

[gemini-code-assist]

Code Review

This pull request refactors the session deletion logic by moving it from the ChatRecordingService class to a standalone utility function, deleteStoredSession, within sessionOperations.ts. This change allows for session deletion without the need to instantiate a full service context, which is now leveraged by the CLI. The update also introduces security improvements, such as preventing the deletion of reserved directories (e.g., 'chats', 'logs', 'tool-outputs') and ensuring session IDs do not contain path separators. Furthermore, the logic now gracefully handles concurrent deletion races by ignoring ENOENT errors during artifact cleanup. I have no feedback to provide as there were no review comments to assess.