Skip to content

Commit ca7074e

Browse files
committed
fix(acp): compare workspaces by filesystem identity, not by spelling
@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
1 parent b945567 commit ca7074e

2 files changed

Lines changed: 200 additions & 2 deletions

File tree

‎internal/acp/agent.go‎

Lines changed: 37 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@ import (
88
"errors"
99
"fmt"
1010
"log"
11+
"os"
1112
"strings"
1213
"sync"
1314

@@ -196,7 +197,7 @@ func (a *Agent) activatePersistedSession(ctx context.Context, p LoadSessionParam
196197
// ACP session cwd is immutable. Loading history under a different root
197198
// would give a conversation from one workspace access to another
198199
// workspace's configuration, files, and tools.
199-
if root != persistedRoot {
200+
if !sameWorkspace(root, persistedRoot) {
200201
return nil, RPCError(codeInvalidParams, "session cwd does not match its persisted workspace")
201202
}
202203
// 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
258259
for _, item := range items {
259260
if cwd != "" {
260261
itemRoot, err := a.deps.ResolveWorkspaceRoot(item.Cwd)
261-
if err != nil || itemRoot != cwd {
262+
if err != nil || !sameWorkspace(itemRoot, cwd) {
262263
continue
263264
}
264265
}
@@ -861,3 +862,37 @@ func (s *acpSession) snapshotHistory() []turnRecord {
861862
defer s.mu.Unlock()
862863
return append([]turnRecord(nil), s.history...)
863864
}
865+
866+
// sameWorkspace reports whether two resolved roots name the same directory.
867+
//
868+
// STRING EQUALITY IS NOT DIRECTORY IDENTITY. ResolveWorkspaceRoot is abs plus
869+
// filepath.Clean plus a stat: it does not fold case and does not resolve
870+
// junctions, so one directory reached by two spellings produces two different
871+
// roots. A session persisted from the TUI was then unresumable from an editor
872+
// holding a different spelling of the same project folder, and session/list
873+
// filtered by the other spelling returned nothing — not a failed resume but an
874+
// invisible one, on exactly the case this feature exists for.
875+
//
876+
// filepath.EvalSymlinks is NOT the fix on Windows: it normalises a drive letter
877+
// but returns a junction path unchanged, so the alias survives it. Junctions need
878+
// no privilege, so this is ordinary rather than exotic. os.SameFile compares the
879+
// filesystem's own identity for the two directories, which is the question being
880+
// asked.
881+
//
882+
// The string comparison stays as the fast path, and a stat failure falls back to
883+
// it rather than widening the match — this gate refuses access to another
884+
// workspace's files and configuration, so an unanswerable comparison denies.
885+
func sameWorkspace(left, right string) bool {
886+
if left == right {
887+
return true
888+
}
889+
leftInfo, err := os.Stat(left)
890+
if err != nil {
891+
return false
892+
}
893+
rightInfo, err := os.Stat(right)
894+
if err != nil {
895+
return false
896+
}
897+
return os.SameFile(leftInfo, rightInfo)
898+
}

‎internal/acp/agent_test.go‎

Lines changed: 163 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@ import (
88
"io"
99
"os"
1010
"path/filepath"
11+
"sort"
1112
"strings"
1213
"testing"
1314
"time"
@@ -763,3 +764,165 @@ func drainTextUntil(t *testing.T, ch <-chan string, done func(string) bool) stri
763764
}
764765
}
765766
}
767+
768+
// normalisingResolver reproduces what ResolveWorkspaceRoot actually does — abs,
769+
// Clean, and a stat that the path exists — WITHOUT resolving symlinks or folding
770+
// case, which is the behaviour that makes two spellings of one directory produce
771+
// two different roots.
772+
//
773+
// The package's own testDeps resolver is the identity function. Under it the
774+
// workspace guard degenerates to "are these two strings different", fed two
775+
// unrelated temp directories, so it can only ever answer yes: the rejection
776+
// direction is pinned and the acceptance direction is asserted nowhere.
777+
func normalisingResolver(t *testing.T) func(string) (string, error) {
778+
t.Helper()
779+
return func(cwd string) (string, error) {
780+
absolute, err := filepath.Abs(cwd)
781+
if err != nil {
782+
return "", err
783+
}
784+
absolute = filepath.Clean(absolute)
785+
info, err := os.Stat(absolute)
786+
if err != nil {
787+
return "", err
788+
}
789+
if !info.IsDir() {
790+
return "", errors.New("workspace is not a directory")
791+
}
792+
return absolute, nil
793+
}
794+
}
795+
796+
// ONE DIRECTORY UNDER TWO SPELLINGS IS ONE WORKSPACE.
797+
//
798+
// A session persisted from the TUI has to stay resumable from an editor holding
799+
// a different spelling of the same project folder — the two processes most
800+
// likely to disagree about spelling, and the case this feature exists for. It
801+
// failed closed, so it blocked legitimate resumes rather than admitting foreign
802+
// ones, but session/list filtered by the other spelling returned nothing, which
803+
// makes it an invisible failure rather than a reported one.
804+
func TestACPResumesAcrossTwoSpellingsOfOneWorkspace(t *testing.T) {
805+
real := t.TempDir()
806+
alias := filepath.Join(t.TempDir(), "alias")
807+
if err := os.Symlink(real, alias); err != nil {
808+
t.Skipf("cannot create a directory alias here: %v", err)
809+
}
810+
// The premise: the resolver really does produce two different strings.
811+
resolve := normalisingResolver(t)
812+
realRoot, err := resolve(real)
813+
if err != nil {
814+
t.Fatal(err)
815+
}
816+
aliasRoot, err := resolve(alias)
817+
if err != nil {
818+
t.Fatal(err)
819+
}
820+
if realRoot == aliasRoot {
821+
t.Skipf("this filesystem folds the alias away (%q == %q); the guard cannot be exercised", realRoot, aliasRoot)
822+
}
823+
824+
deps := testDeps(t)
825+
deps.ResolveWorkspaceRoot = resolve
826+
created, err := deps.Store.Create(sessions.CreateInput{SessionID: "aliased-workspace", Cwd: real})
827+
if err != nil {
828+
t.Fatal(err)
829+
}
830+
h := newHarness(t, deps)
831+
defer h.stop()
832+
ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second)
833+
defer cancel()
834+
835+
// ACCEPTANCE: resume and load under the OTHER spelling must both work.
836+
for _, method := range []string{MethodSessionLoad, MethodSessionResume} {
837+
var result LoadSessionResult
838+
if err := h.client.Call(ctx, method, LoadSessionParams{SessionID: created.SessionID, Cwd: alias, McpServers: []McpServer{}}, &result); err != nil {
839+
t.Errorf("%s under an alias of the persisted workspace failed: %v", method, err)
840+
}
841+
}
842+
843+
// And session/list filtered by the alias must still find it.
844+
var listed ListSessionsResult
845+
if err := h.client.Call(ctx, MethodSessionList, ListSessionsParams{Cwd: alias}, &listed); err != nil {
846+
t.Fatalf("session/list: %v", err)
847+
}
848+
found := false
849+
for _, item := range listed.Sessions {
850+
if item.SessionID == created.SessionID {
851+
found = true
852+
}
853+
}
854+
if !found {
855+
t.Errorf("session/list filtered by an alias of its own workspace returned %d sessions without it", len(listed.Sessions))
856+
}
857+
858+
// REJECTION still holds: a genuinely different directory is refused.
859+
other := t.TempDir()
860+
err = h.client.Call(ctx, MethodSessionResume, ResumeSessionParams{SessionID: created.SessionID, Cwd: other, McpServers: []McpServer{}}, &ResumeSessionResult{})
861+
var rpcErr *rpcError
862+
if !errors.As(err, &rpcErr) || rpcErr.Code != codeInvalidParams {
863+
t.Errorf("resume from an unrelated workspace = %v, want invalid params", err)
864+
}
865+
}
866+
867+
// THE WIRE KEYS ARE AN EXTERNAL CONTRACT, not internal names.
868+
//
869+
// Nothing pinned them, so renaming a Go field — or dropping an omitempty —
870+
// would break every client and leave the suite green. These are the keys this
871+
// feature adds to the protocol; a change here is a change to what editors
872+
// consume.
873+
func TestSessionWireKeysAreStable(t *testing.T) {
874+
marshalled := func(v any) map[string]any {
875+
t.Helper()
876+
raw, err := json.Marshal(v)
877+
if err != nil {
878+
t.Fatal(err)
879+
}
880+
var out map[string]any
881+
if err := json.Unmarshal(raw, &out); err != nil {
882+
t.Fatal(err)
883+
}
884+
return out
885+
}
886+
has := func(where string, got map[string]any, want ...string) {
887+
t.Helper()
888+
for _, key := range want {
889+
if _, ok := got[key]; !ok {
890+
t.Errorf("%s is missing the wire key %q; got %v", where, key, keysOf(got))
891+
}
892+
}
893+
}
894+
895+
has("SessionInfo", marshalled(SessionInfo{
896+
SessionID: "s1", Cwd: "/w", Title: "t", UpdatedAt: "now",
897+
Meta: &SessionInfoMeta{ModelID: "m", CreatedAt: "then"},
898+
}), "sessionId", "cwd", "title", "updatedAt", "_meta")
899+
900+
has("SessionInfoMeta", marshalled(SessionInfoMeta{ModelID: "m", CreatedAt: "then"}),
901+
"modelId", "createdAt")
902+
903+
has("ListSessionsResult", marshalled(ListSessionsResult{
904+
Sessions: []SessionInfo{}, NextCursor: "c",
905+
}), "sessions", "nextCursor")
906+
907+
has("ListSessionsParams", marshalled(ListSessionsParams{Cwd: "/w", Cursor: "c"}),
908+
"cwd", "cursor")
909+
910+
// sessionCapabilities is omitempty, so it must appear when SET — a client
911+
// discovers list/resume support through it.
912+
has("AgentCapabilities", marshalled(AgentCapabilities{
913+
LoadSession: true,
914+
SessionCapabilities: &SessionCapabilities{List: &struct{}{}, Resume: &struct{}{}},
915+
}), "loadSession", "promptCapabilities", "sessionCapabilities")
916+
917+
capabilities := marshalled(SessionCapabilities{List: &struct{}{}, Resume: &struct{}{}})
918+
has("SessionCapabilities", capabilities, "list", "resume")
919+
}
920+
921+
func keysOf(m map[string]any) []string {
922+
out := make([]string, 0, len(m))
923+
for key := range m {
924+
out = append(out, key)
925+
}
926+
sort.Strings(out)
927+
return out
928+
}

0 commit comments

Comments
 (0)