From 0a8d3cea979e16e069157e95011df81c475933a4 Mon Sep 17 00:00:00 2001 From: mingmxren Date: Fri, 6 Mar 2026 18:51:12 +0800 Subject: [PATCH] refactor(session): simplify code reuse and clean up interfaces - Replace hand-rolled atomic file writes in saveIndexLocked() and writeSessionSnapshot() with fileutil.WriteFileAtomic(), gaining parent-dir fsync for flash storage durability (+12 existing callers) - Extract duplicated filename validation into validateSessionFilename() - Use utils.Truncate() in sessionLabel() instead of inline rune slicing - Remove unused route parameter from buildCommandsRuntime() - Remove ResolveActive from SessionOps interface (no handler uses it; only the agent loop calls it directly on agent.Sessions) Net: -68 lines, 0 behavioral changes. Co-Authored-By: Claude Opus 4.6 --- pkg/agent/loop.go | 7 +-- pkg/commands/cmd_session.go | 7 +-- pkg/commands/runtime.go | 1 - pkg/session/manager.go | 85 ++++++------------------------------- 4 files changed, 16 insertions(+), 84 deletions(-) diff --git a/pkg/agent/loop.go b/pkg/agent/loop.go index a2975caff..961e1a812 100644 --- a/pkg/agent/loop.go +++ b/pkg/agent/loop.go @@ -1554,7 +1554,7 @@ func (al *AgentLoop) handleCommand( return "", false } - rt := al.buildCommandsRuntime(route, agent) + rt := al.buildCommandsRuntime(agent) executor := commands.NewExecutor(al.cmdRegistry, rt) var commandReply string @@ -1584,10 +1584,7 @@ func (al *AgentLoop) handleCommand( } } -func (al *AgentLoop) buildCommandsRuntime( - route routing.ResolvedRoute, - agent *AgentInstance, -) *commands.Runtime { +func (al *AgentLoop) buildCommandsRuntime(agent *AgentInstance) *commands.Runtime { rt := &commands.Runtime{ Config: al.cfg, ListAgentIDs: al.registry.ListAgentIDs, diff --git a/pkg/commands/cmd_session.go b/pkg/commands/cmd_session.go index 21e7e13f0..4822f99b7 100644 --- a/pkg/commands/cmd_session.go +++ b/pkg/commands/cmd_session.go @@ -8,6 +8,7 @@ import ( "time" "github.com/sipeed/picoclaw/pkg/session" + "github.com/sipeed/picoclaw/pkg/utils" ) func sessionCommand() Definition { @@ -109,11 +110,7 @@ func sessionLabel(summary, preview string, maxLen int) string { return "(empty)" } text = strings.ReplaceAll(text, "\n", " ") - runes := []rune(text) - if len(runes) <= maxLen { - return text - } - return string(runes[:maxLen]) + "..." + return utils.Truncate(text, maxLen) } // extractSessionTag returns the "#N" suffix from a session key, or "#1" for diff --git a/pkg/commands/runtime.go b/pkg/commands/runtime.go index d98d1ede2..ca4d82752 100644 --- a/pkg/commands/runtime.go +++ b/pkg/commands/runtime.go @@ -9,7 +9,6 @@ import ( // rely on. Implementations are expected to be scope-aware and deterministic // for a given scopeKey. SessionManager satisfies this interface. type SessionOps interface { - ResolveActive(scopeKey string) (string, error) StartNew(scopeKey string) (string, error) List(scopeKey string) ([]session.SessionMeta, error) Resume(scopeKey string, index int) (string, error) diff --git a/pkg/session/manager.go b/pkg/session/manager.go index a47324cca..4afdac0b3 100644 --- a/pkg/session/manager.go +++ b/pkg/session/manager.go @@ -12,6 +12,7 @@ import ( "sync" "time" + "github.com/sipeed/picoclaw/pkg/fileutil" "github.com/sipeed/picoclaw/pkg/providers" ) @@ -654,39 +655,7 @@ func (sm *SessionManager) saveIndexLocked() error { return err } - tmpFile, err := os.CreateTemp(sm.storage, "index-*.tmp") - if err != nil { - return err - } - - tmpPath := tmpFile.Name() - cleanup := true - defer func() { - if cleanup { - _ = os.Remove(tmpPath) - } - }() - - if _, err := tmpFile.Write(data); err != nil { - _ = tmpFile.Close() - return err - } - if err := tmpFile.Chmod(0o644); err != nil { - _ = tmpFile.Close() - return err - } - if err := tmpFile.Sync(); err != nil { - _ = tmpFile.Close() - return err - } - if err := tmpFile.Close(); err != nil { - return err - } - if err := os.Rename(tmpPath, sm.indexPath); err != nil { - return err - } - cleanup = false - return nil + return fileutil.WriteFileAtomic(sm.indexPath, data, 0o644) } func (sm *SessionManager) ensureScopeLocked(scopeKey string, now time.Time) (*scopeIndex, bool) { @@ -855,13 +824,8 @@ func (sm *SessionManager) writeSessionSnapshot(snapshot Session) error { } filename := sanitizeFilename(snapshot.Key) - - // filepath.IsLocal rejects empty names, "..", absolute paths, and - // OS-reserved device names (NUL, COM1 … on Windows). - // The extra checks reject "." and any directory separators so that - // the session file is always written directly inside sm.storage. - if filename == "." || !filepath.IsLocal(filename) || strings.ContainsAny(filename, `/\`) { - return os.ErrInvalid + if err := validateSessionFilename(filename); err != nil { + return err } data, err := json.MarshalIndent(snapshot, "", " ") @@ -870,39 +834,14 @@ func (sm *SessionManager) writeSessionSnapshot(snapshot Session) error { } sessionPath := filepath.Join(sm.storage, filename+".json") - tmpFile, err := os.CreateTemp(sm.storage, "session-*.tmp") - if err != nil { - return err - } + return fileutil.WriteFileAtomic(sessionPath, data, 0o644) +} - tmpPath := tmpFile.Name() - cleanup := true - defer func() { - if cleanup { - _ = os.Remove(tmpPath) - } - }() - - if _, err := tmpFile.Write(data); err != nil { - _ = tmpFile.Close() - return err +// validateSessionFilename rejects filenames that would escape sm.storage. +func validateSessionFilename(filename string) error { + if filename == "." || !filepath.IsLocal(filename) || strings.ContainsAny(filename, `/\`) { + return os.ErrInvalid } - if err := tmpFile.Chmod(0o644); err != nil { - _ = tmpFile.Close() - return err - } - if err := tmpFile.Sync(); err != nil { - _ = tmpFile.Close() - return err - } - if err := tmpFile.Close(); err != nil { - return err - } - - if err := os.Rename(tmpPath, sessionPath); err != nil { - return err - } - cleanup = false return nil } @@ -912,8 +851,8 @@ func (sm *SessionManager) deleteSessionFile(sessionKey string) error { } filename := sanitizeFilename(sessionKey) - if filename == "." || !filepath.IsLocal(filename) || strings.ContainsAny(filename, `/\`) { - return os.ErrInvalid + if err := validateSessionFilename(filename); err != nil { + return err } sessionPath := filepath.Join(sm.storage, filename+".json")