feat(session): integrate JSONL persistence into agent loop

[is-Xiaoen]

Summary

Wires the pkg/memory JSONL store (merged in #732) into the agent loop, replacing the full-JSON serialization backend. This is the integration step that makes the JSONL persistence actually active.

Key design decision: extract a SessionStore interface from SessionManager, so the agent loop doesn't know or care which backend is in use.

Changes

• pkg/session/session_store.go — SessionStore interface with the 8 methods the agent loop calls. Both SessionManager and JSONLBackend satisfy it.
• pkg/session/jsonl_backend.go — Adapter that wraps memory.Store into SessionStore. Write errors are logged (matching SessionManager's fire-and-forget contract). Save() triggers compaction to reclaim space after truncation.
• pkg/agent/instance.go — Field type changed from `*SessionManager` to `SessionStore`. JSONL backend initialized by default with auto-migration from legacy `.json` files. Falls back to `SessionManager` if initialization fails.

What changes at runtime

Operation	Before (SessionManager)	After (JSONLBackend)
AddMessage	In-memory only, waits for Save()	JSONL append + fsync (durable immediately)
Save()	Full JSON serialization to disk	Compact if truncated messages exist, otherwise no-op
TruncateHistory	O(n) array copy	O(1) metadata update
Crash recovery	Lose messages since last Save()	Lose nothing (each write is fsynced)

What does NOT change

• loop.go — zero modifications. Every method call works identically through the interface.
• loop_test.go — zero modifications. Tests create SessionManager which still satisfies the interface.
• Existing sessions — .json files are auto-migrated to .jsonl on first startup, originals renamed to .json.migrated.

Test plan

8 new tests in pkg/session/jsonl_backend_test.go:

• Message roundtrip (AddMessage → GetHistory)
• Tool call preservation (AddFullMessage with ToolCalls)
• Summary CRUD
• Truncation + compaction via Save()
• History replacement (SetHistory)
• Empty session handling
• Session isolation
• Full summarization flow (SetSummary → TruncateHistory → Save)

Plus compile-time interface checks for both backends.

$ go test ./pkg/session/... -v -count=1
ok  github.com/sipeed/picoclaw/pkg/session  2.367s

$ go test ./pkg/agent/... -count=1
ok  github.com/sipeed/picoclaw/pkg/agent  1.918s

Type of change

• New feature (non-breaking change which adds functionality)

Closes #1169 — follows up on #732.

[yinwm]

Code Review

Overview

Item	Details
Title	feat(session): integrate JSONL persistence into agent loop
Changes	+305 / -3 lines, 4 files
Risk	Low (has fallback to SessionManager)

---

✅ Strengths

1. Clean interface extraction - SessionStore interface allows swapping backends without touching loop.go
2. Graceful degradation - Falls back to SessionManager if JSONL store initialization fails
3. Auto-migration - Legacy .json files are migrated to .jsonl on startup
4. Good test coverage - 8 tests covering the main scenarios + compile-time interface checks

---

⚠️ Issues to Fix

Issue 1: Save() swallows errors (Medium priority)

Location: pkg/session/jsonl_backend.go:77

func (b *JSONLBackend) Save(key string) error {
    if err := b.store.Compact(context.Background(), key); err != nil {
        log.Printf("session: compact %s: %v", key, err)
    }
    return nil  // ← Error is swallowed
}

Problem: This is inconsistent with SessionManager.Save() which returns errors.

Suggestion (choose one):

// Option A: Return the error (recommended)
func (b *JSONLBackend) Save(key string) error {
    return b.store.Compact(context.Background(), key)
}

// Option B: Keep as-is but document why
// Save persists session state. Since JSONL fsyncs every write immediately,
// data is already durable. Compact only reclaims disk space from truncated
// messages, so its failure is non-critical and only logged.
func (b *JSONLBackend) Save(key string) error {
    if err := b.store.Compact(context.Background(), key); err != nil {
        log.Printf("session: compact %s: %v", key, err)
    }
    return nil
}

---

Issue 2: Close() not in interface (Low priority)

Location: pkg/session/jsonl_backend.go:82

Problem: JSONLBackend has a Close() method, but it's not in the SessionStore interface. Callers cannot release resources through the interface.

Suggestion: Add Close() to the interface:

type SessionStore interface {
    // ... existing methods
    Close() error
}

And add a no-op implementation to SessionManager:

func (sm *SessionManager) Close() error {
    return nil
}

---

📊 Performance Comparison

Operation	Before (SessionManager)	After (JSONLBackend)
AddMessage	In-memory, waits for Save()	JSONL append + fsync (immediate durability)
Save()	Full JSON rewrite	Compact only if truncated (often no-op)
TruncateHistory	O(n) array copy + rewrite	O(1) metadata update
Crash recovery	Lose messages since last Save()	No data loss

---

Summary

Category	Assessment
Architecture	✅ Good
Code quality	⚠️ 2 minor issues
Test coverage	✅ Good
Documentation	✅ Clear PR description
Risk	✅ Low

Recommendation: Fix Issue 1 before merging. Issue 2 can be addressed in a follow-up PR if preferred.

Overall, this is a well-designed integration. Nice work! 🎉

[is-Xiaoen]

@yinwm 感谢 review！两个问题都修了：

Issue 1 — Save() 直接返回 Compact 的 error 了，不再吞掉。之前想着 compact 失败不影响数据安全（每次写入都 fsync 了），但你说得对，跟 SessionManager 保持一致比较好，调用方能看到错误。

Issue 2 — Close() error 加到 SessionStore 接口了，SessionManager 加了个 no-op 实现。这样上层可以统一释放资源。

两个 fix 分开提交的：

• 1434edc fix(session): propagate compact error from Save
• 848f452 feat(session): add Close to SessionStore interface

[yinwm]

PR Review: JSONL Session Store Integration

Thanks for this well-structured PR! The interface abstraction is clean and the migration strategy is solid. Here are my findings:

---

✅ Strengths

1. Clean interface abstraction - Extracting SessionStore allows loop.go to remain untouched. Good decoupling.
2. Compile-time interface checks - Good practice with var _ SessionStore = (*JSONLBackend)(nil)
3. Auto-migration + fallback - Legacy .json files are migrated automatically, with graceful fallback to SessionManager on init failure
4. Comprehensive tests - 8 tests covering the main scenarios including the full summarization flow
5. Consistent error handling - Maintains the "fire-and-forget" contract

---

⚠️ Issues to Address

1. Resource Leak - Close() is never called (Must Fix)

AgentInstance creates a SessionStore but never calls Close() on shutdown. The JSONLBackend wraps a memory.Store which likely holds file handles that need proper cleanup.

Suggested fix in pkg/agent/instance.go:

// Add a Close method to AgentInstance
func (a *AgentInstance) Close() error {
    return a.Sessions.Close()
}

Then ensure the caller (main/CLI entry point) invokes this on shutdown.

2. Migration failure handling

Current behavior when migration fails:

if n, merr := memory.MigrateFromJSON(context.Background(), dir, store); merr != nil {
    log.Printf("memory: migration: %v", merr)  // Only logs, continues with JSONL
}

This could lead to inconsistent state where some sessions are in JSONL and others remain in JSON format. Consider either:

• Falling back to SessionManager on migration failure
• Documenting this behavior clearly

---

📝 Minor Suggestions

1. Interface documentation - Add comments to SessionStore methods clarifying which are fire-and-forget vs. return errors

2. NewJSONLBackend - Consider adding a compile-time interface check comment for consistency with the test file

---

Summary

Area	Status
Design	✅ Excellent
Code Quality	✅ Good
Tests	✅ Good
Backward Compatibility	✅ Excellent
Resource Management	⚠️ Needs fix

Recommendation: Please address the Close() resource leak before merging. The rest looks good!

---

🤖 Generated with Claude Code

[is-Xiaoen]

@yinwm 感谢二次 review！两个问题都修了：

Issue 1: Close() 资源泄漏 — 给 AgentInstance、AgentRegistry、AgentLoop 都加了 Close()，形成完整的关闭链路：

• gateway 关闭时：agentLoop.Stop() → agentLoop.Close() → registry 遍历关闭所有 agent 的 session store
• CLI 模式：defer agentLoop.Close() 确保退出时释放文件句柄

Issue 2: 迁移失败处理 — migration 失败现在会 close 掉 JSONL store 并回退到 SessionManager，避免出现 JSON/JSONL 混合的脑裂状态。之前只是 log 然后继续用 JSONL store，但 migration 失败说明 store 写入本身有问题，继续用没意义。

Minor: 接口文档 — SessionStore 每个方法都加了注释，特别说明了 write 方法的 fire-and-forget 契约。

commit: de9d470

[yinwm]

Re-Review: All Issues Addressed ✅

Thanks for the quick fixes! All the concerns from my previous review have been properly addressed:

✅ Resource Management Fixed

The close chain is now complete:

CLI/Gateway → AgentLoop.Close() → AgentRegistry.Close() → AgentInstance.Close() → Sessions.Close()

• AgentInstance.Close() - releases session store resources
• AgentLoop.Close() - delegates to registry
• AgentRegistry.Close() - iterates all agents and closes them
• Both CLI and gateway entry points now call Close() on shutdown

✅ Migration Failure Handling Fixed

if merr != nil {
    store.Close()  // Clean up failed store
    return session.NewSessionManager(dir)  // Fallback
}

This prevents the split state issue I was concerned about.

✅ Interface Documentation Added

SessionStore now has clear method documentation explaining the fire-and-forget contract.

---

LGTM! This is ready to merge. 🚀