From 9a73a6f7a084db15fe7212b74631e5bc676409dc Mon Sep 17 00:00:00 2001 From: EduDev Date: Sun, 26 Apr 2026 21:59:00 -0500 Subject: [PATCH] fix(mcp): fall back to cwd basename on ambiguous project (#248) When the MCP server is launched from a directory that is the parent of multiple git repositories, all write tools (mem_save, mem_session_summary, etc.) fail with ErrAmbiguousProject. This is a regression from v1.12.0 where filepath.Base(cwd) was used as a fallback. resolveWriteProject now mirrors the CLI wrapper DetectProject(): on ErrAmbiguousProject it falls back to the lowercased cwd basename, sets Source=SourceDirBasename, and emits a Warning that lists the cwd repos so the user knows the project name was derived from the workspace folder. This makes engram MCP usable again from workspace directories (the common setup with multiple cloned repos under one parent) without removing the structured detection result for non-ambiguous cases. Tests adapted to assert the new fallback behavior (success + envelope + warning) instead of the previous fail-fast error envelope. --- internal/mcp/mcp.go | 27 +++++++- internal/mcp/mcp_test.go | 132 ++++++++++++++++++++++++--------------- 2 files changed, 107 insertions(+), 52 deletions(-) diff --git a/internal/mcp/mcp.go b/internal/mcp/mcp.go index 889923d..56890fd 100644 --- a/internal/mcp/mcp.go +++ b/internal/mcp/mcp.go @@ -19,6 +19,7 @@ import ( "errors" "fmt" "os" + "path/filepath" "strings" "time" @@ -1627,7 +1628,16 @@ func (e *unknownProjectError) Error() string { } // resolveWriteProject detects the current project from the process working -// directory. Returns ErrAmbiguousProject if cwd is a parent of multiple repos. +// directory. +// +// On ErrAmbiguousProject (cwd is parent of multiple git child repos), falls +// back to filepath.Base(cwd) for parity with the CLI wrapper DetectProject(). +// This avoids breaking write tools when the MCP server is launched from a +// workspace folder that contains multiple cloned repositories — a common +// setup that worked in v1.12.0 and regressed when DetectProjectFull added +// Case 4. The fallback sets Source=SourceDirBasename and emits a Warning +// so consumers know the project name was derived from the workspace name +// rather than a specific repo. See issue #248. func resolveWriteProject() (projectpkg.DetectionResult, error) { cwd, err := os.Getwd() if err != nil { @@ -1635,6 +1645,21 @@ func resolveWriteProject() (projectpkg.DetectionResult, error) { } res := projectpkg.DetectProjectFull(cwd) if res.Error != nil { + if errors.Is(res.Error, projectpkg.ErrAmbiguousProject) { + base := filepath.Base(cwd) + if base == "" || base == "." { + base = "unknown" + } + absDir, _ := filepath.Abs(cwd) + available := res.AvailableProjects + res = projectpkg.DetectionResult{ + Project: strings.ToLower(strings.TrimSpace(base)), + Source: projectpkg.SourceDirBasename, + Path: absDir, + Warning: "cwd is parent of multiple git repos; using basename as project name. Available children: " + strings.Join(available, ", "), + } + return res, nil + } return res, res.Error } return res, nil diff --git a/internal/mcp/mcp_test.go b/internal/mcp/mcp_test.go index f81b3dd..bc5c09f 100644 --- a/internal/mcp/mcp_test.go +++ b/internal/mcp/mcp_test.go @@ -2606,8 +2606,13 @@ func TestMemSave_IgnoresLLMProject(t *testing.T) { } } -// TestMemSave_AmbiguousEnvelope asserts error_code=="ambiguous_project", no write (REQ-309). -func TestMemSave_AmbiguousEnvelope(t *testing.T) { +// TestMemSave_AmbiguousFallback asserts that save succeeds with basename +// fallback when cwd is parent of multiple git repos. Previously this returned +// error_code=="ambiguous_project" (REQ-309 strict mode), but that was a +// regression vs v1.12.0 — see issue #248. The handler now uses cwd basename +// as project name and emits a warning so the save can proceed in workspaces +// containing multiple repos. +func TestMemSave_AmbiguousFallback(t *testing.T) { parent := t.TempDir() for _, name := range []string{"repo-x", "repo-y"} { child := filepath.Join(parent, name) @@ -2622,23 +2627,28 @@ func TestMemSave_AmbiguousEnvelope(t *testing.T) { h := handleSave(s, MCPConfig{}, NewSessionActivity(10*time.Minute)) req := mcppkg.CallToolRequest{Params: mcppkg.CallToolParams{Arguments: map[string]any{ - "title": "should not be saved", - "content": "ambiguous test", + "title": "ambiguous fallback test", + "content": "saved under cwd basename", "type": "manual", }}} res, err := h(context.Background(), req) if err != nil { t.Fatalf("handler error: %v", err) } - if !res.IsError { - t.Fatal("expected error result for ambiguous project") + if res.IsError { + t.Fatalf("expected success after ambiguous fallback, got error: %s", callResultText(t, res)) } text := callResultText(t, res) - if !strings.Contains(text, "ambiguous_project") { - t.Errorf("expected error_code 'ambiguous_project', got: %q", text) + if !strings.Contains(text, "warning") { + t.Errorf("expected warning in response envelope, got: %q", text) } - if !strings.Contains(text, "available_projects") { - t.Errorf("expected available_projects in error, got: %q", text) + if !strings.Contains(text, "dir_basename") { + t.Errorf("expected project_source 'dir_basename' in response, got: %q", text) + } + // Project should be the parent dir basename (lowercased). + expectedProject := strings.ToLower(filepath.Base(parent)) + if !strings.Contains(text, `"project":"`+expectedProject+`"`) { + t.Errorf("expected project=%q in response, got: %q", expectedProject, text) } } @@ -2967,8 +2977,11 @@ func TestResolveWriteProject_AutoDetects(t *testing.T) { } } -// TestResolveWriteProject_AmbiguousError: assert errors.Is(err, ErrAmbiguousProject) -func TestResolveWriteProject_AmbiguousError(t *testing.T) { +// TestResolveWriteProject_AmbiguousFallback asserts that ambiguous cwd +// (parent of multiple git repos) no longer errors — it falls back to +// filepath.Base(cwd) lowercased, with Source=SourceDirBasename and a +// non-empty Warning. See issue #248 (regression vs v1.12.0). +func TestResolveWriteProject_AmbiguousFallback(t *testing.T) { parent := t.TempDir() for _, name := range []string{"repo-a", "repo-b"} { child := filepath.Join(parent, name) @@ -2979,9 +2992,19 @@ func TestResolveWriteProject_AmbiguousError(t *testing.T) { } t.Chdir(parent) - _, err := resolveWriteProject() - if !errors.Is(err, project.ErrAmbiguousProject) { - t.Errorf("expected ErrAmbiguousProject, got %v", err) + res, err := resolveWriteProject() + if err != nil { + t.Fatalf("expected nil error after ambiguous fallback, got %v", err) + } + if res.Source != project.SourceDirBasename { + t.Errorf("expected Source=%q, got %q", project.SourceDirBasename, res.Source) + } + expectedProject := strings.ToLower(filepath.Base(parent)) + if res.Project != expectedProject { + t.Errorf("expected Project=%q, got %q", expectedProject, res.Project) + } + if res.Warning == "" { + t.Error("expected non-empty Warning explaining the fallback") } } @@ -3296,8 +3319,10 @@ func TestMemSessionSummary_AutoDetectsProject(t *testing.T) { } } -// TestMemSessionSummary_AmbiguousReturnsError: ambiguous cwd returns error (REQ-309). -func TestMemSessionSummary_AmbiguousReturnsError(t *testing.T) { +// TestMemSessionSummary_AmbiguousFallback: ambiguous cwd falls back to +// basename rather than erroring (issue #248). Session summary must succeed +// with a warning and project_source=dir_basename. +func TestMemSessionSummary_AmbiguousFallback(t *testing.T) { parent := t.TempDir() for _, name := range []string{"repo-summary-1", "repo-summary-2"} { child := filepath.Join(parent, name) @@ -3313,26 +3338,31 @@ func TestMemSessionSummary_AmbiguousReturnsError(t *testing.T) { res, err := h(context.Background(), mcppkg.CallToolRequest{ Params: mcppkg.CallToolParams{Arguments: map[string]any{ - "content": "## Goal\nAmbiguous test", + "content": "## Goal\nAmbiguous fallback test", }}, }) if err != nil { t.Fatalf("handler error: %v", err) } - if !res.IsError { - t.Fatal("expected error for ambiguous project in session_summary") + if res.IsError { + t.Fatalf("expected success with basename fallback, got error: %s", callResultText(t, res)) } text := callResultText(t, res) - if !strings.Contains(text, "ambiguous_project") { - t.Errorf("expected error_code ambiguous_project, got: %q", text) + if !strings.Contains(text, "warning") { + t.Errorf("expected warning explaining the fallback, got: %q", text) + } + if !strings.Contains(text, "dir_basename") { + t.Errorf("expected project_source 'dir_basename', got: %q", text) } } // ─── Judgment Round 1 Hotfix tests ─────────────────────────────────────────── -// JW1: TestWriteTool_AmbiguousErrorUsesCwdRepos_NotAllProjects -// When cwd is ambiguous, error must list the repos in cwd — NOT all store projects. -func TestWriteTool_AmbiguousErrorUsesCwdRepos_NotAllProjects(t *testing.T) { +// JW1: TestWriteTool_AmbiguousFallbackWarningListsCwdRepos +// When cwd is ambiguous, write succeeds with basename fallback. The warning +// message must list the repos found in cwd — NOT all projects in the store — +// so the user knows which children triggered the fallback. (issue #248) +func TestWriteTool_AmbiguousFallbackWarningListsCwdRepos(t *testing.T) { // Set up an ambiguous parent dir with 2 git repos. parent := t.TempDir() for _, name := range []string{"repo-cwd-a", "repo-cwd-b"} { @@ -3345,7 +3375,7 @@ func TestWriteTool_AmbiguousErrorUsesCwdRepos_NotAllProjects(t *testing.T) { t.Chdir(parent) s := newMCPTestStore(t) - // Seed an unrelated project in the store — this must NOT appear in error. + // Seed an unrelated project in the store — this must NOT appear in warning. if err := s.CreateSession("sess-unrelated", "unrelated-store-project", "/tmp"); err != nil { t.Fatal(err) } @@ -3360,20 +3390,20 @@ func TestWriteTool_AmbiguousErrorUsesCwdRepos_NotAllProjects(t *testing.T) { if err != nil { t.Fatalf("handler error: %v", err) } - if !res.IsError { - t.Fatal("expected error for ambiguous cwd") + if res.IsError { + t.Fatalf("expected success with basename fallback, got error: %s", callResultText(t, res)) } text := callResultText(t, res) - // Must list cwd repos (repo-cwd-a, repo-cwd-b). + // Warning must list cwd repos (repo-cwd-a, repo-cwd-b). if !strings.Contains(text, "repo-cwd-a") { - t.Errorf("available_projects must contain repo-cwd-a (cwd repo); got: %q", text) + t.Errorf("warning must mention repo-cwd-a (cwd repo); got: %q", text) } if !strings.Contains(text, "repo-cwd-b") { - t.Errorf("available_projects must contain repo-cwd-b (cwd repo); got: %q", text) + t.Errorf("warning must mention repo-cwd-b (cwd repo); got: %q", text) } // Must NOT list the unrelated store project. if strings.Contains(text, "unrelated-store-project") { - t.Errorf("available_projects must NOT list all store projects; got: %q", text) + t.Errorf("warning must NOT list unrelated store projects; got: %q", text) } } @@ -3547,11 +3577,10 @@ func TestHandleContext_EnvelopeProjectMatchesQueryProject(t *testing.T) { } } -// JR2-3 RED: TestHandleGetObservation_DegradedPathNoEnvelope -// When the cwd is ambiguous (multiple git repos), resolveReadProject returns an error. -// The handler must degrade gracefully: IsError=false, result contains observation content, -// and the response is NOT JSON (no project_source envelope field). -func TestHandleGetObservation_DegradedPathNoEnvelope(t *testing.T) { +// TestHandleGetObservation_AmbiguousFallbackEnvelope: ambiguous cwd no +// longer triggers the degraded path — the read tool succeeds with the +// basename fallback and includes the envelope (with warning). See issue #248. +func TestHandleGetObservation_AmbiguousFallbackEnvelope(t *testing.T) { // Create a parent dir with two child git repos → ambiguous cwd. parent := t.TempDir() for _, name := range []string{"repo-a", "repo-b"} { @@ -3564,15 +3593,15 @@ func TestHandleGetObservation_DegradedPathNoEnvelope(t *testing.T) { t.Chdir(parent) s := newMCPTestStore(t) - if err := s.CreateSession("sess-degraded", "degraded-project", "/tmp"); err != nil { + if err := s.CreateSession("sess-fallback", "fallback-project", "/tmp"); err != nil { t.Fatal(err) } obsID, err := s.AddObservation(store.AddObservationParams{ - SessionID: "sess-degraded", + SessionID: "sess-fallback", Type: "manual", - Title: "degraded obs title", - Content: "degraded obs content", - Project: "degraded-project", + Title: "fallback obs title", + Content: "fallback obs content", + Project: "fallback-project", }) if err != nil { t.Fatal(err) @@ -3588,18 +3617,19 @@ func TestHandleGetObservation_DegradedPathNoEnvelope(t *testing.T) { t.Fatalf("unexpected error: %v", err) } if res.IsError { - t.Fatalf("degraded path must not return IsError=true; text=%q", callResultText(t, res)) + t.Fatalf("expected success with basename fallback; text=%q", callResultText(t, res)) } text := callResultText(t, res) - if !strings.Contains(text, "degraded obs content") { - t.Errorf("degraded path must contain observation content; got: %q", text) + if !strings.Contains(text, "fallback obs content") { + t.Errorf("response must contain observation content; got: %q", text) } - // The degraded path returns plain text (no JSON envelope), so project_source must be absent. - var m map[string]any - if json.Unmarshal([]byte(text), &m) == nil { - if _, hasSource := m["project_source"]; hasSource { - t.Error("degraded path must NOT include 'project_source' envelope field") - } + // With the fallback, the response is the JSON envelope, so project_source + // must be present and equal "dir_basename". + if !strings.Contains(text, "project_source") { + t.Errorf("envelope must include 'project_source'; got: %q", text) + } + if !strings.Contains(text, "dir_basename") { + t.Errorf("project_source should be 'dir_basename' on ambiguous cwd; got: %q", text) } }