From e19cfa22c7378434c3685165428c75b23d378aef Mon Sep 17 00:00:00 2001 From: sogoiii Date: Sat, 3 Jan 2026 17:39:31 -0800 Subject: [PATCH 1/3] feat: implement --continue flag for session resumption Add persistent session storage to resume previous conversations. Sessions are stored per project with: - Full conversation history (IContent[] format for API context) - UI display state (HistoryItem[] for exact visual restoration) - Project hash validation to prevent cross-project session loading - Automatic save on turn completion, automatic restore on --continue flag New SessionPersistenceService handles persistence logic. Sessions saved to: ~/.llxprt/tmp/{project-hash}/chats/persisted-session-{timestamp}.json Closes vybestack#147 --- .../cli/src/config/config.loadMemory.test.ts | 1 + packages/cli/src/config/config.ts | 9 + packages/cli/src/gemini.tsx | 22 ++ packages/cli/src/ui/App.tsx | 3 +- packages/cli/src/ui/AppContainer.tsx | 244 +++++++++++++++++ packages/core/src/config/config.ts | 7 + packages/core/src/index.ts | 5 + .../src/storage/SessionPersistenceService.ts | 246 ++++++++++++++++++ 8 files changed, 536 insertions(+), 1 deletion(-) create mode 100644 packages/core/src/storage/SessionPersistenceService.ts diff --git a/packages/cli/src/config/config.loadMemory.test.ts b/packages/cli/src/config/config.loadMemory.test.ts index 22d48fc7f..60f6e30b7 100644 --- a/packages/cli/src/config/config.loadMemory.test.ts +++ b/packages/cli/src/config/config.loadMemory.test.ts @@ -325,6 +325,7 @@ describe('loadCliConfig memory discovery', () => { promptWords: [], set: undefined, query: undefined, + continue: undefined, }; const { ExtensionEnablementManager, ExtensionStorage } = diff --git a/packages/cli/src/config/config.ts b/packages/cli/src/config/config.ts index aa4a2287e..82b377124 100755 --- a/packages/cli/src/config/config.ts +++ b/packages/cli/src/config/config.ts @@ -167,6 +167,7 @@ export interface CliArgs { promptWords: string[] | undefined; query: string | undefined; set: string[] | undefined; + continue: boolean | undefined; } export async function parseArguments(settings: Settings): Promise { @@ -354,6 +355,12 @@ export async function parseArguments(settings: Settings): Promise { description: 'Dump request body to ~/.llxprt/dumps/ on API errors.', default: false, }) + .option('continue', { + alias: 'C', + type: 'boolean', + description: 'Resume the most recent session for this project', + default: false, + }) .deprecateOption( 'telemetry', 'Use settings.json instead. This flag will be removed in a future version.', @@ -640,6 +647,7 @@ export async function parseArguments(settings: Settings): Promise { promptWords: result.promptWords as string[] | undefined, query: queryFromPromptWords, set: result.set as string[] | undefined, + continue: result.continue as boolean | undefined, }; return cliArgs; @@ -1317,6 +1325,7 @@ export async function loadCliConfig( shouldUseNodePtyShell: effectiveSettings.shouldUseNodePtyShell, enablePromptCompletion: effectiveSettings.enablePromptCompletion ?? false, eventEmitter: appEvents, + continueSession: argv.continue ?? false, }); const enhancedConfig = config; diff --git a/packages/cli/src/gemini.tsx b/packages/cli/src/gemini.tsx index b32ab3a7d..86182f74a 100644 --- a/packages/cli/src/gemini.tsx +++ b/packages/cli/src/gemini.tsx @@ -74,6 +74,8 @@ import { SettingsService, DebugLogger, ProfileManager, + SessionPersistenceService, + type PersistedSession, } from '@vybestack/llxprt-code-core'; import { themeManager } from './ui/themes/theme-manager.js'; import { getStartupWarnings } from './utils/startupWarnings.js'; @@ -218,6 +220,25 @@ export async function startInteractiveUI( workspaceRoot: string, ) { const version = await getCliVersion(); + + // Load previous session if --continue flag was used + let restoredSession: PersistedSession | null = null; + if (config.isContinueSession()) { + const persistence = new SessionPersistenceService( + config.storage, + config.getSessionId(), + ); + restoredSession = await persistence.loadMostRecent(); + + if (restoredSession) { + const formattedTime = + SessionPersistenceService.formatSessionTime(restoredSession); + console.log(chalk.green(`Resumed session from ${formattedTime}`)); + } else { + console.log(chalk.yellow('No previous session found. Starting fresh.')); + } + } + // Detect and enable Kitty keyboard protocol once at startup await detectAndEnableKittyProtocol(); setWindowTitle(basename(workspaceRoot), settings); @@ -257,6 +278,7 @@ export async function startInteractiveUI( settings={settings} startupWarnings={startupWarnings} version={version} + restoredSession={restoredSession ?? undefined} /> diff --git a/packages/cli/src/ui/App.tsx b/packages/cli/src/ui/App.tsx index bab788228..17cae63b6 100644 --- a/packages/cli/src/ui/App.tsx +++ b/packages/cli/src/ui/App.tsx @@ -5,7 +5,7 @@ */ import { useReducer } from 'react'; -import type { Config } from '@vybestack/llxprt-code-core'; +import type { Config, PersistedSession } from '@vybestack/llxprt-code-core'; import type { LoadedSettings } from '../config/settings.js'; import { KeypressProvider } from './contexts/KeypressContext.js'; import { MouseProvider } from './contexts/MouseContext.js'; @@ -28,6 +28,7 @@ interface AppProps { settings: LoadedSettings; startupWarnings?: string[]; version: string; + restoredSession?: PersistedSession; } /** diff --git a/packages/cli/src/ui/AppContainer.tsx b/packages/cli/src/ui/AppContainer.tsx index 1f99d8815..01de2e838 100644 --- a/packages/cli/src/ui/AppContainer.tsx +++ b/packages/cli/src/ui/AppContainer.tsx @@ -19,6 +19,7 @@ import { ToolCallStatus, type HistoryItemWithoutId, type HistoryItem, + type IndividualToolCallDisplay, } from './types.js'; import { useTerminalSize } from './hooks/useTerminalSize.js'; import { useResponsive } from './hooks/useResponsive.js'; @@ -64,6 +65,11 @@ import { getSettingsService, DebugLogger, uiTelemetryService, + SessionPersistenceService, + type PersistedSession, + type IContent, + type ToolCallBlock, + type ToolResponseBlock, } from '@vybestack/llxprt-code-core'; import { IdeIntegrationNudgeResult } from './IdeIntegrationNudge.js'; import { validateAuthMethod } from '../config/auth.js'; @@ -131,6 +137,7 @@ interface AppContainerProps { version: string; appState: AppState; appDispatch: React.Dispatch; + restoredSession?: PersistedSession; } function isToolExecuting(pendingHistoryItems: HistoryItemWithoutId[]) { @@ -152,6 +159,7 @@ export const AppContainer = (props: AppContainerProps) => { startupWarnings = [], appState, appDispatch, + restoredSession, } = props; const runtime = useRuntimeApi(); const isFocused = useFocus(); @@ -346,6 +354,194 @@ export const AppContainer = (props: AppContainerProps) => { lastPublishedHistoryTokensRef.current = null; }; }, [config, updateHistoryTokenCount, tokenLogger]); + + // Convert IContent[] to UI HistoryItem[] for display + const convertToUIHistory = useCallback( + (history: IContent[]): HistoryItem[] => { + const items: HistoryItem[] = []; + let id = 1; + + // First pass: collect all tool responses by callId for lookup + const toolResponseMap = new Map(); + for (const content of history) { + if (content.speaker === 'tool') { + const responseBlocks = content.blocks.filter( + (b): b is ToolResponseBlock => b.type === 'tool_response', + ); + for (const resp of responseBlocks) { + toolResponseMap.set(resp.callId, resp); + } + } + } + + for (const content of history) { + // Extract text blocks + const textBlocks = content.blocks.filter( + (b): b is { type: 'text'; text: string } => b.type === 'text', + ); + const text = textBlocks.map((b) => b.text).join('\n'); + + // Extract tool call blocks for AI + const toolCallBlocks = content.blocks.filter( + (b): b is ToolCallBlock => b.type === 'tool_call', + ); + + if (content.speaker === 'human' && text) { + items.push({ + id: id++, + type: 'user', + text, + } as HistoryItem); + } else if (content.speaker === 'ai') { + // Add text response if present + if (text) { + items.push({ + id: id++, + type: 'gemini', + text, + model: content.metadata?.model, + } as HistoryItem); + } + // Add tool calls as proper tool_group items + if (toolCallBlocks.length > 0) { + const tools: IndividualToolCallDisplay[] = toolCallBlocks.map( + (tc) => { + const response = toolResponseMap.get(tc.id); + // Format result display from tool response + let resultDisplay: string | undefined; + if (response) { + if (response.error) { + resultDisplay = `Error: ${response.error}`; + } else if (response.result !== undefined) { + // Convert result to string for display + const result = response.result as Record; + if (typeof result === 'string') { + resultDisplay = result; + } else if (result && typeof result === 'object') { + // Handle common result formats + if ( + 'output' in result && + typeof result.output === 'string' + ) { + resultDisplay = result.output; + } else { + resultDisplay = JSON.stringify(result, null, 2); + } + } + } + } + return { + callId: tc.id, + name: tc.name, + description: tc.description || '', + resultDisplay, + status: response + ? ToolCallStatus.Success + : ToolCallStatus.Pending, + confirmationDetails: undefined, + }; + }, + ); + items.push({ + id: id++, + type: 'tool_group', + agentId: 'primary', + tools, + } as HistoryItem); + } + } + // Skip tool speaker entries - already processed via map + } + + return items; + }, + [], + ); + + // Session restoration for --continue functionality + // Split into two parts: UI restoration (immediate) and core history (when available) + const sessionRestoredRef = useRef(false); + const coreHistoryRestoredRef = useRef(false); + + // Part 1: Restore UI history immediately on mount + useEffect(() => { + if (!restoredSession || sessionRestoredRef.current) { + return; + } + sessionRestoredRef.current = true; + + try { + // Use saved UI history if available (preserves exact display), otherwise convert from core history + let uiHistoryItems: HistoryItem[]; + if ( + restoredSession.uiHistory && + Array.isArray(restoredSession.uiHistory) + ) { + uiHistoryItems = restoredSession.uiHistory as HistoryItem[]; + debug.log(`Using saved UI history (${uiHistoryItems.length} items)`); + } else { + uiHistoryItems = convertToUIHistory(restoredSession.history); + debug.log( + `Converted core history to UI (${uiHistoryItems.length} items)`, + ); + } + loadHistory(uiHistoryItems); + + debug.log( + `Restored ${restoredSession.history.length} messages (${uiHistoryItems.length} UI items) for display`, + ); + + // Add an info message + addItem( + { + type: 'info', + text: `Session restored (${uiHistoryItems.length} messages from ${new Date(restoredSession.updatedAt || restoredSession.createdAt).toLocaleString()})`, + }, + Date.now(), + ); + } catch (err) { + debug.error('Failed to restore UI history:', err); + addItem( + { + type: 'warning', + text: 'Failed to restore previous session display.', + }, + Date.now(), + ); + } + }, [restoredSession, convertToUIHistory, loadHistory, addItem]); + + // Part 2: Restore core history when historyService becomes available (for AI context) + useEffect(() => { + if (!restoredSession || coreHistoryRestoredRef.current) { + return; + } + + const checkInterval = setInterval(() => { + const geminiClient = config.getGeminiClient(); + const historyService = geminiClient?.getHistoryService?.(); + + if (!historyService) { + return; + } + + clearInterval(checkInterval); + coreHistoryRestoredRef.current = true; + + try { + historyService.validateAndFix(); + historyService.addAll(restoredSession.history); + debug.log('Restored core history for AI context'); + } catch (err) { + debug.error('Failed to restore core history:', err); + } + }, 100); + + return () => { + clearInterval(checkInterval); + }; + }, [restoredSession, config]); + const [_staticNeedsRefresh, setStaticNeedsRefresh] = useState(false); const [staticKey, setStaticKey] = useState(0); const externalEditorStateRef = useRef<{ @@ -1451,6 +1647,54 @@ export const AppContainer = (props: AppContainerProps) => { } }, [streamingState, refreshStatic, _staticNeedsRefresh]); + // Session persistence - always save so sessions can be resumed with --continue + const sessionPersistence = useMemo( + () => new SessionPersistenceService(config.storage, config.getSessionId()), + [config], + ); + + // Track previous streaming state to detect turn completion + const prevStreamingStateRef = useRef(streamingState); + + // Save session when turn completes (streaming goes idle) + useEffect(() => { + const wasActive = + prevStreamingStateRef.current === StreamingState.Responding || + prevStreamingStateRef.current === StreamingState.WaitingForConfirmation; + const isNowIdle = streamingState === StreamingState.Idle; + prevStreamingStateRef.current = streamingState; + + if (!wasActive || !isNowIdle) { + return; + } + + // Get history from gemini client and save + const geminiClient = config.getGeminiClient(); + const historyService = geminiClient?.getHistoryService?.(); + if (!historyService) { + return; + } + + const historyToSave = historyService.getComprehensive(); + if (historyToSave.length === 0) { + return; + } + + sessionPersistence + .save( + historyToSave, + { + provider: config.getProvider?.() ?? undefined, + model: config.getModel(), + tokenCount: historyService.getTotalTokens(), + }, + history, // Save UI history for exact display restoration + ) + .catch((err: unknown) => { + debug.error('Failed to save session:', err); + }); + }, [streamingState, sessionPersistence, config, history]); + const filteredConsoleMessages = useMemo(() => { if (config.getDebugMode()) { return consoleMessages; diff --git a/packages/core/src/config/config.ts b/packages/core/src/config/config.ts index bdf0b4dc6..d80d19d5f 100644 --- a/packages/core/src/config/config.ts +++ b/packages/core/src/config/config.ts @@ -359,6 +359,7 @@ export interface ConfigParameters { enableToolOutputTruncation?: boolean; continueOnFailedApiCall?: boolean; enableShellOutputEfficiency?: boolean; + continueSession?: boolean; } export class Config { @@ -519,6 +520,7 @@ export class Config { enableToolOutputTruncation: boolean; private readonly continueOnFailedApiCall: boolean; private readonly enableShellOutputEfficiency: boolean; + private readonly continueSession: boolean; constructor(params: ConfigParameters) { const providedSettingsService = params.settingsService; @@ -656,6 +658,7 @@ export class Config { this.continueOnFailedApiCall = params.continueOnFailedApiCall ?? true; this.enableShellOutputEfficiency = params.enableShellOutputEfficiency ?? true; + this.continueSession = params.continueSession ?? false; this.extensionManagement = params.extensionManagement ?? false; this.storage = new Storage(this.targetDir); this.enablePromptCompletion = params.enablePromptCompletion ?? false; @@ -856,6 +859,10 @@ export class Config { return this.sessionId; } + isContinueSession(): boolean { + return this.continueSession; + } + shouldLoadMemoryFromIncludeDirectories(): boolean { return this.loadMemoryFromIncludeDirectories; } diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index 0f7e7e48a..ca2d26802 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -353,3 +353,8 @@ export { SESSION_FILE_PREFIX, type ConversationRecord, } from './storage/sessionTypes.js'; + +export { + SessionPersistenceService, + type PersistedSession, +} from './storage/SessionPersistenceService.js'; diff --git a/packages/core/src/storage/SessionPersistenceService.ts b/packages/core/src/storage/SessionPersistenceService.ts new file mode 100644 index 000000000..3d0ad5abb --- /dev/null +++ b/packages/core/src/storage/SessionPersistenceService.ts @@ -0,0 +1,246 @@ +/** + * @license + * Copyright 2025 Vybestack LLC + * SPDX-License-Identifier: Apache-2.0 + */ + +import * as fs from 'node:fs'; +import * as path from 'node:path'; +import * as crypto from 'node:crypto'; +import { type IContent } from '../services/history/IContent.js'; +import { Storage } from '../config/storage.js'; +import { DebugLogger } from '../debug/index.js'; + +const logger = new DebugLogger('llxprt:session:persistence'); + +/** + * Persisted session format for --continue functionality + */ +export interface PersistedSession { + /** Schema version for future migrations */ + version: 1; + /** Unique session identifier */ + sessionId: string; + /** Hash of project root for validation */ + projectHash: string; + /** When session was created */ + createdAt: string; + /** Last update timestamp */ + updatedAt: string; + /** Full conversation history (core format) */ + history: IContent[]; + /** UI history items for display restoration (preserves exactly what user sees) */ + uiHistory?: unknown[]; + /** Optional metadata */ + metadata?: { + provider?: string; + model?: string; + tokenCount?: number; + }; +} + +/** + * Session file prefix for persistence files + */ +const PERSISTED_SESSION_PREFIX = 'persisted-session-'; + +/** + * Service for persisting and restoring conversation sessions. + * Enables the --continue flag to resume previous sessions. + */ +export class SessionPersistenceService { + private readonly storage: Storage; + private readonly sessionId: string; + private readonly chatsDir: string; + private readonly sessionFilePath: string; + + constructor(storage: Storage, sessionId: string) { + this.storage = storage; + this.sessionId = sessionId; + this.chatsDir = path.join(storage.getProjectTempDir(), 'chats'); + + // Use timestamp-based filename for easy "most recent" lookup + const timestamp = new Date().toISOString().replace(/[:.]/g, '-'); + this.sessionFilePath = path.join( + this.chatsDir, + `${PERSISTED_SESSION_PREFIX}${timestamp}.json`, + ); + } + + /** + * Get the directory containing persisted sessions + */ + getChatsDir(): string { + return this.chatsDir; + } + + /** + * Get the current session's file path + */ + getSessionFilePath(): string { + return this.sessionFilePath; + } + + /** + * Save conversation history to disk + */ + async save( + history: IContent[], + metadata?: PersistedSession['metadata'], + uiHistory?: unknown[], + ): Promise { + try { + // Ensure chats directory exists + await fs.promises.mkdir(this.chatsDir, { recursive: true }); + + const session: PersistedSession = { + version: 1, + sessionId: this.sessionId, + projectHash: this.getProjectHash(), + createdAt: this.getCreatedAt(), + updatedAt: new Date().toISOString(), + history, + uiHistory, + metadata, + }; + + // Write to temp file first, then rename for atomic write + const tempPath = `${this.sessionFilePath}.tmp`; + await fs.promises.writeFile( + tempPath, + JSON.stringify(session, null, 2), + 'utf-8', + ); + await fs.promises.rename(tempPath, this.sessionFilePath); + + logger.debug('Session saved:', { + path: this.sessionFilePath, + historyLength: history.length, + metadata, + }); + } catch (error) { + logger.error('Failed to save session:', error); + throw error; + } + } + + /** + * Load the most recent session for this project + */ + async loadMostRecent(): Promise { + try { + // Check if chats directory exists + if (!fs.existsSync(this.chatsDir)) { + logger.debug('No chats directory found'); + return null; + } + + // Find all persisted session files + const files = await fs.promises.readdir(this.chatsDir); + const sessionFiles = files + .filter( + (f) => f.startsWith(PERSISTED_SESSION_PREFIX) && f.endsWith('.json'), + ) + .sort() + .reverse(); // Most recent first (timestamp-based naming) + + if (sessionFiles.length === 0) { + logger.debug('No persisted sessions found'); + return null; + } + + const mostRecentFile = sessionFiles[0]; + const filePath = path.join(this.chatsDir, mostRecentFile); + + logger.debug('Loading most recent session:', filePath); + + const content = await fs.promises.readFile(filePath, 'utf-8'); + const session = JSON.parse(content) as PersistedSession; + + // Validate project hash matches + const currentProjectHash = this.getProjectHash(); + if (session.projectHash !== currentProjectHash) { + logger.warn('Session project hash mismatch, skipping:', { + expected: currentProjectHash, + found: session.projectHash, + }); + return null; + } + + // Validate version + if (session.version !== 1) { + logger.warn('Unknown session version:', session.version); + return null; + } + + logger.debug('Session loaded:', { + sessionId: session.sessionId, + historyLength: session.history.length, + createdAt: session.createdAt, + updatedAt: session.updatedAt, + }); + + return session; + } catch (error) { + logger.error('Failed to load session:', error); + + // If file is corrupted, back it up and return null + if (error instanceof SyntaxError) { + await this.backupCorruptedSession(); + } + + return null; + } + } + + /** + * Get formatted timestamp for display + */ + static formatSessionTime(session: PersistedSession): string { + const date = new Date(session.updatedAt || session.createdAt); + return date.toLocaleString(); + } + + /** + * Get project hash for validation + */ + private getProjectHash(): string { + const projectRoot = this.storage.getProjectRoot(); + return crypto.createHash('sha256').update(projectRoot).digest('hex'); + } + + /** + * Get or track session creation time + */ + private createdAt: string | null = null; + private getCreatedAt(): string { + if (!this.createdAt) { + this.createdAt = new Date().toISOString(); + } + return this.createdAt; + } + + /** + * Back up corrupted session file + */ + private async backupCorruptedSession(): Promise { + try { + const files = await fs.promises.readdir(this.chatsDir); + const sessionFiles = files + .filter( + (f) => f.startsWith(PERSISTED_SESSION_PREFIX) && f.endsWith('.json'), + ) + .sort() + .reverse(); + + if (sessionFiles.length > 0) { + const corruptedFile = path.join(this.chatsDir, sessionFiles[0]); + const backupFile = `${corruptedFile}.corrupted-${Date.now()}`; + await fs.promises.rename(corruptedFile, backupFile); + logger.warn('Backed up corrupted session to:', backupFile); + } + } catch (backupError) { + logger.error('Failed to backup corrupted session:', backupError); + } + } +} From f4e97ee4af99cef5c72b259ea40f1f0abb1c71e3 Mon Sep 17 00:00:00 2001 From: sogoiii Date: Sat, 3 Jan 2026 18:24:09 -0800 Subject: [PATCH 2/3] [core,cli] feat: improve --continue session resumption with validation and error handling Add comprehensive improvements to session persistence for --continue flag: - Add type-safe session interfaces (PersistedUIHistoryItem, PersistedToolCall) - Add 30 unit tests for SessionPersistenceService covering all code paths - Add runtime validation for persisted UI history with graceful fallback - Add user notifications when core history restoration fails - Fix memory leak in useMemo by using stable dependencies - Add clear messaging when combining --continue with --prompt flags - Replace sync fs.existsSync with async readdir (eliminates race condition) - Update --continue help text to document --prompt compatibility Fixes issue #147 session resumption concerns. --- packages/cli/src/config/config.ts | 3 +- packages/cli/src/gemini.tsx | 29 +- packages/cli/src/ui/AppContainer.tsx | 183 ++- packages/core/src/index.ts | 2 + .../storage/SessionPersistenceService.test.ts | 538 +++++++ .../src/storage/SessionPersistenceService.ts | 68 +- .../SESSION-CONTINUE-IMPROVEMENTS.md | 1392 +++++++++++++++++ 7 files changed, 2194 insertions(+), 21 deletions(-) create mode 100644 packages/core/src/storage/SessionPersistenceService.test.ts create mode 100644 project-plans/20260103-continue-improvements/SESSION-CONTINUE-IMPROVEMENTS.md diff --git a/packages/cli/src/config/config.ts b/packages/cli/src/config/config.ts index 82b377124..19eaf76ea 100755 --- a/packages/cli/src/config/config.ts +++ b/packages/cli/src/config/config.ts @@ -358,7 +358,8 @@ export async function parseArguments(settings: Settings): Promise { .option('continue', { alias: 'C', type: 'boolean', - description: 'Resume the most recent session for this project', + description: + 'Resume the most recent session for this project. Can be combined with --prompt to continue with a new message.', default: false, }) .deprecateOption( diff --git a/packages/cli/src/gemini.tsx b/packages/cli/src/gemini.tsx index 86182f74a..2e09927d7 100644 --- a/packages/cli/src/gemini.tsx +++ b/packages/cli/src/gemini.tsx @@ -223,6 +223,8 @@ export async function startInteractiveUI( // Load previous session if --continue flag was used let restoredSession: PersistedSession | null = null; + const initialPrompt = config.getQuestion(); + if (config.isContinueSession()) { const persistence = new SessionPersistenceService( config.storage, @@ -233,9 +235,32 @@ export async function startInteractiveUI( if (restoredSession) { const formattedTime = SessionPersistenceService.formatSessionTime(restoredSession); - console.log(chalk.green(`Resumed session from ${formattedTime}`)); + + if (initialPrompt) { + // User provided both --continue and --prompt + console.log(chalk.cyan(`Resuming session from ${formattedTime}`)); + const truncatedPrompt = + initialPrompt.length > 50 + ? `${initialPrompt.slice(0, 50)}...` + : initialPrompt; + console.log( + chalk.dim( + `Your prompt "${truncatedPrompt}" will be submitted after session loads.`, + ), + ); + } else { + console.log(chalk.green(`Resumed session from ${formattedTime}`)); + } } else { - console.log(chalk.yellow('No previous session found. Starting fresh.')); + if (initialPrompt) { + console.log( + chalk.yellow( + 'No previous session found. Starting fresh with your prompt.', + ), + ); + } else { + console.log(chalk.yellow('No previous session found. Starting fresh.')); + } } } diff --git a/packages/cli/src/ui/AppContainer.tsx b/packages/cli/src/ui/AppContainer.tsx index 01de2e838..83252c82e 100644 --- a/packages/cli/src/ui/AppContainer.tsx +++ b/packages/cli/src/ui/AppContainer.tsx @@ -151,6 +151,32 @@ function isToolExecuting(pendingHistoryItems: HistoryItemWithoutId[]) { }); } +// Valid history item types for session restore validation (must be module-level for stable reference) +const VALID_HISTORY_TYPES = new Set([ + 'user', + 'gemini', + 'gemini_content', + 'oauth_url', + 'info', + 'error', + 'warning', + 'about', + 'help', + 'stats', + 'model_stats', + 'tool_stats', + 'cache_stats', + 'lb_stats', + 'quit', + 'tool_group', + 'user_shell', + 'compression', + 'extensions_list', + 'tools_list', + 'mcp_status', + 'chat_list', +]); + export const AppContainer = (props: AppContainerProps) => { debug.log('AppContainer architecture active (v2)'); const { @@ -463,6 +489,83 @@ export const AppContainer = (props: AppContainerProps) => { const sessionRestoredRef = useRef(false); const coreHistoryRestoredRef = useRef(false); + /** + * Validates that an item matches the HistoryItem schema. + * Uses duck typing for flexibility with minor schema changes. + */ + const isValidHistoryItem = useCallback( + (item: unknown): item is HistoryItem => { + if (typeof item !== 'object' || item === null) { + return false; + } + + const obj = item as Record; + + // Required fields + if (typeof obj.id !== 'number') return false; + if (typeof obj.type !== 'string') return false; + + // Check if type is valid (allow unknown types from newer versions) + if (!VALID_HISTORY_TYPES.has(obj.type)) { + debug.warn(`Unknown history item type: ${obj.type}`); + // Allow unknown types to pass - might be from newer version + } + + // Type-specific validation + switch (obj.type) { + case 'user': + case 'gemini': + case 'gemini_content': + case 'info': + case 'warning': + case 'error': + case 'user_shell': + // Text types should have text (but might be empty) + return typeof obj.text === 'string' || obj.text === undefined; + + case 'tool_group': + // Tool groups must have tools array + if (!Array.isArray(obj.tools)) return false; + return obj.tools.every( + (tool) => + typeof tool === 'object' && + tool !== null && + typeof (tool as Record).callId === 'string' && + typeof (tool as Record).name === 'string', + ); + + default: + // For other types, just having id and type is enough + return true; + } + }, + [], + ); + + /** + * Validates all items in a history array. + * Returns valid items, filters invalid ones. + */ + const validateUIHistory = useCallback( + (items: unknown[]): { valid: HistoryItem[]; invalidCount: number } => { + const valid: HistoryItem[] = []; + let invalidCount = 0; + + for (let i = 0; i < items.length; i++) { + const item = items[i]; + if (isValidHistoryItem(item)) { + valid.push(item); + } else { + debug.warn(`Invalid history item at index ${i}:`, item); + invalidCount++; + } + } + + return { valid, invalidCount }; + }, + [isValidHistoryItem], + ); + // Part 1: Restore UI history immediately on mount useEffect(() => { if (!restoredSession || sessionRestoredRef.current) { @@ -473,14 +576,39 @@ export const AppContainer = (props: AppContainerProps) => { try { // Use saved UI history if available (preserves exact display), otherwise convert from core history let uiHistoryItems: HistoryItem[]; + let usedFallback = false; + let invalidCount = 0; + if ( restoredSession.uiHistory && Array.isArray(restoredSession.uiHistory) ) { - uiHistoryItems = restoredSession.uiHistory as HistoryItem[]; - debug.log(`Using saved UI history (${uiHistoryItems.length} items)`); + // Validate UI history items before loading + const validation = validateUIHistory(restoredSession.uiHistory); + invalidCount = validation.invalidCount; + + if (invalidCount > 0) { + debug.warn(`${invalidCount} invalid UI history items found`); + + if (validation.valid.length === 0) { + // All items invalid - fall back to conversion + debug.warn('All UI history invalid, falling back to conversion'); + uiHistoryItems = convertToUIHistory(restoredSession.history); + usedFallback = true; + } else { + // Some items valid - use them + uiHistoryItems = validation.valid; + } + } else { + uiHistoryItems = validation.valid; + } + + if (!usedFallback) { + debug.log(`Using saved UI history (${uiHistoryItems.length} items)`); + } } else { uiHistoryItems = convertToUIHistory(restoredSession.history); + usedFallback = true; debug.log( `Converted core history to UI (${uiHistoryItems.length} items)`, ); @@ -491,14 +619,31 @@ export const AppContainer = (props: AppContainerProps) => { `Restored ${restoredSession.history.length} messages (${uiHistoryItems.length} UI items) for display`, ); - // Add an info message + // Add info message about restoration + const sessionTime = new Date( + restoredSession.updatedAt || restoredSession.createdAt, + ).toLocaleString(); + const source = usedFallback + ? 'converted from core' + : 'restored from UI cache'; addItem( { type: 'info', - text: `Session restored (${uiHistoryItems.length} messages from ${new Date(restoredSession.updatedAt || restoredSession.createdAt).toLocaleString()})`, + text: `Session restored (${uiHistoryItems.length} messages ${source} from ${sessionTime})`, }, Date.now(), ); + + // Warn if some items were corrupted + if (invalidCount > 0 && !usedFallback) { + addItem( + { + type: 'warning', + text: `${invalidCount} corrupted message(s) could not be displayed.`, + }, + Date.now(), + ); + } } catch (err) { debug.error('Failed to restore UI history:', err); addItem( @@ -509,7 +654,13 @@ export const AppContainer = (props: AppContainerProps) => { Date.now(), ); } - }, [restoredSession, convertToUIHistory, loadHistory, addItem]); + }, [ + restoredSession, + convertToUIHistory, + loadHistory, + addItem, + validateUIHistory, + ]); // Part 2: Restore core history when historyService becomes available (for AI context) useEffect(() => { @@ -531,16 +682,27 @@ export const AppContainer = (props: AppContainerProps) => { try { historyService.validateAndFix(); historyService.addAll(restoredSession.history); - debug.log('Restored core history for AI context'); + + debug.log( + `Restored ${restoredSession.history.length} items to core history for AI context`, + ); } catch (err) { debug.error('Failed to restore core history:', err); + // Notify user that AI context is missing - this is critical! + addItem( + { + type: 'warning', + text: 'Previous session display restored, but AI context could not be loaded. The AI will not remember the previous conversation.', + }, + Date.now(), + ); } }, 100); return () => { clearInterval(checkInterval); }; - }, [restoredSession, config]); + }, [restoredSession, config, addItem]); const [_staticNeedsRefresh, setStaticNeedsRefresh] = useState(false); const [staticKey, setStaticKey] = useState(0); @@ -1648,9 +1810,12 @@ export const AppContainer = (props: AppContainerProps) => { }, [streamingState, refreshStatic, _staticNeedsRefresh]); // Session persistence - always save so sessions can be resumed with --continue + // Use stable dependencies to avoid recreating service (and new file path) on config changes + const storage = config.storage; + const sessionId = config.getSessionId(); const sessionPersistence = useMemo( - () => new SessionPersistenceService(config.storage, config.getSessionId()), - [config], + () => new SessionPersistenceService(storage, sessionId), + [storage, sessionId], ); // Track previous streaming state to detect turn completion diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index ca2d26802..523d1c17d 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -357,4 +357,6 @@ export { export { SessionPersistenceService, type PersistedSession, + type PersistedUIHistoryItem, + type PersistedToolCall, } from './storage/SessionPersistenceService.js'; diff --git a/packages/core/src/storage/SessionPersistenceService.test.ts b/packages/core/src/storage/SessionPersistenceService.test.ts new file mode 100644 index 000000000..8a96f5ee2 --- /dev/null +++ b/packages/core/src/storage/SessionPersistenceService.test.ts @@ -0,0 +1,538 @@ +/** + * @license + * Copyright 2025 Vybestack LLC + * SPDX-License-Identifier: Apache-2.0 + */ + +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; +import * as crypto from 'node:crypto'; + +// Mock fs before importing the module under test +vi.mock('node:fs', async (importOriginal) => { + const actual = await importOriginal(); + return { + ...actual, + existsSync: vi.fn(), + promises: { + ...actual.promises, + mkdir: vi.fn(), + writeFile: vi.fn(), + rename: vi.fn(), + readdir: vi.fn(), + readFile: vi.fn(), + access: vi.fn(), + }, + }; +}); + +import * as fs from 'node:fs'; +import { + SessionPersistenceService, + type PersistedSession, +} from './SessionPersistenceService.js'; +import { Storage } from '../config/storage.js'; + +describe('SessionPersistenceService', () => { + const mockProjectRoot = '/test/project'; + const mockSessionId = 'test-session-123'; + let storage: Storage; + let service: SessionPersistenceService; + + beforeEach(() => { + vi.clearAllMocks(); + storage = new Storage(mockProjectRoot); + service = new SessionPersistenceService(storage, mockSessionId); + }); + + afterEach(() => { + vi.restoreAllMocks(); + }); + + describe('constructor', () => { + it('should create chats directory path based on storage', () => { + const chatsDir = service.getChatsDir(); + expect(chatsDir).toContain('chats'); + expect(chatsDir).toContain(storage.getProjectTempDir()); + }); + + it('should create session file path with timestamp', () => { + const sessionPath = service.getSessionFilePath(); + expect(sessionPath).toContain('persisted-session-'); + expect(sessionPath.endsWith('.json')).toBe(true); + }); + + it('should create unique timestamps for different instances', async () => { + const service1 = new SessionPersistenceService(storage, 'session1'); + // Small delay to ensure different timestamp + await new Promise((resolve) => setTimeout(resolve, 10)); + const service2 = new SessionPersistenceService(storage, 'session2'); + + expect(service1.getSessionFilePath()).not.toBe( + service2.getSessionFilePath(), + ); + }); + }); + + describe('save()', () => { + beforeEach(() => { + vi.mocked(fs.promises.mkdir).mockResolvedValue(undefined); + vi.mocked(fs.promises.writeFile).mockResolvedValue(); + vi.mocked(fs.promises.rename).mockResolvedValue(); + }); + + it('should create chats directory if not exists', async () => { + await service.save([], undefined, []); + + expect(fs.promises.mkdir).toHaveBeenCalledWith( + expect.stringContaining('chats'), + { recursive: true }, + ); + }); + + it('should write to temp file then rename (atomic write)', async () => { + await service.save([], undefined, []); + + // Should write to .tmp file first + expect(fs.promises.writeFile).toHaveBeenCalledWith( + expect.stringMatching(/\.tmp$/), + expect.any(String), + 'utf-8', + ); + + // Then rename to final path + expect(fs.promises.rename).toHaveBeenCalledWith( + expect.stringMatching(/\.tmp$/), + expect.stringMatching(/\.json$/), + ); + }); + + it('should include all required fields in saved session', async () => { + let savedContent = ''; + vi.mocked(fs.promises.writeFile).mockImplementation( + async (_path, content) => { + savedContent = content as string; + }, + ); + + const history = [ + { speaker: 'human', blocks: [{ type: 'text', text: 'hello' }] }, + ]; + const metadata = { provider: 'test', model: 'test-model' }; + const uiHistory = [{ id: 1, type: 'user', text: 'hello' }]; + + await service.save( + history as unknown as Array< + import('../services/history/IContent.js').IContent + >, + metadata, + uiHistory, + ); + + const parsed = JSON.parse(savedContent) as PersistedSession; + expect(parsed.version).toBe(1); + expect(parsed.sessionId).toBe(mockSessionId); + expect(parsed.projectHash).toBeDefined(); + expect(parsed.projectHash.length).toBe(64); // SHA-256 hex length + expect(parsed.createdAt).toBeDefined(); + expect(parsed.updatedAt).toBeDefined(); + expect(parsed.history).toEqual(history); + expect(parsed.uiHistory).toEqual(uiHistory); + expect(parsed.metadata).toEqual(metadata); + }); + + it('should preserve createdAt across multiple saves', async () => { + let firstCreatedAt: string | null = null; + let secondCreatedAt: string | null = null; + + vi.mocked(fs.promises.writeFile).mockImplementation( + async (_path, content) => { + const parsed = JSON.parse(content as string) as PersistedSession; + if (!firstCreatedAt) { + firstCreatedAt = parsed.createdAt; + } else { + secondCreatedAt = parsed.createdAt; + } + }, + ); + + await service.save([], undefined, []); + await new Promise((resolve) => setTimeout(resolve, 10)); + await service.save([], undefined, []); + + expect(firstCreatedAt).toBe(secondCreatedAt); + }); + + it('should update updatedAt on each save', async () => { + let firstUpdatedAt: string | null = null; + let secondUpdatedAt: string | null = null; + + vi.mocked(fs.promises.writeFile).mockImplementation( + async (_path, content) => { + const parsed = JSON.parse(content as string) as PersistedSession; + if (!firstUpdatedAt) { + firstUpdatedAt = parsed.updatedAt; + } else { + secondUpdatedAt = parsed.updatedAt; + } + }, + ); + + await service.save([], undefined, []); + await new Promise((resolve) => setTimeout(resolve, 10)); + await service.save([], undefined, []); + + expect(firstUpdatedAt).not.toBe(secondUpdatedAt); + }); + + it('should throw on mkdir failure', async () => { + vi.mocked(fs.promises.mkdir).mockRejectedValue( + new Error('Permission denied'), + ); + + await expect(service.save([], undefined, [])).rejects.toThrow( + 'Permission denied', + ); + }); + + it('should throw on write failure', async () => { + vi.mocked(fs.promises.writeFile).mockRejectedValue( + new Error('Disk full'), + ); + + await expect(service.save([], undefined, [])).rejects.toThrow( + 'Disk full', + ); + }); + + it('should throw on rename failure', async () => { + vi.mocked(fs.promises.rename).mockRejectedValue(new Error('IO error')); + + await expect(service.save([], undefined, [])).rejects.toThrow('IO error'); + }); + }); + + describe('loadMostRecent()', () => { + const getProjectHash = () => + crypto.createHash('sha256').update(mockProjectRoot).digest('hex'); + + it('should return null if chats directory does not exist', async () => { + vi.mocked(fs.existsSync).mockReturnValue(false); + + const result = await service.loadMostRecent(); + + expect(result).toBeNull(); + }); + + it('should return null if no session files exist', async () => { + vi.mocked(fs.existsSync).mockReturnValue(true); + vi.mocked(fs.promises.readdir).mockResolvedValue([] as unknown as []); + + const result = await service.loadMostRecent(); + + expect(result).toBeNull(); + }); + + it('should ignore non-session files', async () => { + vi.mocked(fs.existsSync).mockReturnValue(true); + vi.mocked(fs.promises.readdir).mockResolvedValue([ + 'other-file.json', + 'persisted-session-backup.json.bak', + 'readme.md', + ] as unknown as []); + + const result = await service.loadMostRecent(); + + expect(result).toBeNull(); + }); + + it('should load the most recent session file (sorted by filename)', async () => { + vi.mocked(fs.existsSync).mockReturnValue(true); + vi.mocked(fs.promises.readdir).mockResolvedValue([ + 'persisted-session-2026-01-01T00-00-00-000Z.json', + 'persisted-session-2026-01-03T00-00-00-000Z.json', // Most recent + 'persisted-session-2026-01-02T00-00-00-000Z.json', + ] as unknown as []); + + const mockSession: PersistedSession = { + version: 1, + sessionId: mockSessionId, + projectHash: getProjectHash(), + createdAt: '2026-01-03T00:00:00.000Z', + updatedAt: '2026-01-03T00:00:00.000Z', + history: [], + }; + + vi.mocked(fs.promises.readFile).mockResolvedValue( + JSON.stringify(mockSession), + ); + + const result = await service.loadMostRecent(); + + expect(result).toEqual(mockSession); + expect(fs.promises.readFile).toHaveBeenCalledWith( + expect.stringContaining('2026-01-03'), + 'utf-8', + ); + }); + + it('should reject session with wrong project hash', async () => { + vi.mocked(fs.existsSync).mockReturnValue(true); + vi.mocked(fs.promises.readdir).mockResolvedValue([ + 'persisted-session-2026-01-03T00-00-00-000Z.json', + ] as unknown as []); + + const mockSession: PersistedSession = { + version: 1, + sessionId: mockSessionId, + projectHash: 'wrong-hash-from-different-project', + createdAt: '2026-01-03T00:00:00.000Z', + updatedAt: '2026-01-03T00:00:00.000Z', + history: [], + }; + + vi.mocked(fs.promises.readFile).mockResolvedValue( + JSON.stringify(mockSession), + ); + + const result = await service.loadMostRecent(); + + expect(result).toBeNull(); + }); + + it('should reject session with unknown version', async () => { + vi.mocked(fs.existsSync).mockReturnValue(true); + vi.mocked(fs.promises.readdir).mockResolvedValue([ + 'persisted-session-2026-01-03T00-00-00-000Z.json', + ] as unknown as []); + + const mockSession = { + version: 99, // Future version + sessionId: mockSessionId, + projectHash: getProjectHash(), + createdAt: '2026-01-03T00:00:00.000Z', + updatedAt: '2026-01-03T00:00:00.000Z', + history: [], + }; + + vi.mocked(fs.promises.readFile).mockResolvedValue( + JSON.stringify(mockSession), + ); + + const result = await service.loadMostRecent(); + + expect(result).toBeNull(); + }); + + it('should handle corrupted JSON gracefully and backup', async () => { + vi.mocked(fs.existsSync).mockReturnValue(true); + vi.mocked(fs.promises.readdir).mockResolvedValue([ + 'persisted-session-2026-01-03T00-00-00-000Z.json', + ] as unknown as []); + vi.mocked(fs.promises.readFile).mockResolvedValue('{ invalid json }}}'); + vi.mocked(fs.promises.rename).mockResolvedValue(); + + const result = await service.loadMostRecent(); + + expect(result).toBeNull(); + // Should backup the corrupted file + expect(fs.promises.rename).toHaveBeenCalledWith( + expect.stringContaining('persisted-session'), + expect.stringContaining('.corrupted-'), + ); + }); + + it('should return session with UI history when present', async () => { + vi.mocked(fs.existsSync).mockReturnValue(true); + vi.mocked(fs.promises.readdir).mockResolvedValue([ + 'persisted-session-2026-01-03T00-00-00-000Z.json', + ] as unknown as []); + + const uiHistory = [ + { id: 1, type: 'user', text: 'hello' }, + { id: 2, type: 'gemini', text: 'hi there' }, + ]; + + const mockSession: PersistedSession = { + version: 1, + sessionId: mockSessionId, + projectHash: getProjectHash(), + createdAt: '2026-01-03T00:00:00.000Z', + updatedAt: '2026-01-03T00:00:00.000Z', + history: [], + uiHistory, + }; + + vi.mocked(fs.promises.readFile).mockResolvedValue( + JSON.stringify(mockSession), + ); + + const result = await service.loadMostRecent(); + + expect(result?.uiHistory).toEqual(uiHistory); + }); + + it('should handle readdir failure gracefully', async () => { + vi.mocked(fs.existsSync).mockReturnValue(true); + vi.mocked(fs.promises.readdir).mockRejectedValue( + new Error('Permission denied'), + ); + + const result = await service.loadMostRecent(); + + expect(result).toBeNull(); + }); + + it('should handle readFile failure gracefully', async () => { + vi.mocked(fs.existsSync).mockReturnValue(true); + vi.mocked(fs.promises.readdir).mockResolvedValue([ + 'persisted-session-2026-01-03T00-00-00-000Z.json', + ] as unknown as []); + vi.mocked(fs.promises.readFile).mockRejectedValue( + new Error('File not found'), + ); + + const result = await service.loadMostRecent(); + + expect(result).toBeNull(); + }); + }); + + describe('formatSessionTime()', () => { + it('should format session time from updatedAt', () => { + const session: PersistedSession = { + version: 1, + sessionId: 'test', + projectHash: 'hash', + createdAt: '2026-01-01T00:00:00.000Z', + updatedAt: '2026-01-03T12:30:00.000Z', + history: [], + }; + + const formatted = SessionPersistenceService.formatSessionTime(session); + + // Should contain date components (locale-dependent format) + expect(formatted).toBeTruthy(); + expect(formatted.length).toBeGreaterThan(0); + }); + + it('should fall back to createdAt if updatedAt is empty', () => { + const session: PersistedSession = { + version: 1, + sessionId: 'test', + projectHash: 'hash', + createdAt: '2026-01-01T00:00:00.000Z', + updatedAt: '', + history: [], + }; + + const formatted = SessionPersistenceService.formatSessionTime(session); + + // Should still return a formatted string (from createdAt) + expect(formatted).toBeTruthy(); + }); + + it('should handle invalid date gracefully', () => { + const session: PersistedSession = { + version: 1, + sessionId: 'test', + projectHash: 'hash', + createdAt: 'invalid-date', + updatedAt: 'also-invalid', + history: [], + }; + + // Should not throw + expect(() => + SessionPersistenceService.formatSessionTime(session), + ).not.toThrow(); + }); + }); + + describe('project hash consistency', () => { + it('should generate consistent hash for same project', () => { + const service1 = new SessionPersistenceService(storage, 'session1'); + const service2 = new SessionPersistenceService(storage, 'session2'); + + // Access private method via any + const hash1 = ( + service1 as unknown as { getProjectHash(): string } + ).getProjectHash(); + const hash2 = ( + service2 as unknown as { getProjectHash(): string } + ).getProjectHash(); + + expect(hash1).toBe(hash2); + }); + + it('should generate different hash for different projects', () => { + const storage1 = new Storage('/project1'); + const storage2 = new Storage('/project2'); + + const service1 = new SessionPersistenceService(storage1, 'session'); + const service2 = new SessionPersistenceService(storage2, 'session'); + + const hash1 = ( + service1 as unknown as { getProjectHash(): string } + ).getProjectHash(); + const hash2 = ( + service2 as unknown as { getProjectHash(): string } + ).getProjectHash(); + + expect(hash1).not.toBe(hash2); + }); + + it('should generate SHA-256 hex hash (64 chars)', () => { + const hash = ( + service as unknown as { getProjectHash(): string } + ).getProjectHash(); + + expect(hash).toMatch(/^[a-f0-9]{64}$/); + }); + }); + + describe('edge cases', () => { + it('should handle empty history array', async () => { + vi.mocked(fs.promises.mkdir).mockResolvedValue(undefined); + vi.mocked(fs.promises.writeFile).mockResolvedValue(); + vi.mocked(fs.promises.rename).mockResolvedValue(); + + await expect( + service.save([], undefined, undefined), + ).resolves.not.toThrow(); + }); + + it('should handle large history arrays', async () => { + vi.mocked(fs.promises.mkdir).mockResolvedValue(undefined); + vi.mocked(fs.promises.writeFile).mockResolvedValue(); + vi.mocked(fs.promises.rename).mockResolvedValue(); + + const largeHistory = Array.from({ length: 1000 }, (_, i) => ({ + speaker: i % 2 === 0 ? 'human' : 'model', + blocks: [{ type: 'text', text: `Message ${i}` }], + })); + + await expect( + service.save( + largeHistory as unknown as Array< + import('../services/history/IContent.js').IContent + >, + undefined, + undefined, + ), + ).resolves.not.toThrow(); + }); + + it('should handle special characters in project path', () => { + const specialStorage = new Storage( + '/path/with spaces/and-dashes/and_underscores', + ); + const specialService = new SessionPersistenceService( + specialStorage, + 'session', + ); + + expect(() => specialService.getChatsDir()).not.toThrow(); + expect(() => specialService.getSessionFilePath()).not.toThrow(); + }); + }); +}); diff --git a/packages/core/src/storage/SessionPersistenceService.ts b/packages/core/src/storage/SessionPersistenceService.ts index 3d0ad5abb..aa864e075 100644 --- a/packages/core/src/storage/SessionPersistenceService.ts +++ b/packages/core/src/storage/SessionPersistenceService.ts @@ -10,9 +10,56 @@ import * as crypto from 'node:crypto'; import { type IContent } from '../services/history/IContent.js'; import { Storage } from '../config/storage.js'; import { DebugLogger } from '../debug/index.js'; +import { + type ToolResultDisplay, + type ToolCallConfirmationDetails, +} from '../tools/tools.js'; const logger = new DebugLogger('llxprt:session:persistence'); +/** + * Persisted tool call display information. + * Matches CLI's IndividualToolCallDisplay interface for type compatibility. + */ +export interface PersistedToolCall { + /** Unique identifier for the tool call */ + callId: string; + /** Tool name */ + name: string; + /** Human-readable description of what the tool is doing */ + description: string; + /** Tool execution status (string to accept CLI's ToolCallStatus enum) */ + status: string; + /** Result display for completed tools */ + resultDisplay: ToolResultDisplay | undefined; + /** Confirmation details for tools requiring user approval */ + confirmationDetails: ToolCallConfirmationDetails | undefined; + /** Whether to render output as markdown */ + renderOutputAsMarkdown?: boolean; + /** Whether this tool is currently focused in UI */ + isFocused?: boolean; +} + +/** + * Minimal interface for persisted UI history items. + * CLI's HistoryItem should satisfy this interface. + * Uses permissive types since CLI has multiple history types with different shapes. + */ +export interface PersistedUIHistoryItem { + /** Unique identifier for the history item */ + id: number; + /** Type discriminator for the history item */ + type: string; + /** Optional text content (for user/gemini/info/warning/error messages) */ + text?: string; + /** Optional model identifier (for gemini responses) */ + model?: string; + /** Optional agent ID (for subagent contexts) */ + agentId?: string; + /** Optional tools array - shape varies by type (tool_group vs tools_list) */ + tools?: unknown[]; +} + /** * Persisted session format for --continue functionality */ @@ -30,7 +77,7 @@ export interface PersistedSession { /** Full conversation history (core format) */ history: IContent[]; /** UI history items for display restoration (preserves exactly what user sees) */ - uiHistory?: unknown[]; + uiHistory?: PersistedUIHistoryItem[]; /** Optional metadata */ metadata?: { provider?: string; @@ -87,7 +134,7 @@ export class SessionPersistenceService { async save( history: IContent[], metadata?: PersistedSession['metadata'], - uiHistory?: unknown[], + uiHistory?: PersistedUIHistoryItem[], ): Promise { try { // Ensure chats directory exists @@ -129,14 +176,17 @@ export class SessionPersistenceService { */ async loadMostRecent(): Promise { try { - // Check if chats directory exists - if (!fs.existsSync(this.chatsDir)) { - logger.debug('No chats directory found'); - return null; + // Find all persisted session files (readdir throws ENOENT if dir doesn't exist) + let files: string[]; + try { + files = await fs.promises.readdir(this.chatsDir); + } catch (err) { + if ((err as NodeJS.ErrnoException).code === 'ENOENT') { + logger.debug('No chats directory found'); + return null; + } + throw err; } - - // Find all persisted session files - const files = await fs.promises.readdir(this.chatsDir); const sessionFiles = files .filter( (f) => f.startsWith(PERSISTED_SESSION_PREFIX) && f.endsWith('.json'), diff --git a/project-plans/20260103-continue-improvements/SESSION-CONTINUE-IMPROVEMENTS.md b/project-plans/20260103-continue-improvements/SESSION-CONTINUE-IMPROVEMENTS.md new file mode 100644 index 000000000..7842de2e2 --- /dev/null +++ b/project-plans/20260103-continue-improvements/SESSION-CONTINUE-IMPROVEMENTS.md @@ -0,0 +1,1392 @@ +# Session Continue (`--continue`) Feature Improvements + +**Date:** 2026-01-03 +**Feature:** `--continue` flag for session resumption +**Commit:** e19cfa22c +**Status:** Review and improvement recommendations + +--- + +## Table of Contents + +1. [Issue #2: Memory Leak from SessionPersistenceService Recreation](#issue-2-memory-leak-from-sessionpersistenceservice-recreation) +2. [Issue #4: Weak Type Safety for uiHistory](#issue-4-weak-type-safety-for-uihistory) +3. [Issue #6: Silent Failure on Corrupted History Restore](#issue-6-silent-failure-on-corrupted-history-restore) +4. [Issue #7: No Validation of HistoryItem Schema](#issue-7-no-validation-of-historyitem-schema) +5. [Issue #8: Synchronous fs.existsSync in Async Context](#issue-8-synchronous-fsexistssync-in-async-context) +6. [Issue #10: --continue + --prompt Interaction Undefined](#issue-10---continue----prompt-interaction-undefined) +7. [Issue #11: No Test Coverage](#issue-11-no-test-coverage) + +--- + +## Issue #2: Memory Leak from SessionPersistenceService Recreation + +### Location +`packages/cli/src/ui/AppContainer.tsx:1651-1654` + +### Current Implementation + +```typescript +const sessionPersistence = useMemo( + () => new SessionPersistenceService(config.storage, config.getSessionId()), + [config], +); +``` + +### Problem Analysis + +The `SessionPersistenceService` constructor generates a **timestamp-based filename** on instantiation: + +```typescript +// From SessionPersistenceService.ts:62-67 +constructor(storage: Storage, sessionId: string) { + // ... + const timestamp = new Date().toISOString().replace(/[:.]/g, '-'); + this.sessionFilePath = path.join( + this.chatsDir, + `${PERSISTED_SESSION_PREFIX}${timestamp}.json`, + ); +} +``` + +**The dependency array `[config]` is problematic because:** + +1. **`config` is an object reference** - React's shallow comparison means ANY property change on `config` triggers recreation +2. The `Config` class has many mutable properties (model, provider, settings, etc.) that change during runtime +3. Each recreation generates a **new timestamp** → **new file path** +4. Previous session files are orphaned (never deleted, never updated) + +### Consequences + +1. **Disk bloat**: Multiple session files created per session (one per config change) +2. **Data loss**: If config changes mid-session, saves go to new file; `--continue` loads the wrong (older) file +3. **Memory churn**: New `SessionPersistenceService` instances created unnecessarily + +### Root Cause + +The `useMemo` dependency should be the **stable identifiers** needed for the service, not the entire config object: + +- `config.storage` (the Storage instance) +- `config.getSessionId()` (the session ID string) + +These rarely change during a session, unlike the `config` object reference. + +### Proposed Fix + +**Option A: Stable Dependencies (Recommended)** + +```typescript +// Extract stable values once +const storage = config.storage; +const sessionId = config.getSessionId(); + +const sessionPersistence = useMemo( + () => new SessionPersistenceService(storage, sessionId), + [storage, sessionId], +); +``` + +**Option B: Use Refs for Stability** + +```typescript +const storageRef = useRef(config.storage); +const sessionIdRef = useRef(config.getSessionId()); + +const sessionPersistence = useMemo( + () => new SessionPersistenceService(storageRef.current, sessionIdRef.current), + [], // Empty deps - created once +); +``` + +**Option C: Singleton Pattern in Service** + +Modify `SessionPersistenceService` to maintain a single file path per session: + +```typescript +export class SessionPersistenceService { + private static instances = new Map(); + + static getInstance(storage: Storage, sessionId: string): SessionPersistenceService { + const key = `${storage.getProjectTempDir()}:${sessionId}`; + if (!this.instances.has(key)) { + this.instances.set(key, new SessionPersistenceService(storage, sessionId)); + } + return this.instances.get(key)!; + } + + // Make constructor private + private constructor(storage: Storage, sessionId: string) { + // ... + } +} +``` + +### Testing Verification + +To verify the fix works: + +```typescript +// Test that service maintains same file path across config changes +it('should maintain same session file when config properties change', () => { + const storage = new Storage('/test'); + const sessionId = 'test-session'; + + const service1 = new SessionPersistenceService(storage, sessionId); + const path1 = service1.getSessionFilePath(); + + // Simulate time passing (would create different timestamp) + jest.advanceTimersByTime(1000); + + // With current bug: this creates new timestamp + // With fix: should reuse existing service + const service2 = SessionPersistenceService.getInstance(storage, sessionId); + const path2 = service2.getSessionFilePath(); + + expect(path1).toBe(path2); +}); +``` + +--- + +## Issue #4: Weak Type Safety for uiHistory + +### Location +`packages/core/src/storage/SessionPersistenceService.ts:33` + +### Current Implementation + +```typescript +export interface PersistedSession { + version: 1; + sessionId: string; + projectHash: string; + createdAt: string; + updatedAt: string; + history: IContent[]; + uiHistory?: unknown[]; // <-- Problem: unknown[] loses all type safety + metadata?: { + provider?: string; + model?: string; + tokenCount?: number; + }; +} +``` + +### Problem Analysis + +Using `unknown[]` for `uiHistory` creates several issues: + +1. **No compile-time checking**: TypeScript can't catch shape mismatches +2. **Requires casting everywhere**: `restoredSession.uiHistory as HistoryItem[]` +3. **Runtime errors possible**: If the shape doesn't match, crashes occur at runtime +4. **Poor developer experience**: No autocomplete, no refactoring support + +### The Architectural Challenge + +The `PersistedSession` interface lives in `@vybestack/llxprt-code-core`, but `HistoryItem` is defined in `@vybestack/llxprt-code-cli`. Core cannot import from CLI (would create circular dependency). + +### Proposed Solutions + +**Option A: Define Minimal Interface in Core (Recommended)** + +Create a minimal, stable interface in core that CLI's `HistoryItem` must satisfy: + +```typescript +// packages/core/src/storage/SessionPersistenceService.ts + +/** + * Minimal interface for persisted UI history items. + * CLI's HistoryItem must satisfy this interface. + */ +export interface PersistedUIHistoryItem { + /** Unique identifier for the history item */ + id: number; + /** Type discriminator for the history item */ + type: string; + /** Optional text content */ + text?: string; + /** Optional model identifier */ + model?: string; + /** Optional tool information for tool_group type */ + tools?: Array<{ + callId: string; + name: string; + status: string; + description?: string; + resultDisplay?: string; + }>; +} + +export interface PersistedSession { + version: 1; + sessionId: string; + projectHash: string; + createdAt: string; + updatedAt: string; + history: IContent[]; + uiHistory?: PersistedUIHistoryItem[]; // <-- Now typed! + metadata?: { + provider?: string; + model?: string; + tokenCount?: number; + }; +} +``` + +Then in CLI, ensure `HistoryItem` extends or satisfies this interface: + +```typescript +// packages/cli/src/ui/types.ts + +import type { PersistedUIHistoryItem } from '@vybestack/llxprt-code-core'; + +// HistoryItem should be a superset of PersistedUIHistoryItem +export interface HistoryItem extends PersistedUIHistoryItem { + // CLI-specific fields that aren't persisted + isStreaming?: boolean; + // ... other runtime-only fields +} +``` + +**Option B: Separate Serialization Types** + +Create explicit serialization/deserialization types: + +```typescript +// packages/core/src/storage/sessionTypes.ts + +export interface SerializedHistoryItem { + id: number; + type: 'user' | 'gemini' | 'info' | 'warning' | 'error' | 'tool_group'; + text?: string; + model?: string; + agentId?: string; + tools?: SerializedToolCall[]; +} + +export interface SerializedToolCall { + callId: string; + name: string; + status: 'pending' | 'executing' | 'success' | 'error' | 'cancelled'; + description: string; + resultDisplay?: string; +} +``` + +**Option C: Use Zod Schema (Runtime Validation)** + +If you want runtime validation as well: + +```typescript +import { z } from 'zod'; + +const PersistedToolCallSchema = z.object({ + callId: z.string(), + name: z.string(), + status: z.string(), + description: z.string().optional(), + resultDisplay: z.string().optional(), +}); + +const PersistedUIHistoryItemSchema = z.object({ + id: z.number(), + type: z.string(), + text: z.string().optional(), + model: z.string().optional(), + tools: z.array(PersistedToolCallSchema).optional(), +}); + +export type PersistedUIHistoryItem = z.infer; +``` + +### Implementation Steps + +1. Define `PersistedUIHistoryItem` in core +2. Update `PersistedSession.uiHistory` to use typed array +3. Update CLI's `HistoryItem` to extend/satisfy the interface +4. Update `convertToUIHistory` to return typed array +5. Remove all `as HistoryItem[]` casts (they become unnecessary) + +--- + +## Issue #6: Silent Failure on Corrupted History Restore + +### Location +`packages/cli/src/ui/AppContainer.tsx:515-543` + +### Current Implementation + +```typescript +// Part 2: Restore core history when historyService becomes available +useEffect(() => { + if (!restoredSession || coreHistoryRestoredRef.current) { + return; + } + + const checkInterval = setInterval(() => { + const geminiClient = config.getGeminiClient(); + const historyService = geminiClient?.getHistoryService?.(); + + if (!historyService) { + return; + } + + clearInterval(checkInterval); + coreHistoryRestoredRef.current = true; + + try { + historyService.validateAndFix(); + historyService.addAll(restoredSession.history); + debug.log('Restored core history for AI context'); + } catch (err) { + debug.error('Failed to restore core history:', err); + // <-- PROBLEM: User sees "Session restored" but AI has no context! + } + }, 100); + + return () => { + clearInterval(checkInterval); + }; +}, [restoredSession, config]); +``` + +### Problem Analysis + +When core history restoration fails: + +1. User already saw "Session restored (X messages from ...)" message (from Part 1) +2. UI shows previous conversation correctly +3. **But AI has no memory of the conversation** +4. User types follow-up question +5. AI responds with confusion: "I don't see any previous context..." +6. User is confused why AI "forgot" the conversation + +This creates a **deceptive UX** where the system appears to work but is fundamentally broken. + +### Failure Scenarios + +1. **Schema mismatch**: `IContent` format changed between versions +2. **Corrupted data**: Partial write, disk error, JSON parse succeeds but data is invalid +3. **Token limit**: History too large for `historyService.addAll()` +4. **Validation failure**: `validateAndFix()` throws on invalid data + +### Proposed Fix + +**Comprehensive error handling with user notification:** + +```typescript +useEffect(() => { + if (!restoredSession || coreHistoryRestoredRef.current) { + return; + } + + const checkInterval = setInterval(() => { + const geminiClient = config.getGeminiClient(); + const historyService = geminiClient?.getHistoryService?.(); + + if (!historyService) { + return; + } + + clearInterval(checkInterval); + coreHistoryRestoredRef.current = true; + + try { + // Validate first + historyService.validateAndFix(); + + // Attempt to add history + const addedCount = historyService.addAll(restoredSession.history); + + if (addedCount === 0 && restoredSession.history.length > 0) { + throw new Error('No history items were added'); + } + + if (addedCount < restoredSession.history.length) { + debug.warn(`Only ${addedCount}/${restoredSession.history.length} history items restored`); + addItem({ + type: 'warning', + text: `Partial session restore: ${addedCount} of ${restoredSession.history.length} messages loaded into AI context.`, + }, Date.now()); + } else { + debug.log('Restored core history for AI context'); + } + } catch (err) { + debug.error('Failed to restore core history:', err); + + // Notify user that AI context is missing + addItem({ + type: 'warning', + text: 'Previous session display restored, but AI context could not be loaded. The AI will not remember the previous conversation.', + }, Date.now()); + + // Optionally offer to clear the corrupted display too + // This prevents the deceptive state + } + }, 100); + + return () => { + clearInterval(checkInterval); + }; +}, [restoredSession, config, addItem]); +``` + +### Additional Improvements + +**Add retry logic for transient failures:** + +```typescript +const MAX_RETRIES = 3; +let retryCount = 0; + +const attemptRestore = () => { + try { + historyService.validateAndFix(); + historyService.addAll(restoredSession.history); + debug.log('Restored core history for AI context'); + } catch (err) { + retryCount++; + if (retryCount < MAX_RETRIES) { + debug.warn(`Retry ${retryCount}/${MAX_RETRIES} for history restore`); + setTimeout(attemptRestore, 500 * retryCount); // Exponential backoff + } else { + debug.error('Failed to restore core history after retries:', err); + addItem({ + type: 'warning', + text: 'AI context restoration failed. Previous messages are shown but AI has no memory of them.', + }, Date.now()); + } + } +}; +``` + +--- + +## Issue #7: No Validation of HistoryItem Schema + +### Location +`packages/cli/src/ui/AppContainer.tsx:476-488` + +### Current Implementation + +```typescript +// Use saved UI history if available, otherwise convert from core history +let uiHistoryItems: HistoryItem[]; +if ( + restoredSession.uiHistory && + Array.isArray(restoredSession.uiHistory) +) { + uiHistoryItems = restoredSession.uiHistory as HistoryItem[]; // <-- Dangerous cast! + debug.log(`Using saved UI history (${uiHistoryItems.length} items)`); +} else { + uiHistoryItems = convertToUIHistory(restoredSession.history); + debug.log(`Converted core history to UI (${uiHistoryItems.length} items)`); +} +loadHistory(uiHistoryItems); +``` + +### Problem Analysis + +The `as HistoryItem[]` cast is a **type assertion**, not a runtime check. If the persisted data doesn't match the expected shape: + +1. **Schema evolution**: `HistoryItem` interface changes, old sessions have different shape +2. **Corruption**: Partial writes, encoding issues +3. **Version mismatch**: User downgrades app, loads newer session format + +**Failure modes:** + +```typescript +// If persisted item looks like this (old format): +{ id: 1, messageType: 'user', content: 'hello' } + +// But code expects: +{ id: 1, type: 'user', text: 'hello' } + +// Then accessing item.type returns undefined +// And item.text returns undefined +// UI renders empty/broken items or crashes +``` + +### Proposed Fix + +**Runtime validation with fallback:** + +```typescript +/** + * Validates that an item matches the HistoryItem schema. + * Uses duck typing for flexibility with minor schema changes. + */ +const isValidHistoryItem = (item: unknown): item is HistoryItem => { + if (typeof item !== 'object' || item === null) { + return false; + } + + const obj = item as Record; + + // Required fields + if (typeof obj.id !== 'number') return false; + if (typeof obj.type !== 'string') return false; + + // Type-specific validation + switch (obj.type) { + case 'user': + case 'gemini': + case 'info': + case 'warning': + case 'error': + // Text types should have text (but might be empty) + return typeof obj.text === 'string' || obj.text === undefined; + + case 'tool_group': + // Tool groups must have tools array + if (!Array.isArray(obj.tools)) return false; + return obj.tools.every(tool => + typeof tool === 'object' && + tool !== null && + typeof (tool as Record).callId === 'string' && + typeof (tool as Record).name === 'string' + ); + + default: + // Unknown type - might be from newer version + debug.warn(`Unknown history item type: ${obj.type}`); + return true; // Allow unknown types to pass through + } +}; + +/** + * Validates all items in a history array. + * Returns valid items, filters invalid ones. + */ +const validateUIHistory = ( + items: unknown[], + debug: DebugLogger +): { valid: HistoryItem[]; invalidCount: number } => { + const valid: HistoryItem[] = []; + let invalidCount = 0; + + for (let i = 0; i < items.length; i++) { + if (isValidHistoryItem(items[i])) { + valid.push(items[i]); + } else { + debug.warn(`Invalid history item at index ${i}:`, items[i]); + invalidCount++; + } + } + + return { valid, invalidCount }; +}; +``` + +**Updated restoration logic:** + +```typescript +useEffect(() => { + if (!restoredSession || sessionRestoredRef.current) { + return; + } + sessionRestoredRef.current = true; + + try { + let uiHistoryItems: HistoryItem[]; + let usedFallback = false; + + if (restoredSession.uiHistory && Array.isArray(restoredSession.uiHistory)) { + const { valid, invalidCount } = validateUIHistory( + restoredSession.uiHistory, + debug + ); + + if (invalidCount > 0) { + debug.warn(`${invalidCount} invalid UI history items found`); + + if (valid.length === 0) { + // All items invalid - fall back to conversion + debug.warn('All UI history invalid, falling back to conversion'); + uiHistoryItems = convertToUIHistory(restoredSession.history); + usedFallback = true; + } else { + // Some items valid - use them but warn + uiHistoryItems = valid; + addItem({ + type: 'warning', + text: `${invalidCount} corrupted message(s) could not be displayed.`, + }, Date.now()); + } + } else { + uiHistoryItems = valid; + } + } else { + uiHistoryItems = convertToUIHistory(restoredSession.history); + usedFallback = true; + } + + loadHistory(uiHistoryItems); + + const source = usedFallback ? 'converted from core' : 'restored from UI cache'; + addItem({ + type: 'info', + text: `Session restored (${uiHistoryItems.length} messages ${source})`, + }, Date.now()); + + } catch (err) { + debug.error('Failed to restore UI history:', err); + addItem({ + type: 'warning', + text: 'Failed to restore previous session display.', + }, Date.now()); + } +}, [restoredSession, convertToUIHistory, loadHistory, addItem]); +``` + +### Schema Migration Support + +For handling schema evolution across versions: + +```typescript +interface HistoryItemV1 { + id: number; + messageType: string; // Old field name + content: string; // Old field name +} + +interface HistoryItemV2 { + id: number; + type: string; // New field name + text: string; // New field name +} + +const migrateHistoryItem = (item: unknown): HistoryItem | null => { + if (typeof item !== 'object' || item === null) return null; + + const obj = item as Record; + + // Already current format + if ('type' in obj && 'text' in obj) { + return isValidHistoryItem(item) ? item as HistoryItem : null; + } + + // Migrate from V1 format + if ('messageType' in obj && 'content' in obj) { + return { + id: obj.id as number, + type: obj.messageType as string, + text: obj.content as string, + } as HistoryItem; + } + + return null; +}; +``` + +--- + +## Issue #8: Synchronous fs.existsSync in Async Context + +### Location +`packages/core/src/storage/SessionPersistenceService.ts:130-136` + +### Current Implementation + +```typescript +async loadMostRecent(): Promise { + try { + // Check if chats directory exists + if (!fs.existsSync(this.chatsDir)) { // <-- Synchronous call in async function + logger.debug('No chats directory found'); + return null; + } + // ... rest of async operations +``` + +### Problem Analysis + +**Why this matters:** + +1. `fs.existsSync()` is a **synchronous, blocking** call +2. It blocks the Node.js event loop until the filesystem responds +3. In an async function, this defeats the purpose of async/await +4. On slow filesystems (network drives, encrypted volumes), this can cause noticeable UI freezes + +**The irony:** The function is correctly async for `readdir` and `readFile`, but uses sync for the existence check. + +### Impact Assessment + +- **Low impact** for local SSDs (< 1ms) +- **Medium impact** for HDDs or encrypted volumes (10-50ms) +- **High impact** for network filesystems (100ms+) + +Given that `loadMostRecent()` is called during startup with `--continue`, this could delay app launch. + +### Proposed Fix + +**Replace with async equivalent:** + +```typescript +async loadMostRecent(): Promise { + try { + // Check if chats directory exists (async) + try { + await fs.promises.access(this.chatsDir, fs.constants.R_OK); + } catch { + logger.debug('No chats directory found'); + return null; + } + + // Find all persisted session files + const files = await fs.promises.readdir(this.chatsDir); + // ... rest unchanged +``` + +**Alternative using stat:** + +```typescript +async loadMostRecent(): Promise { + try { + // Check if chats directory exists and is a directory + try { + const stat = await fs.promises.stat(this.chatsDir); + if (!stat.isDirectory()) { + logger.debug('Chats path exists but is not a directory'); + return null; + } + } catch (err) { + if ((err as NodeJS.ErrnoException).code === 'ENOENT') { + logger.debug('No chats directory found'); + return null; + } + throw err; // Re-throw other errors + } + + const files = await fs.promises.readdir(this.chatsDir); + // ... rest unchanged +``` + +### Full Method Refactor + +```typescript +async loadMostRecent(): Promise { + try { + // Async directory existence check + let files: string[]; + try { + files = await fs.promises.readdir(this.chatsDir); + } catch (err) { + if ((err as NodeJS.ErrnoException).code === 'ENOENT') { + logger.debug('No chats directory found'); + return null; + } + throw err; + } + + // Filter and sort session files + const sessionFiles = files + .filter( + (f) => f.startsWith(PERSISTED_SESSION_PREFIX) && f.endsWith('.json'), + ) + .sort() + .reverse(); + + if (sessionFiles.length === 0) { + logger.debug('No persisted sessions found'); + return null; + } + + // Load most recent + const mostRecentFile = sessionFiles[0]; + const filePath = path.join(this.chatsDir, mostRecentFile); + + logger.debug('Loading most recent session:', filePath); + + const content = await fs.promises.readFile(filePath, 'utf-8'); + const session = JSON.parse(content) as PersistedSession; + + // Validate project hash + const currentProjectHash = this.getProjectHash(); + if (session.projectHash !== currentProjectHash) { + logger.warn('Session project hash mismatch, skipping:', { + expected: currentProjectHash, + found: session.projectHash, + }); + return null; + } + + // Validate version + if (session.version !== 1) { + logger.warn('Unknown session version:', session.version); + return null; + } + + logger.debug('Session loaded:', { + sessionId: session.sessionId, + historyLength: session.history.length, + createdAt: session.createdAt, + updatedAt: session.updatedAt, + }); + + return session; + } catch (error) { + logger.error('Failed to load session:', error); + + if (error instanceof SyntaxError) { + await this.backupCorruptedSession(); + } + + return null; + } +} +``` + +--- + +## Issue #10: --continue + --prompt Interaction Undefined + +### Location +- `packages/cli/src/config/config.ts:358-363` (argument parsing) +- `packages/cli/src/gemini.tsx:225-240` (session loading) + +### Current Behavior + +The code allows both flags simultaneously but behavior is undefined: + +```bash +llxprt --continue --prompt "fix the auth bug" +``` + +What happens: +1. Previous session is loaded (UI history + AI context restored) +2. User sees previous conversation +3. The prompt "fix the auth bug" is... what? + - Submitted immediately after restore? (current behavior, probably) + - Appended to restored context? + - Ignored? + +### Problem Analysis + +There are legitimate use cases for combining these: + +1. **Resume and continue**: "Continue where I left off and now fix the bug" +2. **Resume context only**: "Load context but start with this new task" + +But there are also problematic cases: + +1. **Conflicting context**: Previous session was about feature A, new prompt about feature B +2. **Token limits**: Restored history + new prompt exceeds context window +3. **User confusion**: User expects clean slate but gets old context + +### Proposed Solutions + +**Option A: Mutual Exclusivity (Simplest)** + +Treat them as mutually exclusive: + +```typescript +// In parseArguments() or loadCliConfig() +if (argv.continue && (argv.prompt || argv.promptInteractive)) { + console.error(chalk.red( + 'Error: Cannot use --continue with --prompt or --prompt-interactive.\n' + + 'Use --continue to resume a session, or --prompt to start fresh.' + )); + process.exit(1); +} +``` + +**Option B: Clear Semantics with Warning (Recommended)** + +Allow it but define clear behavior: + +```typescript +// In gemini.tsx startInteractiveUI() +if (config.isContinueSession()) { + const persistence = new SessionPersistenceService( + config.storage, + config.getSessionId(), + ); + restoredSession = await persistence.loadMostRecent(); + + if (restoredSession) { + const formattedTime = SessionPersistenceService.formatSessionTime(restoredSession); + + if (config.getQuestion()) { + // User provided both --continue and --prompt + console.log(chalk.cyan( + `Resuming session from ${formattedTime}\n` + + `Your prompt will be submitted after context is restored.` + )); + } else { + console.log(chalk.green(`Resumed session from ${formattedTime}`)); + } + } else { + console.log(chalk.yellow('No previous session found. Starting fresh.')); + } +} +``` + +**Option C: Explicit Flag for Behavior** + +Add a flag to control the interaction: + +```typescript +.option('continue-with-prompt', { + type: 'boolean', + description: 'When using --continue with --prompt, submit prompt after restoring session', + default: true, +}) +.option('continue-context-only', { + type: 'boolean', + description: 'Load previous session context but start a new conversation', + implies: 'continue', +}) +``` + +### Recommended Implementation + +```typescript +// packages/cli/src/gemini.tsx + +export async function startInteractiveUI( + config: Config, + settings: LoadedSettings, + startupWarnings: string[], + workspaceRoot: string, +) { + const version = await getCliVersion(); + const initialPrompt = config.getQuestion(); + + let restoredSession: PersistedSession | null = null; + + if (config.isContinueSession()) { + const persistence = new SessionPersistenceService( + config.storage, + config.getSessionId(), + ); + restoredSession = await persistence.loadMostRecent(); + + if (restoredSession) { + const formattedTime = SessionPersistenceService.formatSessionTime(restoredSession); + + if (initialPrompt) { + // Both --continue and --prompt provided + console.log(chalk.cyan( + `Resuming session from ${formattedTime}` + )); + console.log(chalk.dim( + `Your prompt "${initialPrompt.slice(0, 50)}${initialPrompt.length > 50 ? '...' : ''}" ` + + `will be submitted after session loads.` + )); + } else { + console.log(chalk.green(`Resumed session from ${formattedTime}`)); + } + } else { + if (initialPrompt) { + console.log(chalk.yellow( + 'No previous session found. Starting fresh with your prompt.' + )); + } else { + console.log(chalk.yellow('No previous session found. Starting fresh.')); + } + } + } + + // ... rest of startup +} +``` + +### Documentation Update + +Add to help text: + +```typescript +.option('continue', { + alias: 'C', + type: 'boolean', + description: + 'Resume the most recent session for this project. ' + + 'Can be combined with --prompt to continue with a new message.', + default: false, +}) +``` + +--- + +## Issue #11: No Test Coverage + +### Location +The feature spans multiple files but no test files were added: +- `packages/core/src/storage/SessionPersistenceService.ts` - No tests +- `packages/cli/src/ui/AppContainer.tsx` - Session restoration logic untested + +### Problem Analysis + +The `--continue` feature is a critical persistence feature that: + +1. Writes to disk (can fail, corrupt, race) +2. Reads from disk (can fail, corrupt, migrate) +3. Restores complex state (can mismatch, crash) +4. Interacts with multiple subsystems (GeminiClient, HistoryService, UI) + +Without tests: +- Regressions go unnoticed +- Edge cases are discovered in production +- Refactoring is risky +- Cross-version compatibility is untested + +### Proposed Test Suite + +**File: `packages/core/src/storage/SessionPersistenceService.test.ts`** + +```typescript +import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; +import * as fs from 'node:fs'; +import * as path from 'node:path'; +import { SessionPersistenceService, PersistedSession } from './SessionPersistenceService'; +import { Storage } from '../config/storage'; + +// Mock the filesystem +vi.mock('node:fs'); + +describe('SessionPersistenceService', () => { + let storage: Storage; + let service: SessionPersistenceService; + const mockProjectRoot = '/test/project'; + const mockSessionId = 'test-session-123'; + + beforeEach(() => { + storage = new Storage(mockProjectRoot); + service = new SessionPersistenceService(storage, mockSessionId); + vi.clearAllMocks(); + }); + + describe('save()', () => { + it('should create chats directory if not exists', async () => { + const mkdirSpy = vi.spyOn(fs.promises, 'mkdir').mockResolvedValue(undefined); + vi.spyOn(fs.promises, 'writeFile').mockResolvedValue(); + vi.spyOn(fs.promises, 'rename').mockResolvedValue(); + + await service.save([], undefined, []); + + expect(mkdirSpy).toHaveBeenCalledWith( + expect.stringContaining('chats'), + { recursive: true } + ); + }); + + it('should write to temp file then rename (atomic write)', async () => { + vi.spyOn(fs.promises, 'mkdir').mockResolvedValue(undefined); + const writeFileSpy = vi.spyOn(fs.promises, 'writeFile').mockResolvedValue(); + const renameSpy = vi.spyOn(fs.promises, 'rename').mockResolvedValue(); + + await service.save([], undefined, []); + + // Should write to .tmp file first + expect(writeFileSpy).toHaveBeenCalledWith( + expect.stringMatching(/\.tmp$/), + expect.any(String), + 'utf-8' + ); + + // Then rename to final path + expect(renameSpy).toHaveBeenCalledWith( + expect.stringMatching(/\.tmp$/), + expect.stringMatching(/\.json$/) + ); + }); + + it('should include all required fields in saved session', async () => { + vi.spyOn(fs.promises, 'mkdir').mockResolvedValue(undefined); + let savedContent = ''; + vi.spyOn(fs.promises, 'writeFile').mockImplementation(async (_path, content) => { + savedContent = content as string; + }); + vi.spyOn(fs.promises, 'rename').mockResolvedValue(); + + const history = [{ speaker: 'human', blocks: [{ type: 'text', text: 'hello' }] }]; + const metadata = { provider: 'test', model: 'test-model' }; + const uiHistory = [{ id: 1, type: 'user', text: 'hello' }]; + + await service.save(history as any, metadata, uiHistory); + + const parsed = JSON.parse(savedContent) as PersistedSession; + expect(parsed.version).toBe(1); + expect(parsed.sessionId).toBe(mockSessionId); + expect(parsed.projectHash).toBeDefined(); + expect(parsed.createdAt).toBeDefined(); + expect(parsed.updatedAt).toBeDefined(); + expect(parsed.history).toEqual(history); + expect(parsed.uiHistory).toEqual(uiHistory); + expect(parsed.metadata).toEqual(metadata); + }); + + it('should throw on write failure', async () => { + vi.spyOn(fs.promises, 'mkdir').mockResolvedValue(undefined); + vi.spyOn(fs.promises, 'writeFile').mockRejectedValue(new Error('Disk full')); + + await expect(service.save([], undefined, [])).rejects.toThrow('Disk full'); + }); + }); + + describe('loadMostRecent()', () => { + it('should return null if chats directory does not exist', async () => { + vi.spyOn(fs.promises, 'access').mockRejectedValue({ code: 'ENOENT' }); + + const result = await service.loadMostRecent(); + + expect(result).toBeNull(); + }); + + it('should return null if no session files exist', async () => { + vi.spyOn(fs.promises, 'access').mockResolvedValue(); + vi.spyOn(fs.promises, 'readdir').mockResolvedValue([]); + + const result = await service.loadMostRecent(); + + expect(result).toBeNull(); + }); + + it('should load the most recent session file', async () => { + vi.spyOn(fs.promises, 'access').mockResolvedValue(); + vi.spyOn(fs.promises, 'readdir').mockResolvedValue([ + 'persisted-session-2026-01-01.json', + 'persisted-session-2026-01-03.json', // Most recent + 'persisted-session-2026-01-02.json', + ] as any); + + const mockSession: PersistedSession = { + version: 1, + sessionId: mockSessionId, + projectHash: service['getProjectHash'](), // Access private method + createdAt: '2026-01-03T00:00:00.000Z', + updatedAt: '2026-01-03T00:00:00.000Z', + history: [], + }; + + vi.spyOn(fs.promises, 'readFile').mockResolvedValue(JSON.stringify(mockSession)); + + const result = await service.loadMostRecent(); + + expect(result).toEqual(mockSession); + expect(fs.promises.readFile).toHaveBeenCalledWith( + expect.stringContaining('2026-01-03'), + 'utf-8' + ); + }); + + it('should reject session with wrong project hash', async () => { + vi.spyOn(fs.promises, 'access').mockResolvedValue(); + vi.spyOn(fs.promises, 'readdir').mockResolvedValue([ + 'persisted-session-2026-01-03.json', + ] as any); + + const mockSession: PersistedSession = { + version: 1, + sessionId: mockSessionId, + projectHash: 'wrong-hash', // Different project + createdAt: '2026-01-03T00:00:00.000Z', + updatedAt: '2026-01-03T00:00:00.000Z', + history: [], + }; + + vi.spyOn(fs.promises, 'readFile').mockResolvedValue(JSON.stringify(mockSession)); + + const result = await service.loadMostRecent(); + + expect(result).toBeNull(); + }); + + it('should reject session with unknown version', async () => { + vi.spyOn(fs.promises, 'access').mockResolvedValue(); + vi.spyOn(fs.promises, 'readdir').mockResolvedValue([ + 'persisted-session-2026-01-03.json', + ] as any); + + const mockSession = { + version: 99, // Future version + sessionId: mockSessionId, + projectHash: service['getProjectHash'](), + createdAt: '2026-01-03T00:00:00.000Z', + updatedAt: '2026-01-03T00:00:00.000Z', + history: [], + }; + + vi.spyOn(fs.promises, 'readFile').mockResolvedValue(JSON.stringify(mockSession)); + + const result = await service.loadMostRecent(); + + expect(result).toBeNull(); + }); + + it('should handle corrupted JSON gracefully', async () => { + vi.spyOn(fs.promises, 'access').mockResolvedValue(); + vi.spyOn(fs.promises, 'readdir').mockResolvedValue([ + 'persisted-session-2026-01-03.json', + ] as any); + vi.spyOn(fs.promises, 'readFile').mockResolvedValue('{ invalid json }}}'); + const renameSpy = vi.spyOn(fs.promises, 'rename').mockResolvedValue(); + + const result = await service.loadMostRecent(); + + expect(result).toBeNull(); + // Should backup the corrupted file + expect(renameSpy).toHaveBeenCalledWith( + expect.stringContaining('persisted-session'), + expect.stringContaining('.corrupted-') + ); + }); + }); + + describe('formatSessionTime()', () => { + it('should format session time from updatedAt', () => { + const session: PersistedSession = { + version: 1, + sessionId: 'test', + projectHash: 'hash', + createdAt: '2026-01-01T00:00:00.000Z', + updatedAt: '2026-01-03T12:30:00.000Z', + history: [], + }; + + const formatted = SessionPersistenceService.formatSessionTime(session); + + expect(formatted).toContain('2026'); + expect(formatted).toContain('1'); // Month + expect(formatted).toContain('3'); // Day + }); + + it('should fall back to createdAt if updatedAt missing', () => { + const session: PersistedSession = { + version: 1, + sessionId: 'test', + projectHash: 'hash', + createdAt: '2026-01-01T00:00:00.000Z', + updatedAt: '', // Empty + history: [], + }; + + const formatted = SessionPersistenceService.formatSessionTime(session); + + expect(formatted).toContain('2026'); + }); + }); + + describe('getProjectHash()', () => { + it('should generate consistent hash for same project', () => { + const service1 = new SessionPersistenceService(storage, 'session1'); + const service2 = new SessionPersistenceService(storage, 'session2'); + + // Same storage = same project = same hash + expect(service1['getProjectHash']()).toBe(service2['getProjectHash']()); + }); + + it('should generate different hash for different projects', () => { + const storage1 = new Storage('/project1'); + const storage2 = new Storage('/project2'); + + const service1 = new SessionPersistenceService(storage1, 'session'); + const service2 = new SessionPersistenceService(storage2, 'session'); + + expect(service1['getProjectHash']()).not.toBe(service2['getProjectHash']()); + }); + }); +}); +``` + +**File: `packages/cli/src/ui/__tests__/sessionRestore.test.tsx`** + +```typescript +import { describe, it, expect, vi, beforeEach } from 'vitest'; +import { renderHook, act } from '@testing-library/react-hooks'; + +describe('Session Restoration', () => { + const mockAddItem = vi.fn(); + const mockLoadHistory = vi.fn(); + + beforeEach(() => { + vi.clearAllMocks(); + }); + + describe('UI History Restoration', () => { + it('should use saved UI history when available', () => { + const restoredSession = { + history: [/* core history */], + uiHistory: [ + { id: 1, type: 'user', text: 'hello' }, + { id: 2, type: 'gemini', text: 'hi there' }, + ], + }; + + // Test that uiHistory is preferred over conversion + // ... implementation + }); + + it('should convert core history when UI history missing', () => { + const restoredSession = { + history: [ + { speaker: 'human', blocks: [{ type: 'text', text: 'hello' }] }, + ], + uiHistory: undefined, + }; + + // Test that conversion is used as fallback + // ... implementation + }); + + it('should validate UI history items before loading', () => { + const restoredSession = { + history: [], + uiHistory: [ + { id: 1, type: 'user', text: 'valid' }, + { invalid: 'item' }, // Missing required fields + { id: 3, type: 'gemini', text: 'also valid' }, + ], + }; + + // Should filter out invalid item and load only valid ones + // ... implementation + }); + }); + + describe('Core History Restoration', () => { + it('should wait for historyService before restoring', async () => { + // Test polling behavior until historyService available + // ... implementation + }); + + it('should warn user if core history restoration fails', async () => { + // Test error notification to user + // ... implementation + }); + }); +}); +``` + +### Test Categories + +| Category | Purpose | Priority | +|----------|---------|----------| +| Unit: SessionPersistenceService | Core persistence logic | High | +| Unit: History validation | Schema validation | High | +| Unit: Error handling | Graceful degradation | Medium | +| Integration: Full flow | End-to-end --continue | High | +| Edge cases: Corruption | Recovery from bad data | Medium | +| Edge cases: Race conditions | Concurrent saves | Low | + +--- + +## Summary + +| Issue | Severity | Effort | Priority | +|-------|----------|--------|----------| +| #2: Memory leak | Medium | Low | P1 | +| #4: Type safety | Low | Medium | P3 | +| #6: Silent failure | High | Low | P1 | +| #7: No validation | High | Medium | P1 | +| #8: Sync fs call | Low | Low | P3 | +| #10: Flag interaction | Medium | Low | P2 | +| #11: No tests | High | High | P1 | + +**Recommended implementation order:** +1. #11 (Tests) - Enables safe refactoring +2. #6 + #7 (Error handling + validation) - Prevents data loss/crashes +3. #2 (Memory leak) - Quick fix, prevents resource issues +4. #10 (Flag interaction) - UX clarity +5. #4 + #8 (Type safety + async) - Code quality From ddf5dc00f4472f37c1a18b9fc4cab7464efadd0e Mon Sep 17 00:00:00 2001 From: sogoiii Date: Sat, 3 Jan 2026 23:56:53 -0800 Subject: [PATCH 3/3] =?UTF-8?q?[cli,core]=20=F0=9F=90=9B=20fix:=20improve?= =?UTF-8?q?=20session=20restoration=20with=20timeout=20and=20fix=20test=20?= =?UTF-8?q?mocks?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Add 30s timeout to core history restoration polling to prevent infinite waits when historyService unavailable - Fix test mock for directory-not-exist case to properly use readdir ENOENT error instead of unused existsSync - Remove redundant existsSync mocks from all loadMostRecent tests --- packages/cli/src/ui/AppContainer.tsx | 18 ++++++++++++++++++ .../storage/SessionPersistenceService.test.ts | 14 +++----------- 2 files changed, 21 insertions(+), 11 deletions(-) diff --git a/packages/cli/src/ui/AppContainer.tsx b/packages/cli/src/ui/AppContainer.tsx index 83252c82e..99e9b07bb 100644 --- a/packages/cli/src/ui/AppContainer.tsx +++ b/packages/cli/src/ui/AppContainer.tsx @@ -668,6 +668,22 @@ export const AppContainer = (props: AppContainerProps) => { return; } + const TIMEOUT_MS = 30000; // 30 second timeout + + const timeout = setTimeout(() => { + clearInterval(checkInterval); + debug.warn( + 'Timed out waiting for history service to restore core history', + ); + addItem( + { + type: 'warning', + text: 'Could not restore AI context - history service unavailable.', + }, + Date.now(), + ); + }, TIMEOUT_MS); + const checkInterval = setInterval(() => { const geminiClient = config.getGeminiClient(); const historyService = geminiClient?.getHistoryService?.(); @@ -677,6 +693,7 @@ export const AppContainer = (props: AppContainerProps) => { } clearInterval(checkInterval); + clearTimeout(timeout); coreHistoryRestoredRef.current = true; try { @@ -701,6 +718,7 @@ export const AppContainer = (props: AppContainerProps) => { return () => { clearInterval(checkInterval); + clearTimeout(timeout); }; }, [restoredSession, config, addItem]); diff --git a/packages/core/src/storage/SessionPersistenceService.test.ts b/packages/core/src/storage/SessionPersistenceService.test.ts index 8a96f5ee2..bcd5eed17 100644 --- a/packages/core/src/storage/SessionPersistenceService.test.ts +++ b/packages/core/src/storage/SessionPersistenceService.test.ts @@ -12,7 +12,6 @@ vi.mock('node:fs', async (importOriginal) => { const actual = await importOriginal(); return { ...actual, - existsSync: vi.fn(), promises: { ...actual.promises, mkdir: vi.fn(), @@ -216,7 +215,9 @@ describe('SessionPersistenceService', () => { crypto.createHash('sha256').update(mockProjectRoot).digest('hex'); it('should return null if chats directory does not exist', async () => { - vi.mocked(fs.existsSync).mockReturnValue(false); + const enoentError = new Error('ENOENT') as NodeJS.ErrnoException; + enoentError.code = 'ENOENT'; + vi.mocked(fs.promises.readdir).mockRejectedValue(enoentError); const result = await service.loadMostRecent(); @@ -224,7 +225,6 @@ describe('SessionPersistenceService', () => { }); it('should return null if no session files exist', async () => { - vi.mocked(fs.existsSync).mockReturnValue(true); vi.mocked(fs.promises.readdir).mockResolvedValue([] as unknown as []); const result = await service.loadMostRecent(); @@ -233,7 +233,6 @@ describe('SessionPersistenceService', () => { }); it('should ignore non-session files', async () => { - vi.mocked(fs.existsSync).mockReturnValue(true); vi.mocked(fs.promises.readdir).mockResolvedValue([ 'other-file.json', 'persisted-session-backup.json.bak', @@ -246,7 +245,6 @@ describe('SessionPersistenceService', () => { }); it('should load the most recent session file (sorted by filename)', async () => { - vi.mocked(fs.existsSync).mockReturnValue(true); vi.mocked(fs.promises.readdir).mockResolvedValue([ 'persisted-session-2026-01-01T00-00-00-000Z.json', 'persisted-session-2026-01-03T00-00-00-000Z.json', // Most recent @@ -276,7 +274,6 @@ describe('SessionPersistenceService', () => { }); it('should reject session with wrong project hash', async () => { - vi.mocked(fs.existsSync).mockReturnValue(true); vi.mocked(fs.promises.readdir).mockResolvedValue([ 'persisted-session-2026-01-03T00-00-00-000Z.json', ] as unknown as []); @@ -300,7 +297,6 @@ describe('SessionPersistenceService', () => { }); it('should reject session with unknown version', async () => { - vi.mocked(fs.existsSync).mockReturnValue(true); vi.mocked(fs.promises.readdir).mockResolvedValue([ 'persisted-session-2026-01-03T00-00-00-000Z.json', ] as unknown as []); @@ -324,7 +320,6 @@ describe('SessionPersistenceService', () => { }); it('should handle corrupted JSON gracefully and backup', async () => { - vi.mocked(fs.existsSync).mockReturnValue(true); vi.mocked(fs.promises.readdir).mockResolvedValue([ 'persisted-session-2026-01-03T00-00-00-000Z.json', ] as unknown as []); @@ -342,7 +337,6 @@ describe('SessionPersistenceService', () => { }); it('should return session with UI history when present', async () => { - vi.mocked(fs.existsSync).mockReturnValue(true); vi.mocked(fs.promises.readdir).mockResolvedValue([ 'persisted-session-2026-01-03T00-00-00-000Z.json', ] as unknown as []); @@ -372,7 +366,6 @@ describe('SessionPersistenceService', () => { }); it('should handle readdir failure gracefully', async () => { - vi.mocked(fs.existsSync).mockReturnValue(true); vi.mocked(fs.promises.readdir).mockRejectedValue( new Error('Permission denied'), ); @@ -383,7 +376,6 @@ describe('SessionPersistenceService', () => { }); it('should handle readFile failure gracefully', async () => { - vi.mocked(fs.existsSync).mockReturnValue(true); vi.mocked(fs.promises.readdir).mockResolvedValue([ 'persisted-session-2026-01-03T00-00-00-000Z.json', ] as unknown as []);