fix(acp): resolve session by exact id and handle listener cleanup on session failure - #29580

Code Review

This pull request enhances the Agent Client Protocol (ACP) session management by introducing support for loading empty sessions (sessions without resumable content) and ensuring proper resource cleanup. It adds an allowEmpty option to SessionSelector.resolveSession and implements resolveSessionById to bypass interactive terminal list filtering. Additionally, it wraps session loading and authentication steps in try...catch blocks to guarantee that the session configuration is disposed of on failure, preventing resource and event listener leaks. The review feedback highlights a potential resource leak in prepareSessionConfig where an error during file system service setup could leak the config object before it is returned, and provides a code suggestion to handle this failure safely.

Code Review

This pull request updates the ACP session management to support loading empty sessions and ensures proper cleanup of session configurations by invoking dispose on failure or session termination. It introduces a new resolveSessionById helper to bypass interactive CLI filters when loading a session by its full UUID, along with comprehensive unit tests. The review feedback highlights three key areas for improvement: validating the sessionId parameter to prevent path traversal vulnerabilities, catching potential promise rejections during asynchronous configuration disposal to prevent process crashes, and ensuring numeric indices are not incorrectly routed to resolveSessionById when allowEmpty is enabled.

_{Note: Security Review did not run due to the size of the PR.}

Code Review

This pull request introduces changes to session loading and resolution within the ACP session manager, including path traversal validation for session identifiers, direct session resolution by UUID, and improved resource disposal on initialization failures. The review feedback highlights two key areas for improvement: first, a potential runtime TypeError when calling .catch() on an optional-chained dispose call that may evaluate to undefined; second, an optimization to resolve the session before initializing storage to prevent unnecessary directory creation on disk for invalid session IDs.

Code Review

This pull request enhances the ACP session manager by adding path traversal validation for session identifiers, ensuring proper cleanup of configurations via config.dispose() during initialization or authentication failures, and introducing a resolveSessionById method to support loading empty sessions. Feedback was provided to safely handle the optional sessionData.lastUpdated property using the nullish coalescing operator (??) to prevent potential runtime issues when formatting the session display info.

Code Review

This pull request enhances the Agent Client Protocol (ACP) session management by adding validation to reject invalid session identifiers early, refactoring session loading to handle configuration initialization and disposal robustly, and introducing a method to resolve sessions directly by UUID. Comprehensive unit tests have been added to verify these changes. The review feedback suggests wrapping configuration disposal calls in try/catch blocks across acpSessionManager.ts to prevent disposal errors from masking original authentication or initialization failures. Additionally, it is recommended to handle the optional lastUpdated property with a fallback default in sessionUtils.ts to comply with repository rules and avoid potential runtime issues.

Code Review

This pull request introduces safe disposal of session configurations during session teardown and error handling, and adds support for resolving empty sessions by UUID (bypassing interactive terminal filters) via a new resolveSessionById method. It also adds validation to prevent path traversal via the session identifier. The reviewer recommends strengthening the session identifier validation by using a strict regular expression to prevent other potential vulnerabilities like null byte injection or control characters.

Code Review

This pull request introduces secure session loading and proper resource disposal for the Agent Client Protocol (ACP) session manager. It adds path traversal validation for session identifiers, implements direct session resolution by UUID (bypassing interactive filters when empty sessions are allowed), and ensures that session configurations are properly disposed of during initialization failures or authentication errors. Feedback on the changes highlights a potential resource conflict where an existing session is disposed of only after the new session's configuration is initialized; disposing of the existing session beforehand is recommended to prevent port binding or file lock contention from concurrent MCP servers.

Code Review

This pull request introduces secure session loading and proper configuration disposal in the Agent Client Protocol (ACP) session manager. Key changes include validating session identifiers against path traversal, allowing resolution of empty sessions by UUID, and ensuring config.dispose() is called during session termination or authentication failures. The review feedback highlights two critical issues: first, Storage should be instantiated with the sessionId to ensure correct scoping; second, the initialization sequence in session creation should be wrapped in a try-catch block to prevent resource leaks of partially initialized configurations when subsequent steps fail.

