From 64207b5b1380ae7937386dee6fdba24e713247b9 Mon Sep 17 00:00:00 2001 From: Alan Buscaglia Date: Wed, 27 May 2026 16:39:30 +0200 Subject: [PATCH] =?UTF-8?q?fix(mcp):=20harden=20handleSessionSummary=20?= =?UTF-8?q?=E2=80=94=20process=20override=20+=20empty-content=20guard?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes #403 Closes #413 Closes #393 handleSessionSummary was the one write tool missed in commit b094052: it called bare resolveWriteProject() instead of resolveWriteProjectWithProcessOverride(), so ENGRAM_PROJECT / `engram mcp --project` was silently ignored, causing writes to the wrong project and an OpenCode timeout on ambiguous cwd. Additionally, no empty-content guard existed before AddObservation; an empty mem_session_summary would persist a blank observation that created a stuck sync_mutation blocking cloud upgrade. Fixes: - Switch to resolveWriteProjectWithProcessOverride(cfg.DefaultProject) so the process-level override takes precedence over cwd detection, matching all other write tools. - Add strings.TrimSpace(content) == "" guard (same pattern as handleSave / #374) before any project resolution or store write. Tests: 4 new table-driven tests covering process-override with no-git dir, process-override bypassing ambiguous cwd, empty content rejection, and whitespace-only content rejection. --- internal/mcp/mcp.go | 11 +++- internal/mcp/mcp_test.go | 129 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 138 insertions(+), 2 deletions(-) diff --git a/internal/mcp/mcp.go b/internal/mcp/mcp.go index 268f916b..3b72500d 100644 --- a/internal/mcp/mcp.go +++ b/internal/mcp/mcp.go @@ -1600,8 +1600,15 @@ func handleSessionSummary(s *store.Store, cfg MCPConfig, activity *SessionActivi sessionID, _ := req.GetArguments()["session_id"].(string) // project field intentionally not read — auto-detect only (REQ-308 write-tool contract) - // Auto-detect project from cwd; fail fast on ambiguous (REQ-308, REQ-309) - detRes, err := resolveWriteProject() + // Reject empty/whitespace-only content before any project resolution (#393). + if strings.TrimSpace(content) == "" { + return mcp.NewToolResultError("content is required for mem_session_summary"), nil + } + + // Honour process-level project override (cfg.DefaultProject) set via + // ENGRAM_PROJECT or `engram mcp --project` (#403/#413). Falls back to cwd + // detection when no override is configured. + detRes, err := resolveWriteProjectWithProcessOverride(cfg.DefaultProject) if err != nil { return writeProjectErrorResult(nil, "", detRes, err), nil } diff --git a/internal/mcp/mcp_test.go b/internal/mcp/mcp_test.go index b5d8e30d..b523ade6 100644 --- a/internal/mcp/mcp_test.go +++ b/internal/mcp/mcp_test.go @@ -6519,3 +6519,132 @@ func TestProcessOverrideSaveHandlerWritesToDefaultProject(t *testing.T) { t.Fatalf("results in trusted project = %d; want 1", len(results)) } } + +// ─── #403/#413: handleSessionSummary process-override tests ────────────────── + +// TestSessionSummary_ProcessOverrideWritesToDefaultProject verifies that when +// cfg.DefaultProject is set (process-level override via ENGRAM_PROJECT / --project), +// handleSessionSummary writes under that project instead of falling back to cwd +// detection. Mirrors TestProcessOverrideSaveHandlerWritesToDefaultProject for save. +func TestSessionSummary_ProcessOverrideWritesToDefaultProject(t *testing.T) { + // Use a temp dir that has no git repo — without the fix, resolveWriteProject() + // would return an error or a wrong project; with the fix it uses the override. + dir := t.TempDir() + t.Chdir(dir) + + s := newMCPTestStore(t) + h := handleSessionSummary(s, MCPConfig{DefaultProject: "Trusted Project"}, NewSessionActivity(10*time.Minute)) + + res, err := h(context.Background(), mcppkg.CallToolRequest{Params: mcppkg.CallToolParams{Arguments: map[string]any{ + "content": "## Goal\nProcess override session summary", + }}}) + if err != nil || res.IsError { + t.Fatalf("session summary error: err=%v isError=%v text=%q", err, res.IsError, callResultText(t, res)) + } + + obs, err := s.RecentObservations("trusted project", "project", 5) + if err != nil { + t.Fatalf("RecentObservations: %v", err) + } + if len(obs) == 0 { + t.Fatal("expected session_summary observation under 'trusted project' (process override); got none") + } + + m := callResultJSON(t, res) + if got := m["project"]; got != "trusted project" { + t.Errorf("response envelope project = %v; want 'trusted project'", got) + } + if got := m["project_source"]; got != sourceProcessOverride { + t.Errorf("response envelope project_source = %v; want %s", got, sourceProcessOverride) + } +} + +// TestSessionSummary_ProcessOverrideBypassesAmbiguousCWD verifies that an +// ambiguous cwd (parent dir with multiple git repos) is bypassed when +// cfg.DefaultProject is set. +func TestSessionSummary_ProcessOverrideBypassesAmbiguousCWD(t *testing.T) { + parent := t.TempDir() + for _, name := range []string{"repo-ss-1", "repo-ss-2"} { + child := filepath.Join(parent, name) + if err := os.MkdirAll(child, 0o755); err != nil { + t.Fatal(err) + } + initTestGitRepo(t, child) + } + t.Chdir(parent) + + s := newMCPTestStore(t) + h := handleSessionSummary(s, MCPConfig{DefaultProject: "override-project"}, NewSessionActivity(10*time.Minute)) + + res, err := h(context.Background(), mcppkg.CallToolRequest{Params: mcppkg.CallToolParams{Arguments: map[string]any{ + "content": "## Goal\nAmbiguous override test", + }}}) + if err != nil || res.IsError { + t.Fatalf("expected success via process override; err=%v isError=%v text=%q", err, res.IsError, callResultText(t, res)) + } + + obs, err := s.RecentObservations("override-project", "project", 5) + if err != nil { + t.Fatalf("RecentObservations: %v", err) + } + if len(obs) == 0 { + t.Fatal("expected session_summary under 'override-project'; got none") + } +} + +// ─── #393: handleSessionSummary empty-content guard tests ──────────────────── + +// TestSessionSummary_EmptyContentRejected verifies that an empty content string +// is rejected before AddObservation is called, mirroring the guard in handleSave. +func TestSessionSummary_EmptyContentRejected(t *testing.T) { + dir := t.TempDir() + initTestGitRepo(t, dir) + t.Chdir(dir) + + s := newMCPTestStore(t) + h := handleSessionSummary(s, MCPConfig{}, NewSessionActivity(10*time.Minute)) + + res, err := h(context.Background(), mcppkg.CallToolRequest{Params: mcppkg.CallToolParams{Arguments: map[string]any{ + "content": "", + }}}) + if err != nil { + t.Fatalf("handler returned Go error: %v", err) + } + if !res.IsError { + t.Fatal("expected tool error for empty content; got success") + } + text := callResultText(t, res) + if !strings.Contains(text, "content") { + t.Errorf("error message should mention 'content'; got: %q", text) + } + + // No observation must have been persisted. + obs, err := s.RecentObservations("", "project", 10) + if err != nil { + t.Fatalf("RecentObservations: %v", err) + } + if len(obs) != 0 { + t.Fatalf("expected 0 observations after empty-content rejection; got %d", len(obs)) + } +} + +// TestSessionSummary_WhitespaceOnlyContentRejected verifies that whitespace-only +// content is also rejected (mirrors handleSave behaviour). +func TestSessionSummary_WhitespaceOnlyContentRejected(t *testing.T) { + dir := t.TempDir() + initTestGitRepo(t, dir) + t.Chdir(dir) + + s := newMCPTestStore(t) + h := handleSessionSummary(s, MCPConfig{}, NewSessionActivity(10*time.Minute)) + + res, err := h(context.Background(), mcppkg.CallToolRequest{Params: mcppkg.CallToolParams{Arguments: map[string]any{ + "content": " \n\t ", + }}}) + if err != nil { + t.Fatalf("handler returned Go error: %v", err) + } + if !res.IsError { + t.Fatal("expected tool error for whitespace-only content; got success") + } +}