Skip to content

Commit 2743714

Browse files
fix(mcp): tighten ambiguous project recovery
1 parent 21f530e commit 2743714

3 files changed

Lines changed: 147 additions & 5 deletions

File tree

docs/AGENT-SETUP.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -50,7 +50,7 @@ If a write tool returns `ambiguous_project`, the agent must not guess. Ask the u
5050
}
5151
```
5252

53-
This recovery path is accepted only after cwd detection is ambiguous and only when `project` exactly matches one of the reported `available_projects`. In all non-ambiguous cases, `.engram/config.json`/git/cwd detection remains authoritative and the explicit `project` is ignored. Alternatives: `cd` into the target repo before starting the MCP server, or add repo `.engram/config.json`.
53+
This recovery path is accepted only after cwd detection is ambiguous and only when `project`, after trimming surrounding whitespace, exactly matches one of the reported `available_projects`. Do not send normalized variants or guesses: if `available_projects` contains `foo--bar`, the retry must use `foo--bar`, not `foo-bar`; empty/whitespace choices are rejected. In all non-ambiguous cases, `.engram/config.json`/git/cwd detection remains authoritative and the explicit `project` is ignored. Alternatives: `cd` into the target repo before starting the MCP server, or add repo `.engram/config.json`.
5454

5555
**Read tools** (`mem_search`, `mem_context`, `mem_get_observation`, `mem_stats`, `mem_timeline`) accept an optional `project` override validated against the store. Omit it to auto-detect.
5656

internal/mcp/mcp.go

Lines changed: 44 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@ import (
1919
"errors"
2020
"fmt"
2121
"os"
22+
"path/filepath"
2223
"strings"
2324
"time"
2425

@@ -1921,7 +1922,7 @@ func resolveWriteProjectWithChoice(projectChoice, reason string) (projectpkg.Det
19211922
return res, err
19221923
}
19231924

1924-
choice, _ := store.NormalizeProject(projectChoice)
1925+
choice := strings.TrimSpace(projectChoice)
19251926
if choice == "" || !containsProjectChoice(res.AvailableProjects, choice) {
19261927
return res, &invalidProjectChoiceError{
19271928
Name: choice,
@@ -1931,20 +1932,54 @@ func resolveWriteProjectWithChoice(projectChoice, reason string) (projectpkg.Det
19311932

19321933
res.Project = choice
19331934
res.Source = projectpkg.SourceUserSelectedAfterAmbiguousProject
1935+
res.Path = resolveAmbiguousChoicePath(res.Path, choice)
19341936
res.Warning = "project selected by user after ambiguous_project recovery"
19351937
return res, nil
19361938
}
19371939

19381940
func containsProjectChoice(available []string, choice string) bool {
1941+
choice = strings.TrimSpace(choice)
19391942
for _, candidate := range available {
1940-
normalized, _ := store.NormalizeProject(candidate)
1941-
if normalized == choice {
1943+
if strings.TrimSpace(candidate) == choice {
19421944
return true
19431945
}
19441946
}
19451947
return false
19461948
}
19471949

1950+
func resolveAmbiguousChoicePath(ambiguousParent, choice string) string {
1951+
parent := strings.TrimSpace(ambiguousParent)
1952+
if parent == "" || strings.TrimSpace(choice) == "" {
1953+
return ""
1954+
}
1955+
1956+
entries, err := os.ReadDir(parent)
1957+
if err != nil {
1958+
return ""
1959+
}
1960+
for _, entry := range entries {
1961+
if !entry.IsDir() {
1962+
continue
1963+
}
1964+
// Match the same name shape used by project.DetectProjectFull for
1965+
// available_projects: trim + lowercase only. Do not use store.NormalizeProject
1966+
// here because it collapses repeated '-'/'_' and can create collisions.
1967+
if strings.TrimSpace(strings.ToLower(entry.Name())) != choice {
1968+
continue
1969+
}
1970+
childPath := filepath.Join(parent, entry.Name())
1971+
if _, err := os.Stat(filepath.Join(childPath, ".git")); err != nil {
1972+
continue
1973+
}
1974+
absChild, err := filepath.Abs(childPath)
1975+
if err != nil {
1976+
return childPath
1977+
}
1978+
return absChild
1979+
}
1980+
return ""
1981+
}
1982+
19481983
// resolveReadProject validates an optional project override against the store.
19491984
// If override is empty, falls back to auto-detection from cwd.
19501985
// JW2: normalizes the override (lowercase+trim) before ProjectExists lookup so
@@ -2001,6 +2036,12 @@ func writeProjectErrorResult(res projectpkg.DetectionResult, err error) *mcp.Cal
20012036
}
20022037
var choiceErr *invalidProjectChoiceError
20032038
if errors.As(err, &choiceErr) {
2039+
if choiceErr.Name == "" {
2040+
return errorWithMeta("invalid_project_choice",
2041+
"Project choice is empty; choose exactly one value from available_projects and retry with project_choice_reason=user_selected_after_ambiguous_project",
2042+
choiceErr.AvailableProjects,
2043+
)
2044+
}
20042045
return errorWithMeta("invalid_project_choice",
20052046
fmt.Sprintf("Project choice %q is not one of available_projects", choiceErr.Name),
20062047
choiceErr.AvailableProjects,

internal/mcp/mcp_test.go

Lines changed: 102 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2791,7 +2791,7 @@ func TestSessionStartWithExplicitDirectoryResolvesProjectFromDirectory(t *testin
27912791

27922792
// TestWriteSchema_ProjectFieldOnlyForAmbiguousRecovery asserts that only the
27932793
// write tools with explicit ambiguous-project recovery expose project fields.
2794-
func TestWriteSchema_NoProjectField(t *testing.T) {
2794+
func TestWriteSchema_ProjectFieldOnlyForAmbiguousRecovery(t *testing.T) {
27952795
s := newMCPTestStore(t)
27962796
srv := NewServer(s)
27972797

@@ -2976,12 +2976,110 @@ func TestMemSave_AmbiguousWithValidUserChoiceSucceeds(t *testing.T) {
29762976
if body["project"] != "repo-choice-b" || body["project_source"] != project.SourceUserSelectedAfterAmbiguousProject {
29772977
t.Fatalf("expected explicit user choice envelope, got %v", body)
29782978
}
2979+
if body["project_path"] != filepath.Join(parent, "repo-choice-b") {
2980+
t.Fatalf("expected project_path to point at selected repo root, got %v", body)
2981+
}
29792982
obs, err := s.Search("chosen project memory", store.SearchOptions{Project: "repo-choice-b", Limit: 5})
29802983
if err != nil || len(obs) != 1 {
29812984
t.Fatalf("expected observation in selected project, obs=%d err=%v", len(obs), err)
29822985
}
29832986
}
29842987

2988+
func TestMemSave_AmbiguousChoiceRequiresExactAvailableProject(t *testing.T) {
2989+
parent := t.TempDir()
2990+
for _, name := range []string{"foo--bar", "baz__qux"} {
2991+
child := filepath.Join(parent, name)
2992+
if err := os.MkdirAll(child, 0o755); err != nil {
2993+
t.Fatal(err)
2994+
}
2995+
initTestGitRepo(t, child)
2996+
}
2997+
t.Chdir(parent)
2998+
2999+
s := newMCPTestStore(t)
3000+
h := handleSave(s, MCPConfig{}, NewSessionActivity(10*time.Minute))
3001+
res, err := h(context.Background(), mcppkg.CallToolRequest{Params: mcppkg.CallToolParams{Arguments: map[string]any{
3002+
"title": "normalized choice must fail",
3003+
"content": "must not save under normalized collision",
3004+
"project": "foo-bar",
3005+
"project_choice_reason": project.SourceUserSelectedAfterAmbiguousProject,
3006+
}}})
3007+
if err != nil {
3008+
t.Fatalf("handler error: %v", err)
3009+
}
3010+
if !res.IsError {
3011+
t.Fatal("expected invalid project choice for normalized-but-not-exact value")
3012+
}
3013+
body := callResultJSON(t, res)
3014+
if body["error_code"] != "invalid_project_choice" {
3015+
t.Fatalf("expected invalid_project_choice, got %v", body)
3016+
}
3017+
available, ok := body["available_projects"].([]any)
3018+
foundFooBar := false
3019+
for _, candidate := range available {
3020+
if candidate == "foo--bar" {
3021+
foundFooBar = true
3022+
break
3023+
}
3024+
}
3025+
if !ok || !foundFooBar {
3026+
t.Fatalf("expected exact available project names, got %v", body["available_projects"])
3027+
}
3028+
if strings.Contains(body["message"].(string), "foo--bar") {
3029+
t.Fatalf("message should report the rejected trimmed choice, not a normalized available value: %v", body)
3030+
}
3031+
obs, searchErr := s.Search("normalized choice must fail", store.SearchOptions{Project: "foo-bar", Limit: 5})
3032+
if searchErr != nil || len(obs) != 0 {
3033+
t.Fatalf("normalized collision must not receive writes, obs=%d err=%v", len(obs), searchErr)
3034+
}
3035+
3036+
res, err = h(context.Background(), mcppkg.CallToolRequest{Params: mcppkg.CallToolParams{Arguments: map[string]any{
3037+
"title": "exact choice succeeds",
3038+
"content": "saved after exact available project choice",
3039+
"project": " baz__qux ",
3040+
"project_choice_reason": project.SourceUserSelectedAfterAmbiguousProject,
3041+
}}})
3042+
if err != nil || res.IsError {
3043+
t.Fatalf("exact trimmed choice should succeed: err=%v isError=%v text=%q", err, res.IsError, callResultText(t, res))
3044+
}
3045+
body = callResultJSON(t, res)
3046+
if body["project_path"] != filepath.Join(parent, "baz__qux") {
3047+
t.Fatalf("expected project_path to selected exact repo root, got %v", body)
3048+
}
3049+
}
3050+
3051+
func TestMemSave_AmbiguousEmptyProjectChoiceIsActionable(t *testing.T) {
3052+
parent := t.TempDir()
3053+
for _, name := range []string{"repo-empty-a", "repo-empty-b"} {
3054+
child := filepath.Join(parent, name)
3055+
if err := os.MkdirAll(child, 0o755); err != nil {
3056+
t.Fatal(err)
3057+
}
3058+
initTestGitRepo(t, child)
3059+
}
3060+
t.Chdir(parent)
3061+
3062+
s := newMCPTestStore(t)
3063+
h := handleSave(s, MCPConfig{}, NewSessionActivity(10*time.Minute))
3064+
res, err := h(context.Background(), mcppkg.CallToolRequest{Params: mcppkg.CallToolParams{Arguments: map[string]any{
3065+
"title": "empty choice must fail",
3066+
"content": "must not save",
3067+
"project": " \t\n ",
3068+
"project_choice_reason": project.SourceUserSelectedAfterAmbiguousProject,
3069+
}}})
3070+
if err != nil {
3071+
t.Fatalf("handler error: %v", err)
3072+
}
3073+
if !res.IsError {
3074+
t.Fatal("expected invalid project choice for whitespace project")
3075+
}
3076+
body := callResultJSON(t, res)
3077+
message, _ := body["message"].(string)
3078+
if body["error_code"] != "invalid_project_choice" || !strings.Contains(message, "Project choice is empty") || !strings.Contains(message, "available_projects") {
3079+
t.Fatalf("expected actionable empty choice error, got %v", body)
3080+
}
3081+
}
3082+
29853083
func TestMemSave_AmbiguousWithInventedProjectRejected(t *testing.T) {
29863084
parent := t.TempDir()
29873085
for _, name := range []string{"repo-valid-a", "repo-valid-b"} {
@@ -3045,6 +3143,9 @@ func TestMemSavePrompt_AmbiguousWithValidUserChoiceSucceeds(t *testing.T) {
30453143
if body["project"] != "repo-prompt-a" || body["project_source"] != project.SourceUserSelectedAfterAmbiguousProject {
30463144
t.Fatalf("expected explicit user choice envelope, got %v", body)
30473145
}
3146+
if body["project_path"] != filepath.Join(parent, "repo-prompt-a") {
3147+
t.Fatalf("expected project_path to point at selected prompt repo root, got %v", body)
3148+
}
30483149
prompts, err := s.RecentPrompts("repo-prompt-a", 5)
30493150
if err != nil || len(prompts) != 1 {
30503151
t.Fatalf("expected prompt in selected project, prompts=%d err=%v", len(prompts), err)

0 commit comments

Comments
 (0)