From 23dba24f6307710c26fe48294b12a2f02d74c7d7 Mon Sep 17 00:00:00 2001 From: KRATOS <84986124+gnanam1990@users.noreply.github.com> Date: Sun, 16 Aug 2026 22:32:09 +0530 Subject: [PATCH 01/13] feat(acp): add standard session list and resume Origin-Session: local-abff1c | Claude Code | 7 prompts Origin-Snapshot: b7d0806d49f9 Origin-Session: local-13d543 | Claude Code | 5 prompts Origin-Snapshot: 6b5eb8ba4e5b Origin-Session: local-c962d7 | Claude Code | 3 prompts Origin-Snapshot: d2b8f44a9abc --- internal/acp/agent.go | 107 ++++++++++++++++++++++++++++---- internal/acp/agent_test.go | 121 +++++++++++++++++++++++++++++++++---- internal/acp/translate.go | 8 +++ internal/acp/types.go | 44 +++++++++++++- 4 files changed, 256 insertions(+), 24 deletions(-) diff --git a/internal/acp/agent.go b/internal/acp/agent.go index 6050c7c8b..58ea96f01 100644 --- a/internal/acp/agent.go +++ b/internal/acp/agent.go @@ -2,9 +2,11 @@ package acp import ( "context" + "crypto/sha256" "encoding/base64" "encoding/json" "errors" + "fmt" "log" "strings" "sync" @@ -82,6 +84,8 @@ func NewAgent(conn *Conn, deps Deps) *Agent { conn.Handle(MethodInitialize, a.handleInitialize) conn.Handle(MethodSessionNew, a.handleSessionNew) conn.Handle(MethodSessionLoad, a.handleSessionLoad) + conn.Handle(MethodSessionList, a.handleSessionList) + conn.Handle(MethodSessionResume, a.handleSessionResume) conn.Handle(MethodSessionPrompt, a.handleSessionPrompt) conn.Handle(MethodSessionSetMode, a.handleSetMode) conn.Handle(MethodSessionSetConfigOption, a.handleSetConfigOption) @@ -110,11 +114,11 @@ func (a *Agent) handleInitialize(_ context.Context, params json.RawMessage) (any return InitializeResult{ ProtocolVersion: negotiated, AgentCapabilities: AgentCapabilities{ - // Only advertise what ZERO actually implements: session/load (loadSession) - // and image prompts. session/resume + the session-capability sub-object - // are intentionally omitted since there is no resume handler yet. - LoadSession: true, - PromptCapabilities: PromptCapabilities{Image: true}, + // ACP v1 optional methods are advertised as empty capability objects; + // clients must gate session/list and session/resume on their presence. + LoadSession: true, + PromptCapabilities: PromptCapabilities{Image: true}, + SessionCapabilities: &SessionCapabilities{List: &struct{}{}, Resume: &struct{}{}}, }, AgentInfo: &info, // ZERO owns credentials (BYOK) and does not delegate auth to the editor. @@ -154,6 +158,22 @@ func (a *Agent) handleSessionLoad(ctx context.Context, params json.RawMessage) ( if err := json.Unmarshal(params, &p); err != nil { return nil, RPCError(codeInvalidParams, "invalid session/load params") } + return a.activatePersistedSession(ctx, p, true) +} + +func (a *Agent) handleSessionResume(ctx context.Context, params json.RawMessage) (any, error) { + var p ResumeSessionParams + if err := json.Unmarshal(params, &p); err != nil { + return nil, RPCError(codeInvalidParams, "invalid session/resume params") + } + return a.activatePersistedSession(ctx, p, false) +} + +// activatePersistedSession restores the agent's internal conversation context +// for both lifecycle methods. session/load additionally replays user-visible +// history as ordered session/update notifications; session/resume deliberately +// does not, which makes it safe for an already-rendered desktop reconnect. +func (a *Agent) activatePersistedSession(ctx context.Context, p LoadSessionParams, replay bool) (any, error) { meta, err := a.deps.Store.Get(p.SessionID) if err != nil || meta == nil { return nil, RPCError(codeInvalidParams, "session not found: "+p.SessionID) @@ -169,7 +189,7 @@ func (a *Agent) handleSessionLoad(ctx context.Context, params json.RawMessage) ( // Load history BEFORE publishing the session so no concurrent prompt observes // a half-initialized session (registerSession sets history under the lock and // reuses an already-live session rather than orphaning its in-flight turn). - history, historyErr := a.loadHistory(meta.SessionID) + history, messages, historyErr := a.loadHistory(meta.SessionID) model, models, restrictModels, err := a.resolveModelChoices(ctx, root) if err != nil { return nil, RPCError(codeInternalError, "config: "+err.Error()) @@ -181,8 +201,14 @@ func (a *Agent) handleSessionLoad(ctx context.Context, params json.RawMessage) ( } } sess := a.registerSession(meta.SessionID, root, history, model, models, restrictModels) + note := ¬ifier{conn: a.conn, sessionID: sess.id} + if replay && historyErr == nil { + for _, message := range messages { + note.send(replayMessageChunk(message.role, replayMessageID(message.eventID), message.content)) + } + } a.warnPersistence( - ¬ifier{conn: a.conn, sessionID: sess.id}, + note, "load session history", "Could not load session history. The session is open, but earlier turns may be missing until storage recovers.", historyErr, @@ -193,6 +219,37 @@ func (a *Agent) handleSessionLoad(ctx context.Context, params json.RawMessage) ( }, nil } +func (a *Agent) handleSessionList(_ context.Context, params json.RawMessage) (any, error) { + var p ListSessionsParams + if len(params) > 0 { + if err := json.Unmarshal(params, &p); err != nil { + return nil, RPCError(codeInvalidParams, "invalid session/list params") + } + } + if p.Cursor != "" { + return nil, RPCError(codeInvalidParams, "invalid session/list cursor") + } + items, err := a.deps.Store.ListResumable() + if err != nil { + return nil, RPCError(codeInternalError, "list sessions: "+err.Error()) + } + cwd := strings.TrimSpace(p.Cwd) + result := ListSessionsResult{Sessions: make([]SessionInfo, 0, len(items))} + for _, item := range items { + if cwd != "" && item.Cwd != cwd { + continue + } + result.Sessions = append(result.Sessions, SessionInfo{ + SessionID: item.SessionID, + Title: item.Title, + Cwd: item.Cwd, + UpdatedAt: item.UpdatedAt, + Meta: &SessionInfoMeta{ModelID: item.ModelID, CreatedAt: item.CreatedAt}, + }) + } + return result, nil +} + // ---- prompt turn ---- func (a *Agent) handleSessionPrompt(ctx context.Context, params json.RawMessage) (any, error) { @@ -567,15 +624,22 @@ func (a *Agent) persistTurn(sess *acpSession, user, assistant string) error { return err } -func (a *Agent) loadHistory(sessionID string) ([]turnRecord, error) { +type persistedMessage struct { + eventID string + role string + content string +} + +func (a *Agent) loadHistory(sessionID string) ([]turnRecord, []persistedMessage, error) { if a.deps.Store == nil { - return nil, nil + return nil, nil, nil } events, err := a.deps.Store.ReadEvents(sessionID) if err != nil { - return nil, err + return nil, nil, err } var records []turnRecord + var messages []persistedMessage var pendingUser string havePending := false for _, e := range events { @@ -595,12 +659,14 @@ func (a *Agent) loadHistory(sessionID string) ([]turnRecord, error) { } switch msg.Role { case "user": + messages = append(messages, persistedMessage{eventID: persistedMessageIdentity(sessionID, e), role: msg.Role, content: msg.Content}) if havePending { records = append(records, turnRecord{user: pendingUser}) } pendingUser = msg.Content havePending = true case "assistant": + messages = append(messages, persistedMessage{eventID: persistedMessageIdentity(sessionID, e), role: msg.Role, content: msg.Content}) records = append(records, turnRecord{user: pendingUser, assistant: msg.Content}) pendingUser = "" havePending = false @@ -609,7 +675,26 @@ func (a *Agent) loadHistory(sessionID string) ([]turnRecord, error) { if havePending { records = append(records, turnRecord{user: pendingUser}) } - return records, nil + return records, messages, nil +} + +func persistedMessageIdentity(sessionID string, event sessions.Event) string { + if event.ID != "" { + return event.ID + } + return fmt.Sprintf("%s:%d", sessionID, event.Sequence) +} + +// replayMessageID maps ZERO's stable event identity to a standards-shaped UUID +// without leaking or parsing the event id on the wire. The same stored message +// receives the same opaque id across loads, which also gives clients an exact +// chunk boundary when two adjacent persisted messages have the same role. +func replayMessageID(eventID string) string { + sum := sha256.Sum256([]byte("zero-acp-message:" + eventID)) + b := sum[:16] + b[6] = (b[6] & 0x0f) | 0x50 + b[8] = (b[8] & 0x3f) | 0x80 + return fmt.Sprintf("%x-%x-%x-%x-%x", b[:4], b[4:6], b[6:8], b[8:10], b[10:16]) } func (a *Agent) warnPersistence(note *notifier, action string, message string, err error) { diff --git a/internal/acp/agent_test.go b/internal/acp/agent_test.go index 4fa97a258..a27e806d2 100644 --- a/internal/acp/agent_test.go +++ b/internal/acp/agent_test.go @@ -67,9 +67,10 @@ func testDeps(t *testing.T) Deps { // clientHarness wires a client Conn to an Agent over in-memory pipes and collects // session/update text chunks. type clientHarness struct { - client *Conn - updates chan string - stop func() + client *Conn + updates chan string + notifications chan ContentChunk + stop func() } func newHarness(t *testing.T, deps Deps) *clientHarness { @@ -80,19 +81,17 @@ func newHarness(t *testing.T, deps Deps) *clientHarness { client := NewConn(br, bw) a := NewAgent(agentConn, deps) - h := &clientHarness{client: client, updates: make(chan string, 128)} + h := &clientHarness{client: client, updates: make(chan string, 128), notifications: make(chan ContentChunk, 128)} client.HandleNotify(MethodSessionUpdate, func(_ context.Context, params json.RawMessage) { var probe struct { - Update struct { - SessionUpdate string `json:"sessionUpdate"` - Content struct { - Text string `json:"text"` - } `json:"content"` - } `json:"update"` + Update ContentChunk `json:"update"` } if json.Unmarshal(params, &probe) != nil { return } + if probe.Update.SessionUpdate == UpdateAgentMessageChunk || probe.Update.SessionUpdate == UpdateUserMessageChunk { + h.notifications <- probe.Update + } if probe.Update.SessionUpdate == UpdateAgentMessageChunk { h.updates <- probe.Update.Content.Text } @@ -124,7 +123,8 @@ func TestACPEndToEndPrompt(t *testing.T) { if initRes.ProtocolVersion != ProtocolVersion { t.Fatalf("protocol version = %d", initRes.ProtocolVersion) } - if !initRes.AgentCapabilities.LoadSession || !initRes.AgentCapabilities.PromptCapabilities.Image { + if !initRes.AgentCapabilities.LoadSession || !initRes.AgentCapabilities.PromptCapabilities.Image || + initRes.AgentCapabilities.SessionCapabilities == nil || initRes.AgentCapabilities.SessionCapabilities.List == nil || initRes.AgentCapabilities.SessionCapabilities.Resume == nil { t.Fatalf("unexpected capabilities: %+v", initRes.AgentCapabilities) } @@ -164,6 +164,105 @@ func TestACPEndToEndPrompt(t *testing.T) { } } +func TestACPListsOnlyResumableSessionMetadata(t *testing.T) { + deps := testDeps(t) + workspaceA := t.TempDir() + workspaceB := t.TempDir() + for _, input := range []sessions.CreateInput{ + {SessionID: "desktop-a", Title: "First", Cwd: workspaceA, ModelID: "model-a"}, + {SessionID: "desktop-b", Title: "Second", Cwd: workspaceB, ModelID: "model-b"}, + {SessionID: "child-run", SessionKind: sessions.SessionKindChild, Title: "Internal child", Cwd: workspaceA}, + } { + if _, err := deps.Store.Create(input); err != nil { + t.Fatalf("create %s: %v", input.SessionID, err) + } + } + + h := newHarness(t, deps) + defer h.stop() + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() + + var all ListSessionsResult + if err := h.client.Call(ctx, MethodSessionList, ListSessionsParams{}, &all); err != nil { + t.Fatalf("session/list: %v", err) + } + if len(all.Sessions) != 2 { + t.Fatalf("all sessions = %+v, want two resumable sessions", all.Sessions) + } + byID := make(map[string]SessionInfo, len(all.Sessions)) + for _, item := range all.Sessions { + byID[item.SessionID] = item + } + if _, found := byID["child-run"]; found { + t.Fatal("agent-owned child session leaked into the desktop session picker") + } + if got := byID["desktop-a"]; got.Title != "First" || got.Cwd != workspaceA || got.Meta == nil || got.Meta.ModelID != "model-a" || got.Meta.CreatedAt == "" || got.UpdatedAt == "" { + t.Fatalf("desktop-a summary = %+v", got) + } + + var filtered ListSessionsResult + if err := h.client.Call(ctx, MethodSessionList, ListSessionsParams{Cwd: workspaceB}, &filtered); err != nil { + t.Fatalf("filtered session/list: %v", err) + } + if len(filtered.Sessions) != 1 || filtered.Sessions[0].SessionID != "desktop-b" { + t.Fatalf("filtered sessions = %+v, want only desktop-b", filtered.Sessions) + } +} + +func TestACPLoadReplaysHistoryAndResumeDoesNot(t *testing.T) { + deps := testDeps(t) + workspace := t.TempDir() + created, err := deps.Store.Create(sessions.CreateInput{SessionID: "replay-session", Title: "Replay", Cwd: workspace}) + if err != nil { + t.Fatal(err) + } + if _, err := deps.Store.AppendEvents(created.SessionID, []sessions.AppendEventInput{ + {Type: sessions.EventMessage, Payload: map[string]any{"role": "user", "content": "first user"}}, + {Type: sessions.EventMessage, Payload: map[string]any{"role": "assistant", "content": "first answer"}}, + {Type: sessions.EventMessage, Payload: map[string]any{"role": "user", "content": "second user"}}, + }); err != nil { + t.Fatal(err) + } + + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() + loader := newHarness(t, deps) + var loaded LoadSessionResult + if err := loader.client.Call(ctx, MethodSessionLoad, LoadSessionParams{SessionID: created.SessionID, Cwd: workspace, McpServers: []McpServer{}}, &loaded); err != nil { + t.Fatalf("session/load: %v", err) + } + wantKinds := []string{UpdateUserMessageChunk, UpdateAgentMessageChunk, UpdateUserMessageChunk} + wantText := []string{"first user", "first answer", "second user"} + seenIDs := map[string]bool{} + for i := range wantKinds { + select { + case update := <-loader.notifications: + if update.SessionUpdate != wantKinds[i] || update.Content.Text != wantText[i] { + t.Fatalf("history update %d = %+v", i, update) + } + if update.MessageID == "" || seenIDs[update.MessageID] { + t.Fatalf("history update %d has missing/duplicate message id %q", i, update.MessageID) + } + seenIDs[update.MessageID] = true + case <-ctx.Done(): + t.Fatalf("history update %d was not replayed", i) + } + } + loader.stop() + + resumer := newHarness(t, deps) + defer resumer.stop() + if err := resumer.client.Call(ctx, MethodSessionResume, ResumeSessionParams{SessionID: created.SessionID, Cwd: workspace, McpServers: []McpServer{}}, &ResumeSessionResult{}); err != nil { + t.Fatalf("session/resume: %v", err) + } + select { + case update := <-resumer.notifications: + t.Fatalf("session/resume replayed history: %+v", update) + case <-time.After(100 * time.Millisecond): + } +} + func TestACPModelConfigOptionsCatalogSelectionAndLoad(t *testing.T) { deps := testDeps(t) deps.ResolveConfig = func(_ string, o config.Overrides) (config.ResolvedConfig, error) { diff --git a/internal/acp/translate.go b/internal/acp/translate.go index 565174904..aa5f83872 100644 --- a/internal/acp/translate.go +++ b/internal/acp/translate.go @@ -17,6 +17,14 @@ func agentMessageChunk(delta string) ContentChunk { return ContentChunk{SessionUpdate: UpdateAgentMessageChunk, Content: TextBlock(delta)} } +func replayMessageChunk(role, messageID, text string) ContentChunk { + update := UpdateAgentMessageChunk + if role == "user" { + update = UpdateUserMessageChunk + } + return ContentChunk{SessionUpdate: update, MessageID: messageID, Content: TextBlock(text)} +} + func agentThoughtChunk(delta string) ContentChunk { return ContentChunk{SessionUpdate: UpdateAgentThoughtChunk, Content: TextBlock(delta)} } diff --git a/internal/acp/types.go b/internal/acp/types.go index b00bf672a..33e3db33d 100644 --- a/internal/acp/types.go +++ b/internal/acp/types.go @@ -12,6 +12,8 @@ const ( MethodAuthenticate = "authenticate" MethodSessionNew = "session/new" MethodSessionLoad = "session/load" + MethodSessionList = "session/list" + MethodSessionResume = "session/resume" MethodSessionPrompt = "session/prompt" MethodSessionCancel = "session/cancel" // notification MethodSessionUpdate = "session/update" // notification (agent -> client) @@ -62,8 +64,16 @@ type PromptCapabilities struct { } type AgentCapabilities struct { - LoadSession bool `json:"loadSession"` - PromptCapabilities PromptCapabilities `json:"promptCapabilities"` + LoadSession bool `json:"loadSession"` + PromptCapabilities PromptCapabilities `json:"promptCapabilities"` + SessionCapabilities *SessionCapabilities `json:"sessionCapabilities,omitempty"` +} + +// Empty capability objects are presence flags in ACP v1. Pointers preserve +// the wire distinction between an advertised `{}` and an omitted capability. +type SessionCapabilities struct { + List *struct{} `json:"list,omitempty"` + Resume *struct{} `json:"resume,omitempty"` } type AuthMethod struct { @@ -140,6 +150,35 @@ type LoadSessionResult struct { Modes *SessionModeState `json:"modes,omitempty"` } +// ListSessionsParams and the following types implement ACP v1 session/list. +// Transcript contents stay behind session/load; this method returns metadata +// only and leaves the optional pagination cursor opaque. +type ListSessionsParams struct { + Cwd string `json:"cwd,omitempty"` + Cursor string `json:"cursor,omitempty"` +} + +type SessionInfoMeta struct { + ModelID string `json:"modelId,omitempty"` + CreatedAt string `json:"createdAt,omitempty"` +} + +type SessionInfo struct { + SessionID string `json:"sessionId"` + Cwd string `json:"cwd"` + Title string `json:"title,omitempty"` + UpdatedAt string `json:"updatedAt,omitempty"` + Meta *SessionInfoMeta `json:"_meta,omitempty"` +} + +type ListSessionsResult struct { + Sessions []SessionInfo `json:"sessions"` + NextCursor string `json:"nextCursor,omitempty"` +} + +type ResumeSessionParams = LoadSessionParams +type ResumeSessionResult = LoadSessionResult + // ---- prompt turn ---- type PromptParams struct { @@ -174,6 +213,7 @@ type SessionNotification struct { // ContentBlock under "content"; the variant is set via SessionUpdate. type ContentChunk struct { SessionUpdate string `json:"sessionUpdate"` + MessageID string `json:"messageId,omitempty"` Content ContentBlock `json:"content"` } From fecc6f14ab53530f25a895f0d8f99aa4ab58e7e0 Mon Sep 17 00:00:00 2001 From: KRATOS <84986124+gnanam1990@users.noreply.github.com> Date: Sun, 16 Aug 2026 22:40:58 +0530 Subject: [PATCH 02/13] fix(acp): preserve notification wire order Origin-Session: local-abff1c | Claude Code | 7 prompts Origin-Snapshot: b7d0806d49f9 Origin-Session: local-13d543 | Claude Code | 5 prompts Origin-Snapshot: 6b5eb8ba4e5b Origin-Session: local-c962d7 | Claude Code | 3 prompts Origin-Snapshot: d2b8f44a9abc --- internal/acp/jsonrpc.go | 53 +++++++++++++++++++----- internal/acp/jsonrpc_test.go | 80 ++++++++++++++++++++++++++++++++++++ 2 files changed, 123 insertions(+), 10 deletions(-) diff --git a/internal/acp/jsonrpc.go b/internal/acp/jsonrpc.go index 7d9171692..a0852d8b3 100644 --- a/internal/acp/jsonrpc.go +++ b/internal/acp/jsonrpc.go @@ -89,6 +89,13 @@ type Conn struct { handlers map[string]HandlerFunc notifiers map[string]NotifyFunc + // Notifications for one method are an ordered stream: session/update in + // particular is stateful, so dispatching every frame in an unrelated + // goroutine can make a later chunk overtake an earlier one. Different + // methods retain independent tails, keeping session/cancel responsive while + // an update handler is busy. + notifyMu sync.Mutex + notifyTails map[string]chan struct{} mu sync.Mutex nextID int64 @@ -106,11 +113,12 @@ type Conn struct { // lifetime. func NewConn(r io.Reader, w io.Writer) *Conn { return &Conn{ - rawReader: r, - w: w, - handlers: make(map[string]HandlerFunc), - notifiers: make(map[string]NotifyFunc), - pending: make(map[int64]chan rpcMessage), + rawReader: r, + w: w, + handlers: make(map[string]HandlerFunc), + notifiers: make(map[string]NotifyFunc), + notifyTails: make(map[string]chan struct{}), + pending: make(map[int64]chan rpcMessage), } } @@ -303,11 +311,7 @@ func (c *Conn) handleLine(ctx context.Context, line []byte) { }(msg) case msg.isNotify(): if fn := c.notifiers[msg.Method]; fn != nil { - c.wg.Add(1) - go func(m rpcMessage) { - defer c.wg.Done() - fn(ctx, m.Params) - }(msg) + c.dispatchNotification(ctx, msg, fn) } default: // Malformed frame; reply only if we can identify a request id. @@ -317,6 +321,35 @@ func (c *Conn) handleLine(ctx context.Context, line []byte) { } } +// dispatchNotification preserves wire order within one method without making +// unrelated notification methods wait for each other. The read loop installs +// each tail before starting its goroutine, so goroutine scheduling cannot +// reorder the chain it observes. +func (c *Conn) dispatchNotification(ctx context.Context, msg rpcMessage, fn NotifyFunc) { + c.notifyMu.Lock() + previous := c.notifyTails[msg.Method] + done := make(chan struct{}) + c.notifyTails[msg.Method] = done + c.notifyMu.Unlock() + + c.wg.Add(1) + go func() { + defer c.wg.Done() + defer func() { + close(done) + c.notifyMu.Lock() + if c.notifyTails[msg.Method] == done { + delete(c.notifyTails, msg.Method) + } + c.notifyMu.Unlock() + }() + if previous != nil { + <-previous + } + fn(ctx, msg.Params) + }() +} + func (c *Conn) dispatchRequest(ctx context.Context, msg rpcMessage) { fn := c.handlers[msg.Method] if fn == nil { diff --git a/internal/acp/jsonrpc_test.go b/internal/acp/jsonrpc_test.go index 7d3b0c9da..333854bf4 100644 --- a/internal/acp/jsonrpc_test.go +++ b/internal/acp/jsonrpc_test.go @@ -360,6 +360,86 @@ func TestConnNotification(t *testing.T) { } } +func TestConnNotificationsForOneMethodStayInWireOrder(t *testing.T) { + a, b, stop := connPair(t) + defer stop() + + firstStarted := make(chan struct{}) + releaseFirst := make(chan struct{}) + delivered := make(chan int, 2) + b.HandleNotify("update", func(_ context.Context, params json.RawMessage) { + var in struct{ Sequence int } + _ = json.Unmarshal(params, &in) + if in.Sequence == 1 { + close(firstStarted) + <-releaseFirst + } + delivered <- in.Sequence + }) + + if err := a.Notify("update", map[string]int{"Sequence": 1}); err != nil { + t.Fatalf("first notify: %v", err) + } + select { + case <-firstStarted: + case <-time.After(2 * time.Second): + t.Fatal("first notification did not start") + } + if err := a.Notify("update", map[string]int{"Sequence": 2}); err != nil { + t.Fatalf("second notify: %v", err) + } + select { + case sequence := <-delivered: + t.Fatalf("notification %d overtook the blocked first notification", sequence) + case <-time.After(25 * time.Millisecond): + } + close(releaseFirst) + for want := 1; want <= 2; want++ { + select { + case got := <-delivered: + if got != want { + t.Fatalf("notification order = ...,%d; want %d", got, want) + } + case <-time.After(2 * time.Second): + t.Fatalf("notification %d was not delivered", want) + } + } +} + +func TestConnDifferentNotificationMethodsStayConcurrent(t *testing.T) { + a, b, stop := connPair(t) + defer stop() + + updateStarted := make(chan struct{}) + releaseUpdate := make(chan struct{}) + cancelDelivered := make(chan struct{}) + b.HandleNotify("update", func(context.Context, json.RawMessage) { + close(updateStarted) + <-releaseUpdate + }) + b.HandleNotify("cancel", func(context.Context, json.RawMessage) { + close(cancelDelivered) + }) + + if err := a.Notify("update", nil); err != nil { + t.Fatalf("update notify: %v", err) + } + select { + case <-updateStarted: + case <-time.After(2 * time.Second): + t.Fatal("update notification did not start") + } + if err := a.Notify("cancel", nil); err != nil { + t.Fatalf("cancel notify: %v", err) + } + select { + case <-cancelDelivered: + case <-time.After(2 * time.Second): + t.Fatal("different notification method was blocked behind update") + } + close(releaseUpdate) +} + func TestConnMethodNotFound(t *testing.T) { a, _, stop := connPair(t) defer stop() From 4b16f0bc38dd434bd969c8cf974a5aecc1936757 Mon Sep 17 00:00:00 2001 From: KRATOS <84986124+gnanam1990@users.noreply.github.com> Date: Sun, 16 Aug 2026 22:43:01 +0530 Subject: [PATCH 03/13] fix(acp): bind sessions to persisted workspaces Origin-Session: local-abff1c | Claude Code | 7 prompts Origin-Snapshot: b7d0806d49f9 Origin-Session: local-13d543 | Claude Code | 5 prompts Origin-Snapshot: 6b5eb8ba4e5b Origin-Session: local-c962d7 | Claude Code | 3 prompts Origin-Snapshot: d2b8f44a9abc --- internal/acp/agent.go | 29 +++++++++++++++-- internal/acp/agent_test.go | 65 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 91 insertions(+), 3 deletions(-) diff --git a/internal/acp/agent.go b/internal/acp/agent.go index 58ea96f01..00559d996 100644 --- a/internal/acp/agent.go +++ b/internal/acp/agent.go @@ -178,6 +178,13 @@ func (a *Agent) activatePersistedSession(ctx context.Context, p LoadSessionParam if err != nil || meta == nil { return nil, RPCError(codeInvalidParams, "session not found: "+p.SessionID) } + if strings.TrimSpace(meta.Cwd) == "" { + return nil, RPCError(codeInvalidParams, "session has no persisted workspace: "+p.SessionID) + } + persistedRoot, err := a.deps.ResolveWorkspaceRoot(meta.Cwd) + if err != nil { + return nil, RPCError(codeInvalidParams, "persisted session workspace is unavailable: "+err.Error()) + } cwdInput := p.Cwd if strings.TrimSpace(cwdInput) == "" { cwdInput = meta.Cwd @@ -186,6 +193,12 @@ func (a *Agent) activatePersistedSession(ctx context.Context, p LoadSessionParam if err != nil { return nil, RPCError(codeInvalidParams, err.Error()) } + // ACP session cwd is immutable. Loading history under a different root + // would give a conversation from one workspace access to another + // workspace's configuration, files, and tools. + if root != persistedRoot { + return nil, RPCError(codeInvalidParams, "session cwd does not match its persisted workspace") + } // Load history BEFORE publishing the session so no concurrent prompt observes // a half-initialized session (registerSession sets history under the lock and // reuses an already-live session rather than orphaning its in-flight turn). @@ -233,11 +246,21 @@ func (a *Agent) handleSessionList(_ context.Context, params json.RawMessage) (an if err != nil { return nil, RPCError(codeInternalError, "list sessions: "+err.Error()) } - cwd := strings.TrimSpace(p.Cwd) + var cwd string + if strings.TrimSpace(p.Cwd) != "" { + var err error + cwd, err = a.deps.ResolveWorkspaceRoot(p.Cwd) + if err != nil { + return nil, RPCError(codeInvalidParams, err.Error()) + } + } result := ListSessionsResult{Sessions: make([]SessionInfo, 0, len(items))} for _, item := range items { - if cwd != "" && item.Cwd != cwd { - continue + if cwd != "" { + itemRoot, err := a.deps.ResolveWorkspaceRoot(item.Cwd) + if err != nil || itemRoot != cwd { + continue + } } result.Sessions = append(result.Sessions, SessionInfo{ SessionID: item.SessionID, diff --git a/internal/acp/agent_test.go b/internal/acp/agent_test.go index a27e806d2..5cff92842 100644 --- a/internal/acp/agent_test.go +++ b/internal/acp/agent_test.go @@ -166,6 +166,7 @@ func TestACPEndToEndPrompt(t *testing.T) { func TestACPListsOnlyResumableSessionMetadata(t *testing.T) { deps := testDeps(t) + deps.ResolveWorkspaceRoot = func(cwd string) (string, error) { return filepath.Clean(cwd), nil } workspaceA := t.TempDir() workspaceB := t.TempDir() for _, input := range []sessions.CreateInput{ @@ -208,6 +209,21 @@ func TestACPListsOnlyResumableSessionMetadata(t *testing.T) { if len(filtered.Sessions) != 1 || filtered.Sessions[0].SessionID != "desktop-b" { t.Fatalf("filtered sessions = %+v, want only desktop-b", filtered.Sessions) } + + var equivalent ListSessionsResult + equivalentPath := workspaceB + string(os.PathSeparator) + "." + if err := h.client.Call(ctx, MethodSessionList, ListSessionsParams{Cwd: equivalentPath}, &equivalent); err != nil { + t.Fatalf("equivalent-path session/list: %v", err) + } + if len(equivalent.Sessions) != 1 || equivalent.Sessions[0].SessionID != "desktop-b" { + t.Fatalf("equivalent-path sessions = %+v, want only desktop-b", equivalent.Sessions) + } + + err := h.client.Call(ctx, MethodSessionList, ListSessionsParams{Cursor: "not-issued"}, &ListSessionsResult{}) + var rpcErr *rpcError + if !errors.As(err, &rpcErr) || rpcErr.Code != codeInvalidParams { + t.Fatalf("invalid cursor error = %v, want invalid params", err) + } } func TestACPLoadReplaysHistoryAndResumeDoesNot(t *testing.T) { @@ -235,6 +251,7 @@ func TestACPLoadReplaysHistoryAndResumeDoesNot(t *testing.T) { wantKinds := []string{UpdateUserMessageChunk, UpdateAgentMessageChunk, UpdateUserMessageChunk} wantText := []string{"first user", "first answer", "second user"} seenIDs := map[string]bool{} + firstLoadIDs := make([]string, 0, len(wantKinds)) for i := range wantKinds { select { case update := <-loader.notifications: @@ -245,11 +262,27 @@ func TestACPLoadReplaysHistoryAndResumeDoesNot(t *testing.T) { t.Fatalf("history update %d has missing/duplicate message id %q", i, update.MessageID) } seenIDs[update.MessageID] = true + firstLoadIDs = append(firstLoadIDs, update.MessageID) case <-ctx.Done(): t.Fatalf("history update %d was not replayed", i) } } loader.stop() + secondLoader := newHarness(t, deps) + if err := secondLoader.client.Call(ctx, MethodSessionLoad, LoadSessionParams{SessionID: created.SessionID, Cwd: workspace, McpServers: []McpServer{}}, &LoadSessionResult{}); err != nil { + t.Fatalf("second session/load: %v", err) + } + for i, wantID := range firstLoadIDs { + select { + case update := <-secondLoader.notifications: + if update.MessageID != wantID { + t.Fatalf("second load message id %d = %q, want stable %q", i, update.MessageID, wantID) + } + case <-ctx.Done(): + t.Fatalf("second load history update %d was not replayed", i) + } + } + secondLoader.stop() resumer := newHarness(t, deps) defer resumer.stop() @@ -263,6 +296,38 @@ func TestACPLoadReplaysHistoryAndResumeDoesNot(t *testing.T) { } } +func TestACPLoadAndResumeStayBoundToThePersistedWorkspace(t *testing.T) { + deps := testDeps(t) + workspaceA := t.TempDir() + workspaceB := t.TempDir() + created, err := deps.Store.Create(sessions.CreateInput{SessionID: "workspace-bound", Cwd: workspaceA}) + if err != nil { + t.Fatal(err) + } + h := newHarness(t, deps) + defer h.stop() + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() + + for _, method := range []string{MethodSessionLoad, MethodSessionResume} { + err := h.client.Call(ctx, method, LoadSessionParams{SessionID: created.SessionID, Cwd: workspaceB, McpServers: []McpServer{}}, &LoadSessionResult{}) + var rpcErr *rpcError + if !errors.As(err, &rpcErr) || rpcErr.Code != codeInvalidParams || !strings.Contains(rpcErr.Message, "persisted workspace") { + t.Fatalf("%s with mismatched cwd error = %v, want persisted-workspace invalid params", method, err) + } + } + + missing, err := deps.Store.Create(sessions.CreateInput{SessionID: "workspace-missing"}) + if err != nil { + t.Fatal(err) + } + err = h.client.Call(ctx, MethodSessionResume, ResumeSessionParams{SessionID: missing.SessionID, Cwd: workspaceA, McpServers: []McpServer{}}, &ResumeSessionResult{}) + var rpcErr *rpcError + if !errors.As(err, &rpcErr) || rpcErr.Code != codeInvalidParams || !strings.Contains(rpcErr.Message, "no persisted workspace") { + t.Fatalf("resume with missing persisted cwd error = %v, want invalid params", err) + } +} + func TestACPModelConfigOptionsCatalogSelectionAndLoad(t *testing.T) { deps := testDeps(t) deps.ResolveConfig = func(_ string, o config.Overrides) (config.ResolvedConfig, error) { From 57ccbd5f23468c261d2d7581017e1b1d310b364f Mon Sep 17 00:00:00 2001 From: KRATOS <84986124+gnanam1990@users.noreply.github.com> Date: Tue, 18 Aug 2026 00:36:53 +0530 Subject: [PATCH 04/13] fix(acp): compare workspaces by filesystem identity, not by spelling MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit @Vasanthdev2004's three findings, all reproduced before changing anything. TWO SPELLINGS OF ONE DIRECTORY WERE TWO WORKSPACES. Both comparisons were string equality on ResolveWorkspaceRoot output, and that resolver is abs plus filepath.Clean plus a stat — it does not fold case and does not resolve junctions. A session persisted from the TUI was unresumable from an editor holding a different spelling of the same project folder, and session/list filtered by the other spelling returned nothing, which makes it an invisible failure rather than a reported one. It lands on exactly the case this feature exists for, and on the two processes most likely to disagree about spelling. os.SameFile asks the filesystem which directories these are, which is the question. filepath.EvalSymlinks is NOT the fix on Windows — it normalises a drive letter and returns a junction path unchanged, so the alias survives it, and junctions need no privilege. String equality stays as the fast path, and a stat failure falls back to it rather than widening the match: this gate refuses access to another workspace's files and configuration, so an unanswerable comparison denies. THE TEST COULD NOT SEE ANY OF IT. testDeps resolves with the identity function, so the guard degenerated to "are these two strings different" fed two unrelated temp directories — it could only ever answer yes. The rejection direction was pinned and the acceptance direction was asserted nowhere. The new test uses a resolver reproducing the production normalisation and drives the ACCEPTANCE direction through an alias, skipping if the filesystem folds the alias away so it never passes vacuously. Reverting to string equality fails it three ways: load, resume, and a list that returns zero. THE WIRE KEYS ARE AN EXTERNAL CONTRACT. Nothing pinned sessionId, cwd, title, updatedAt, _meta, modelId, createdAt, sessions, nextCursor, cursor, loadSession, promptCapabilities, sessionCapabilities, list or resume, so renaming a Go field would break every client and leave the suite green. Renaming modelId to model_id now fails. Origin-Session: local-79d7a0 | Claude Code | 5 prompts Origin-Snapshot: c175cabb9d50 Origin-Session: local-13d543 | Claude Code | 5 prompts Origin-Snapshot: 6b5eb8ba4e5b Origin-Session: local-c962d7 | Claude Code | 3 prompts Origin-Snapshot: d2b8f44a9abc --- internal/acp/agent.go | 39 ++++++++- internal/acp/agent_test.go | 163 +++++++++++++++++++++++++++++++++++++ 2 files changed, 200 insertions(+), 2 deletions(-) diff --git a/internal/acp/agent.go b/internal/acp/agent.go index 00559d996..2de080e44 100644 --- a/internal/acp/agent.go +++ b/internal/acp/agent.go @@ -8,6 +8,7 @@ import ( "errors" "fmt" "log" + "os" "strings" "sync" @@ -196,7 +197,7 @@ func (a *Agent) activatePersistedSession(ctx context.Context, p LoadSessionParam // ACP session cwd is immutable. Loading history under a different root // would give a conversation from one workspace access to another // workspace's configuration, files, and tools. - if root != persistedRoot { + if !sameWorkspace(root, persistedRoot) { return nil, RPCError(codeInvalidParams, "session cwd does not match its persisted workspace") } // Load history BEFORE publishing the session so no concurrent prompt observes @@ -258,7 +259,7 @@ func (a *Agent) handleSessionList(_ context.Context, params json.RawMessage) (an for _, item := range items { if cwd != "" { itemRoot, err := a.deps.ResolveWorkspaceRoot(item.Cwd) - if err != nil || itemRoot != cwd { + if err != nil || !sameWorkspace(itemRoot, cwd) { continue } } @@ -875,3 +876,37 @@ func (s *acpSession) snapshotHistory() []turnRecord { defer s.mu.Unlock() return append([]turnRecord(nil), s.history...) } + +// sameWorkspace reports whether two resolved roots name the same directory. +// +// STRING EQUALITY IS NOT DIRECTORY IDENTITY. ResolveWorkspaceRoot is abs plus +// filepath.Clean plus a stat: it does not fold case and does not resolve +// junctions, so one directory reached by two spellings produces two different +// roots. A session persisted from the TUI was then unresumable from an editor +// holding a different spelling of the same project folder, and session/list +// filtered by the other spelling returned nothing — not a failed resume but an +// invisible one, on exactly the case this feature exists for. +// +// filepath.EvalSymlinks is NOT the fix on Windows: it normalises a drive letter +// but returns a junction path unchanged, so the alias survives it. Junctions need +// no privilege, so this is ordinary rather than exotic. os.SameFile compares the +// filesystem's own identity for the two directories, which is the question being +// asked. +// +// The string comparison stays as the fast path, and a stat failure falls back to +// it rather than widening the match — this gate refuses access to another +// workspace's files and configuration, so an unanswerable comparison denies. +func sameWorkspace(left, right string) bool { + if left == right { + return true + } + leftInfo, err := os.Stat(left) + if err != nil { + return false + } + rightInfo, err := os.Stat(right) + if err != nil { + return false + } + return os.SameFile(leftInfo, rightInfo) +} diff --git a/internal/acp/agent_test.go b/internal/acp/agent_test.go index 5cff92842..81bc0bd9d 100644 --- a/internal/acp/agent_test.go +++ b/internal/acp/agent_test.go @@ -8,6 +8,7 @@ import ( "io" "os" "path/filepath" + "sort" "strings" "testing" "time" @@ -763,3 +764,165 @@ func drainTextUntil(t *testing.T, ch <-chan string, done func(string) bool) stri } } } + +// normalisingResolver reproduces what ResolveWorkspaceRoot actually does — abs, +// Clean, and a stat that the path exists — WITHOUT resolving symlinks or folding +// case, which is the behaviour that makes two spellings of one directory produce +// two different roots. +// +// The package's own testDeps resolver is the identity function. Under it the +// workspace guard degenerates to "are these two strings different", fed two +// unrelated temp directories, so it can only ever answer yes: the rejection +// direction is pinned and the acceptance direction is asserted nowhere. +func normalisingResolver(t *testing.T) func(string) (string, error) { + t.Helper() + return func(cwd string) (string, error) { + absolute, err := filepath.Abs(cwd) + if err != nil { + return "", err + } + absolute = filepath.Clean(absolute) + info, err := os.Stat(absolute) + if err != nil { + return "", err + } + if !info.IsDir() { + return "", errors.New("workspace is not a directory") + } + return absolute, nil + } +} + +// ONE DIRECTORY UNDER TWO SPELLINGS IS ONE WORKSPACE. +// +// A session persisted from the TUI has to stay resumable from an editor holding +// a different spelling of the same project folder — the two processes most +// likely to disagree about spelling, and the case this feature exists for. It +// failed closed, so it blocked legitimate resumes rather than admitting foreign +// ones, but session/list filtered by the other spelling returned nothing, which +// makes it an invisible failure rather than a reported one. +func TestACPResumesAcrossTwoSpellingsOfOneWorkspace(t *testing.T) { + real := t.TempDir() + alias := filepath.Join(t.TempDir(), "alias") + if err := os.Symlink(real, alias); err != nil { + t.Skipf("cannot create a directory alias here: %v", err) + } + // The premise: the resolver really does produce two different strings. + resolve := normalisingResolver(t) + realRoot, err := resolve(real) + if err != nil { + t.Fatal(err) + } + aliasRoot, err := resolve(alias) + if err != nil { + t.Fatal(err) + } + if realRoot == aliasRoot { + t.Skipf("this filesystem folds the alias away (%q == %q); the guard cannot be exercised", realRoot, aliasRoot) + } + + deps := testDeps(t) + deps.ResolveWorkspaceRoot = resolve + created, err := deps.Store.Create(sessions.CreateInput{SessionID: "aliased-workspace", Cwd: real}) + if err != nil { + t.Fatal(err) + } + h := newHarness(t, deps) + defer h.stop() + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() + + // ACCEPTANCE: resume and load under the OTHER spelling must both work. + for _, method := range []string{MethodSessionLoad, MethodSessionResume} { + var result LoadSessionResult + if err := h.client.Call(ctx, method, LoadSessionParams{SessionID: created.SessionID, Cwd: alias, McpServers: []McpServer{}}, &result); err != nil { + t.Errorf("%s under an alias of the persisted workspace failed: %v", method, err) + } + } + + // And session/list filtered by the alias must still find it. + var listed ListSessionsResult + if err := h.client.Call(ctx, MethodSessionList, ListSessionsParams{Cwd: alias}, &listed); err != nil { + t.Fatalf("session/list: %v", err) + } + found := false + for _, item := range listed.Sessions { + if item.SessionID == created.SessionID { + found = true + } + } + if !found { + t.Errorf("session/list filtered by an alias of its own workspace returned %d sessions without it", len(listed.Sessions)) + } + + // REJECTION still holds: a genuinely different directory is refused. + other := t.TempDir() + err = h.client.Call(ctx, MethodSessionResume, ResumeSessionParams{SessionID: created.SessionID, Cwd: other, McpServers: []McpServer{}}, &ResumeSessionResult{}) + var rpcErr *rpcError + if !errors.As(err, &rpcErr) || rpcErr.Code != codeInvalidParams { + t.Errorf("resume from an unrelated workspace = %v, want invalid params", err) + } +} + +// THE WIRE KEYS ARE AN EXTERNAL CONTRACT, not internal names. +// +// Nothing pinned them, so renaming a Go field — or dropping an omitempty — +// would break every client and leave the suite green. These are the keys this +// feature adds to the protocol; a change here is a change to what editors +// consume. +func TestSessionWireKeysAreStable(t *testing.T) { + marshalled := func(v any) map[string]any { + t.Helper() + raw, err := json.Marshal(v) + if err != nil { + t.Fatal(err) + } + var out map[string]any + if err := json.Unmarshal(raw, &out); err != nil { + t.Fatal(err) + } + return out + } + has := func(where string, got map[string]any, want ...string) { + t.Helper() + for _, key := range want { + if _, ok := got[key]; !ok { + t.Errorf("%s is missing the wire key %q; got %v", where, key, keysOf(got)) + } + } + } + + has("SessionInfo", marshalled(SessionInfo{ + SessionID: "s1", Cwd: "/w", Title: "t", UpdatedAt: "now", + Meta: &SessionInfoMeta{ModelID: "m", CreatedAt: "then"}, + }), "sessionId", "cwd", "title", "updatedAt", "_meta") + + has("SessionInfoMeta", marshalled(SessionInfoMeta{ModelID: "m", CreatedAt: "then"}), + "modelId", "createdAt") + + has("ListSessionsResult", marshalled(ListSessionsResult{ + Sessions: []SessionInfo{}, NextCursor: "c", + }), "sessions", "nextCursor") + + has("ListSessionsParams", marshalled(ListSessionsParams{Cwd: "/w", Cursor: "c"}), + "cwd", "cursor") + + // sessionCapabilities is omitempty, so it must appear when SET — a client + // discovers list/resume support through it. + has("AgentCapabilities", marshalled(AgentCapabilities{ + LoadSession: true, + SessionCapabilities: &SessionCapabilities{List: &struct{}{}, Resume: &struct{}{}}, + }), "loadSession", "promptCapabilities", "sessionCapabilities") + + capabilities := marshalled(SessionCapabilities{List: &struct{}{}, Resume: &struct{}{}}) + has("SessionCapabilities", capabilities, "list", "resume") +} + +func keysOf(m map[string]any) []string { + out := make([]string, 0, len(m)) + for key := range m { + out = append(out, key) + } + sort.Strings(out) + return out +} From e7ee49def78fdffaa5742231e581aedfc7a78ba2 Mon Sep 17 00:00:00 2001 From: KRATOS <84986124+gnanam1990@users.noreply.github.com> Date: Tue, 18 Aug 2026 12:39:16 +0530 Subject: [PATCH 05/13] fix(acp): test the alias guard where the bug lives, and stop listing unusable sessions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit THE TEST COULD NOT RUN ON THE PLATFORM THE BUG IS FROM. @Vasanthdev2004's point, and he is right that it is not a nit. The alias test built its second name with os.Symlink, which needs a privilege an ordinary Windows session does not hold, so it SKIPPED there — and Windows is where junctions exist and where this defect came from. The identity guard was verified by CI on the two platforms that never had the problem. It now builds the alias with mklink /J on Windows, which needs no privilege and is how he found the defect in the first place, and keeps the symlink arm elsewhere. This is the second time in this series a correct fix shipped with a test that could not exercise it: the same helper shape was added to internal/memory for the same reason a day earlier. A SESSION WITH NO PERSISTED WORKSPACE IS NOT RESUMABLE, SO IT IS NOT LISTED. CodeRabbit's finding. activatePersistedSession refuses an empty Cwd, but the listing advertised it anyway — a menu entry that only fails when taken. The test asserts both halves, because the listing is only correct relative to what resume will accept: the omitted session is checked to really fail on resume, and a usable session is checked to survive the filter. Mutation-checked: removing the filter advertises the unusable session again. Origin-Session: local-13d543 | Claude Code | 5 prompts Origin-Snapshot: 6b5eb8ba4e5b Origin-Session: local-c962d7 | Claude Code | 3 prompts Origin-Snapshot: d2b8f44a9abc --- internal/acp/agent.go | 7 ++++ internal/acp/agent_test.go | 73 ++++++++++++++++++++++++++++++++++++-- 2 files changed, 77 insertions(+), 3 deletions(-) diff --git a/internal/acp/agent.go b/internal/acp/agent.go index 2de080e44..e55c819e3 100644 --- a/internal/acp/agent.go +++ b/internal/acp/agent.go @@ -257,6 +257,13 @@ func (a *Agent) handleSessionList(_ context.Context, params json.RawMessage) (an } result := ListSessionsResult{Sessions: make([]SessionInfo, 0, len(items))} for _, item := range items { + // A session with no persisted workspace cannot be resumed — + // activatePersistedSession refuses it — so advertising it offers the + // client something that only fails when taken. Listing is a menu, and + // every entry on it has to be orderable. + if strings.TrimSpace(item.Cwd) == "" { + continue + } if cwd != "" { itemRoot, err := a.deps.ResolveWorkspaceRoot(item.Cwd) if err != nil || !sameWorkspace(itemRoot, cwd) { diff --git a/internal/acp/agent_test.go b/internal/acp/agent_test.go index 81bc0bd9d..cb3387fb6 100644 --- a/internal/acp/agent_test.go +++ b/internal/acp/agent_test.go @@ -7,7 +7,9 @@ import ( "fmt" "io" "os" + "os/exec" "path/filepath" + "runtime" "sort" "strings" "testing" @@ -804,9 +806,7 @@ func normalisingResolver(t *testing.T) func(string) (string, error) { func TestACPResumesAcrossTwoSpellingsOfOneWorkspace(t *testing.T) { real := t.TempDir() alias := filepath.Join(t.TempDir(), "alias") - if err := os.Symlink(real, alias); err != nil { - t.Skipf("cannot create a directory alias here: %v", err) - } + directoryAlias(t, real, alias) // The premise: the resolver really does produce two different strings. resolve := normalisingResolver(t) realRoot, err := resolve(real) @@ -926,3 +926,70 @@ func keysOf(m map[string]any) []string { sort.Strings(out) return out } + +// directoryAlias makes `alias` a second name for `target`. +// +// A JUNCTION ON WINDOWS, not a symlink. os.Symlink needs a privilege an ordinary +// Windows session does not hold, so a test built on it SKIPS there — and Windows +// is where junctions exist and where this bug came from. The guard would have +// been verified only on the two platforms that never had the problem. mklink /J +// needs no privilege, which is also how the defect was found. +func directoryAlias(t *testing.T, target, alias string) { + t.Helper() + if runtime.GOOS == "windows" { + if out, err := exec.Command("cmd", "/c", "mklink", "/J", alias, target).CombinedOutput(); err != nil { + t.Skipf("cannot create a junction: %v %s", err, out) + } + return + } + if err := os.Symlink(target, alias); err != nil { + t.Skipf("cannot create a directory alias here: %v", err) + } +} + +// A SESSION WITH NO PERSISTED WORKSPACE IS NOT RESUMABLE, SO IT IS NOT LISTED. +// +// activatePersistedSession refuses a session whose Cwd is empty, but the listing +// advertised it anyway — offering the client a menu entry that only fails when +// taken. Both halves are asserted together, because the listing is only correct +// relative to what resume will accept. +func TestSessionListOmitsSessionsWithoutAWorkspace(t *testing.T) { + deps := testDeps(t) + workspace := t.TempDir() + usable, err := deps.Store.Create(sessions.CreateInput{SessionID: "has-cwd", Cwd: workspace}) + if err != nil { + t.Fatal(err) + } + if _, err := deps.Store.Create(sessions.CreateInput{SessionID: "no-cwd"}); err != nil { + t.Fatal(err) + } + h := newHarness(t, deps) + defer h.stop() + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() + + var listed ListSessionsResult + if err := h.client.Call(ctx, MethodSessionList, ListSessionsParams{}, &listed); err != nil { + t.Fatal(err) + } + sawUsable := false + for _, item := range listed.Sessions { + if item.SessionID == "no-cwd" { + t.Errorf("session/list advertised a session with no persisted workspace") + } + if item.SessionID == usable.SessionID { + sawUsable = true + } + } + if !sawUsable { + t.Errorf("session/list dropped a usable session while filtering; got %d", len(listed.Sessions)) + } + + // The other half of the contract: resuming the omitted one really does fail, + // which is why omitting it is right rather than merely tidy. + err = h.client.Call(ctx, MethodSessionResume, ResumeSessionParams{SessionID: "no-cwd", Cwd: workspace, McpServers: []McpServer{}}, &ResumeSessionResult{}) + var rpcErr *rpcError + if !errors.As(err, &rpcErr) || !strings.Contains(rpcErr.Message, "persisted workspace") { + t.Errorf("resume of a workspace-less session = %v, want a persisted-workspace error", err) + } +} From 97983335c6013e49cd64717a886eeaa09a1502ef Mon Sep 17 00:00:00 2001 From: KRATOS <84986124+gnanam1990@users.noreply.github.com> Date: Wed, 19 Aug 2026 18:40:14 +0530 Subject: [PATCH 06/13] fix(acp): resolve every listed workspace, not only when a filter was supplied MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit @anandh8x's P1, both halves reproduced before changing anything. The loop resolved item.Cwd only when a cwd filter was present, and skipped only a blank one. Two shapes stayed on the menu that session/resume then refuses: - a session whose persisted workspace has since been deleted, advertised as resumable - a legacy entry holding a relative path, reported as cwd "." although ACP requires SessionInfo.cwd to be absolute Every entry is now resolved unconditionally, anything that cannot resolve is omitted, and the RESOLVED root is what SessionInfo carries — absolute as the contract requires, and the same value the client hands back on resume. The optional identity filter then applies to the resolved roots, which is also where it belonged. Listing is a menu: activatePersistedSession resolves and refuses what it cannot reach, so anything this loop cannot resolve is something a client would be offered and then denied. Mutation-checked: restoring the resolve-only-when-filtered shape re-advertises the deleted workspace and re-emits the relative cwd. Origin-Session: local-76c8d7 | Claude Code | 6 prompts Origin-Snapshot: 259b715cf0fd Origin-Session: local-13d543 | Claude Code | 5 prompts Origin-Snapshot: 6b5eb8ba4e5b Origin-Session: local-c962d7 | Claude Code | 3 prompts Origin-Snapshot: d2b8f44a9abc --- internal/acp/agent.go | 29 +++++++++++++-------- internal/acp/agent_test.go | 52 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 71 insertions(+), 10 deletions(-) diff --git a/internal/acp/agent.go b/internal/acp/agent.go index e55c819e3..d54f9784d 100644 --- a/internal/acp/agent.go +++ b/internal/acp/agent.go @@ -257,23 +257,32 @@ func (a *Agent) handleSessionList(_ context.Context, params json.RawMessage) (an } result := ListSessionsResult{Sessions: make([]SessionInfo, 0, len(items))} for _, item := range items { - // A session with no persisted workspace cannot be resumed — - // activatePersistedSession refuses it — so advertising it offers the - // client something that only fails when taken. Listing is a menu, and - // every entry on it has to be orderable. + // EVERY ENTRY IS RESOLVED, FILTER OR NO FILTER. Listing is a menu, and + // every item on it has to be orderable: activatePersistedSession + // resolves the persisted workspace and refuses what it cannot reach, so + // anything this loop cannot resolve is something the client would be + // offered and then denied. + // + // Resolving only when a cwd filter was supplied left two shapes through — + // a session whose workspace has since been deleted, and a legacy entry + // holding a relative path, which was then reported as cwd "." even though + // ACP requires SessionInfo.cwd to be absolute. if strings.TrimSpace(item.Cwd) == "" { continue } - if cwd != "" { - itemRoot, err := a.deps.ResolveWorkspaceRoot(item.Cwd) - if err != nil || !sameWorkspace(itemRoot, cwd) { - continue - } + itemRoot, err := a.deps.ResolveWorkspaceRoot(item.Cwd) + if err != nil { + continue + } + if cwd != "" && !sameWorkspace(itemRoot, cwd) { + continue } result.Sessions = append(result.Sessions, SessionInfo{ SessionID: item.SessionID, Title: item.Title, - Cwd: item.Cwd, + // The RESOLVED root, not the stored string: absolute as ACP + // requires, and the same value the client will hand back on resume. + Cwd: itemRoot, UpdatedAt: item.UpdatedAt, Meta: &SessionInfoMeta{ModelID: item.ModelID, CreatedAt: item.CreatedAt}, }) diff --git a/internal/acp/agent_test.go b/internal/acp/agent_test.go index cb3387fb6..00cbd3509 100644 --- a/internal/acp/agent_test.go +++ b/internal/acp/agent_test.go @@ -993,3 +993,55 @@ func TestSessionListOmitsSessionsWithoutAWorkspace(t *testing.T) { t.Errorf("resume of a workspace-less session = %v, want a persisted-workspace error", err) } } + +// EVERY LISTED SESSION IS ONE THE CLIENT CAN ACTUALLY TAKE. +// +// Resolving the persisted workspace only when a cwd filter was supplied left two +// shapes on the menu that resume then refuses: a session whose workspace has +// since been deleted, and a legacy entry holding a relative path — reported as +// cwd "." although ACP requires SessionInfo.cwd to be absolute. +func TestSessionListResolvesEveryWorkspace(t *testing.T) { + deps := testDeps(t) + deps.ResolveWorkspaceRoot = normalisingResolver(t) + + gone := t.TempDir() + if _, err := deps.Store.Create(sessions.CreateInput{SessionID: "gone-ws", Cwd: gone}); err != nil { + t.Fatal(err) + } + if err := os.RemoveAll(gone); err != nil { + t.Fatal(err) + } + if _, err := deps.Store.Create(sessions.CreateInput{SessionID: "relative-ws", Cwd: "."}); err != nil { + t.Fatal(err) + } + live := t.TempDir() + if _, err := deps.Store.Create(sessions.CreateInput{SessionID: "live-ws", Cwd: live}); err != nil { + t.Fatal(err) + } + + h := newHarness(t, deps) + defer h.stop() + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() + var listed ListSessionsResult + if err := h.client.Call(ctx, MethodSessionList, ListSessionsParams{}, &listed); err != nil { + t.Fatal(err) + } + + seen := map[string]string{} + for _, item := range listed.Sessions { + seen[item.SessionID] = item.Cwd + } + if _, listedGone := seen["gone-ws"]; listedGone { + t.Error("a session whose workspace no longer exists was advertised; resume would refuse it") + } + if _, listedLive := seen["live-ws"]; !listedLive { + t.Error("a usable session was dropped while filtering unusable ones") + } + // EVERY reported cwd is absolute, which is the contract clients rely on. + for id, cwd := range seen { + if !filepath.IsAbs(cwd) { + t.Errorf("session %s was listed with a relative cwd %q; ACP requires an absolute path", id, cwd) + } + } +} From 9fb0b221d315e106bf32d39fe20182dbbac0f02d Mon Sep 17 00:00:00 2001 From: KRATOS <84986124+gnanam1990@users.noreply.github.com> Date: Wed, 19 Aug 2026 19:16:37 +0530 Subject: [PATCH 07/13] test(acp): assert the relative entry is retained, not merely absolute MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CodeRabbit's catch, and the test was genuinely weaker than it looked. It checked that the deleted workspace was gone, the live one kept, and every listed cwd absolute — all of which a "fix" that simply DISCARDED any non-absolute entry would satisfy, while losing a resumable session. Presence is now asserted separately from spelling: the relative entry must still be listed, and listed with an absolute path. Mutation-checked: skipping non-absolute entries instead of resolving them now fails with "a session with a resolvable relative workspace was dropped rather than normalised". The first attempt at that mutation did not compile, so it proved nothing until it was rewritten — worth saying, because a mutation that fails to build looks exactly like a test that passes. Origin-Session: local-13d543 | Claude Code | 5 prompts Origin-Snapshot: 6b5eb8ba4e5b Origin-Session: local-c962d7 | Claude Code | 3 prompts Origin-Snapshot: d2b8f44a9abc --- internal/acp/agent_test.go | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/internal/acp/agent_test.go b/internal/acp/agent_test.go index 00cbd3509..7ef28fd31 100644 --- a/internal/acp/agent_test.go +++ b/internal/acp/agent_test.go @@ -1038,6 +1038,16 @@ func TestSessionListResolvesEveryWorkspace(t *testing.T) { if _, listedLive := seen["live-ws"]; !listedLive { t.Error("a usable session was dropped while filtering unusable ones") } + // THE RELATIVE ENTRY IS RETAINED, not quietly dropped. Resolving every item + // could have been "fixed" by discarding anything not already absolute, which + // would pass the absolute-path check below while losing a resumable session — + // so presence is asserted separately from spelling. + relative, listedRelative := seen["relative-ws"] + if !listedRelative { + t.Error("a session with a resolvable relative workspace was dropped rather than normalised") + } else if !filepath.IsAbs(relative) { + t.Errorf("the relative entry was listed as %q; it must be normalised to an absolute path", relative) + } // EVERY reported cwd is absolute, which is the contract clients rely on. for id, cwd := range seen { if !filepath.IsAbs(cwd) { From 6fa23fb01f93cd58ee275e646776c0417fbaaba2 Mon Sep 17 00:00:00 2001 From: KRATOS <84986124+gnanam1990@users.noreply.github.com> Date: Thu, 20 Aug 2026 20:53:20 +0530 Subject: [PATCH 08/13] fix(acp): a relative persisted workspace is omitted, not rebased Reported by @anandh8x. Resolving a stored relative cwd does not recover the session's workspace, it invents one: ResolveWorkspaceRoot joins it against whatever directory the ACP server happens to be running in, and that invented absolute path was then advertised as the session's workspace and accepted as its home on resume. Reproduced on f2c6fc9a, a session persisted with cwd ".": LISTED legacy-rel as cwd="/Users/kratos/dev/f914/internal/acp" resume with an UNRELATED workspace -> err=... cwd does not match its persisted workspace resume with NO cwd (falls back to ".") -> err= The mismatch check does its job when the client names a workspace, so the only opening was the fallback path, where the rebased value was compared against itself and always agreed. A conversation created for one project could be resumed against another project's files, configuration and tools. Both doors now take the same guard: handleSessionList omits an entry whose persisted cwd is not absolute, and activatePersistedSession refuses one rather than resolving it. The original base is not knowable from the metadata, so guessing at it is not an option a fix can take. This reverses an earlier assertion in TestSessionListResolvesEveryWorkspace, which expected the relative entry to be normalised and retained. That was requested in review on the grounds that dropping it loses a resumable session. It does, but the entry was never resumable into its own workspace, only into this process's. The test now asserts it is dropped, and a new TestResumeRefusesARelativePersistedWorkspace covers the fallback path that the listing filter alone leaves open. Both guards mutation-checked: removing either one fails its test. Pre-existing on this branch and on its merge-base, unrelated to this change: TestRunDoctorFormatsRedactedProviderDiagnostics and TestRunDoctorConnectivityProbesProvider both exit 3 in this environment. Origin-Session: local-8cd239 | Claude Code | 11 prompts Origin-Snapshot: 365efe3045f2 Origin-Session: local-13d543 | Claude Code | 5 prompts Origin-Snapshot: 6b5eb8ba4e5b Origin-Session: local-c962d7 | Claude Code | 3 prompts Origin-Snapshot: d2b8f44a9abc --- internal/acp/agent.go | 19 +++++++++++++++ internal/acp/agent_test.go | 48 +++++++++++++++++++++++++++++++------- 2 files changed, 58 insertions(+), 9 deletions(-) diff --git a/internal/acp/agent.go b/internal/acp/agent.go index d54f9784d..533d8f5c4 100644 --- a/internal/acp/agent.go +++ b/internal/acp/agent.go @@ -9,6 +9,7 @@ import ( "fmt" "log" "os" + "path/filepath" "strings" "sync" @@ -182,6 +183,14 @@ func (a *Agent) activatePersistedSession(ctx context.Context, p LoadSessionParam if strings.TrimSpace(meta.Cwd) == "" { return nil, RPCError(codeInvalidParams, "session has no persisted workspace: "+p.SessionID) } + // SAME RULE ON THE WAY IN. Omitting a relative entry from the listing is not + // enough: session/resume falls back to meta.Cwd when the client sends no cwd, + // so a stored "." resolved against this process's directory and bound the + // conversation to it — verified returning no error at all. A workspace that + // cannot be identified is not one this session can be restored into. + if !filepath.IsAbs(meta.Cwd) { + return nil, RPCError(codeInvalidParams, "session workspace is not an absolute path, so it cannot be identified: "+p.SessionID) + } persistedRoot, err := a.deps.ResolveWorkspaceRoot(meta.Cwd) if err != nil { return nil, RPCError(codeInvalidParams, "persisted session workspace is unavailable: "+err.Error()) @@ -270,6 +279,16 @@ func (a *Agent) handleSessionList(_ context.Context, params json.RawMessage) (an if strings.TrimSpace(item.Cwd) == "" { continue } + // A RELATIVE PERSISTED CWD HAS NO RECOVERABLE IDENTITY. Resolving one + // rebases it onto wherever this ACP server happens to be running and + // advertises that invented absolute path as the session's workspace — so a + // conversation created for one project could be resumed against another + // project's configuration, files and tools. The original base is not + // knowable from the metadata, so the honest answer is to omit the entry + // rather than to guess at it. + if !filepath.IsAbs(item.Cwd) { + continue + } itemRoot, err := a.deps.ResolveWorkspaceRoot(item.Cwd) if err != nil { continue diff --git a/internal/acp/agent_test.go b/internal/acp/agent_test.go index 7ef28fd31..adf132578 100644 --- a/internal/acp/agent_test.go +++ b/internal/acp/agent_test.go @@ -1038,15 +1038,16 @@ func TestSessionListResolvesEveryWorkspace(t *testing.T) { if _, listedLive := seen["live-ws"]; !listedLive { t.Error("a usable session was dropped while filtering unusable ones") } - // THE RELATIVE ENTRY IS RETAINED, not quietly dropped. Resolving every item - // could have been "fixed" by discarding anything not already absolute, which - // would pass the absolute-path check below while losing a resumable session — - // so presence is asserted separately from spelling. - relative, listedRelative := seen["relative-ws"] - if !listedRelative { - t.Error("a session with a resolvable relative workspace was dropped rather than normalised") - } else if !filepath.IsAbs(relative) { - t.Errorf("the relative entry was listed as %q; it must be normalised to an absolute path", relative) + // THE RELATIVE ENTRY IS DROPPED. An earlier revision of this test asserted the + // opposite — that "." be normalised and kept — which reads as the generous + // choice but resolves the stored path against whatever directory this ACP + // server was started in. That does not recover the session's workspace; it + // invents one, and then advertises the invention as fact, so a conversation + // created for one project becomes resumable against another project's files, + // configuration and tools. The original base is not knowable from the + // metadata, so no entry is the only honest answer. + if invented, listedRelative := seen["relative-ws"]; listedRelative { + t.Errorf("a relative persisted workspace was listed as %q, rebased onto the current process directory", invented) } // EVERY reported cwd is absolute, which is the contract clients rely on. for id, cwd := range seen { @@ -1055,3 +1056,32 @@ func TestSessionListResolvesEveryWorkspace(t *testing.T) { } } } + +func TestResumeRefusesARelativePersistedWorkspace(t *testing.T) { + // OMITTING THE ENTRY FROM THE LISTING IS NOT THE WHOLE FIX. A client can ask + // to resume any id it already knows, and session/resume falls back to the + // persisted cwd when the request carries none — so a stored "." was resolved + // against this process's directory and the session bound to it, with no error + // returned at all. Both doors need the same lock. + deps := testDeps(t) + deps.ResolveWorkspaceRoot = normalisingResolver(t) + if _, err := deps.Store.Create(sessions.CreateInput{SessionID: "relative-ws", Cwd: "."}); err != nil { + t.Fatal(err) + } + + h := newHarness(t, deps) + defer h.stop() + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() + + // No cwd at all: the request that used to succeed by rebasing. + var out LoadSessionResult + if err := h.client.Call(ctx, MethodSessionLoad, LoadSessionParams{SessionID: "relative-ws"}, &out); err == nil { + t.Error("resuming a relative persisted workspace succeeded; the session was rebound to the ACP process directory") + } + // And an unrelated workspace must not be accepted as its home either. + elsewhere := t.TempDir() + if err := h.client.Call(ctx, MethodSessionLoad, LoadSessionParams{SessionID: "relative-ws", Cwd: elsewhere}, &out); err == nil { + t.Errorf("resuming a relative persisted workspace into %q succeeded", elsewhere) + } +} From 1970457a5ae01ea09dddb24b599f2af93e805065 Mon Sep 17 00:00:00 2001 From: KRATOS <84986124+gnanam1990@users.noreply.github.com> Date: Thu, 20 Aug 2026 21:09:13 +0530 Subject: [PATCH 09/13] test(acp): the resume guard is asserted through both activating methods MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Raised by CodeRabbit: the test is named for resume and called session/load. session/load and session/resume are separate entry points that today share activatePersistedSession, so an assertion through either one passes while the guard holds — but the name promised a surface it was not touching. Both are now named explicitly, which keeps that true: if resume is ever given its own path, this fails rather than quietly covering half of what it claims to. With the guard removed, both methods accept a relative persisted workspace on the no-cwd fallback. The named-workspace case was already refused by the existing mismatch check; the fallback was the only door open, and it is open on both. Not taken in this PR, from the same review: threading a rooted directory handle through ResolveWorkspaceRoot and workspace construction so a root rename or link swap cannot redirect later file operations. That is a real question and a pre-existing one — this change adds a refusal and no path handling — but it is a capability refactor across workspace and tool access with its own race test, not something to fold into a session-list fix. Worth its own issue. Origin-Session: local-8cd239 | Claude Code | 11 prompts Origin-Snapshot: 365efe3045f2 Origin-Session: local-13d543 | Claude Code | 5 prompts Origin-Snapshot: 6b5eb8ba4e5b Origin-Session: local-c962d7 | Claude Code | 3 prompts Origin-Snapshot: d2b8f44a9abc --- internal/acp/agent_test.go | 24 ++++++++++++++++-------- 1 file changed, 16 insertions(+), 8 deletions(-) diff --git a/internal/acp/agent_test.go b/internal/acp/agent_test.go index adf132578..c3f266e09 100644 --- a/internal/acp/agent_test.go +++ b/internal/acp/agent_test.go @@ -1074,14 +1074,22 @@ func TestResumeRefusesARelativePersistedWorkspace(t *testing.T) { ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) defer cancel() - // No cwd at all: the request that used to succeed by rebasing. - var out LoadSessionResult - if err := h.client.Call(ctx, MethodSessionLoad, LoadSessionParams{SessionID: "relative-ws"}, &out); err == nil { - t.Error("resuming a relative persisted workspace succeeded; the session was rebound to the ACP process directory") - } - // And an unrelated workspace must not be accepted as its home either. + // BOTH ACTIVATING METHODS, not just one. session/load and session/resume are + // separate entry points that today share activatePersistedSession — so a test + // through either passes while the guard holds. Naming both here is what keeps + // that true: if resume is ever given its own path, this fails rather than + // silently covering half the surface it claims to. elsewhere := t.TempDir() - if err := h.client.Call(ctx, MethodSessionLoad, LoadSessionParams{SessionID: "relative-ws", Cwd: elsewhere}, &out); err == nil { - t.Errorf("resuming a relative persisted workspace into %q succeeded", elsewhere) + var out LoadSessionResult + for _, method := range []string{MethodSessionLoad, MethodSessionResume} { + // No cwd at all: the request that used to succeed by rebasing onto + // whatever directory this process happens to be running in. + if err := h.client.Call(ctx, method, LoadSessionParams{SessionID: "relative-ws"}, &out); err == nil { + t.Errorf("%s of a relative persisted workspace succeeded; the session was rebound to the ACP process directory", method) + } + // And an unrelated workspace must not be accepted as its home either. + if err := h.client.Call(ctx, method, ResumeSessionParams{SessionID: "relative-ws", Cwd: elsewhere}, &out); err == nil { + t.Errorf("%s of a relative persisted workspace into %q succeeded", method, elsewhere) + } } } From f58e34fb6e471196e32b50acfd3ad390582f21bc Mon Sep 17 00:00:00 2001 From: KRATOS <84986124+gnanam1990@users.noreply.github.com> Date: Mon, 24 Aug 2026 13:31:09 +0530 Subject: [PATCH 10/13] fix(acp): activation requires an absolute cwd in the REQUEST, not just the record MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reported by @jatmn. ResumeSessionParams is a type alias for LoadSessionParams, so JSON decoding turns an OMITTED resume cwd into an empty string. That blank reached the shared activation path, whose blank-cwd fallback substitutes meta.Cwd — so {"sessionId":"known"} activated a persisted session, even though ACP v1 requires session/resume to carry an absolute working directory. This is a different hole from the persisted-cwd one fixed earlier on this branch. That guard asks whether the STORED workspace is identifiable; this asks whether the CALLER named one at all. The earlier fix does not cover it, because a stored absolute cwd passes that check and the blank request then silently inherits it. requestedWorkspace validates the request's own cwd before anything reaches the fallback: absent, empty and whitespace-only are all invalid params, and so is a relative path. Applied to both activating methods rather than to resume alone — load's omitted-cwd fallback was inheriting the same way, and leaving one door open is how this class survived the last fix. Mutation-checked at the wire, which is where the defect lives: making the blank case return no error compiles and fails the regression on four separate requests — session/load and session/resume, each with cwd omitted and with cwd blank. A Go-level test would not have caught it, since the defect is in decoding an absent field. Two notes from a verification pass, neither a defect: Error precedence changed: a blank cwd with an UNKNOWN session id now reports the cwd problem instead of "session not found". Kept deliberately and pinned — it stops the server confirming whether a session exists to a request that named no workspace. AdditionalDirectories is declared on two params structs and consumed nowhere in the repo. Not a hole today, but it is the same shape — client-supplied paths with no absoluteness rule — so the field now carries a note saying it must go through requestedWorkspace when wired up. Rebased onto ad34dc8d. go test -race ./internal/acp/ -count=5: clean. Pre-existing here and on main: TestRunDoctorFormatsRedactedProviderDiagnostics and TestRunDoctorConnectivityProbesProvider exit 3 in this environment. Origin-Session: local-c962d7 | Claude Code | 3 prompts Origin-Snapshot: d2b8f44a9abc --- internal/acp/agent.go | 66 +++++++++-- internal/acp/agent_test.go | 232 +++++++++++++++++++++++++++++++++++-- internal/acp/types.go | 15 ++- 3 files changed, 286 insertions(+), 27 deletions(-) diff --git a/internal/acp/agent.go b/internal/acp/agent.go index 533d8f5c4..fcb27027e 100644 --- a/internal/acp/agent.go +++ b/internal/acp/agent.go @@ -130,12 +130,41 @@ func (a *Agent) handleInitialize(_ context.Context, params json.RawMessage) (any // ---- session lifecycle ---- +// requestedWorkspace is the single rule for a cwd that arrived from the client, +// applied before the value can reach ResolveWorkspaceRoot. +// +// THE REQUEST'S OWN CWD IS THE ONLY THING ALLOWED TO PICK A ROOT. Every params +// type spells cwd as a plain string, so an absent field and an empty one decode +// to exactly the same "", and the resolver treats "" as "use the directory this +// process was started in" and joins a relative cwd onto it. Those two defaults +// meet in the middle: `{"sessionId":"known"}` used to activate a persisted +// session, and `{}` used to create one, both rooted at wherever the editor +// happened to spawn `zero acp` — which then becomes the sandbox and file-tool +// confinement root. The client never named that directory, so it is a root the +// server invented, and ACP requires an absolute cwd on every method carrying +// one. Rejecting here is what keeps a missing field from being answered with a +// guess. +func requestedWorkspace(cwd string) (string, error) { + trimmed := strings.TrimSpace(cwd) + if trimmed == "" { + return "", RPCError(codeInvalidParams, "cwd is required and must be an absolute path") + } + if !filepath.IsAbs(trimmed) { + return "", RPCError(codeInvalidParams, "cwd must be an absolute path: "+trimmed) + } + return trimmed, nil +} + func (a *Agent) handleSessionNew(ctx context.Context, params json.RawMessage) (any, error) { var p NewSessionParams if err := json.Unmarshal(params, &p); err != nil { return nil, RPCError(codeInvalidParams, "invalid session/new params") } - root, err := a.deps.ResolveWorkspaceRoot(p.Cwd) + cwd, err := requestedWorkspace(p.Cwd) + if err != nil { + return nil, err + } + root, err := a.deps.ResolveWorkspaceRoot(cwd) if err != nil { return nil, RPCError(codeInvalidParams, err.Error()) } @@ -176,6 +205,16 @@ func (a *Agent) handleSessionResume(ctx context.Context, params json.RawMessage) // history as ordered session/update notifications; session/resume deliberately // does not, which makes it safe for an already-rendered desktop reconnect. func (a *Agent) activatePersistedSession(ctx context.Context, p LoadSessionParams, replay bool) (any, error) { + // BOTH METHODS, BEFORE ANYTHING IS LOOKED UP. session/resume shares this + // params type with session/load, so the wire cannot tell an omitted cwd from + // an empty one; the blank case used to be answered with the persisted cwd, + // which made {"sessionId":"known"} a complete, successful activation request. + // Validating here rather than in one handler is what keeps the two entry + // points from drifting apart. + cwd, err := requestedWorkspace(p.Cwd) + if err != nil { + return nil, err + } meta, err := a.deps.Store.Get(p.SessionID) if err != nil || meta == nil { return nil, RPCError(codeInvalidParams, "session not found: "+p.SessionID) @@ -184,10 +223,10 @@ func (a *Agent) activatePersistedSession(ctx context.Context, p LoadSessionParam return nil, RPCError(codeInvalidParams, "session has no persisted workspace: "+p.SessionID) } // SAME RULE ON THE WAY IN. Omitting a relative entry from the listing is not - // enough: session/resume falls back to meta.Cwd when the client sends no cwd, - // so a stored "." resolved against this process's directory and bound the - // conversation to it — verified returning no error at all. A workspace that - // cannot be identified is not one this session can be restored into. + // enough: a stored "." still resolves against this process's directory, so a + // client that hands back that same directory as its cwd would be sold the + // invented root as a match. A workspace that cannot be identified is not one + // this session can be restored into. if !filepath.IsAbs(meta.Cwd) { return nil, RPCError(codeInvalidParams, "session workspace is not an absolute path, so it cannot be identified: "+p.SessionID) } @@ -195,11 +234,7 @@ func (a *Agent) activatePersistedSession(ctx context.Context, p LoadSessionParam if err != nil { return nil, RPCError(codeInvalidParams, "persisted session workspace is unavailable: "+err.Error()) } - cwdInput := p.Cwd - if strings.TrimSpace(cwdInput) == "" { - cwdInput = meta.Cwd - } - root, err := a.deps.ResolveWorkspaceRoot(cwdInput) + root, err := a.deps.ResolveWorkspaceRoot(cwd) if err != nil { return nil, RPCError(codeInvalidParams, err.Error()) } @@ -256,10 +291,17 @@ func (a *Agent) handleSessionList(_ context.Context, params json.RawMessage) (an if err != nil { return nil, RPCError(codeInternalError, "list sessions: "+err.Error()) } + // The filter is the one cwd ACP leaves optional, so a blank one means "no + // filter" rather than an error — but a relative one is rebased onto this + // process's directory exactly like the others, and would then silently answer + // about a workspace the client never named. var cwd string if strings.TrimSpace(p.Cwd) != "" { - var err error - cwd, err = a.deps.ResolveWorkspaceRoot(p.Cwd) + filter, err := requestedWorkspace(p.Cwd) + if err != nil { + return nil, err + } + cwd, err = a.deps.ResolveWorkspaceRoot(filter) if err != nil { return nil, RPCError(codeInvalidParams, err.Error()) } diff --git a/internal/acp/agent_test.go b/internal/acp/agent_test.go index c3f266e09..7cf9acd4b 100644 --- a/internal/acp/agent_test.go +++ b/internal/acp/agent_test.go @@ -352,8 +352,9 @@ func TestACPModelConfigOptionsCatalogSelectionAndLoad(t *testing.T) { ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) defer cancel() + workspace := t.TempDir() var created NewSessionResult - if err := h.client.Call(ctx, MethodSessionNew, NewSessionParams{Cwd: t.TempDir()}, &created); err != nil { + if err := h.client.Call(ctx, MethodSessionNew, NewSessionParams{Cwd: workspace}, &created); err != nil { t.Fatalf("session/new: %v", err) } option := created.ConfigOptions[0] @@ -383,7 +384,7 @@ func TestACPModelConfigOptionsCatalogSelectionAndLoad(t *testing.T) { h = newHarness(t, deps) defer h.stop() var loaded LoadSessionResult - if err := h.client.Call(ctx, MethodSessionLoad, LoadSessionParams{SessionID: created.SessionID}, &loaded); err != nil { + if err := h.client.Call(ctx, MethodSessionLoad, LoadSessionParams{SessionID: created.SessionID, Cwd: workspace}, &loaded); err != nil { t.Fatalf("session/load: %v", err) } if len(loaded.ConfigOptions) != 2 || loaded.ConfigOptions[0].CurrentValue != "gpt-5.4-mini" { @@ -434,8 +435,9 @@ func TestACPCustomProviderAllowsUnadvertisedModel(t *testing.T) { ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) defer cancel() + workspace := t.TempDir() var created NewSessionResult - if err := h.client.Call(ctx, MethodSessionNew, NewSessionParams{Cwd: t.TempDir()}, &created); err != nil { + if err := h.client.Call(ctx, MethodSessionNew, NewSessionParams{Cwd: workspace}, &created); err != nil { t.Fatalf("session/new: %v", err) } var selected SetSessionConfigOptionResult @@ -451,7 +453,7 @@ func TestACPCustomProviderAllowsUnadvertisedModel(t *testing.T) { h = newHarness(t, deps) defer h.stop() var loaded LoadSessionResult - if err := h.client.Call(ctx, MethodSessionLoad, LoadSessionParams{SessionID: created.SessionID}, &loaded); err != nil { + if err := h.client.Call(ctx, MethodSessionLoad, LoadSessionParams{SessionID: created.SessionID, Cwd: workspace}, &loaded); err != nil { t.Fatalf("session/load: %v", err) } option := loaded.ConfigOptions[0] @@ -1059,10 +1061,10 @@ func TestSessionListResolvesEveryWorkspace(t *testing.T) { func TestResumeRefusesARelativePersistedWorkspace(t *testing.T) { // OMITTING THE ENTRY FROM THE LISTING IS NOT THE WHOLE FIX. A client can ask - // to resume any id it already knows, and session/resume falls back to the - // persisted cwd when the request carries none — so a stored "." was resolved - // against this process's directory and the session bound to it, with no error - // returned at all. Both doors need the same lock. + // to resume any id it already knows, and a stored "." still resolves against + // this process's directory — so a request naming that directory would be told + // it matched, and the conversation bound to a workspace it never belonged to. + // Both doors need the same lock. deps := testDeps(t) deps.ResolveWorkspaceRoot = normalisingResolver(t) if _, err := deps.Store.Create(sessions.CreateInput{SessionID: "relative-ws", Cwd: "."}); err != nil { @@ -1074,6 +1076,14 @@ func TestResumeRefusesARelativePersistedWorkspace(t *testing.T) { ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) defer cancel() + // The directory the stored "." rebases onto. Naming it explicitly is what + // makes this test reach the persisted-cwd guard: a request with no cwd is now + // refused earlier, for a different reason, and would pass either way. + here, err := os.Getwd() + if err != nil { + t.Fatal(err) + } + // BOTH ACTIVATING METHODS, not just one. session/load and session/resume are // separate entry points that today share activatePersistedSession — so a test // through either passes while the guard holds. Naming both here is what keeps @@ -1082,10 +1092,12 @@ func TestResumeRefusesARelativePersistedWorkspace(t *testing.T) { elsewhere := t.TempDir() var out LoadSessionResult for _, method := range []string{MethodSessionLoad, MethodSessionResume} { - // No cwd at all: the request that used to succeed by rebasing onto - // whatever directory this process happens to be running in. - if err := h.client.Call(ctx, method, LoadSessionParams{SessionID: "relative-ws"}, &out); err == nil { - t.Errorf("%s of a relative persisted workspace succeeded; the session was rebound to the ACP process directory", method) + // The invented root offered back as the answer: without the guard the two + // sides agree, because both were produced by resolving "." here. + err := h.client.Call(ctx, method, LoadSessionParams{SessionID: "relative-ws", Cwd: here}, &out) + var rpcErr *rpcError + if !errors.As(err, &rpcErr) || !strings.Contains(rpcErr.Message, "not an absolute path") { + t.Errorf("%s of a relative persisted workspace into the ACP process directory %q = %v, want a persisted-workspace error", method, here, err) } // And an unrelated workspace must not be accepted as its home either. if err := h.client.Call(ctx, method, ResumeSessionParams{SessionID: "relative-ws", Cwd: elsewhere}, &out); err == nil { @@ -1093,3 +1105,199 @@ func TestResumeRefusesARelativePersistedWorkspace(t *testing.T) { } } } + +// processDirectoryResolver mirrors internal/cli/exec.go resolveWorkspaceRoot, +// the resolver the real ACP surface is wired with: a blank cwd becomes the +// directory the process was started in, a relative one is joined onto it. base +// stands in for that directory so the test never depends on where `go test` +// happens to run. +func processDirectoryResolver(t *testing.T, base string) func(string) (string, error) { + t.Helper() + return func(cwd string) (string, error) { + resolved := strings.TrimSpace(cwd) + if resolved == "" { + resolved = base + } else if !filepath.IsAbs(resolved) { + resolved = filepath.Join(base, resolved) + } + resolved = filepath.Clean(resolved) + info, err := os.Stat(resolved) + if err != nil { + return "", err + } + if !info.IsDir() { + return "", errors.New("workspace is not a directory") + } + return resolved, nil + } +} + +// A REQUEST THAT NAMES NO WORKSPACE ACTIVATES NOTHING. +// +// ResumeSessionParams is an alias for LoadSessionParams, and cwd is a plain +// string on both, so JSON decoding cannot tell an omitted field from an empty +// one. The blank case used to be answered with the persisted cwd, which made +// {"sessionId":"known"} — a request carrying no workspace at all — a complete +// and successful activation. A relative cwd was worse than useless rather than +// rejected: it was joined onto whatever directory the editor spawned `zero acp` +// in, so it could agree with the persisted root by accident and hand the session +// over anyway. +// +// The cases go over the wire as raw JSON on purpose. Marshalling a Go struct +// always emits "cwd":"" and would never exercise the absent field, which is +// where the defect lives. +func TestActivationRequiresAnAbsoluteRequestWorkspace(t *testing.T) { + deps := testDeps(t) + processDir := t.TempDir() + workspace := filepath.Join(processDir, "project") + if err := os.MkdirAll(workspace, 0o755); err != nil { + t.Fatal(err) + } + deps.ResolveWorkspaceRoot = processDirectoryResolver(t, processDir) + if _, err := deps.Store.Create(sessions.CreateInput{SessionID: "persisted", Cwd: workspace}); err != nil { + t.Fatal(err) + } + + cases := []struct { + name string + params json.RawMessage + message string + }{ + // The finding: cwd absent from the wire entirely. + {"omitted", json.RawMessage(`{"sessionId":"persisted","mcpServers":[]}`), "cwd is required and must be an absolute path"}, + {"empty", json.RawMessage(`{"sessionId":"persisted","cwd":"","mcpServers":[]}`), "cwd is required and must be an absolute path"}, + {"whitespace", json.RawMessage(`{"sessionId":"persisted","cwd":" ","mcpServers":[]}`), "cwd is required and must be an absolute path"}, + // "project" resolves onto processDir and MATCHES the persisted root, so + // this is the relative case that used to be accepted, not merely mis-erroring. + {"relative", json.RawMessage(`{"sessionId":"persisted","cwd":"project","mcpServers":[]}`), "cwd must be an absolute path: project"}, + {"dot relative", json.RawMessage(`{"sessionId":"persisted","cwd":"./project","mcpServers":[]}`), "cwd must be an absolute path: ./project"}, + } + + // BOTH ACTIVATING METHODS. They share activatePersistedSession today; naming + // both keeps this honest if resume is ever given its own path. + for _, method := range []string{MethodSessionLoad, MethodSessionResume} { + for _, tc := range cases { + t.Run(method+"/"+tc.name, func(t *testing.T) { + h := newHarness(t, deps) + defer h.stop() + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() + + var out LoadSessionResult + err := h.client.Call(ctx, method, tc.params, &out) + var rpcErr *rpcError + if !errors.As(err, &rpcErr) { + t.Fatalf("%s %s = %v, want a JSON-RPC error", method, tc.params, err) + } + if rpcErr.Code != codeInvalidParams { + t.Errorf("%s %s error code = %d, want %d", method, tc.params, rpcErr.Code, codeInvalidParams) + } + if rpcErr.Message != tc.message { + t.Errorf("%s %s error = %q, want %q", method, tc.params, rpcErr.Message, tc.message) + } + // AND THE SESSION IS NOT LIVE. The error alone would not prove it: + // activation publishes the session before it returns, so a refusal + // that still registered would leave session/prompt and session/set_mode + // working on a workspace the client never named. + modeErr := h.client.Call(ctx, MethodSessionSetMode, + SetSessionModeParams{SessionID: "persisted", ModeID: string(agent.PermissionModeAuto)}, + &SetSessionModeResult{}) + if !errors.As(modeErr, &rpcErr) || rpcErr.Message != "unknown session: persisted" { + t.Errorf("after a refused %s the session was live: set_mode = %v, want %q", method, modeErr, "unknown session: persisted") + } + }) + } + } + + // The same request with the workspace spelled out is what the client is + // expected to send, and it still works. + h := newHarness(t, deps) + defer h.stop() + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() + var out LoadSessionResult + if err := h.client.Call(ctx, MethodSessionResume, ResumeSessionParams{SessionID: "persisted", Cwd: workspace, McpServers: []McpServer{}}, &out); err != nil { + t.Fatalf("session/resume with an absolute cwd: %v", err) + } +} + +// session/new roots a BRAND NEW session, and its sandbox and file-tool +// confinement, at the client's cwd. The same absent-field decoding applied +// there: {"mcpServers":[]} created a session rooted at the ACP process's own +// directory and persisted that invented path as the session's workspace. +func TestSessionNewRequiresAnAbsoluteWorkspace(t *testing.T) { + deps := testDeps(t) + processDir := t.TempDir() + deps.ResolveWorkspaceRoot = processDirectoryResolver(t, processDir) + if err := os.MkdirAll(filepath.Join(processDir, "project"), 0o755); err != nil { + t.Fatal(err) + } + + h := newHarness(t, deps) + defer h.stop() + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() + + for _, tc := range []struct { + params json.RawMessage + message string + }{ + {json.RawMessage(`{"mcpServers":[]}`), "cwd is required and must be an absolute path"}, + {json.RawMessage(`{"cwd":"","mcpServers":[]}`), "cwd is required and must be an absolute path"}, + {json.RawMessage(`{"cwd":"project","mcpServers":[]}`), "cwd must be an absolute path: project"}, + } { + var created NewSessionResult + err := h.client.Call(ctx, MethodSessionNew, tc.params, &created) + var rpcErr *rpcError + if !errors.As(err, &rpcErr) || rpcErr.Code != codeInvalidParams || rpcErr.Message != tc.message { + t.Errorf("session/new %s = %v, want invalid params %q", tc.params, err, tc.message) + } + if created.SessionID != "" { + t.Errorf("session/new %s created session %q despite refusing the request", tc.params, created.SessionID) + } + } + + listed, err := deps.Store.ListResumable() + if err != nil { + t.Fatal(err) + } + if len(listed) != 0 { + t.Errorf("refused session/new requests persisted %d session(s); the first is rooted at %q", len(listed), listed[0].Cwd) + } +} + +// The cwd filter is the one ACP leaves optional, so blank still means "no +// filter" — but a relative one would be rebased onto the ACP process directory +// and answer about a workspace the client never named. +func TestSessionListRejectsARelativeCwdFilter(t *testing.T) { + deps := testDeps(t) + processDir := t.TempDir() + workspace := filepath.Join(processDir, "project") + if err := os.MkdirAll(workspace, 0o755); err != nil { + t.Fatal(err) + } + deps.ResolveWorkspaceRoot = processDirectoryResolver(t, processDir) + if _, err := deps.Store.Create(sessions.CreateInput{SessionID: "persisted", Cwd: workspace}); err != nil { + t.Fatal(err) + } + + h := newHarness(t, deps) + defer h.stop() + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() + + var out ListSessionsResult + err := h.client.Call(ctx, MethodSessionList, ListSessionsParams{Cwd: "project"}, &out) + var rpcErr *rpcError + if !errors.As(err, &rpcErr) || rpcErr.Code != codeInvalidParams || rpcErr.Message != "cwd must be an absolute path: project" { + t.Errorf("session/list with a relative cwd filter = %v, want invalid params %q", err, "cwd must be an absolute path: project") + } + + // Omitting the filter is still legal, and still lists the session. + if err := h.client.Call(ctx, MethodSessionList, ListSessionsParams{}, &out); err != nil { + t.Fatalf("session/list without a filter: %v", err) + } + if len(out.Sessions) != 1 || out.Sessions[0].SessionID != "persisted" { + t.Errorf("unfiltered session/list = %+v, want the one persisted session", out.Sessions) + } +} diff --git a/internal/acp/types.go b/internal/acp/types.go index 33e3db33d..2d1abe1ee 100644 --- a/internal/acp/types.go +++ b/internal/acp/types.go @@ -127,9 +127,14 @@ type McpServer struct { } type NewSessionParams struct { - Cwd string `json:"cwd"` - McpServers []McpServer `json:"mcpServers"` - AdditionalDirectories []string `json:"additionalDirectories,omitempty"` + Cwd string `json:"cwd"` + McpServers []McpServer `json:"mcpServers"` + // AdditionalDirectories is declared by the protocol and consumed nowhere in + // this repo yet. When it is wired up it must go through requestedWorkspace + // like Cwd does: these are client-supplied paths with no absoluteness rule of + // their own, which is the same shape as the resume-cwd hole that made + // {"sessionId":"known"} activate a session against this process's directory. + AdditionalDirectories []string `json:"additionalDirectories,omitempty"` } type NewSessionResult struct { @@ -176,6 +181,10 @@ type ListSessionsResult struct { NextCursor string `json:"nextCursor,omitempty"` } +// session/resume takes session/load's wire shape, so its cwd is a plain string +// too and an omitted one decodes exactly like an empty one. Sharing the type is +// safe only because activatePersistedSession requires an absolute cwd from the +// request itself — nothing downstream may supply a default for either method. type ResumeSessionParams = LoadSessionParams type ResumeSessionResult = LoadSessionResult From dea95600e75fc0cd967defc82af4ba67984d4a87 Mon Sep 17 00:00:00 2001 From: KRATOS Date: Tue, 25 Aug 2026 20:53:09 +0530 Subject: [PATCH 11/13] fix(acp): resume the conversation a session became, and fail closed when it cannot Three defects in ACP session restoration, all reported by @jatmn. loadHistory read the raw event log and kept only EventMessage. A compacted session stores its original prefix alongside an EventCompaction naming the events it replaced and carrying their summary, so restoring from the raw log replayed superseded turns AND dropped the summary that replaced them. It now reads the same rehydrated view the TUI and exec paths use, and explicitly projects the compaction summary -- switching readers alone would still drop it, because rehydration substitutes the compaction event in place of what it replaced. historyErr only suppressed replay and raised a warning: the session was registered and reported ready regardless, so an unreadable events file left the caller holding a live, promptable session ID whose next prompt ran as a fresh conversation under the old identity. Resume now fails. Load keeps the best-effort policy deliberately rather than by inheriting the shared helper. Tool calls and their results were dropped from session/load, so a restored transcript showed prose asserting edits with no record that any tool ran. They now replay through the same toolCallStart/toolCallResult mapping a live turn uses, keyed on the stored toolCallId so results pair with their calls. They do not enter turnRecord, so load and resume still consume the same effective history. Resume stays replay-free. Also asserts that an unset sessionCapabilities is omitted rather than serialized as null, raised by CodeRabbit. --- internal/acp/agent.go | 133 ++++++++++++++++++- internal/acp/agent_test.go | 256 ++++++++++++++++++++++++++++++++++++- 2 files changed, 386 insertions(+), 3 deletions(-) diff --git a/internal/acp/agent.go b/internal/acp/agent.go index fcb27027e..59fa59d04 100644 --- a/internal/acp/agent.go +++ b/internal/acp/agent.go @@ -248,6 +248,20 @@ func (a *Agent) activatePersistedSession(ctx context.Context, p LoadSessionParam // a half-initialized session (registerSession sets history under the lock and // reuses an already-live session rather than orphaning its in-flight turn). history, messages, historyErr := a.loadHistory(meta.SessionID) + // RESUME PROMISES RESTORED CONTEXT, SO A FAILED RESTORATION IS A FAILED + // RESUME. historyErr used to do nothing but suppress replay and raise a + // warning: the session was registered and reported ready regardless, so a + // corrupt record or an unreadable events file left the caller holding a live, + // promptable session ID whose next prompt ran as a brand-new conversation + // under the old identity. Nothing in the response said so. + // + // Publication is now gated on restoration for resume. LOAD keeps the + // best-effort policy deliberately rather than by inheriting this helper: it + // replays what it has and warns about the rest, which is the right trade for + // a client rebuilding a transcript it can still scroll. Reported by @jatmn. + if !replay && historyErr != nil { + return nil, RPCError(codeInternalError, "restore session history: "+historyErr.Error()) + } model, models, restrictModels, err := a.resolveModelChoices(ctx, root) if err != nil { return nil, RPCError(codeInternalError, "config: "+err.Error()) @@ -262,6 +276,10 @@ func (a *Agent) activatePersistedSession(ctx context.Context, p LoadSessionParam note := ¬ifier{conn: a.conn, sessionID: sess.id} if replay && historyErr == nil { for _, message := range messages { + if message.tool != nil { + note.send(*message.tool) + continue + } note.send(replayMessageChunk(message.role, replayMessageID(message.eventID), message.content)) } } @@ -725,17 +743,40 @@ func (a *Agent) persistTurn(sess *acpSession, user, assistant string) error { return err } +// persistedMessage is one entry of the restored transcript in stored order. It +// is either a message (role/content) or a tool-call notification (tool), never +// both: keeping them in ONE ordered slice is what preserves the interleaving a +// client needs to redraw the conversation as it originally happened. type persistedMessage struct { eventID string role string content string + // tool is set for a replayed tool call or its result, and is already in wire + // shape — it comes from the same toolCallStart/toolCallResult mapping the + // live turn uses, so a replayed tool call renders identically to a fresh one + // and there is no second mapping to drift out of sync. + tool *ToolCallUpdate } func (a *Agent) loadHistory(sessionID string) ([]turnRecord, []persistedMessage, error) { if a.deps.Store == nil { return nil, nil, nil } - events, err := a.deps.Store.ReadEvents(sessionID) + // THE EFFECTIVE CONVERSATION, NOT THE RAW LOG. A compacted session keeps its + // original prefix on disk alongside an EventCompaction that names the events + // it replaced and carries their durable summary. ReadEvents hands back both, + // so restoring from it replayed superseded turns AND dropped the summary that + // replaced them — resume then seeded the next turn with context the + // conversation had already moved past, and load visibly resurrected + // transcript entries the user had seen compacted away. + // + // ReadRehydratedEvents is the projection the TUI and exec paths already use + // (internal/tui/session.go, internal/sessions/exec_session.go), so all three + // now agree on what the conversation IS. Switching readers alone is not + // enough: rehydration substitutes the compaction event in place of the events + // it replaced, so a loop that skips everything but EventMessage would drop the + // summary exactly as before. It is projected below. Reported by @jatmn. + events, err := a.deps.Store.ReadRehydratedEvents(sessionID) if err != nil { return nil, nil, err } @@ -744,6 +785,47 @@ func (a *Agent) loadHistory(sessionID string) ([]turnRecord, []persistedMessage, var pendingUser string havePending := false for _, e := range events { + if e.Type == sessions.EventCompaction { + // The summary stands in for the turns it replaced, so it enters the + // history as an assistant message: it is prior context the next turn + // must see, and the wire has no separate role for it. + raw, marshalErr := json.Marshal(e.Payload) + if marshalErr != nil { + continue + } + var payload struct { + Summary string `json:"summary"` + } + if json.Unmarshal(raw, &payload) != nil || strings.TrimSpace(payload.Summary) == "" { + continue + } + messages = append(messages, persistedMessage{ + eventID: persistedMessageIdentity(sessionID, e), + role: "assistant", + content: payload.Summary, + }) + records = append(records, turnRecord{user: pendingUser, assistant: payload.Summary}) + pendingUser = "" + havePending = false + continue + } + // TOOL ACTIVITY IS PART OF THE TRANSCRIPT, NOT DECORATION. A restored + // conversation that shows only the prose reads as if the agent asserted + // its edits and command output from nowhere: the user sees "I updated + // scope.go" with no record that any tool ran. These are collected for + // replay only. They deliberately do NOT enter turnRecord, so the prompt + // context a resumed turn receives stays byte-identical to what it was — + // load and resume continue to consume the SAME effective history, and + // only the wire rendering differs. Reported by @jatmn. + if e.Type == sessions.EventToolCall || e.Type == sessions.EventToolResult { + if upd := replayToolUpdate(e); upd != nil { + messages = append(messages, persistedMessage{ + eventID: persistedMessageIdentity(sessionID, e), + tool: upd, + }) + } + continue + } if e.Type != sessions.EventMessage { continue } @@ -779,6 +861,55 @@ func (a *Agent) loadHistory(sessionID string) ([]turnRecord, []persistedMessage, return records, messages, nil } +// replayToolUpdate rebuilds the ACP notification for one stored tool event. +// +// The stored toolCallId is reused verbatim rather than re-derived, because the +// client pairs a result with its call by that id alone; minting a new one would +// replay the result as an orphan update against a call the client never saw. +// Older records wrote the field as "id", so both spellings are accepted — the +// TUI projection already does the same (internal/tui/session.go). +// +// A record that carries no usable id is skipped rather than replayed under a +// synthesized one: an unpairable update is worse than a missing row. +func replayToolUpdate(event sessions.Event) *ToolCallUpdate { + raw, err := json.Marshal(event.Payload) + if err != nil { + return nil + } + var payload struct { + Name string `json:"name"` + ToolCallID string `json:"toolCallId"` + ID string `json:"id"` + Arguments string `json:"arguments"` + Status string `json:"status"` + Output string `json:"output"` + } + if json.Unmarshal(raw, &payload) != nil { + return nil + } + id := payload.ToolCallID + if id == "" { + id = payload.ID + } + if id == "" { + return nil + } + if event.Type == sessions.EventToolCall { + upd := toolCallStart(agent.ToolCall{ID: id, Name: payload.Name, Arguments: payload.Arguments}) + // The call already ran; replaying it as in_progress would leave a + // permanently spinning row when no result event follows it (a session + // killed mid-call). Its own result event supplies the real outcome. + upd.Status = ToolStatusCompleted + return &upd + } + status := tools.Status(payload.Status) + if status == "" { + status = tools.StatusOK + } + upd := toolCallResult(agent.ToolResult{ToolCallID: id, Name: payload.Name, Status: status, Output: payload.Output}) + return &upd +} + func persistedMessageIdentity(sessionID string, event sessions.Event) string { if event.ID != "" { return event.ID diff --git a/internal/acp/agent_test.go b/internal/acp/agent_test.go index 7cf9acd4b..7d4cb0082 100644 --- a/internal/acp/agent_test.go +++ b/internal/acp/agent_test.go @@ -73,7 +73,11 @@ type clientHarness struct { client *Conn updates chan string notifications chan ContentChunk - stop func() + // tools carries replayed and live tool-call notifications. They are a + // different wire shape from a message chunk, so decoding them into + // ContentChunk would silently blank every field the assertions need. + tools chan ToolCallUpdate + stop func() } func newHarness(t *testing.T, deps Deps) *clientHarness { @@ -84,8 +88,30 @@ func newHarness(t *testing.T, deps Deps) *clientHarness { client := NewConn(br, bw) a := NewAgent(agentConn, deps) - h := &clientHarness{client: client, updates: make(chan string, 128), notifications: make(chan ContentChunk, 128)} + h := &clientHarness{client: client, updates: make(chan string, 128), notifications: make(chan ContentChunk, 128), tools: make(chan ToolCallUpdate, 128)} client.HandleNotify(MethodSessionUpdate, func(_ context.Context, params json.RawMessage) { + // Decode the discriminator ALONE first. ContentChunk and ToolCallUpdate + // disagree on the shape of "content" (a block vs an array of them), so + // decoding straight into ContentChunk makes every tool_call_update fail + // to unmarshal and vanish — which reads exactly like a notification that + // was never sent. + var kind struct { + Update struct { + SessionUpdate string `json:"sessionUpdate"` + } `json:"update"` + } + if json.Unmarshal(params, &kind) != nil { + return + } + if kind.Update.SessionUpdate == UpdateToolCall || kind.Update.SessionUpdate == UpdateToolCallUpdate { + var toolProbe struct { + Update ToolCallUpdate `json:"update"` + } + if json.Unmarshal(params, &toolProbe) == nil { + h.tools <- toolProbe.Update + } + return + } var probe struct { Update ContentChunk `json:"update"` } @@ -1301,3 +1327,229 @@ func TestSessionListRejectsARelativeCwdFilter(t *testing.T) { t.Errorf("unfiltered session/list = %+v, want the one persisted session", out.Sessions) } } + +// A COMPACTED SESSION MUST RESUME AS THE CONVERSATION IT BECAME, NOT THE ONE IT +// WAS. This session's first three turns were compacted into one summary. Reading +// the raw log would replay those superseded turns and drop the summary; reading +// the rehydrated view without projecting EventCompaction would drop the summary +// and the turns both. Load and resume must agree, so both are asserted. +func TestACPCompactedHistoryReplacesCompactedTurnsWithTheirSummary(t *testing.T) { + deps := testDeps(t) + workspace := t.TempDir() + created, err := deps.Store.Create(sessions.CreateInput{SessionID: "compacted-session", Title: "Compacted", Cwd: workspace}) + if err != nil { + t.Fatal(err) + } + appended, err := deps.Store.AppendEvents(created.SessionID, []sessions.AppendEventInput{ + {Type: sessions.EventMessage, Payload: map[string]any{"role": "user", "content": "compacted question"}}, + {Type: sessions.EventMessage, Payload: map[string]any{"role": "assistant", "content": "compacted answer"}}, + {Type: sessions.EventMessage, Payload: map[string]any{"role": "user", "content": "surviving question"}}, + }) + if err != nil { + t.Fatal(err) + } + if len(appended) != 3 { + t.Fatalf("appended %d events, want 3", len(appended)) + } + const summary = "Earlier: the user asked about scope roots and got an answer." + if _, err := deps.Store.AppendEvents(created.SessionID, []sessions.AppendEventInput{{ + Type: sessions.EventCompaction, + Payload: map[string]any{ + "summary": summary, + "preserveLast": 1, + "compactableEvents": []map[string]any{ + {"id": appended[0].ID, "sequence": appended[0].Sequence}, + {"id": appended[1].ID, "sequence": appended[1].Sequence}, + }, + }, + }}); err != nil { + t.Fatal(err) + } + + ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) + defer cancel() + + loader := newHarness(t, deps) + if err := loader.client.Call(ctx, MethodSessionLoad, LoadSessionParams{SessionID: created.SessionID, Cwd: workspace, McpServers: []McpServer{}}, &LoadSessionResult{}); err != nil { + t.Fatalf("session/load: %v", err) + } + wantKinds := []string{UpdateAgentMessageChunk, UpdateUserMessageChunk} + wantText := []string{summary, "surviving question"} + for i := range wantKinds { + select { + case update := <-loader.notifications: + if update.SessionUpdate != wantKinds[i] || update.Content.Text != wantText[i] { + t.Fatalf("replayed update %d = %+v, want %s %q", i, update, wantKinds[i], wantText[i]) + } + case <-ctx.Done(): + t.Fatalf("replayed update %d never arrived", i) + } + } + // The compacted-away turns must NOT also be replayed after the summary. + select { + case update := <-loader.notifications: + t.Fatalf("a compacted-away turn was replayed: %+v", update) + case <-time.After(150 * time.Millisecond): + } + loader.stop() + + // Resume takes the same history. Its prompt context is what proves it: the + // summary must be in there and the compacted originals must not, so the + // prompt the agent loop actually receives is captured rather than inferred. + prompts := make(chan string, 4) + realRun := deps.RunAgent + deps.RunAgent = func(ctx context.Context, prompt string, provider zeroruntime.Provider, opts agent.Options) (agent.Result, error) { + prompts <- prompt + return realRun(ctx, prompt, provider, opts) + } + resumer := newHarness(t, deps) + defer resumer.stop() + if err := resumer.client.Call(ctx, MethodSessionResume, ResumeSessionParams{SessionID: created.SessionID, Cwd: workspace, McpServers: []McpServer{}}, &ResumeSessionResult{}); err != nil { + t.Fatalf("session/resume: %v", err) + } + if err := resumer.client.Call(ctx, MethodSessionPrompt, PromptParams{ + SessionID: created.SessionID, + Prompt: []ContentBlock{{Type: "text", Text: "carry on"}}, + }, &PromptResult{}); err != nil { + t.Fatalf("session/prompt: %v", err) + } + prompt := <-prompts + if !strings.Contains(prompt, summary) { + t.Fatalf("resumed prompt lost the compaction summary:\n%s", prompt) + } + if strings.Contains(prompt, "compacted answer") { + t.Fatalf("resumed prompt replayed a compacted-away turn:\n%s", prompt) + } +} + +// Tool activity is part of the transcript a client redraws on session/load, and +// a result is only renderable if it pairs with its call by the SAME id. Resume +// stays replay-free. +func TestACPLoadReplaysToolCallsPairedByTheirStoredID(t *testing.T) { + deps := testDeps(t) + workspace := t.TempDir() + created, err := deps.Store.Create(sessions.CreateInput{SessionID: "tool-replay-session", Title: "Tools", Cwd: workspace}) + if err != nil { + t.Fatal(err) + } + if _, err := deps.Store.AppendEvents(created.SessionID, []sessions.AppendEventInput{ + {Type: sessions.EventMessage, Payload: map[string]any{"role": "user", "content": "read the file"}}, + {Type: sessions.EventToolCall, Payload: map[string]any{"name": "read_file", "toolCallId": "call-77", "arguments": `{"path":"scope.go"}`}}, + {Type: sessions.EventToolResult, Payload: map[string]any{"name": "read_file", "toolCallId": "call-77", "status": "ok", "output": "package sandbox"}}, + // An older record spells the id "id" rather than "toolCallId". + {Type: sessions.EventToolCall, Payload: map[string]any{"name": "bash", "id": "call-88", "arguments": `{"command":"go build"}`}}, + {Type: sessions.EventToolResult, Payload: map[string]any{"name": "bash", "id": "call-88", "status": "error", "output": "build failed"}}, + // No id at all: unpairable, so it must be skipped rather than replayed. + {Type: sessions.EventToolCall, Payload: map[string]any{"name": "orphan", "arguments": "{}"}}, + }); err != nil { + t.Fatal(err) + } + + ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) + defer cancel() + loader := newHarness(t, deps) + if err := loader.client.Call(ctx, MethodSessionLoad, LoadSessionParams{SessionID: created.SessionID, Cwd: workspace, McpServers: []McpServer{}}, &LoadSessionResult{}); err != nil { + t.Fatalf("session/load: %v", err) + } + type want struct { + kind, id, status string + } + wants := []want{ + {UpdateToolCall, "call-77", ToolStatusCompleted}, + {UpdateToolCallUpdate, "call-77", ToolStatusCompleted}, + {UpdateToolCall, "call-88", ToolStatusCompleted}, + {UpdateToolCallUpdate, "call-88", ToolStatusFailed}, + } + for i, w := range wants { + select { + case update := <-loader.tools: + if update.SessionUpdate != w.kind || update.ToolCallID != w.id || update.Status != w.status { + t.Fatalf("tool update %d = %+v, want %s/%s/%s", i, update, w.kind, w.id, w.status) + } + case <-ctx.Done(): + t.Fatalf("tool update %d never arrived", i) + } + } + select { + case update := <-loader.tools: + t.Fatalf("an unpairable tool call was replayed: %+v", update) + case <-time.After(150 * time.Millisecond): + } + loader.stop() + + resumer := newHarness(t, deps) + defer resumer.stop() + if err := resumer.client.Call(ctx, MethodSessionResume, ResumeSessionParams{SessionID: created.SessionID, Cwd: workspace, McpServers: []McpServer{}}, &ResumeSessionResult{}); err != nil { + t.Fatalf("session/resume: %v", err) + } + select { + case update := <-resumer.tools: + t.Fatalf("session/resume replayed tool activity: %+v", update) + case <-time.After(150 * time.Millisecond): + } +} + +// sessionCapabilities is omitempty. The positive case is asserted above; this is +// the other half — an agent that does NOT support list/resume must omit the key +// entirely rather than send a null a client could read as "supported". Raised by +// CodeRabbit. +func TestSessionCapabilitiesAreOmittedWhenUnset(t *testing.T) { + encoded, err := json.Marshal(AgentCapabilities{LoadSession: true}) + if err != nil { + t.Fatal(err) + } + var got map[string]any + if err := json.Unmarshal(encoded, &got); err != nil { + t.Fatal(err) + } + if _, ok := got["sessionCapabilities"]; ok { + t.Fatalf("unset sessionCapabilities was still serialized: %s", encoded) + } +} + +// A RESUME THAT CANNOT RESTORE ITS CONTEXT MUST FAIL, NOT SUCCEED QUIETLY. +// Resume's entire contract is "carry on from where this left off". If the events +// file is unreadable, the old behaviour registered the session anyway and +// returned success: the client held a live session ID under the old identity +// whose next prompt ran as a fresh conversation. Nothing on the wire said so. +// +// Load keeps the best-effort policy deliberately — it replays what it can and +// warns — so both halves are asserted here to keep the two from being +// accidentally unified again. +func TestACPResumeFailsWhenHistoryCannotBeRestored(t *testing.T) { + deps := testDeps(t) + cwd := t.TempDir() + meta, err := deps.Store.Create(sessions.CreateInput{Title: "ACP session", Cwd: cwd}) + if err != nil { + t.Fatalf("create session: %v", err) + } + eventsPath := filepath.Join(deps.Store.RootDir, meta.SessionID, sessions.EventsFile) + if err := os.WriteFile(eventsPath, []byte("{bad json}\n"), 0o600); err != nil { + t.Fatalf("write corrupt events: %v", err) + } + + ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) + defer cancel() + + resumer := newHarness(t, deps) + defer resumer.stop() + err = resumer.client.Call(ctx, MethodSessionResume, ResumeSessionParams{SessionID: meta.SessionID, Cwd: cwd, McpServers: []McpServer{}}, &ResumeSessionResult{}) + if err == nil { + t.Fatal("session/resume reported success despite unreadable history") + } + // And the session must not have been published: a prompt against it has to + // be refused, not silently answered with no context. + if err := resumer.client.Call(ctx, MethodSessionPrompt, PromptParams{ + SessionID: meta.SessionID, + Prompt: []ContentBlock{TextBlock("carry on")}, + }, &PromptResult{}); err == nil { + t.Fatal("a session whose resume failed was still promptable") + } + + // Load is the deliberate exception and still opens. + loader := newHarness(t, deps) + defer loader.stop() + if err := loader.client.Call(ctx, MethodSessionLoad, LoadSessionParams{SessionID: meta.SessionID, Cwd: cwd, McpServers: []McpServer{}}, &LoadSessionResult{}); err != nil { + t.Fatalf("session/load must stay best-effort, got: %v", err) + } +} From 2a9a25f74aa6cfe7e9a52ac85a3c75324fe966fd Mon Sep 17 00:00:00 2001 From: KRATOS <84986124+gnanam1990@users.noreply.github.com> Date: Fri, 28 Aug 2026 17:35:37 +0530 Subject: [PATCH 12/13] fix(acp): persist tool activity for session replay --- internal/acp/agent.go | 102 +++++++++++++++++++++++------------ internal/acp/agent_test.go | 108 +++++++++++++++++++++++++++++++++++-- 2 files changed, 172 insertions(+), 38 deletions(-) diff --git a/internal/acp/agent.go b/internal/acp/agent.go index 59fa59d04..b1fbb89cb 100644 --- a/internal/acp/agent.go +++ b/internal/acp/agent.go @@ -424,6 +424,15 @@ func (a *Agent) runTurn(ctx context.Context, sess *acpSession, userText string, return "", RPCError(codeInternalError, "workspace: "+err.Error()) } note := ¬ifier{conn: a.conn, sessionID: sess.id} + // Persist the user message before the agent can emit tool callbacks. Tool + // starts and results are appended synchronously from those callbacks, so the + // durable log has the same order the client observed and an interrupted call + // remains visible after a fresh-process load. + var persistenceErr error + persist := func(input sessions.AppendEventInput) { + persistenceErr = errors.Join(persistenceErr, a.persistEvent(sess.id, input)) + } + persist(messageEvent("user", userText)) opts := agent.Options{ Cwd: sess.cwd, @@ -437,8 +446,12 @@ func (a *Agent) runTurn(ctx context.Context, sess *acpSession, userText string, Images: images, OnText: note.text, OnReasoning: note.thought, - OnToolCall: note.toolCall, + OnToolCall: func(call agent.ToolCall) { + persist(toolCallEvent(call)) + note.toolCall(call) + }, OnToolResult: func(result agent.ToolResult) { + persist(toolResultEvent(result)) note.toolResult(result) if result.Name == "update_plan" { a.emitPlan(registry, note) @@ -451,19 +464,21 @@ func (a *Agent) runTurn(ctx context.Context, sess *acpSession, userText string, agentPrompt := buildPrompt(sess.snapshotHistory(), userText) result, runErr := a.deps.RunAgent(ctx, agentPrompt, provider, opts) + if result.FinalAnswer != "" { + persist(messageEvent("assistant", result.FinalAnswer)) + } + sess.appendHistory(turnRecord{user: userText, assistant: result.FinalAnswer}) + a.warnPersistence( + note, + "save session history", + "Could not save session history. This turn is available in memory, but future resume may miss it until storage recovers.", + persistenceErr, + ) reason, stopErr := stopReasonFor(result, runErr) if stopErr != nil { return "", RPCError(codeInternalError, stopErr.Error()) } - if err := a.persistTurn(sess, userText, result.FinalAnswer); err != nil { - a.warnPersistence( - note, - "save session history", - "Could not save session history. This turn is available in memory, but future resume may miss it until storage recovers.", - err, - ) - } return reason, nil } @@ -722,25 +737,43 @@ func (a *Agent) configOptions(s *acpSession) []SessionConfigOption { // ---- persistence + continuity ---- -func (a *Agent) persistTurn(sess *acpSession, user, assistant string) error { - defer sess.appendHistory(turnRecord{user: user, assistant: assistant}) +func (a *Agent) persistEvent(sessionID string, input sessions.AppendEventInput) error { if a.deps.Store == nil { return nil } - events := []sessions.AppendEventInput{ - { - Type: sessions.EventMessage, - Payload: map[string]any{"role": "user", "content": user}, + _, err := a.deps.Store.AppendEvents(sessionID, []sessions.AppendEventInput{input}) + return err +} + +func messageEvent(role, content string) sessions.AppendEventInput { + return sessions.AppendEventInput{ + Type: sessions.EventMessage, + Payload: map[string]any{"role": role, "content": content}, + } +} + +func toolCallEvent(call agent.ToolCall) sessions.AppendEventInput { + return sessions.AppendEventInput{ + Type: sessions.EventToolCall, + Payload: map[string]any{ + "toolCallId": call.ID, + "name": call.Name, + "arguments": call.Arguments, }, } - if assistant != "" { - events = append(events, sessions.AppendEventInput{ - Type: sessions.EventMessage, - Payload: map[string]any{"role": "assistant", "content": assistant}, - }) +} + +func toolResultEvent(result agent.ToolResult) sessions.AppendEventInput { + return sessions.AppendEventInput{ + Type: sessions.EventToolResult, + Payload: map[string]any{ + "toolCallId": result.ToolCallID, + "name": result.Name, + "status": result.Status, + "output": result.Output, + "changedFiles": append([]string(nil), result.ChangedFiles...), + }, } - _, err := a.deps.Store.AppendEvents(sess.id, events) - return err } // persistedMessage is one entry of the restored transcript in stored order. It @@ -877,12 +910,13 @@ func replayToolUpdate(event sessions.Event) *ToolCallUpdate { return nil } var payload struct { - Name string `json:"name"` - ToolCallID string `json:"toolCallId"` - ID string `json:"id"` - Arguments string `json:"arguments"` - Status string `json:"status"` - Output string `json:"output"` + Name string `json:"name"` + ToolCallID string `json:"toolCallId"` + ID string `json:"id"` + Arguments string `json:"arguments"` + Status string `json:"status"` + Output string `json:"output"` + ChangedFiles []string `json:"changedFiles"` } if json.Unmarshal(raw, &payload) != nil { return nil @@ -896,17 +930,19 @@ func replayToolUpdate(event sessions.Event) *ToolCallUpdate { } if event.Type == sessions.EventToolCall { upd := toolCallStart(agent.ToolCall{ID: id, Name: payload.Name, Arguments: payload.Arguments}) - // The call already ran; replaying it as in_progress would leave a - // permanently spinning row when no result event follows it (a session - // killed mid-call). Its own result event supplies the real outcome. - upd.Status = ToolStatusCompleted return &upd } status := tools.Status(payload.Status) if status == "" { status = tools.StatusOK } - upd := toolCallResult(agent.ToolResult{ToolCallID: id, Name: payload.Name, Status: status, Output: payload.Output}) + upd := toolCallResult(agent.ToolResult{ + ToolCallID: id, + Name: payload.Name, + Status: status, + Output: payload.Output, + ChangedFiles: append([]string(nil), payload.ChangedFiles...), + }) return &upd } diff --git a/internal/acp/agent_test.go b/internal/acp/agent_test.go index 7d4cb0082..dba62779d 100644 --- a/internal/acp/agent_test.go +++ b/internal/acp/agent_test.go @@ -1435,10 +1435,12 @@ func TestACPLoadReplaysToolCallsPairedByTheirStoredID(t *testing.T) { if _, err := deps.Store.AppendEvents(created.SessionID, []sessions.AppendEventInput{ {Type: sessions.EventMessage, Payload: map[string]any{"role": "user", "content": "read the file"}}, {Type: sessions.EventToolCall, Payload: map[string]any{"name": "read_file", "toolCallId": "call-77", "arguments": `{"path":"scope.go"}`}}, - {Type: sessions.EventToolResult, Payload: map[string]any{"name": "read_file", "toolCallId": "call-77", "status": "ok", "output": "package sandbox"}}, + {Type: sessions.EventToolResult, Payload: map[string]any{"name": "read_file", "toolCallId": "call-77", "status": "ok", "output": "package sandbox", "changedFiles": []string{"scope.go", "scope_test.go"}}}, // An older record spells the id "id" rather than "toolCallId". {Type: sessions.EventToolCall, Payload: map[string]any{"name": "bash", "id": "call-88", "arguments": `{"command":"go build"}`}}, {Type: sessions.EventToolResult, Payload: map[string]any{"name": "bash", "id": "call-88", "status": "error", "output": "build failed"}}, + // A start with no result is an interrupted call, not a completed one. + {Type: sessions.EventToolCall, Payload: map[string]any{"name": "grep", "toolCallId": "call-99", "arguments": `{"pattern":"TODO"}`}}, // No id at all: unpairable, so it must be skipped rather than replayed. {Type: sessions.EventToolCall, Payload: map[string]any{"name": "orphan", "arguments": "{}"}}, }); err != nil { @@ -1453,12 +1455,14 @@ func TestACPLoadReplaysToolCallsPairedByTheirStoredID(t *testing.T) { } type want struct { kind, id, status string + locations []string } wants := []want{ - {UpdateToolCall, "call-77", ToolStatusCompleted}, - {UpdateToolCallUpdate, "call-77", ToolStatusCompleted}, - {UpdateToolCall, "call-88", ToolStatusCompleted}, - {UpdateToolCallUpdate, "call-88", ToolStatusFailed}, + {UpdateToolCall, "call-77", ToolStatusInProgress, nil}, + {UpdateToolCallUpdate, "call-77", ToolStatusCompleted, []string{"scope.go", "scope_test.go"}}, + {UpdateToolCall, "call-88", ToolStatusInProgress, nil}, + {UpdateToolCallUpdate, "call-88", ToolStatusFailed, nil}, + {UpdateToolCall, "call-99", ToolStatusInProgress, nil}, } for i, w := range wants { select { @@ -1466,6 +1470,14 @@ func TestACPLoadReplaysToolCallsPairedByTheirStoredID(t *testing.T) { if update.SessionUpdate != w.kind || update.ToolCallID != w.id || update.Status != w.status { t.Fatalf("tool update %d = %+v, want %s/%s/%s", i, update, w.kind, w.id, w.status) } + if len(update.Locations) != len(w.locations) { + t.Fatalf("tool update %d locations = %+v, want %v", i, update.Locations, w.locations) + } + for j, path := range w.locations { + if update.Locations[j].Path != path { + t.Fatalf("tool update %d location %d = %+v, want %q", i, j, update.Locations[j], path) + } + } case <-ctx.Done(): t.Fatalf("tool update %d never arrived", i) } @@ -1489,6 +1501,92 @@ func TestACPLoadReplaysToolCallsPairedByTheirStoredID(t *testing.T) { } } +// Drive the callback-to-store boundary through session/prompt rather than +// seeding replay-shaped events by hand. A fresh agent must then reconstruct the +// same call/result pair and changed-file locations from the durable log. +func TestACPPromptPersistsToolActivityForFreshLoad(t *testing.T) { + deps := testDeps(t) + deps.RunAgent = func(_ context.Context, _ string, _ zeroruntime.Provider, opts agent.Options) (agent.Result, error) { + call := agent.ToolCall{ID: "call-live", Name: "edit_file", Arguments: `{"path":"a.go"}`} + result := agent.ToolResult{ + ToolCallID: call.ID, + Name: call.Name, + Status: tools.StatusOK, + Output: "updated", + ChangedFiles: []string{"a.go", "b.go"}, + } + opts.OnToolCall(call) + opts.OnToolResult(result) + return agent.Result{FinalAnswer: "done"}, nil + } + + ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) + defer cancel() + workspace := t.TempDir() + writer := newHarness(t, deps) + var created NewSessionResult + if err := writer.client.Call(ctx, MethodSessionNew, NewSessionParams{Cwd: workspace}, &created); err != nil { + t.Fatalf("session/new: %v", err) + } + if err := writer.client.Call(ctx, MethodSessionPrompt, PromptParams{ + SessionID: created.SessionID, + Prompt: []ContentBlock{{Type: "text", Text: "edit the files"}}, + }, &PromptResult{}); err != nil { + t.Fatalf("session/prompt: %v", err) + } + writer.stop() + + events, err := deps.Store.ReadEvents(created.SessionID) + if err != nil { + t.Fatalf("read persisted prompt events: %v", err) + } + wantTypes := []sessions.EventType{ + sessions.EventMessage, + sessions.EventToolCall, + sessions.EventToolResult, + sessions.EventMessage, + } + if len(events) != len(wantTypes) { + t.Fatalf("persisted %d events, want %d: %+v", len(events), len(wantTypes), events) + } + for i, want := range wantTypes { + if events[i].Type != want { + t.Fatalf("event %d type = %s, want %s", i, events[i].Type, want) + } + } + rawResult, err := json.Marshal(events[2].Payload) + if err != nil { + t.Fatal(err) + } + var storedResult struct { + ToolCallID string `json:"toolCallId"` + ChangedFiles []string `json:"changedFiles"` + } + if err := json.Unmarshal(rawResult, &storedResult); err != nil { + t.Fatal(err) + } + if storedResult.ToolCallID != "call-live" || strings.Join(storedResult.ChangedFiles, ",") != "a.go,b.go" { + t.Fatalf("stored tool result = %+v", storedResult) + } + + loader := newHarness(t, deps) + defer loader.stop() + if err := loader.client.Call(ctx, MethodSessionLoad, LoadSessionParams{SessionID: created.SessionID, Cwd: workspace}, &LoadSessionResult{}); err != nil { + t.Fatalf("fresh session/load: %v", err) + } + start := <-loader.tools + result := <-loader.tools + if start.SessionUpdate != UpdateToolCall || start.ToolCallID != "call-live" || start.Status != ToolStatusInProgress { + t.Fatalf("replayed tool start = %+v", start) + } + if result.SessionUpdate != UpdateToolCallUpdate || result.ToolCallID != "call-live" || result.Status != ToolStatusCompleted { + t.Fatalf("replayed tool result = %+v", result) + } + if len(result.Locations) != 2 || result.Locations[0].Path != "a.go" || result.Locations[1].Path != "b.go" { + t.Fatalf("replayed tool locations = %+v", result.Locations) + } +} + // sessionCapabilities is omitempty. The positive case is asserted above; this is // the other half — an agent that does NOT support list/resume must omit the key // entirely rather than send a null a client could read as "supported". Raised by From cd474d78c576d5ac6d451db6819585254d66f56f Mon Sep 17 00:00:00 2001 From: KRATOS <84986124+gnanam1990@users.noreply.github.com> Date: Wed, 2 Sep 2026 12:30:18 +0530 Subject: [PATCH 13/13] test(acp): bound replay notification waits --- internal/acp/agent_test.go | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/internal/acp/agent_test.go b/internal/acp/agent_test.go index dba62779d..2cf02fea4 100644 --- a/internal/acp/agent_test.go +++ b/internal/acp/agent_test.go @@ -1574,8 +1574,18 @@ func TestACPPromptPersistsToolActivityForFreshLoad(t *testing.T) { if err := loader.client.Call(ctx, MethodSessionLoad, LoadSessionParams{SessionID: created.SessionID, Cwd: workspace}, &LoadSessionResult{}); err != nil { t.Fatalf("fresh session/load: %v", err) } - start := <-loader.tools - result := <-loader.tools + nextReplay := func(label string) ToolCallUpdate { + t.Helper() + select { + case update := <-loader.tools: + return update + case <-ctx.Done(): + t.Fatalf("replayed %s never arrived: %v", label, ctx.Err()) + return ToolCallUpdate{} + } + } + start := nextReplay("tool start") + result := nextReplay("tool result") if start.SessionUpdate != UpdateToolCall || start.ToolCallID != "call-live" || start.Status != ToolStatusInProgress { t.Fatalf("replayed tool start = %+v", start) }