diff --git a/CHANGELOG.md b/CHANGELOG.md index 34eff70f8..5842a5db7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -52,6 +52,7 @@ Releases are cut by hand from a release PR; see [docs/RELEASING.md](docs/RELEASI - **Dead shell-injection surface removed**: `AssumptionTracker.VerifyCommandSucceeds` ran caller-supplied strings through `sh -c`, bypassing the permission stack; it had zero callers and is deleted. `SelfHealer.RunScript` no longer shell-evaluates the script path (double evaluation) and invokes it directly via `/bin/sh`. ### Fixed +- Simple greetings stay conversational without listing files or reading project instructions, including after earlier tool use; real requests such as "Hi fix bug" keep their tools. - Batch tool execution now runs post-processing, redaction, and completion hooks and emits one final result per call. - Plan and Review modes enforce read-only execution even after tool discovery. Loading a local skill uses its read-only permission policy. - Installed plugin MCP servers and skills are discovered through the normal trust boundary; MCP child processes outlive the handshake timeout. diff --git a/internal/bridge/sessioncapture/terminal_context.go b/internal/bridge/sessioncapture/terminal_context.go index ac57534c5..0a9af2280 100644 --- a/internal/bridge/sessioncapture/terminal_context.go +++ b/internal/bridge/sessioncapture/terminal_context.go @@ -11,6 +11,8 @@ import ( "strings" "sync" "time" + + "github.com/GrayCodeAI/reflect/internal/prompt" ) // TerminalContext captures and tracks terminal state changes between queries. @@ -65,6 +67,12 @@ func (tc *TerminalContext) MarkExitCode(code int) { // BuildContext returns a delta-based context string and resets tracking state. func (tc *TerminalContext) BuildContext(query string) string { + // Keep greetings as the user's actual message. Metadata would turn "Hi" + // into a workspace-shaped request and defeat the engine's no-tool path. + // Preserve pending deltas for the next real task instead of consuming them. + if prompt.IsSmallTalk(query) { + return query + } tc.mu.Lock() defer tc.mu.Unlock() diff --git a/internal/bridge/sessioncapture/terminal_context_test.go b/internal/bridge/sessioncapture/terminal_context_test.go index da45f67b6..3c0f82bbb 100644 --- a/internal/bridge/sessioncapture/terminal_context_test.go +++ b/internal/bridge/sessioncapture/terminal_context_test.go @@ -5,15 +5,33 @@ import ( "testing" ) -func TestTerminalContext_BuildContext_Empty(t *testing.T) { +func TestTerminalContext_BuildContext_FirstTask(t *testing.T) { tc := NewTerminalContext() - got := tc.BuildContext("hello") + got := tc.BuildContext("list files") // First call always includes CWD since lastCWD is empty - if got == "hello" { + if got == "list files" { t.Error("expected CWD to be included on first call") } } +func TestTerminalContext_GreetingPreservesPendingContext(t *testing.T) { + tc := NewTerminalContext() + tc.captureCmd = "" + tc.MarkCommand("go test ./...") + tc.MarkExitCode(1) + for _, message := range []string{"Hi", "Hello!", "thanks", "who are you?"} { + if got := tc.BuildContext(message); got != message { + t.Fatalf("greeting gained terminal context: %q", got) + } + } + got := tc.BuildContext("why did the tests fail?") + for _, want := range []string{"[cwd:", "[recent: go test ./...]", "[exit: 1]"} { + if !contains(got, want) { + t.Fatalf("greeting consumed pending task context %q: %s", want, got) + } + } +} + func TestTerminalContext_BuildContext_Delta(t *testing.T) { tc := NewTerminalContext() tc.captureCmd = "" // disable terminal capture for test diff --git a/internal/engine/stream.go b/internal/engine/stream.go index 04bbfaf91..c7d15cd5c 100644 --- a/internal/engine/stream.go +++ b/internal/engine/stream.go @@ -56,10 +56,11 @@ func (s *Session) buildTurnOptions(tc turnContext) types.ChatOptions { baseSystem := s.Persistence().System() if smallTalk { - // The identity preamble already coaches the model to answer - // greetings without tools — the role/tool/practice sections - // below it only add prefill cost on this turn. - baseSystem = prompt.System() + // Keep this path free of workspace/memory/workflow nudges as well as + // schemas. Those instructions can turn a greeting into repository work. + opts := s.ChatLLM().BuildOptions(prompt.System(), activeModel, maxTok, nil) + opts.ToolChoice = &types.ToolChoiceOption{Type: "none"} + return opts } baseOpts := s.ChatLLM().BuildOptions(baseSystem, activeModel, maxTok, nil) opts := baseOpts @@ -130,9 +131,7 @@ func (s *Session) buildTurnOptions(tc turnContext) types.ChatOptions { if lastUserMsg != "" { s.Tools().Registry().PromoteForIntent(lastUserMsg) } - if !smallTalk { - opts.Tools = s.Tools().Registry().RelayTools() - } + opts.Tools = s.Tools().Registry().RelayTools() } return opts } @@ -404,7 +403,7 @@ func (s *Session) agentLoop(ctx context.Context, ch chan<- StreamEvent) { memoryDone: // Payload tiering: classify the latest user request once per turn. - // Early conversational turns get a minimal system prompt and no tool + // Conversational turns get a minimal system prompt and no tool // schemas; anything resembling work keeps the full prompt and the // promoted tool surface. This keeps simple turns cheap on slow // (local/remote) models without starving real requests of tools. @@ -416,7 +415,7 @@ func (s *Session) agentLoop(ctx context.Context, ch chan<- StreamEvent) { break } } - smallTalk := lastUserMsg != "" && isSmallTalkPrompt(lastUserMsg) && !sessionHasToolUse(s.Persistence().RawMessages()) + smallTalk := isSmallTalkPrompt(lastUserMsg) // Assemble the per-turn ChatOptions (system prompt tiering, ephemeral // injections, promoted tools) via the extracted helper so agentLoop @@ -726,6 +725,17 @@ func (s *Session) agentLoop(ctx context.Context, ch chan<- StreamEvent) { toolCalls = append(toolCalls, inlineCalls...) } } + // Tool-free turns are authoritative even if a provider emits unsolicited + // tool calls. Do not inspect the repository or render tool activity just + // because a model ignored tool_choice=none. + if smallTalk && len(toolCalls) > 0 { + toolCalls = nil + if strings.TrimSpace(textContent.String()) == "" { + reply := "I'm Reflect, developed by GrayCodeAI. How can I help?" + textContent.WriteString(reply) + emit(StreamEvent{Type: "content", Content: reply}) + } + } // Cap tool calls per step const maxToolCallsPerStep = 32 @@ -1053,59 +1063,10 @@ func (s *Session) agentLoop(ctx context.Context, ch chan<- StreamEvent) { } } -// isSmallTalkPrompt reports whether prompt is a pure conversational -// exchange (greeting, pleasantry, identity question) that needs neither the -// full system prompt nor any tool schemas. The system prompt instructs the -// model to answer these directly, so sending the tool surface would only -// add prompt-prefill cost. -// isSmallTalkPrompt detects short greetings/closings so the payload can be -// tiered down. The match must be conservative: this function decides whether -// to SKIP the full prompt context, so a false positive (dropping context for a -// real request like "hi there, who fixes this bug?") is far costlier than a -// false negative (sending a slightly bigger payload for genuine small talk). -// -// It returns true when the whole prompt is small talk: an exact match against -// a closed phrase list, or a phrase followed only by short filler (no comma or -// clause that introduces a real request). -func isSmallTalkPrompt(prompt string) bool { - text := strings.ToLower(strings.TrimSpace(prompt)) - text = strings.Trim(text, " \t\r\n.,!?;:") - for _, phrase := range smallTalkPhrases { - if text == phrase { - return true - } - // Allow a trailing filler word (e.g. "hi there") but stop at a comma or - // anything that looks like a real request clause. - if strings.HasPrefix(text, phrase+" ") { - remainder := text[len(phrase)+1:] - if !strings.ContainsAny(remainder, ",;:?") && len(remainder) <= 12 { - return true - } - } - } - return false -} - -var smallTalkPhrases = []string{ - "hi", "hi there", "hello", "hey", "hey there", - "how are you", "how are you doing", "how's it going", "how's it going today", - "what's up", "who are you", "what can you do", - "thanks", "thank you", "thanks a lot", - "good morning", "good afternoon", "good evening", "nice to meet you", - "goodbye", "bye", -} - -// sessionHasToolUse reports whether any message in the conversation already -// executed a tool. Once tools are in play, later turns keep the full prompt -// and tool surface even if they read like small talk ("thanks"), so the -// follow-up context is not lost. -func sessionHasToolUse(msgs []types.RelayMessage) bool { - for _, m := range msgs { - if len(m.ToolResults) > 0 { - return true - } - } - return false +// isSmallTalkPrompt shares the same conservative classification as the +// terminal-context boundary, before environment metadata is added. +func isSmallTalkPrompt(message string) bool { + return prompt.IsSmallTalk(message) } // extractDataURI extracts the first base64 data URI from a string. diff --git a/internal/engine/stream_prompt_test.go b/internal/engine/stream_prompt_test.go index f4c6e6b5c..74fe48610 100644 --- a/internal/engine/stream_prompt_test.go +++ b/internal/engine/stream_prompt_test.go @@ -1,8 +1,15 @@ package engine import ( + "context" + "strings" + "sync/atomic" "testing" + "time" + "github.com/GrayCodeAI/reflect/internal/bridge/sessioncapture" + "github.com/GrayCodeAI/reflect/internal/engine/safety" + "github.com/GrayCodeAI/reflect/internal/tool" "github.com/GrayCodeAI/reflect/internal/types" ) @@ -18,6 +25,8 @@ func TestIsSmallTalkPrompt(t *testing.T) { {prompt: "good morning", skip: true}, {prompt: "thanks", skip: true}, {prompt: "Hi, inspect this repository", skip: false}, + {prompt: "Hi fix bug", skip: false}, + {prompt: "hi run ls", skip: false}, {prompt: "run the tests", skip: false}, {prompt: "hello there, who fixes this bug?", skip: false}, } @@ -30,17 +39,84 @@ func TestIsSmallTalkPrompt(t *testing.T) { } } -func TestSessionHasToolUse(t *testing.T) { - plain := []types.RelayMessage{{Role: "user", Content: "hi"}} - used := []types.RelayMessage{ - {Role: "user", Content: "read stream.go"}, - {Role: "assistant", ToolUse: []types.ToolCall{{Name: "Read"}}}, - {Role: "user", Content: "thanks", ToolResults: []types.ToolResult{{}}}, - } - if sessionHasToolUse(plain) { - t.Fatal("sessionHasToolUse(plain) = true, want false") +type conversationalStreamClient struct { + *resilienceBoundaryClient + options []types.ChatOptions +} + +func (c *conversationalStreamClient) StreamChatContinue(ctx context.Context, messages []types.RelayMessage, opts types.ChatOptions, cfg types.ContinuationConfig) (*types.StreamResult, error) { + c.options = append(c.options, opts) + return c.resilienceBoundaryClient.StreamChatContinue(ctx, messages, opts, cfg) +} + +func TestConversationStaysToolFreeAfterTerminalContextAndHistory(t *testing.T) { + for _, tc := range []struct { + name string + history bool + content string + }{ + {name: "fresh", content: "Hi! What would you like to work on?"}, + {name: "after tool use", history: true, content: "Hi! What would you like to work on?"}, + {name: "provider returned only tools"}, + } { + t.Run(tc.name, func(t *testing.T) { + read := &countedReadTool{} + client := &conversationalStreamClient{resilienceBoundaryClient: &resilienceBoundaryClient{streams: [][]types.RelayStreamEvent{{ + {Type: "content", Content: tc.content}, + // A provider may ignore tool_choice=none. The host must still + // avoid repository inspection and tool activity in the UI. + {Type: "tool_call", ToolCall: &types.ToolCall{ID: "unexpected", Name: "Read", Arguments: map[string]interface{}{"id": 1}}}, + {Type: "done", StopReason: "tool_use"}, + }}}} + s := NewSessionWithClient(client, "test", "test-model", "WORKSPACE_INSTRUCTIONS", tool.NewRegistry(read), false) + s.PermSvc().SetAutonomy(safety.AutonomyYOLO) + if tc.history { + s.AddUser("read a file") + s.Persistence().AppendAssistantJournaled(types.RelayMessage{Role: "assistant", ToolUse: []types.ToolCall{{ID: "prior", Name: "Read"}}}) + s.Persistence().AppendUserJournaled(types.RelayMessage{Role: "user", ToolResults: []types.ToolResult{{ToolUseID: "prior", Content: "file contents"}}}) + s.AddAssistant("Read the file.") + } + s.AddUser(sessioncapture.NewTerminalContext().BuildContext("Hi")) + ctx, cancel := context.WithTimeout(t.Context(), 5*time.Second) + defer cancel() + events, err := s.Stream(ctx) + if err != nil { + t.Fatal(err) + } + var reply strings.Builder + for event := range events { + switch event.Type { + case "tool_use", "tool_result", "error": + t.Errorf("greeting emitted %s: %s", event.Type, event.Content) + case "content": + reply.WriteString(event.Content) + } + } + if got := atomic.LoadInt32(&read.max); got != 0 { + t.Fatalf("greeting executed a tool: %d", got) + } + wantReply := tc.content + if wantReply == "" { + wantReply = "I'm Reflect, developed by GrayCodeAI. How can I help?" + } + if reply.String() != wantReply || len(client.options) != 1 { + t.Fatalf("unexpected greeting: %q, requests=%d", reply.String(), len(client.options)) + } + opts := client.options[0] + if len(opts.Tools) != 0 || opts.ToolChoice == nil || opts.ToolChoice.Type != "none" || strings.Contains(opts.System, "WORKSPACE_INSTRUCTIONS") { + t.Fatalf("greeting exposed project tools or instructions: %+v", opts) + } + }) } - if !sessionHasToolUse(used) { - t.Fatal("sessionHasToolUse(used) = false, want true") +} + +func TestWorkFollowingGreetingKeepsTools(t *testing.T) { + s := newMockSession(newMockClient()) + s.Tools().registry = tool.NewRegistry(&countedReadTool{}) + for _, text := range []string{"Hi fix bug", "hi run ls", "read main.go"} { + opts := s.buildTurnOptions(turnContext{ctx: t.Context(), activeModel: "test-model", maxTok: 256, lastUserMsg: text, smallTalk: isSmallTalkPrompt(text)}) + if len(opts.Tools) == 0 || opts.ToolChoice != nil && opts.ToolChoice.Type == "none" { + t.Fatalf("work lost tools: %q", text) + } } } diff --git a/internal/prompt/conversation.go b/internal/prompt/conversation.go new file mode 100644 index 000000000..baad95571 --- /dev/null +++ b/internal/prompt/conversation.go @@ -0,0 +1,22 @@ +package prompt + +import "strings" + +// IsSmallTalk recognizes complete conversational messages that need no tools +// or workspace context. Match whole phrases so "hi, fix this" remains a task. +func IsSmallTalk(message string) bool { + text := strings.ToLower(strings.TrimSpace(message)) + text = strings.Trim(text, " \t\r\n.,!?;:") + text = strings.Join(strings.Fields(text), " ") + switch text { + case "hi", "hi there", "hi reflect", "hello", "hello there", "hello reflect", "hey", "hey there", "hey reflect", + "how are you", "how are you doing", "how are you doing today", "how's it going", "how's it going today", + "what's up", "who are you", "what can you do", + "thanks", "thank you", "thanks a lot", "thanks for that", + "good morning", "good afternoon", "good evening", "nice to meet you", + "goodbye", "bye": + return true + default: + return false + } +} diff --git a/internal/prompt/conversation_test.go b/internal/prompt/conversation_test.go new file mode 100644 index 000000000..245e356e0 --- /dev/null +++ b/internal/prompt/conversation_test.go @@ -0,0 +1,16 @@ +package prompt + +import "testing" + +func TestIsSmallTalk(t *testing.T) { + for _, message := range []string{"Hi", " Hello! ", "hey there", "hi reflect", "WHO ARE YOU?", "what can you do?", "thanks a lot", "good morning", "bye"} { + if !IsSmallTalk(message) { + t.Errorf("conversation was classified as work: %q", message) + } + } + for _, message := range []string{"", "Hi fix bug", "hi run ls", "hey read x", "thanks do it", "hi, inspect this repository", "hello there, who fixes this bug?", "how are you handling retries?", "what can you do about this error?", "hi.js"} { + if IsSmallTalk(message) { + t.Errorf("work request was classified as conversation: %q", message) + } + } +}