Code Review

This pull request enhances session management in the Gemini CLI by introducing safer configuration disposal during authentication and initialization failures, validating session identifiers to prevent path traversal, and allowing direct session resolution by UUID. While these changes improve robustness and security, the feedback highlights two key areas for improvement: addressing a potential race condition when reloading a session due to synchronous disposal, and wrapping the initialization phase of new sessions in a try-catch block to ensure proper cleanup of active event listeners and subprocesses on failure.

Code Review

This pull request makes the Session.dispose method asynchronous to safely handle configuration disposal, introduces path traversal validation for session identifiers, and adds a resolveSessionById method to bypass interactive terminal list filtering when resolving sessions by UUID. The review feedback highlights critical resource leak issues where zombie sessions and event listeners are left behind if errors occur after session creation in newSession and loadSession. Additionally, the reviewer recommends making the session manager's dispose method asynchronous to properly await the now-asynchronous session disposals and prevent race conditions.

Code Review

This pull request refactors the disposal lifecycle across GeminiAgent, AcpSessionManager, and Session to be asynchronous, ensuring proper cleanup of configurations and sessions. It also introduces robust error handling during session creation and loading to prevent resource leaks, adds path traversal validation for session identifiers, and implements direct session resolution by UUID via resolveSessionById. A review comment correctly points out that the promise returned by session?.sendAvailableCommands() in acpSessionManager.ts is unhandled and suggests adding a .catch block to prevent unhandled rejections.

Code Review

This pull request refactors the session disposal lifecycle across the ACP (Agent Client Protocol) implementation to be asynchronous, ensuring proper cleanup of resources. It updates GeminiAgent, AcpSessionManager, and Session to support async dispose, and improves error handling during session creation and loading by ensuring configs and sessions are disposed of if initialization fails. Additionally, it introduces path traversal validation for session identifiers, adds a resolveSessionById helper to bypass interactive terminal filtering, and includes comprehensive unit tests. The feedback highlights an opportunity to improve reliability by explicitly catching and logging errors on floating promises in loadSession instead of bypassing the linter with comments.

Code Review

This pull request refactors the session disposal mechanism to be asynchronous, ensuring proper cleanup of configurations and sessions, and adds robust error handling and disposal during session creation and loading. It also introduces a path traversal check for session identifiers and allows resolving empty sessions via a new allowEmpty option in SessionSelector. The review feedback recommends moving deferred background tasks (such as sendAvailableCommands and streamHistory) to the very end of the initialization blocks in newSession and loadSession to prevent side-effects from executing if the session setup subsequently fails.

Code Review

This pull request transitions session disposal to be asynchronous across GeminiAgent, Session, and AcpSessionManager to ensure proper resource cleanup, and enhances session loading by allowing direct resolution of sessions by UUID. Feedback on these changes focuses on improving robustness and safety: validating sessionId with a strict regular expression to prevent path traversal bypasses, safely handling the optional messages property on sessionData with fallbacks to avoid runtime errors, and consistently trimming optional timestamp strings when formatting session display information.

Code Review

This pull request transitions the session disposal flow to be asynchronous across the ACP session management classes, adds validation to prevent path traversal via session identifiers, and introduces a mechanism to resolve empty sessions by UUID. The review feedback highlights two critical robustness improvements in AcpSessionManager: wrapping individual session disposals in try-catch blocks within dispose() to prevent a single failure from halting the entire process, and wrapping existingSession.dispose() in a try-catch-finally block during session loading to guarantee stale sessions are always removed from the manager.

_{Note: Security Review did not run due to the size of the PR.}

Code Review

This pull request updates the session disposal lifecycle across GeminiAgent, Session, and AcpSessionManager to be asynchronous, ensuring proper resource cleanup. It enhances error handling during session creation and loading to guarantee that sessions and configurations are safely disposed of if initialization fails. Additionally, it introduces input validation on session identifiers to prevent path traversal vulnerabilities and adds a new resolveSessionById method to bypass interactive terminal filters when resolving sessions by UUID. Comprehensive unit tests have been added to verify these changes. No review comments were provided, so I have no additional feedback to offer.