fix(cli): preserve shell session id for backgrounding#25132
fix(cli): preserve shell session id for backgrounding#25132SH20RAJ wants to merge 4 commits intogoogle-gemini:mainfrom
Conversation
…ogle-gemini#24820) - Import uiTelemetryService and call hydrate when resuming via --resume - This ensures the session ID and metrics are correctly restored, so the quit summary shows the correct resume command Fixes google-gemini#24820
|
You already have 7 pull requests open. Please work on getting existing PRs merged before opening more. |
Summary of ChangesHello, I'm Gemini Code Assist1! 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 addresses a critical issue where interactive shell sessions, when backgrounded, would lose their session context, preventing proper resumption. By ensuring the session ID is consistently captured and preserved within the shell execution configuration and restored upon session resume, the system can now reliably manage and re-attach to backgrounded PTY sessions. This enhances the stability and user experience of the CLI's interactive features. Highlights
Using Gemini Code AssistThe 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
Customization To customize 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 Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. 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
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces session ID tracking within the shell execution lifecycle and implements telemetry hydration when resuming sessions. It also expands the setShellExecutionConfig method to support additional properties such as colors, sandbox configurations, and permissions, accompanied by new unit tests. Feedback was provided regarding the implementation of setShellExecutionConfig, noting that the manual mapping of properties is redundant and brittle, and suggesting a more dynamic approach to merging configuration updates.
Note: Security Review did not run due to the size of the PR.
| this.shellExecutionConfig = { | ||
| ...this.shellExecutionConfig, | ||
| terminalWidth: | ||
| config.terminalWidth ?? this.shellExecutionConfig.terminalWidth, | ||
| terminalHeight: | ||
| config.terminalHeight ?? this.shellExecutionConfig.terminalHeight, | ||
| showColor: config.showColor ?? this.shellExecutionConfig.showColor, | ||
| pager: config.pager ?? this.shellExecutionConfig.pager, | ||
| defaultFg: config.defaultFg ?? this.shellExecutionConfig.defaultFg, | ||
| defaultBg: config.defaultBg ?? this.shellExecutionConfig.defaultBg, | ||
| sanitizationConfig: | ||
| config.sanitizationConfig ?? | ||
| this.shellExecutionConfig.sanitizationConfig, | ||
| sandboxManager: | ||
| config.sandboxManager ?? this.shellExecutionConfig.sandboxManager, | ||
| sandboxConfig: | ||
| config.sandboxConfig ?? this.shellExecutionConfig.sandboxConfig, | ||
| backgroundCompletionBehavior: | ||
| config.backgroundCompletionBehavior ?? | ||
| this.shellExecutionConfig.backgroundCompletionBehavior, | ||
| additionalPermissions: | ||
| config.additionalPermissions ?? | ||
| this.shellExecutionConfig.additionalPermissions, | ||
| disableDynamicLineTrimming: | ||
| config.disableDynamicLineTrimming ?? | ||
| this.shellExecutionConfig.disableDynamicLineTrimming, | ||
| scrollback: config.scrollback ?? this.shellExecutionConfig.scrollback, | ||
| maxSerializedLines: | ||
| config.maxSerializedLines ?? | ||
| this.shellExecutionConfig.maxSerializedLines, | ||
| originalCommand: | ||
| config.originalCommand ?? this.shellExecutionConfig.originalCommand, | ||
| sessionId: config.sessionId ?? this.shellExecutionConfig.sessionId, | ||
| }; |
There was a problem hiding this comment.
The implementation of setShellExecutionConfig is redundant and brittle. The spread operator at the beginning is negated by the explicit assignment of every property in the interface using nullish coalescing. Furthermore, this manual mapping requires updates whenever the ShellExecutionConfig interface changes, which is prone to errors. A more robust approach would be to merge only the defined properties from the input config using a loop or Object.fromEntries.
setShellExecutionConfig(config: ShellExecutionConfig): void {
const updates = Object.fromEntries(
Object.entries(config).filter(([_, v]) => v !== undefined),
);
this.shellExecutionConfig = {
...this.shellExecutionConfig,
...updates,
};
}References
- When constructing an object from multiple data sources, consistently use a merge operation (e.g., spread syntax {...a, ...b}) instead of assignment to avoid accidental overwrites and order-dependent logic.
- Rely on the schema as the single source of truth for configuration defaults, avoiding redundant nullish coalescing operators.
1 participant
Fixes #25111
Summary
Preserve the current session ID in the interactive shell execution config so backgrounding can resolve the active PTY/session later.
Validation
npm test packages/cli/src/ui/hooks/useExecutionLifecycle.test.tsx