reafctor: renaming persistence store for clarity#1653
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis pull request systematically renames the "conversation" domain to "session" throughout the codebase, affecting API endpoints, type definitions, storage interfaces, internal managers, configuration schemas, and UI components. The core functionality remains unchanged; all references to conversations are replaced with sessions with consistent naming. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/cmn/config/definition.go (1)
175-192:⚠️ Potential issue | 🟠 MajorAdd legacy
conversationsDirmapping to prevent breaking existing configs.The rename from
conversationsDirtosessionsDirbreaks existing configurations. No migration path exists inloadLegacyPaths(), which handles legacy field mappings for other paths. AddconversationsDirto the legacy paths list or keep both fields with a deprecated alias to maintain backward compatibility.
🤖 Fix all issues with AI agents
In `@internal/agent/session.go`:
- Around line 193-203: GetSession currently returns CreatedAt from the mutable
sm.lastActivity, causing the creation time to drift; add a new immutable field
(e.g., createdAt) to SessionManager, initialize it in NewSessionManager when the
manager is constructed, and update GetSession to set CreatedAt to sm.createdAt
(leave UpdatedAt as sm.lastActivity); make the analogous change in
SubscribeWithSnapshot so that snapshots use sm.createdAt for CreatedAt instead
of sm.lastActivity.
In `@internal/cmn/config/loader.go`:
- Around line 956-958: The default sessions path changed from
"agent/conversations" to "agent/sessions", which will orphan existing data;
update finalizePaths (or startup path initialization where cfg.Paths.SessionsDir
is set) to perform a one-time migration: if cfg.Paths.SessionsDir is empty set
it as now (filepath.Join(cfg.Paths.DataDir, "agent", "sessions")), then check
for an existing oldDir := filepath.Join(cfg.Paths.DataDir, "agent",
"conversations") and if oldDir exists and newDir does not, atomically
rename/move oldDir to newDir (handle errors and permissions), otherwise leave
existing newDir intact; reference cfg.Paths.SessionsDir and the finalizePaths
initialization code to implement this migration.
In `@internal/persis/filesession/store.go`:
- Around line 194-197: The sessionFilePath currently constructs paths using
userID and sessionID directly, allowing path traversal; add a validation helper
(e.g., validateSessionIDs or sanitizeID) and call it from validateSession (or
before any call to sessionFilePath) to reject or return an error for IDs
containing path separators ('/', '\') or path traversal segments (".."); ensure
both userID and sessionID are checked and that Store.sessionFilePath continues
to assume validated IDs so callers cannot escape Store.baseDir using crafted
IDs.
In `@internal/service/frontend/server.go`:
- Line 452: The audit field was renamed from conversation_id to session_id which
breaks downstream queries; update the audit population to set both keys so
existing consumers keep working by assigning info.SessionID to both
details["session_id"] and details["conversation_id"] (locate where the details
map is populated and where info.SessionID is used), ensuring both keys are
emitted for backward compatibility.
🧹 Nitpick comments (9)
internal/runtime/builtin/chat/tools.go (1)
1-1: Comment wording aligns with PR objectives.The terminology update from "conversation" to "session" is consistent with the PR's refactoring goals.
The phrase "chat (LLM-based session) steps" is grammatically correct but could be simplified for clarity. Consider:
-// Package chat provides an executor for chat (LLM-based session) steps. +// Package chat provides an executor for LLM-based session steps.or
-// Package chat provides an executor for chat (LLM-based session) steps. +// Package chat provides an executor for chat steps using LLM-based sessions.internal/agent/hooks_test.go (1)
29-36: Test data still uses"conv-1"forSessionID.The field was renamed to
SessionID, but the test value still uses the old"conv-"prefix. Consider updating to"sess-1"for consistency with the new terminology — the AI summary mentions other tests already usesessX-style IDs.Suggested fix
info := ToolExecInfo{ ToolName: "bash", Input: json.RawMessage(`{"command":"ls"}`), - SessionID: "conv-1", + SessionID: "sess-1", UserID: "user-1", Username: "alice", IPAddress: "10.0.0.1", }And the corresponding assertion:
- assert.Equal(t, "conv-1", captured.info.SessionID) + assert.Equal(t, "sess-1", captured.info.SessionID)internal/cmn/config/loader_test.go (1)
536-547:TestLoad_EdgeCases_DerivedPathsdoesn't assertSessionsDir.
SessionsDiris derived fromDataDir(as seen in the other tests at Lines 184 and 442), but this dedicated derived-paths test doesn't verify it. Consider adding an assertion for completeness.Suggested addition
assert.Equal(t, "/custom/data/service-registry", cfg.Paths.ServiceRegistryDir) assert.Equal(t, "/custom/data/users", cfg.Paths.UsersDir) + assert.Equal(t, "/custom/data/agent/sessions", cfg.Paths.SessionsDir) }ui/src/features/agent/types.ts (1)
81-99: Verify UI session types stay aligned with backend payloads.
BackendSessionincludesuser_idandtitle, andSessionWithStateincludestotal_cost. If those fields are still returned/used, add them here to avoid silent drops or TS friction.💡 Suggested parity update
export interface Session { id: string; + user_id?: string; + title?: string; created_at: string; updated_at: string; } export interface SessionWithState { session: Session; working: boolean; model?: string; + total_cost?: number; }internal/service/frontend/agent_audit_test.go (1)
21-28: Test fixture values still use "conv-" prefix for session IDs.The
SessionIDfields use values like"conv-123","conv-456","conv-789". While functionally harmless, these could be updated to"sess-123"etc. to avoid confusion now that the domain term is "session."ui/src/features/agent/components/AgentChatModal.tsx (1)
126-135:totalCostsourced fromsessionState?.total_cost— note frontend/backend type gap onSessionWithState.The
totalCostprop is correctly sourced fromsessionState?.total_cost(which is of typeSessionState). However, the backend GoSessionWithStatestruct includes aTotalCost float64field (json:"total_cost") atinternal/agent/api.go:92, but the frontend TypeScriptSessionWithStateinterface (ui/src/features/agent/types.ts:94-98) omitstotal_cost. If you intend to display per-session cost in the session list in the future, the TS type will need updating.internal/persis/filesession/store.go (1)
271-296:ListSessionsre-reads every session file from disk individually.For each session ID,
ListSessionscallsGetSessionwhich callsloadSessionByID→loadSessionFromFile, deserializing the full JSON (including all messages) just to return session metadata. For users with many sessions, this is N file reads with full message deserialization.This is acceptable for a file-based store with small-to-moderate usage, but worth noting for future scalability. An alternative would be to cache session metadata in the index or store metadata separately from messages.
ui/src/features/agent/hooks/useAgentChat.ts (1)
146-159:startSessionimmediately fetches all sessions after creation — consider optimistic update.After creating a new session,
startSessionmakes a second API call to fetch the full sessions list. This works correctly but adds latency. An optimistic local insert (appending the new session to the existing list) could improve perceived responsiveness, with a background refresh for consistency.This is not a blocking concern for a rename PR.
internal/agent/session.go (1)
409-440:createRecordMessageFuncsilently swallows persistence errors.The returned function always returns
nil(line 438), even whensm.onMessagefails (line 433-435). The error is logged but not propagated to the Loop. This means the Loop will continue operating normally even if every message fails to persist.If this is intentional (session continues despite persistence failures), consider adding a brief comment explaining the design decision. If persistence failures should eventually stop the session, the error should be returned.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1653 +/- ##
==========================================
- Coverage 70.21% 69.93% -0.29%
==========================================
Files 344 344
Lines 38307 38316 +9
==========================================
- Hits 26899 26798 -101
- Misses 9274 9289 +15
- Partials 2134 2229 +95
... and 4 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
1 participant
Summary by CodeRabbit
Release Notes
Refactoring
/conversations/to/sessions/endpoints