-
-
Notifications
You must be signed in to change notification settings - Fork 417
feat(gemini): support --resume flag and fix stuck session on abort #262
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Closed
Closed
Changes from all commits
Commits
Show all changes
18 commits
Select commit
Hold shift + click to select a range
4c8fa7f
feat(gemini): support --resume flag to restore previous gemini session
junmo-kim 851c535
fix(gemini): use session/load ACP call when resuming existing session
junmo-kim fa31111
tidy(gemini): extract findGeminiTranscriptPath and readGeminiTranscri…
junmo-kim 10d69bc
test(gemini): add tests for findGeminiTranscriptPath and readGeminiTr…
junmo-kim 8421d57
feat(gemini): replay historical messages in UI when resuming a session
junmo-kim 89cbde9
fix(gemini): send historical messages to web client on session resume
junmo-kim c18e74f
fix(gemini): fall back to new session when loadSession fails on resume
junmo-kim c3974f5
fix(gemini): handle structured content in user transcript messages
junmo-kim e58d1fb
fix(gemini): prevent duplicate history replay on local-to-remote mode…
junmo-kim 37a6077
fix(gemini): remove hub message replay on session resume
junmo-kim f055e60
fix(gemini): abort stuck ACP prompt via AbortSignal on session cancel
junmo-kim 82db71a
fix(gemini): send historical messages to web UI on remote resume
junmo-kim f377aa1
fix(acp): fix TypeScript error in AcpStdioTransport sendRequest
junmo-kim 6ee6eee
fix(gemini): suppress false 'prompt failed' error on user abort
junmo-kim f3264ad
fix(acp): address bot review comments
junmo-kim 8901487
test: add TDD tests for abort cleanup and history replay fixes
junmo-kim e02a7da
fix(gemini): fix local→remote history gap and array content handling
junmo-kim 1061603
fix(gemini): prevent duplicate history replay on remote→local→remote …
junmo-kim File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Oops, something went wrong.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,91 @@ | ||
| import { describe, expect, it, vi, beforeEach } from 'vitest'; | ||
| import { AcpStdioTransport } from './AcpStdioTransport'; | ||
| import * as childProcess from 'node:child_process'; | ||
| import { EventEmitter } from 'node:events'; | ||
|
|
||
| vi.mock('node:child_process'); | ||
|
|
||
| function makeFakeProcess() { | ||
| const stdin = { write: vi.fn(), end: vi.fn() }; | ||
| const stdout = new EventEmitter() as EventEmitter & { setEncoding: (enc: string) => void }; | ||
| stdout.setEncoding = vi.fn(); | ||
| const stderr = new EventEmitter() as EventEmitter & { setEncoding: (enc: string) => void }; | ||
| stderr.setEncoding = vi.fn(); | ||
| const proc = new EventEmitter() as EventEmitter & { | ||
| stdin: typeof stdin; | ||
| stdout: typeof stdout; | ||
| stderr: typeof stderr; | ||
| pid: number; | ||
| }; | ||
| proc.stdin = stdin; | ||
| proc.stdout = stdout; | ||
| proc.stderr = stderr; | ||
| proc.pid = 12345; | ||
| return proc; | ||
| } | ||
|
|
||
| describe('AcpStdioTransport.sendRequest', () => { | ||
| beforeEach(() => { | ||
| vi.clearAllMocks(); | ||
| }); | ||
|
|
||
| it('rejects immediately when signal is already aborted', async () => { | ||
| const fakeProc = makeFakeProcess(); | ||
| vi.mocked(childProcess.spawn).mockReturnValue(fakeProc as unknown as ReturnType<typeof childProcess.spawn>); | ||
|
|
||
| const transport = new AcpStdioTransport({ command: 'gemini' }); | ||
| const controller = new AbortController(); | ||
| controller.abort(); | ||
|
|
||
| await expect( | ||
| transport.sendRequest('session/prompt', {}, { timeoutMs: Infinity, signal: controller.signal }) | ||
| ).rejects.toMatchObject({ name: 'AbortError' }); | ||
| }); | ||
|
|
||
| it('clears the timeout timer when signal is already aborted', async () => { | ||
| const clearTimeoutSpy = vi.spyOn(globalThis, 'clearTimeout'); | ||
|
|
||
| const fakeProc = makeFakeProcess(); | ||
| vi.mocked(childProcess.spawn).mockReturnValue(fakeProc as unknown as ReturnType<typeof childProcess.spawn>); | ||
|
|
||
| const transport = new AcpStdioTransport({ command: 'gemini' }); | ||
| const controller = new AbortController(); | ||
| controller.abort(); | ||
|
|
||
| await expect( | ||
| transport.sendRequest('session/prompt', {}, { timeoutMs: 1000, signal: controller.signal }) | ||
| ).rejects.toMatchObject({ name: 'AbortError' }); | ||
|
|
||
| expect(clearTimeoutSpy).toHaveBeenCalled(); | ||
| clearTimeoutSpy.mockRestore(); | ||
| }); | ||
|
|
||
| it('rejects when signal fires after request is sent', async () => { | ||
| const fakeProc = makeFakeProcess(); | ||
| vi.mocked(childProcess.spawn).mockReturnValue(fakeProc as unknown as ReturnType<typeof childProcess.spawn>); | ||
|
|
||
| const transport = new AcpStdioTransport({ command: 'gemini' }); | ||
| const controller = new AbortController(); | ||
|
|
||
| const requestPromise = transport.sendRequest('session/prompt', {}, { timeoutMs: Infinity, signal: controller.signal }); | ||
|
|
||
| controller.abort(); | ||
|
|
||
| await expect(requestPromise).rejects.toMatchObject({ name: 'AbortError' }); | ||
| }); | ||
|
|
||
| it('resolves normally when response arrives before abort', async () => { | ||
| const fakeProc = makeFakeProcess(); | ||
| vi.mocked(childProcess.spawn).mockReturnValue(fakeProc as unknown as ReturnType<typeof childProcess.spawn>); | ||
|
|
||
| const transport = new AcpStdioTransport({ command: 'gemini' }); | ||
| const controller = new AbortController(); | ||
|
|
||
| const requestPromise = transport.sendRequest('session/prompt', {}, { timeoutMs: Infinity, signal: controller.signal }); | ||
|
|
||
| // Simulate Gemini CLI responding with id=1 | ||
| fakeProc.stdout.emit('data', JSON.stringify({ jsonrpc: '2.0', id: 1, result: { stopReason: 'end_turn' } }) + '\n'); | ||
|
|
||
| await expect(requestPromise).resolves.toEqual({ stopReason: 'end_turn' }); | ||
| }); | ||
| }); |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,115 @@ | ||
| import { describe, it, expect, vi, afterEach } from 'vitest'; | ||
| import { geminiLocalLauncher } from './geminiLocalLauncher'; | ||
|
|
||
| vi.mock('./geminiLocal', () => ({ geminiLocal: vi.fn().mockResolvedValue(undefined) })); | ||
|
|
||
| vi.mock('./utils/sessionScanner', async (importOriginal) => { | ||
| const actual = await importOriginal<typeof import('./utils/sessionScanner')>(); | ||
| return { | ||
| ...actual, | ||
| readGeminiTranscript: vi.fn(), | ||
| createGeminiSessionScanner: vi.fn().mockResolvedValue({ | ||
| cleanup: vi.fn().mockResolvedValue(undefined), | ||
| onNewSession: vi.fn(), | ||
| }), | ||
| }; | ||
| }); | ||
|
|
||
| vi.mock('@/modules/common/launcher/BaseLocalLauncher', () => ({ | ||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
| BaseLocalLauncher: vi.fn().mockImplementation(function(this: any) { | ||
| this.run = vi.fn().mockResolvedValue('exit'); | ||
| }), | ||
| })); | ||
|
|
||
| afterEach(() => { | ||
| vi.clearAllMocks(); | ||
| }); | ||
|
|
||
| function makeMockSession(opts: { | ||
| startingMode: 'local' | 'remote'; | ||
| transcriptPath?: string; | ||
| historyReplayed?: boolean; | ||
| }) { | ||
| return { | ||
| path: '/test/path', | ||
| logPath: '/test/log', | ||
| sessionId: null, | ||
| transcriptPath: opts.transcriptPath ?? null, | ||
| historyReplayed: opts.historyReplayed ?? false, | ||
| historyReplayCutoff: 0, | ||
| startedBy: 'runner' as const, | ||
| startingMode: opts.startingMode, | ||
| queue: { waitForMessagesAndGetAsString: vi.fn(), size: vi.fn(), reset: vi.fn() }, | ||
| client: { rpcHandlerManager: { registerHandler: vi.fn() } }, | ||
| sendSessionEvent: vi.fn(), | ||
| sendUserMessage: vi.fn(), | ||
| sendCodexMessage: vi.fn(), | ||
| onSessionFound: vi.fn(), | ||
| onThinkingChange: vi.fn(), | ||
| getPermissionMode: vi.fn().mockReturnValue('auto'), | ||
| addTranscriptPathCallback: vi.fn(), | ||
| removeTranscriptPathCallback: vi.fn(), | ||
| recordLocalLaunchFailure: vi.fn(), | ||
| }; | ||
| } | ||
|
|
||
| describe('geminiLocalLauncher', () => { | ||
| describe('historyReplayCutoff on ensureScanner', () => { | ||
| it('sets historyReplayed=true and historyReplayCutoff=0 when startingMode is remote (regardless of existing messages)', async () => { | ||
| const { readGeminiTranscript } = await import('./utils/sessionScanner'); | ||
| vi.mocked(readGeminiTranscript).mockResolvedValue({ | ||
| messages: [ | ||
| { type: 'user', content: 'msg1' }, | ||
| { type: 'gemini', content: 'reply1' }, | ||
| ] | ||
| }); | ||
|
|
||
| const session = makeMockSession({ startingMode: 'remote', transcriptPath: '/some/path.json' }); | ||
| await geminiLocalLauncher(session as never, {}); | ||
|
|
||
| expect(session.historyReplayed).toBe(true); | ||
| expect(session.historyReplayCutoff).toBe(0); | ||
| }); | ||
|
|
||
| it('sets historyReplayCutoff to existing count when startingMode is local and messages exist', async () => { | ||
| const { readGeminiTranscript } = await import('./utils/sessionScanner'); | ||
| vi.mocked(readGeminiTranscript).mockResolvedValue({ | ||
| messages: [ | ||
| { type: 'user', content: 'msg1' }, | ||
| { type: 'gemini', content: 'reply1' }, | ||
| { type: 'user', content: 'msg2' }, | ||
| ] | ||
| }); | ||
|
|
||
| const session = makeMockSession({ startingMode: 'local', transcriptPath: '/some/path.json' }); | ||
| await geminiLocalLauncher(session as never, {}); | ||
|
|
||
| expect(session.historyReplayCutoff).toBe(3); | ||
| expect(session.historyReplayed).toBe(false); | ||
| }); | ||
|
|
||
| it('does not overwrite historyReplayed or historyReplayCutoff if already replayed', async () => { | ||
| const { readGeminiTranscript } = await import('./utils/sessionScanner'); | ||
| vi.mocked(readGeminiTranscript).mockResolvedValue({ | ||
| messages: [ | ||
| { type: 'user', content: 'msg1' }, | ||
| { type: 'user', content: 'msg2' }, | ||
| { type: 'user', content: 'msg3' }, | ||
| ] | ||
| }); | ||
|
|
||
| const session = makeMockSession({ | ||
| startingMode: 'local', | ||
| transcriptPath: '/some/path.json', | ||
| historyReplayed: true, | ||
| }); | ||
| session.historyReplayCutoff = 0; | ||
| await geminiLocalLauncher(session as never, {}); | ||
|
|
||
| // historyReplayed was already true; should not be reset by scanner | ||
| expect(session.historyReplayed).toBe(true); | ||
| expect(session.historyReplayCutoff).toBe(0); | ||
| }); | ||
| }); | ||
| }); |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[MINOR]
signal.abortedcheck after timer setup leaves a pending timerIf the signal is already aborted, the timeout timer is still scheduled and left to fire later. Move the aborted check before creating the timer, or clear the timer before returning.
Suggested fix:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in commit f3264ad — cleanup() is now called before rejecting on a pre-aborted signal, which clears the pending timer.