Skip to content

Commit 2087c5b

Browse files
fix(mcp): harden handleSessionSummary — process override + empty-content guard (#424)
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.
1 parent a13915a commit 2087c5b

2 files changed

Lines changed: 140 additions & 3 deletions

File tree

internal/mcp/mcp.go

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1616,8 +1616,15 @@ func handleSessionSummary(s *store.Store, cfg MCPConfig, activity *SessionActivi
16161616
sessionID, _ := req.GetArguments()["session_id"].(string)
16171617
// project field intentionally not read — auto-detect only (REQ-308 write-tool contract)
16181618

1619-
// Auto-detect project from cwd; fail fast on ambiguous (REQ-308, REQ-309)
1620-
detRes, err := resolveWriteProject()
1619+
// Reject empty/whitespace-only content before any project resolution (#393).
1620+
if strings.TrimSpace(content) == "" {
1621+
return mcp.NewToolResultError("content is required for mem_session_summary"), nil
1622+
}
1623+
1624+
// Honour process-level project override (cfg.DefaultProject) set via
1625+
// ENGRAM_PROJECT or `engram mcp --project` (#403/#413). Falls back to cwd
1626+
// detection when no override is configured.
1627+
detRes, err := resolveWriteProjectWithProcessOverride(cfg.DefaultProject)
16211628
if err != nil {
16221629
return writeProjectErrorResult(nil, "", detRes, err), nil
16231630
}

internal/mcp/mcp_test.go

Lines changed: 131 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6520,6 +6520,137 @@ func TestProcessOverrideSaveHandlerWritesToDefaultProject(t *testing.T) {
65206520
}
65216521
}
65226522

6523+
// ─── #403/#413: handleSessionSummary process-override tests ──────────────────
6524+
6525+
// TestSessionSummary_ProcessOverrideWritesToDefaultProject verifies that when
6526+
// cfg.DefaultProject is set (process-level override via ENGRAM_PROJECT / --project),
6527+
// handleSessionSummary writes under that project instead of falling back to cwd
6528+
// detection. Mirrors TestProcessOverrideSaveHandlerWritesToDefaultProject for save.
6529+
func TestSessionSummary_ProcessOverrideWritesToDefaultProject(t *testing.T) {
6530+
// Use a temp dir that has no git repo — without the fix, resolveWriteProject()
6531+
// would return an error or a wrong project; with the fix it uses the override.
6532+
dir := t.TempDir()
6533+
t.Chdir(dir)
6534+
6535+
s := newMCPTestStore(t)
6536+
h := handleSessionSummary(s, MCPConfig{DefaultProject: "Trusted Project"}, NewSessionActivity(10*time.Minute))
6537+
6538+
res, err := h(context.Background(), mcppkg.CallToolRequest{Params: mcppkg.CallToolParams{Arguments: map[string]any{
6539+
"content": "## Goal\nProcess override session summary",
6540+
}}})
6541+
if err != nil || res.IsError {
6542+
t.Fatalf("session summary error: err=%v isError=%v text=%q", err, res.IsError, callResultText(t, res))
6543+
}
6544+
6545+
obs, err := s.RecentObservations("trusted project", "project", 5)
6546+
if err != nil {
6547+
t.Fatalf("RecentObservations: %v", err)
6548+
}
6549+
if len(obs) == 0 {
6550+
t.Fatal("expected session_summary observation under 'trusted project' (process override); got none")
6551+
}
6552+
6553+
m := callResultJSON(t, res)
6554+
if got := m["project"]; got != "trusted project" {
6555+
t.Errorf("response envelope project = %v; want 'trusted project'", got)
6556+
}
6557+
if got := m["project_source"]; got != sourceProcessOverride {
6558+
t.Errorf("response envelope project_source = %v; want %s", got, sourceProcessOverride)
6559+
}
6560+
}
6561+
6562+
// TestSessionSummary_ProcessOverrideBypassesAmbiguousCWD verifies that an
6563+
// ambiguous cwd (parent dir with multiple git repos) is bypassed when
6564+
// cfg.DefaultProject is set.
6565+
func TestSessionSummary_ProcessOverrideBypassesAmbiguousCWD(t *testing.T) {
6566+
parent := t.TempDir()
6567+
for _, name := range []string{"repo-ss-1", "repo-ss-2"} {
6568+
child := filepath.Join(parent, name)
6569+
if err := os.MkdirAll(child, 0o755); err != nil {
6570+
t.Fatal(err)
6571+
}
6572+
initTestGitRepo(t, child)
6573+
}
6574+
t.Chdir(parent)
6575+
6576+
s := newMCPTestStore(t)
6577+
h := handleSessionSummary(s, MCPConfig{DefaultProject: "override-project"}, NewSessionActivity(10*time.Minute))
6578+
6579+
res, err := h(context.Background(), mcppkg.CallToolRequest{Params: mcppkg.CallToolParams{Arguments: map[string]any{
6580+
"content": "## Goal\nAmbiguous override test",
6581+
}}})
6582+
if err != nil || res.IsError {
6583+
t.Fatalf("expected success via process override; err=%v isError=%v text=%q", err, res.IsError, callResultText(t, res))
6584+
}
6585+
6586+
obs, err := s.RecentObservations("override-project", "project", 5)
6587+
if err != nil {
6588+
t.Fatalf("RecentObservations: %v", err)
6589+
}
6590+
if len(obs) == 0 {
6591+
t.Fatal("expected session_summary under 'override-project'; got none")
6592+
}
6593+
}
6594+
6595+
// ─── #393: handleSessionSummary empty-content guard tests ────────────────────
6596+
6597+
// TestSessionSummary_EmptyContentRejected verifies that an empty content string
6598+
// is rejected before AddObservation is called, mirroring the guard in handleSave.
6599+
func TestSessionSummary_EmptyContentRejected(t *testing.T) {
6600+
dir := t.TempDir()
6601+
initTestGitRepo(t, dir)
6602+
t.Chdir(dir)
6603+
6604+
s := newMCPTestStore(t)
6605+
h := handleSessionSummary(s, MCPConfig{}, NewSessionActivity(10*time.Minute))
6606+
6607+
res, err := h(context.Background(), mcppkg.CallToolRequest{Params: mcppkg.CallToolParams{Arguments: map[string]any{
6608+
"content": "",
6609+
}}})
6610+
if err != nil {
6611+
t.Fatalf("handler returned Go error: %v", err)
6612+
}
6613+
if !res.IsError {
6614+
t.Fatal("expected tool error for empty content; got success")
6615+
}
6616+
text := callResultText(t, res)
6617+
if !strings.Contains(text, "content") {
6618+
t.Errorf("error message should mention 'content'; got: %q", text)
6619+
}
6620+
6621+
// No observation must have been persisted.
6622+
obs, err := s.RecentObservations("", "project", 10)
6623+
if err != nil {
6624+
t.Fatalf("RecentObservations: %v", err)
6625+
}
6626+
if len(obs) != 0 {
6627+
t.Fatalf("expected 0 observations after empty-content rejection; got %d", len(obs))
6628+
}
6629+
}
6630+
6631+
// TestSessionSummary_WhitespaceOnlyContentRejected verifies that whitespace-only
6632+
// content is also rejected (mirrors handleSave behaviour).
6633+
func TestSessionSummary_WhitespaceOnlyContentRejected(t *testing.T) {
6634+
dir := t.TempDir()
6635+
initTestGitRepo(t, dir)
6636+
t.Chdir(dir)
6637+
6638+
s := newMCPTestStore(t)
6639+
h := handleSessionSummary(s, MCPConfig{}, NewSessionActivity(10*time.Minute))
6640+
6641+
res, err := h(context.Background(), mcppkg.CallToolRequest{Params: mcppkg.CallToolParams{Arguments: map[string]any{
6642+
"content": " \n\t ",
6643+
}}})
6644+
if err != nil {
6645+
t.Fatalf("handler returned Go error: %v", err)
6646+
}
6647+
if !res.IsError {
6648+
t.Fatal("expected tool error for whitespace-only content; got success")
6649+
}
6650+
}
6651+
6652+
// ─── #408/#346: cross-project search and timezone tests ──────────────────────
6653+
65236654
// seedCrossProjectMemories inserts one observation per project so cross-project
65246655
// search tests have something to find. Returns the session IDs created.
65256656
func seedCrossProjectMemories(t *testing.T, s *store.Store) {
@@ -6647,4 +6778,3 @@ func TestHandleSearchWithoutAllProjectsStillScopesToCurrentProject(t *testing.T)
66476778
t.Fatalf("beta result should not leak into a scoped search; got: %s", text)
66486779
}
66496780
}
6650-

0 commit comments

Comments
 (0)