Skip to content

fix(mcp): fall back to cwd basename on ambiguous project (#248) #258

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
CarlosPlasencia wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix(mcp): fall back to cwd basename on ambiguous project (#248) #258

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 26 additions & 1 deletion internal/mcp/mcp.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import (
"errors"
"fmt"
"os"
"path/filepath"
"strings"
"time"

Expand Down Expand Up @@ -1627,14 +1628,38 @@ 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 {
cwd = "."
}
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
Expand Down
132 changes: 81 additions & 51 deletions internal/mcp/mcp_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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)
}
}

Expand Down Expand Up @@ -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)
Expand All @@ -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")
}
}

Expand Down Expand Up @@ -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)
Expand All @@ -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"} {
Expand All @@ -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)
}
Expand All @@ -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)
}
}

Expand Down Expand Up @@ -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"} {
Expand All @@ -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)
Expand All @@ -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)
}
}

Expand Down