refactor(sdk): introduce session-based architecture - #19180
Conversation
This change introduces the GeminiCliSession class to manage conversation state, separating it from the main GeminiCliAgent. This allows for cleaner session management and enables session resumption. BREAKING CHANGE: The `sendStream` method on `GeminiCliAgent` has been removed. Use `agent.session().sendStream()` instead.
|
Hi there! Thank you for your contribution to Gemini CLI. To improve our contribution process and better track changes, we now require all pull requests to be associated with an existing issue, as announced in our recent discussion and as detailed in our CONTRIBUTING.md. This pull request is being closed because it is not currently linked to an issue. Once you have updated the description of this PR to link an issue (e.g., by adding How to link an issue: Thank you for your understanding and for being a part of our community! |
|
Hi @mbleigh, thank you so much for your contribution to Gemini CLI! We really appreciate the time and effort you've put into this. We're making some updates to our contribution process to improve how we track and review changes. Please take a moment to review our recent discussion post: Improving Our Contribution Process & Introducing New Guidelines. Key Update: Starting January 26, 2026, the Gemini CLI project will require all pull requests to be associated with an existing issue. Any pull requests not linked to an issue by that date will be automatically closed. Thank you for your understanding and for being a part of our community! |
Summary of ChangesHello @mbleigh, 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 introduces a significant architectural refactor to the SDK by implementing a session-based approach for managing conversations. The core Highlights
Changelog
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. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request introduces a session-based architecture, separating conversation state from the main GeminiCliAgent into a new GeminiCliSession class, which improves modularity and enables session resumption. A critical security concern has been identified: the default configuration of the policy engine within the new GeminiCliSession class defaults to allowing all tool calls. This could lead to Remote Code Execution (RCE) if an agent is compromised via prompt injection, and it is recommended to change this default to a more secure 'deny-by-default' stance. Additionally, errors in dynamic instruction loading are silently ignored, and there is an inconsistent type definition for SessionContext in the public API.
|
Size Change: +1.69 kB (+0.01%) Total Size: 24.6 MB
ℹ️ View Unchanged
|
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
|
Hi there! Thank you for your contribution to Gemini CLI. To improve our contribution process and better track changes, we now require all pull requests to be associated with an existing issue, as announced in our recent discussion and as detailed in our CONTRIBUTING.md. This pull request is being closed because it is not currently linked to an issue. Once you have updated the description of this PR to link an issue (e.g., by adding How to link an issue: Thank you for your understanding and for being a part of our community! |
| const registry = this.config.getToolRegistry(); | ||
| const messageBus = this.config.getMessageBus(); | ||
| async resumeSession(sessionId?: string): Promise<GeminiCliSession> { | ||
| const cwd = this.options.cwd || process.cwd(); |
There was a problem hiding this comment.
It'd be interesting to try and bind a session to a specific file path. Otherwise resuming from a different one would be a bit odd
There was a problem hiding this comment.
The sessions are essentially bound to a specific file path, because they are loaded from the project temp dir. So it can only resume a session for the same cwd path as implemented now.
| registry.registerTool(sdkTool); | ||
| } | ||
| } | ||
| const chatsDir = path.join(storage.getProjectTempDir(), 'chats'); |
There was a problem hiding this comment.
There should be options for this BUT I'd imagine we'd want to utilize the storage object directly. Does it make sense to surface the normal dir that's used by the CLI?
There was a problem hiding this comment.
Yeah, good callout. Added some new methods to storage to do this more directly.
| // Sort by mtime desc | ||
| fileStats.sort((a, b) => b.mtime - a.mtime); | ||
|
|
||
| let targetFile: { filePath: string } | undefined; |
There was a problem hiding this comment.
Feels odd to surface target file as a json array. After sorting above might just be worth mapping the file paths down to their final file path for readability
| prompt_id: sessionId, | ||
| }); | ||
| } catch (_e) { | ||
| // Ignore parse errors |
There was a problem hiding this comment.
Hmm, feels like a debug log should be here. I'd imagine not finding your chat history would be super frustrating otherwise
There was a problem hiding this comment.
Adding a TODO for this to come back to when I do logging.
| targetFile = fileStats[0]; | ||
| } | ||
|
|
||
| const content = await fs.promises.readFile(targetFile.filePath, 'utf8'); |
There was a problem hiding this comment.
Instead of re-reading the file path (since we already did) it might be worth eagerly returning above instead of breaking
| private readonly tools: Array<Tool<any>>; | ||
| private readonly skillRefs: SkillReference[]; | ||
| private readonly instructions: SystemInstructions | undefined; | ||
| private client: GeminiClient | undefined; |
There was a problem hiding this comment.
What is a session without a client?
There was a problem hiding this comment.
Client is initialized async based on the config, so it can't be required. But it's effectively required.
| this.tools = options.tools || []; | ||
| this.skillRefs = options.skills || []; | ||
|
|
||
| const initialMemory = |
There was a problem hiding this comment.
Should we be throwing if the instructions aren't a string? Feels like falling back to empty string will bite a lot of people
|
|
||
| const loadPromises = this.skillRefs.map(async (ref) => { | ||
| try { | ||
| if (ref.type === 'dir' || ref.type === 'root') { |
There was a problem hiding this comment.
Left a comment in another PR around using "user" and "workspace" to align with existing CLI verbiage
There was a problem hiding this comment.
I actually cleaned this up there but this is a straggler - the type here is not 'user' or 'workspace' since it's allowing passing in arbitrary directories. But the 'dir' and 'root' distinction doesn't matter because the loader will automatically detect and handle nested directories. Once we have in-memory skills we'll add a second, non-"dir" type here.
| } | ||
| } catch (e) { | ||
| // eslint-disable-next-line no-console | ||
| console.error(`Failed to load skills from ${ref.path}:`, e); |
There was a problem hiding this comment.
For SDK's we should probably have a dedicated logger stack instead of relying on console
There was a problem hiding this comment.
Will do this in a followup as it's a bit of a big change to tack in here.
| } | ||
|
|
||
| // Re-register ActivateSkillTool if we have skills | ||
| const skillManager = this.config.getSkillManager(); |
There was a problem hiding this comment.
I'll stop reviewing this bit, had a few other comments in other PRs about surfacing a lot of these bits in the core CLI instead of re-implementing
766b2cf to
2118700
Compare
2118700 to
33e4c12
Compare
|
|
||
| const loadPromises = this.skillRefs.map(async (ref) => { | ||
| try { | ||
| if (ref.type === 'dir' || ref.type === 'root') { |
There was a problem hiding this comment.
I actually cleaned this up there but this is a straggler - the type here is not 'user' or 'workspace' since it's allowing passing in arbitrary directories. But the 'dir' and 'root' distinction doesn't matter because the loader will automatically detect and handle nested directories. Once we have in-memory skills we'll add a second, non-"dir" type here.
| prompt_id: sessionId, | ||
| }); | ||
| } catch (_e) { | ||
| // Ignore parse errors |
There was a problem hiding this comment.
Adding a TODO for this to come back to when I do logging.
| registry.registerTool(sdkTool); | ||
| } | ||
| } | ||
| const chatsDir = path.join(storage.getProjectTempDir(), 'chats'); |
There was a problem hiding this comment.
Yeah, good callout. Added some new methods to storage to do this more directly.
| const registry = this.config.getToolRegistry(); | ||
| const messageBus = this.config.getMessageBus(); | ||
| async resumeSession(sessionId?: string): Promise<GeminiCliSession> { | ||
| const cwd = this.options.cwd || process.cwd(); |
There was a problem hiding this comment.
The sessions are essentially bound to a specific file path, because they are loaded from the project temp dir. So it can only resume a session for the same cwd path as implemented now.
| private readonly tools: Array<Tool<any>>; | ||
| private readonly skillRefs: SkillReference[]; | ||
| private readonly instructions: SystemInstructions | undefined; | ||
| private client: GeminiClient | undefined; |
There was a problem hiding this comment.
Client is initialized async based on the config, so it can't be required. But it's effectively required.
| return await loadSkillsFromDir(ref.path); | ||
| } | ||
| } catch (e) { | ||
| // eslint-disable-next-line no-console |
There was a problem hiding this comment.
| // eslint-disable-next-line no-console | |
| // TODO: refactor this to use a proper logger interface | |
| // eslint-disable-next-line no-console |
| } | ||
| } catch (e) { | ||
| // eslint-disable-next-line no-console | ||
| console.error(`Failed to load skills from ${ref.path}:`, e); |
There was a problem hiding this comment.
Will do this in a followup as it's a bit of a big change to tack in here.
|
Hi there! Thank you for your contribution to Gemini CLI. To improve our contribution process and better track changes, we now require all pull requests to be associated with an existing issue, as announced in our recent discussion and as detailed in our CONTRIBUTING.md. This pull request is being closed because it is not currently linked to an issue. Once you have updated the description of this PR to link an issue (e.g., by adding How to link an issue: Thank you for your understanding and for being a part of our community! |
…mini-cli into sdk-05-refactor-session
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
This change introduces the GeminiCliSession class to manage conversation state, separating it from the main GeminiCliAgent. This allows for cleaner session management and enables session resumption.
BREAKING CHANGE: The
sendStreammethod onGeminiCliAgenthas been removed. Useagent.session().sendStream()instead.