-
-
Notifications
You must be signed in to change notification settings - Fork 925
feat: Add session management with conversation history and provider affinity #829
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
feat: Add session management with conversation history and provider affinity #829
Conversation
…ffinity Implements cross-provider session continuity allowing conversation history to persist across requests with automatic provider preference. New Features: - Session management with 24-hour TTL (configurable) - Conversation history auto-injection via X-Session-ID header - Provider affinity (prefers provider used in session) - Management API for session CRUD operations - File-based storage with atomic writes and background cleanup Architecture: - Core session package (types, store, manager, inject) - Handler integration (automatic history management) - Management API (4 new endpoints) - Provider affinity in auth conductor - Graceful degradation on failures Breaking Changes: None (feature is opt-in via X-Session-ID header) Files: 14 total (7 new, 7 modified) Lines: ~1,500 added
Summary of ChangesHello @chindris-mihai-alexandru, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the CLIProxyAPI by integrating a robust session management system. The primary goal is to provide seamless conversation continuity across multiple requests by automatically managing and injecting chat history. This feature is opt-in and includes mechanisms for provider affinity, ensuring that subsequent requests within a session prefer the same AI provider. Additionally, a new management API allows for comprehensive control over active sessions, while the underlying file-based storage is designed for reliability and performance with atomic operations and background cleanup. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces a comprehensive session management feature, including conversation history, provider affinity, and a management API. The overall architecture is well-designed, with a clear separation of concerns between the session store, manager, and integration points. However, the review identified several critical and high-severity issues related to thread safety in the file store, correctness of the provider affinity logic, and accurate token accounting. There are also opportunities to improve maintainability by refactoring duplicated code. Addressing these points is crucial for the stability and correctness of this new feature.
| s.mu.RLock() | ||
| path := filepath.Join(s.baseDir, id+".json") | ||
| s.mu.RUnlock() | ||
|
|
||
| data, err := os.ReadFile(path) | ||
| if err != nil { | ||
| if os.IsNotExist(err) { | ||
| return nil, nil // Session doesn't exist | ||
| } | ||
| return nil, fmt.Errorf("session filestore: read failed: %w", err) | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The read lock is released before os.ReadFile is called. This creates a race condition where another goroutine could delete or modify the file after the lock is released but before it's read. The read lock should be held for the entire duration of the file operation to ensure atomicity.
| s.mu.RLock() | |
| path := filepath.Join(s.baseDir, id+".json") | |
| s.mu.RUnlock() | |
| data, err := os.ReadFile(path) | |
| if err != nil { | |
| if os.IsNotExist(err) { | |
| return nil, nil // Session doesn't exist | |
| } | |
| return nil, fmt.Errorf("session filestore: read failed: %w", err) | |
| } | |
| s.mu.RLock() | |
| defer s.mu.RUnlock() | |
| path := filepath.Join(s.baseDir, id+".json") | |
| data, err := os.ReadFile(path) | |
| if err != nil { | |
| if os.IsNotExist(err) { | |
| return nil, nil // Session doesn't exist | |
| } | |
| return nil, fmt.Errorf("session filestore: read failed: %w", err) | |
| } |
| if msg.Role == "assistant" && msg.Provider != "" { | ||
| s.Metadata.LastProvider = msg.Provider | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The AddMessage function updates s.Metadata.LastProvider, but the provider affinity logic in sdk/api/handlers/handlers.go reads from s.Metadata.PreferredProvider. This disconnect will cause the provider affinity feature to fail. To fix this, AddMessage should also update PreferredProvider to ensure the last used provider is preferred in the next request.
if msg.Role == "assistant" && msg.Provider != "" {
s.Metadata.LastProvider = msg.Provider
s.Metadata.PreferredProvider = msg.Provider
}
sdk/cliproxy/session/inject.go
Outdated
| // Try total_token_count (Gemini) | ||
| if tokens, ok := usage["total_token_count"].(float64); ok { | ||
| return int(tokens) | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For Gemini responses, extractTokenUsage incorrectly uses total_token_count. This field represents the sum of prompt and completion tokens. To accurately track the token consumption for just the assistant's message, you should parse the usageMetadata object in the Gemini response and use the candidatesTokenCount field instead. Using the total count will lead to incorrect token accounting for the session.
| s.mu.RLock() | ||
| baseDir := s.baseDir | ||
| s.mu.RUnlock() | ||
|
|
||
| sessions := make([]*Session, 0) | ||
|
|
||
| err := filepath.WalkDir(baseDir, func(path string, d fs.DirEntry, walkErr error) error { | ||
| if walkErr != nil { | ||
| // Skip files we can't read | ||
| return nil | ||
| } | ||
|
|
||
| if d.IsDir() { | ||
| return nil | ||
| } | ||
|
|
||
| // Only process .json files | ||
| if !strings.HasSuffix(strings.ToLower(d.Name()), ".json") { | ||
| return nil | ||
| } | ||
|
|
||
| data, err := os.ReadFile(path) | ||
| if err != nil { | ||
| // Skip files we can't read | ||
| return nil | ||
| } | ||
|
|
||
| if len(data) == 0 { | ||
| return nil | ||
| } | ||
|
|
||
| var session Session | ||
| if err := json.Unmarshal(data, &session); err != nil { | ||
| // Skip malformed files | ||
| return nil | ||
| } | ||
|
|
||
| // Only include non-expired sessions | ||
| if !session.IsExpired() { | ||
| sessions = append(sessions, &session) | ||
| } | ||
|
|
||
| return nil | ||
| }) | ||
|
|
||
| if err != nil { | ||
| return nil, fmt.Errorf("session filestore: list failed: %w", err) | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The read lock is released before filepath.WalkDir is called. This can lead to race conditions where session files are modified or deleted by other goroutines while the directory is being walked, resulting in inconsistent results or errors. The read lock should be held for the entire duration of the directory walk to ensure a consistent view of the session files.
s.mu.RLock()
defer s.mu.RUnlock()
baseDir := s.baseDir
sessions := make([]*Session, 0)
err := filepath.WalkDir(baseDir, func(path string, d fs.DirEntry, walkErr error) error {
if walkErr != nil {
// Log or propagate the error instead of ignoring it.
return walkErr
}
if d.IsDir() || !strings.HasSuffix(strings.ToLower(d.Name()), ".json") {
return nil
}
data, err := os.ReadFile(path)
if err != nil {
// Skip files that can't be read, but consider logging.
return nil
}
if len(data) == 0 {
return nil
}
var session Session
if err := json.Unmarshal(data, &session); err != nil {
// Skip malformed files, but consider logging.
return nil
}
if !session.IsExpired() {
sessions = append(sessions, &session)
}
return nil
})
if err != nil {
return nil, fmt.Errorf("session filestore: list failed: %w", err)
}
sdk/api/handlers/handlers.go
Outdated
| provider := "unknown" | ||
| saveAssistantMessage(ctx, h.SessionManager, sessionID, resp.Payload, provider, normalizedModel) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The provider is hardcoded to "unknown" when saving the assistant's message. This breaks the provider affinity feature, as the session will not be able to determine which provider to prefer for subsequent requests. The actual provider that handled the request should be identified and saved.
You can likely retrieve the AuthID from the resp object and use h.AuthManager.GetByID(resp.AuthID) to find the Auth object and its associated provider.
provider := "unknown"
if resp.AuthID != "" {
if auth, ok := h.AuthManager.GetByID(resp.AuthID); ok {
provider = auth.Provider
} else {
log.Warnf("Could not find auth with ID %s to determine provider for session", resp.AuthID)
}
}
saveAssistantMessage(ctx, h.SessionManager, sessionID, resp.Payload, provider, normalizedModel)
SESSION_INTEGRATION_COMPLETE.md
Outdated
| @@ -0,0 +1,642 @@ | |||
| # Session Management Integration - COMPLETE | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| if walkErr != nil { | ||
| // Skip files we can't read | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sdk/api/handlers/handlers.go
Outdated
| return | ||
| } | ||
|
|
||
| _ = mgr.AppendMessage(ctx, sessionID, *msg) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The error returned by mgr.AppendMessage is being ignored. While the design is to not fail the request on a session error, this error should be logged to help diagnose potential issues with session persistence.
if err := mgr.AppendMessage(ctx, sessionID, *msg); err != nil {
log.WithError(err).Warn("Failed to append message to session")
}
sdk/cliproxy/session/manager.go
Outdated
| case <-m.cleanupTicker.C: | ||
| // Run cleanup in background | ||
| ctx := context.Background() | ||
| _, _ = m.Cleanup(ctx) // Ignore errors in background cleanup |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The background cleanup goroutine ignores errors returned from m.Cleanup(ctx). While it's reasonable not to stop the goroutine, these errors should be logged to provide visibility into any persistent issues with the session cleanup process.
| _, _ = m.Cleanup(ctx) // Ignore errors in background cleanup | |
| if _, err := m.Cleanup(ctx); err != nil { | |
| log.WithError(err).Warn("Background session cleanup failed") | |
| } |
| if err != nil { | ||
| c.JSON(http.StatusInternalServerError, gin.H{"error": err.Error()}) | ||
| return |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Returning err.Error() directly in the JSON response can expose internal implementation details and potential vulnerabilities. It's better to log the detailed error for debugging purposes and return a generic error message to the client.
| if err != nil { | |
| c.JSON(http.StatusInternalServerError, gin.H{"error": err.Error()}) | |
| return | |
| if err != nil { | |
| log.WithError(err).Error("Failed to list sessions") | |
| c.JSON(http.StatusInternalServerError, gin.H{"error": "failed to retrieve sessions"}) | |
| return | |
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR implements a comprehensive session management system for the CLIProxyAPI, enabling conversation history persistence and provider affinity across requests. The feature is opt-in via the X-Session-ID header and maintains full backward compatibility.
Key changes include:
- Session management infrastructure with file-based persistence, 24-hour TTL, and automatic cleanup
- Conversation history injection that prepends past messages to new requests
- Provider affinity system that prioritizes the previously-used provider for session continuity
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 19 comments.
Show a summary per file
| File | Description |
|---|---|
sdk/config/config.go |
Exports SessionConfig type for public API |
sdk/cliproxy/session/types.go |
Defines core Session, Message, and SessionMetadata structures |
sdk/cliproxy/session/store.go |
Defines Store interface for session persistence |
sdk/cliproxy/session/manager.go |
Implements session lifecycle management with cleanup goroutine |
sdk/cliproxy/session/inject.go |
Handles history injection and response extraction for multiple providers |
sdk/cliproxy/session/filestore.go |
Implements file-based session storage with atomic writes |
sdk/cliproxy/pipeline/context.go |
Adds SessionID and Session fields to pipeline context |
sdk/cliproxy/auth/conductor.go |
Implements provider affinity by reordering provider rotation |
sdk/api/handlers/handlers.go |
Integrates session loading, history injection, and response saving |
internal/config/sdk_config.go |
Adds SessionConfig with storage, TTL, and cleanup settings |
internal/api/server.go |
Initializes session manager and registers management routes |
internal/api/handlers/management/sessions.go |
Implements 4 session management API endpoints |
internal/api/handlers/management/handler.go |
Adds sessionManager field and setter method |
SESSION_INTEGRATION_COMPLETE.md |
Documentation file describing the implementation |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| func (s *FileStore) Save(ctx context.Context, session *Session) (string, error) { | ||
| if session == nil { | ||
| return "", fmt.Errorf("session filestore: session is nil") | ||
| } | ||
| if session.ID == "" { | ||
| return "", fmt.Errorf("session filestore: session ID is empty") | ||
| } | ||
|
|
||
| s.mu.Lock() | ||
| defer s.mu.Unlock() | ||
|
|
||
| path := filepath.Join(s.baseDir, session.ID+".json") |
Copilot
AI
Jan 1, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The same path traversal vulnerability exists in the Save method. Session ID validation should be performed before constructing the file path.
| // Initialize session manager if enabled in config | ||
| if cfg.SDKConfig.Session.Enabled { | ||
| storageDir := cfg.SDKConfig.Session.StorageDir | ||
| if storageDir == "" { | ||
| storageDir = "~/.cli-proxy-api/sessions" | ||
| } | ||
| store, err := coresession.NewFileStore(storageDir) | ||
| if err != nil { | ||
| log.Warnf("Failed to create session store: %v (sessions disabled)", err) | ||
| } else { | ||
| ttl := 24 * time.Hour | ||
| if cfg.SDKConfig.Session.DefaultTTLHours > 0 { | ||
| ttl = time.Duration(cfg.SDKConfig.Session.DefaultTTLHours) * time.Hour | ||
| } | ||
| cleanupInterval := 1 * time.Hour | ||
| if cfg.SDKConfig.Session.CleanupIntervalHours > 0 { | ||
| cleanupInterval = time.Duration(cfg.SDKConfig.Session.CleanupIntervalHours) * time.Hour | ||
| } | ||
|
|
||
| sessionManager, err := coresession.NewManager(coresession.Config{ | ||
| Store: store, | ||
| DefaultTTL: ttl, | ||
| CleanupInterval: cleanupInterval, | ||
| }) | ||
| if err != nil { | ||
| log.Warnf("Failed to create session manager: %v (sessions disabled)", err) | ||
| } else { | ||
| s.sessionManager = sessionManager | ||
| s.handlers.SessionManager = sessionManager | ||
| s.mgmt.SetSessionManager(sessionManager) | ||
| log.Infof("Session management enabled (storage: %s, ttl: %v)", storageDir, ttl) | ||
| } | ||
| } | ||
| } |
Copilot
AI
Jan 1, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The session manager doesn't have a graceful shutdown mechanism. When the server shuts down, the cleanup goroutine should be stopped to prevent it from running during shutdown. Consider adding a Close or Shutdown method that calls StopCleanup, and ensure it's called in the server's shutdown sequence.
| // extractSessionID extracts the X-Session-ID header from the Gin context. | ||
| func extractSessionID(ctx context.Context) string { | ||
| if ctx == nil { | ||
| return "" | ||
| } | ||
| if ginCtx, ok := ctx.Value("gin").(*gin.Context); ok && ginCtx != nil && ginCtx.Request != nil { | ||
| return strings.TrimSpace(ginCtx.GetHeader("X-Session-ID")) | ||
| } | ||
| return "" | ||
| } |
Copilot
AI
Jan 1, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The extractSessionID function accesses the Gin context through a context value lookup, but there's no guarantee that the "gin" key exists or contains the expected type. If the key is missing or contains a different type, this could cause runtime panics when dereferencing nil pointers. Consider adding additional nil checks after the type assertion.
| if err := os.MkdirAll(baseDir, 0o700); err != nil { | ||
| return nil, fmt.Errorf("session filestore: create directory failed: %w", err) | ||
| } | ||
|
|
Copilot
AI
Jan 1, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The session storage directory is created with permissions 0o700 (owner-only access), but there's no validation that the directory is actually secure. If the directory already exists with more permissive permissions (e.g., 0o777), those permissions won't be changed. Consider checking and enforcing the correct permissions on existing directories.
| // Ensure the directory exists, is a directory, and has secure permissions. | |
| info, err := os.Stat(baseDir) | |
| if err != nil { | |
| return nil, fmt.Errorf("session filestore: stat directory failed: %w", err) | |
| } | |
| if !info.IsDir() { | |
| return nil, fmt.Errorf("session filestore: %s is not a directory", baseDir) | |
| } | |
| if info.Mode().Perm() != 0o700 { | |
| if err := os.Chmod(baseDir, 0o700); err != nil { | |
| return nil, fmt.Errorf("session filestore: enforce directory permissions failed: %w", err) | |
| } | |
| } |
| // Write to temp file first, then atomic rename | ||
| tmp := path + ".tmp" | ||
| if err := os.WriteFile(tmp, data, 0o600); err != nil { | ||
| return "", fmt.Errorf("session filestore: write temp failed: %w", err) | ||
| } | ||
|
|
||
| if err := os.Rename(tmp, path); err != nil { | ||
| os.Remove(tmp) // Clean up temp file on error | ||
| return "", fmt.Errorf("session filestore: rename failed: %w", err) | ||
| } |
Copilot
AI
Jan 1, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Session files are created with permissions 0o600 (owner read/write only), but the temporary files are also created with the same permissions. However, if os.Rename fails and the cleanup of the temp file also fails, there could be leftover temp files with sensitive data. Consider using defer to ensure temp file cleanup, or at least log when cleanup fails.
| // Session doesn't exist or expired, create new one with the requested ID | ||
| now := time.Now() | ||
| session = &Session{ | ||
| ID: id, | ||
| CreatedAt: now, | ||
| UpdatedAt: now, | ||
| ExpiresAt: now.Add(m.defaultTTL), | ||
| Messages: []Message{}, | ||
| Metadata: SessionMetadata{}, | ||
| } | ||
|
|
||
| if _, err := m.store.Save(ctx, session); err != nil { | ||
| return nil, fmt.Errorf("session manager: create failed: %w", err) | ||
| } | ||
|
|
||
| return session, nil |
Copilot
AI
Jan 1, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The GetOrCreate method creates a new session with the client-provided ID when the session doesn't exist. This could allow clients to create sessions with arbitrary IDs, potentially causing collisions or security issues. Consider validating the session ID format (e.g., must be a valid UUID) or always generating new IDs server-side.
| // Session doesn't exist or expired, create new one with the requested ID | |
| now := time.Now() | |
| session = &Session{ | |
| ID: id, | |
| CreatedAt: now, | |
| UpdatedAt: now, | |
| ExpiresAt: now.Add(m.defaultTTL), | |
| Messages: []Message{}, | |
| Metadata: SessionMetadata{}, | |
| } | |
| if _, err := m.store.Save(ctx, session); err != nil { | |
| return nil, fmt.Errorf("session manager: create failed: %w", err) | |
| } | |
| return session, nil | |
| // Session doesn't exist or expired; create a new session with a server-generated ID | |
| return m.Create(ctx) |
sdk/cliproxy/session/filestore.go
Outdated
| func (s *FileStore) Get(ctx context.Context, id string) (*Session, error) { | ||
| id = strings.TrimSpace(id) | ||
| if id == "" { | ||
| return nil, fmt.Errorf("session filestore: id is empty") | ||
| } | ||
|
|
||
| s.mu.RLock() | ||
| path := filepath.Join(s.baseDir, id+".json") | ||
| s.mu.RUnlock() |
Copilot
AI
Jan 1, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The FileStore Get method lacks validation of the session ID format. Malicious input could potentially cause directory traversal issues if the ID contains path separators. Consider validating that the ID contains only safe characters (alphanumeric, hyphens, underscores) before using it in filepath operations.
| func (s *FileStore) Delete(ctx context.Context, id string) error { | ||
| id = strings.TrimSpace(id) | ||
| if id == "" { | ||
| return fmt.Errorf("session filestore: id is empty") | ||
| } | ||
|
|
||
| s.mu.Lock() | ||
| defer s.mu.Unlock() | ||
|
|
||
| path := filepath.Join(s.baseDir, id+".json") |
Copilot
AI
Jan 1, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Delete method also needs session ID validation to prevent path traversal attacks.
| // Convert session history to generic message format | ||
| historyMsgs := make([]interface{}, 0, len(session.Messages)) | ||
| for _, msg := range session.Messages { | ||
| historyMsg := map[string]interface{}{ | ||
| "role": msg.Role, | ||
| "content": msg.Content, | ||
| } | ||
| historyMsgs = append(historyMsgs, historyMsg) | ||
| } | ||
|
|
||
| // Prepend history to existing messages | ||
| allMessages := append(historyMsgs, existingMsgs...) | ||
| req["messages"] = allMessages |
Copilot
AI
Jan 1, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The InjectHistory function prepends all session history to every request. For long conversations, this could significantly increase request size and token usage. Consider implementing a message window limit (e.g., last N messages) or allowing configuration of history depth to prevent unbounded growth.
sdk/cliproxy/session/manager.go
Outdated
| return fmt.Errorf("session manager: session not found: %s", sessionID) | ||
| } | ||
|
|
||
| session.ExpiresAt = session.ExpiresAt.Add(duration) |
Copilot
AI
Jan 1, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The ExtendTTL function extends the expiration time from the current ExpiresAt, not from the current time. This means that if a session is already expired, extending it won't make it valid again. Consider using session.ExpiresAt = time.Now().Add(duration) to extend from the current time, or session.ExpiresAt = max(session.ExpiresAt, time.Now()).Add(duration) to ensure the session is valid after extension.
| session.ExpiresAt = session.ExpiresAt.Add(duration) | |
| base := session.ExpiresAt | |
| now := time.Now() | |
| if now.After(base) { | |
| base = now | |
| } | |
| session.ExpiresAt = base.Add(duration) |
Fixes issues identified by GitHub Copilot and Gemini Code Assist: Security: - Add session ID validation (alphanumeric, hyphens, underscores only) - Prevent path traversal attacks in file operations - Return generic error messages to API clients (log details internally) Thread Safety: - Hold RLock during entire file read/list operations - Fix race conditions in FileStore.Get() and FileStore.List() Provider Affinity: - Store provider in response metadata during execution - Extract provider from metadata for session tracking - Update PreferredProvider when adding assistant messages Error Handling: - Log errors for AppendMessage failures - Log errors for background cleanup failures - Use log.WithError() pattern consistently Code Quality: - Extract duplicated session logic into prepareSessionContext() helper - Add sessionContext struct for cleaner data flow - Add applySessionAffinity() method to sessionContext - Fix Gemini token extraction (use candidatesTokenCount) - Fix ExtendTTL to extend from current time if session expired - Add graceful shutdown for session manager cleanup goroutine - Remove development documentation artifact All changes verified with successful build (44MB executable).
- Use sync.Once in StopCleanup to prevent race condition on channel close - Optimize GetHistory to return slice directly without copying (callers must not mutate) - Extract applySessionAffinity helper to reduce code duplication in conductor.go - Use in-place swap instead of slice append for provider reordering
Response to AI Review Comments on Commit c96e6d2The remaining AI reviewer comments are false positives - the issues they flag are already addressed in this commit: 1. StopCleanup Race Condition (Copilot - manager.go:239-252)Already fixed. The m.cleanupOnce.Do(func() {
close(m.cleanupStop)
})This ensures the channel is closed exactly once, preventing the panic the reviewer warns about. 2. PreferredProvider Not Set (Gemini - types.go:69-73)Already fixed. Line 72 already sets s.Metadata.PreferredProvider = msg.ProviderThis is exactly what Gemini suggests adding, but it's already there. 3. GetOrCreate Client ID (Copilot - manager.go:117-132)By design. Client-provided session IDs are validated by All high and medium severity issues from the initial review have been addressed. The remaining comments are either already fixed or intentional design choices. |
Add unit tests covering: - FileStore: CRUD operations, atomic writes, session ID validation, concurrent access safety, cleanup of expired sessions - Manager: session lifecycle, TTL management, GetOrCreate behavior, cleanup goroutine, idempotent StopCleanup - Inject: history injection for OpenAI/Claude/Gemini formats, assistant message extraction, token usage parsing - Types: Session expiration, message handling, metadata updates All 64 tests pass with 100% coverage of critical paths.
Summary
This PR implements cross-provider session continuity, allowing conversation history to persist across requests with automatic provider preference.
New Features
Architecture
Technical Details
Testing
Breaking Changes
None - feature is completely opt-in via X-Session-ID header
Files Changed
Configuration Example
Usage Example