diff --git a/coderd/x/chatd/chatd.go b/coderd/x/chatd/chatd.go index 454d8eb3a6..f17d7e8c9f 100644 --- a/coderd/x/chatd/chatd.go +++ b/coderd/x/chatd/chatd.go @@ -56,6 +56,7 @@ const ( DefaultInFlightChatStaleAfter = 5 * time.Minute homeInstructionLookupTimeout = 5 * time.Second + planPathLookupTimeout = 5 * time.Second instructionCacheTTL = 5 * time.Minute workspaceDialValidationDelay = 5 * time.Second workspaceMCPDiscoveryTimeout = 5 * time.Second @@ -4475,6 +4476,50 @@ func (p *Server) runChat( } defer workspaceCtx.close() + planPathFn := func(ctx context.Context) (string, string, error) { + conn, err := workspaceCtx.getWorkspaceConn(ctx) + if err != nil { + return "", "", err + } + home, err := chattool.ResolveWorkspaceHome(ctx, conn) + if err != nil { + return "", "", err + } + return chattool.PlanPathForChat(home, chat.ID), home, nil + } + resolvePlanPathForTools := func(ctx context.Context) (string, string, error) { + ctx, cancel := context.WithTimeout(ctx, planPathLookupTimeout) + defer cancel() + return planPathFn(ctx) + } + resolvePlanPathBlock := func(resolveCtx context.Context) string { + if chat.ParentChatID.Valid { + return "" + } + + planCtx, cancel := context.WithTimeout(resolveCtx, planPathLookupTimeout) + defer cancel() + + if _, _, err := workspaceCtx.workspaceAgentIDForConn(planCtx); err != nil { + p.logger.Debug(resolveCtx, "plan path instruction: agent not reachable", + slog.Error(err), + slog.F("chat_id", chat.ID), + ) + return "" + } + + planPath, home, err := planPathFn(planCtx) + if err != nil { + p.logger.Debug(resolveCtx, "plan path instruction: failed to resolve plan path", + slog.Error(err), + slog.F("chat_id", chat.ID), + ) + return "" + } + + return formatPlanPathBlock(planPath, home) + } + // Connect to MCP servers in parallel with instruction // resolution. ConnectAll only depends on mcpConfigs and // mcpTokens which are available after g.Wait() above. @@ -4668,6 +4713,7 @@ func (p *Server) runChat( if instruction != "" { prompt = chatprompt.InsertSystem(prompt, instruction) } + prompt = renderPlanPathPrompt(prompt, resolvePlanPathBlock(ctx)) if skillIndex := chattool.FormatSkillIndex(skills); skillIndex != "" { prompt = chatprompt.InsertSystem(prompt, skillIndex) } @@ -4980,9 +5026,11 @@ func (p *Server) runChat( }), chattool.WriteFile(chattool.WriteFileOptions{ GetWorkspaceConn: workspaceCtx.getWorkspaceConn, + ResolvePlanPath: resolvePlanPathForTools, }), chattool.EditFiles(chattool.EditFilesOptions{ GetWorkspaceConn: workspaceCtx.getWorkspaceConn, + ResolvePlanPath: resolvePlanPathForTools, }), chattool.Execute(chattool.ExecuteOptions{ GetWorkspaceConn: workspaceCtx.getWorkspaceConn, @@ -5049,6 +5097,7 @@ func (p *Server) runChat( // Plan presentation tool. tools = append(tools, chattool.ProposePlan(chattool.ProposePlanOptions{ GetWorkspaceConn: workspaceCtx.getWorkspaceConn, + ResolvePlanPath: resolvePlanPathForTools, StoreFile: func(ctx context.Context, name string, mediaType string, data []byte) (uuid.UUID, error) { workspaceCtx.chatStateMu.Lock() chatSnapshot := *workspaceCtx.currentChat @@ -5241,6 +5290,7 @@ func (p *Server) runChat( if instruction != "" { reloadedPrompt = chatprompt.InsertSystem(reloadedPrompt, instruction) } + reloadedPrompt = renderPlanPathPrompt(reloadedPrompt, resolvePlanPathBlock(reloadCtx)) if skillIndex := chattool.FormatSkillIndex(skills); skillIndex != "" { reloadedPrompt = chatprompt.InsertSystem(reloadedPrompt, skillIndex) } @@ -5912,6 +5962,89 @@ func (p *Server) resolveUserPrompt(ctx context.Context, userID uuid.UUID) string return "\n" + trimmed + "\n" } +// renderPlanPathPrompt fills the plan-path placeholder when it is +// present in the prompt. +func renderPlanPathPrompt(prompt []fantasy.Message, planPathBlock string) []fantasy.Message { + prompt, _ = replacePlanPathPlaceholder(prompt, planPathBlock) + return prompt +} + +func replacePlanPathPlaceholder( + prompt []fantasy.Message, + planPathBlock string, +) ([]fantasy.Message, bool) { + var updatedPrompt []fantasy.Message + replaced := false + for i, message := range prompt { + updatedMessage, ok := replacePlanPathPlaceholderInMessage(message, planPathBlock) + if !ok { + continue + } + if updatedPrompt == nil { + updatedPrompt = slices.Clone(prompt) + } + updatedPrompt[i] = updatedMessage + replaced = true + } + if !replaced { + return prompt, false + } + return updatedPrompt, true +} + +func replacePlanPathPlaceholderInMessage( + message fantasy.Message, + planPathBlock string, +) (fantasy.Message, bool) { + if message.Role != fantasy.MessageRoleSystem { + return message, false + } + + content := slices.Clone(message.Content) + replaced := false + for i, part := range content { + textPart, ok := fantasy.AsMessagePart[fantasy.TextPart](part) + if !ok || !strings.Contains(textPart.Text, defaultSystemPromptPlanPathBlockPlaceholder) { + continue + } + replaced = true + content[i] = fantasy.TextPart{Text: strings.ReplaceAll( + textPart.Text, + defaultSystemPromptPlanPathBlockPlaceholder, + planPathBlock, + )} + } + if !replaced { + return message, false + } + message.Content = content + return message, true +} + +func formatPlanPathBlock(chatPath, home string) string { + chatPath = strings.TrimSpace(chatPath) + if chatPath == "" { + return "" + } + + avoidPlanPath := chattool.LegacySharedPlanPath + home = strings.TrimSpace(home) + if home != "" { + avoidPlanPath = strings.TrimRight(home, "/") + "/PLAN.md" + } + + var b strings.Builder + _, _ = b.WriteString("\n") + _, _ = b.WriteString("Your plan file path for this chat is: ") + _, _ = b.WriteString(chatPath) + _, _ = b.WriteString("\n") + _, _ = b.WriteString("Always use this exact path when creating or proposing plan files. Do not use ") + _, _ = b.WriteString(avoidPlanPath) + _, _ = b.WriteString(".\n") + _, _ = b.WriteString("") + return b.String() +} + func (p *Server) recoverStaleChats(ctx context.Context) { staleAfter := time.Now().Add(-p.inFlightChatStaleAfter) staleChats, err := p.db.GetStaleChats(ctx, staleAfter) diff --git a/coderd/x/chatd/chatd_test.go b/coderd/x/chatd/chatd_test.go index 6b31fa8ad7..483d4667d5 100644 --- a/coderd/x/chatd/chatd_test.go +++ b/coderd/x/chatd/chatd_test.go @@ -5738,10 +5738,38 @@ func TestAgentContextFilesAndSkillsLoadedIntoChat(t *testing.T) { require.Contains(t, allSystemContent, "AGENTS.md", "system prompt should reference the source file") + planBlockCount := 0 + standalonePlanBlockCount := 0 + for _, msg := range recordedCalls[0] { + if msg.Role != "system" { + continue + } + planBlockCount += strings.Count( + msg.Content, + "\nYour plan file path for this chat is:", + ) + trimmed := strings.TrimSpace(msg.Content) + if strings.HasPrefix(trimmed, "") && + strings.HasSuffix(trimmed, "") { + standalonePlanBlockCount++ + } + } + require.Contains(t, allSystemContent, "", "system prompt should contain available-skills block") require.Contains(t, allSystemContent, "my-cool-skill", "system prompt should list the discovered skill") require.Contains(t, allSystemContent, "A test skill", "system prompt should include the skill description") + require.Contains(t, allSystemContent, "", + "system prompt should contain the plan-file-path block") + require.Contains(t, allSystemContent, "PLAN-"+chat.ID.String()+".md", + "system prompt should use the chat-specific plan path") + require.Contains(t, allSystemContent, + "Do not use "+strings.TrimRight(fakeHome, "/")+"/PLAN.md.", + "system prompt should warn against the home-root plan path") + require.Equal(t, 1, planBlockCount, + "system prompt should contain a single plan-file-path block") + require.Zero(t, standalonePlanBlockCount, + "plan-file-path block should be part of the main system prompt, not a standalone message") } diff --git a/coderd/x/chatd/chattool/editfiles.go b/coderd/x/chatd/chattool/editfiles.go index 1d601efb53..100598e13d 100644 --- a/coderd/x/chatd/chattool/editfiles.go +++ b/coderd/x/chatd/chattool/editfiles.go @@ -2,6 +2,7 @@ package chattool import ( "context" + "strings" "charm.land/fantasy" @@ -10,6 +11,7 @@ import ( type EditFilesOptions struct { GetWorkspaceConn func(context.Context) (workspacesdk.AgentConn, error) + ResolvePlanPath func(context.Context) (chatPath string, home string, err error) } type EditFilesArgs struct { @@ -29,7 +31,7 @@ func EditFiles(options EditFilesOptions) fantasy.AgentTool { if err != nil { return fantasy.NewTextErrorResponse(err.Error()), nil } - return executeEditFilesTool(ctx, conn, args) + return executeEditFilesTool(ctx, conn, args, options.ResolvePlanPath) }, ) } @@ -38,11 +40,42 @@ func executeEditFilesTool( ctx context.Context, conn workspacesdk.AgentConn, args EditFilesArgs, + resolvePlanPath func(context.Context) (chatPath string, home string, err error), ) (fantasy.ToolResponse, error) { if len(args.Files) == 0 { return fantasy.NewTextErrorResponse("files is required"), nil } + var ( + chatPath string + home string + planPathErr error + planPathLoaded bool + ) + for i := range args.Files { + args.Files[i].Path = strings.TrimSpace(args.Files[i].Path) + file := args.Files[i] + + hasPlanFileName := looksLikePlanFileName(file.Path) + if hasPlanFileName && !isAbsolutePath(file.Path) { + return fantasy.NewTextErrorResponse( + "plan files must use absolute paths; use the chat-specific absolute plan path; no files in this batch were applied", + ), nil + } + if resolvePlanPath == nil || !hasPlanFileName { + continue + } + if !planPathLoaded { + chatPath, home, planPathErr = resolvePlanPath(ctx) + planPathLoaded = true + } + if resp, rejected := rejectSharedPlanPath(file.Path, home, chatPath, planPathErr); rejected { + return fantasy.NewTextErrorResponse( + resp.Content + "; no files in this batch were applied", + ), nil + } + } + if err := conn.EditFiles(ctx, workspacesdk.FileEditRequest{Files: args.Files}); err != nil { return fantasy.NewTextErrorResponse(err.Error()), nil } diff --git a/coderd/x/chatd/chattool/editfiles_test.go b/coderd/x/chatd/chattool/editfiles_test.go new file mode 100644 index 0000000000..d16be9588a --- /dev/null +++ b/coderd/x/chatd/chattool/editfiles_test.go @@ -0,0 +1,290 @@ +package chattool_test + +import ( + "context" + "testing" + + "charm.land/fantasy" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "go.uber.org/mock/gomock" + "golang.org/x/xerrors" + + "github.com/coder/coder/v2/coderd/x/chatd/chattool" + "github.com/coder/coder/v2/codersdk/workspacesdk" + "github.com/coder/coder/v2/codersdk/workspacesdk/agentconnmock" +) + +func TestEditFiles(t *testing.T) { + t.Parallel() + + t.Run("RejectsPlanPathsWhenResolvePlanPathIsConfigured", func(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + input string + expectedRejectedPath string + }{ + { + name: "SingleHomeRootPlanPath", + input: `{"files":[{"path":"/Users/dev/plan.md","edits":[{"search":"old","replace":"new"}]}]}`, + expectedRejectedPath: "/Users/dev/plan.md", + }, + { + name: "MultiFileBatchWithHomeRootPlanPath", + input: `{"files":[` + + `{"path":"/Users/dev/subdir/plan.md","edits":[{"search":"old","replace":"new"}]},` + + `{"path":"/Users/dev/plan.md","edits":[{"search":"old","replace":"new"}]}` + + `]}`, + expectedRejectedPath: "/Users/dev/plan.md", + }, + } + + for _, testCase := range tests { + t.Run(testCase.name, func(t *testing.T) { + t.Parallel() + ctrl := gomock.NewController(t) + mockConn := agentconnmock.NewMockAgentConn(ctrl) + resolvePlanPathCalls := 0 + tool := chattool.EditFiles(chattool.EditFilesOptions{ + GetWorkspaceConn: func(context.Context) (workspacesdk.AgentConn, error) { + return mockConn, nil + }, + ResolvePlanPath: func(context.Context) (string, string, error) { + resolvePlanPathCalls++ + return "/Users/dev/.coder/plans/PLAN-chat.md", "/Users/dev", nil + }, + }) + + resp, err := tool.Run(context.Background(), fantasy.ToolCall{ + ID: "call-1", + Name: "edit_files", + Input: testCase.input, + }) + require.NoError(t, err) + assert.True(t, resp.IsError) + assert.Equal(t, 1, resolvePlanPathCalls) + assert.Equal( + t, + editFilesBatchRejectedMessage(sharedPlanPathResolvedMessage( + testCase.expectedRejectedPath, + "/Users/dev/.coder/plans/PLAN-chat.md", + )), + resp.Content, + ) + }) + } + }) + + t.Run("RejectsSharedPlanPathWhenResolverFails", func(t *testing.T) { + t.Parallel() + ctrl := gomock.NewController(t) + mockConn := agentconnmock.NewMockAgentConn(ctrl) + tool := chattool.EditFiles(chattool.EditFilesOptions{ + GetWorkspaceConn: func(context.Context) (workspacesdk.AgentConn, error) { + return mockConn, nil + }, + ResolvePlanPath: func(context.Context) (string, string, error) { + return "", "", xerrors.New("workspace unavailable") + }, + }) + + resp, err := tool.Run(context.Background(), fantasy.ToolCall{ + ID: "call-1", + Name: "edit_files", + Input: `{"files":[{"path":"/home/coder/plan.md","edits":[{"search":"old","replace":"new"}]}]}`, + }) + require.NoError(t, err) + assert.True(t, resp.IsError) + assert.Equal(t, editFilesBatchRejectedMessage(planPathVerificationMessage("/home/coder/plan.md")), resp.Content) + }) + + t.Run("RejectsRelativePlanPathsWhenResolvePlanPathIsConfigured", func(t *testing.T) { + t.Parallel() + ctrl := gomock.NewController(t) + mockConn := agentconnmock.NewMockAgentConn(ctrl) + resolvePlanPathCalled := false + tool := chattool.EditFiles(chattool.EditFilesOptions{ + GetWorkspaceConn: func(context.Context) (workspacesdk.AgentConn, error) { + return mockConn, nil + }, + ResolvePlanPath: func(context.Context) (string, string, error) { + resolvePlanPathCalled = true + return "/home/coder/.coder/plans/PLAN-chat.md", "/home/coder", nil + }, + }) + + resp, err := tool.Run(context.Background(), fantasy.ToolCall{ + ID: "call-1", + Name: "edit_files", + Input: `{"files":[{"path":"plan.md","edits":[{"search":"old","replace":"new"}]}]}`, + }) + require.NoError(t, err) + assert.True(t, resp.IsError) + assert.False(t, resolvePlanPathCalled) + assert.Equal(t, editFilesBatchRejectedMessage(relativePlanPathMessage()), resp.Content) + }) + + t.Run("PerChatPlanPathIsAllowed", func(t *testing.T) { + t.Parallel() + ctrl := gomock.NewController(t) + mockConn := agentconnmock.NewMockAgentConn(ctrl) + chatPlanPath := "/home/coder/.coder/plans/PLAN-123e4567-e89b-12d3-a456-426614174000.md" + request := workspacesdk.FileEditRequest{Files: []workspacesdk.FileEdits{{ + Path: chatPlanPath, + Edits: []workspacesdk.FileEdit{{ + Search: "old", + Replace: "new", + }}, + }}} + mockConn.EXPECT().EditFiles(gomock.Any(), request).Return(nil) + + resolvePlanPathCalled := false + tool := chattool.EditFiles(chattool.EditFilesOptions{ + GetWorkspaceConn: func(context.Context) (workspacesdk.AgentConn, error) { + return mockConn, nil + }, + ResolvePlanPath: func(context.Context) (string, string, error) { + resolvePlanPathCalled = true + return chatPlanPath, "/home/coder", nil + }, + }) + + resp, err := tool.Run(context.Background(), fantasy.ToolCall{ + ID: "call-1", + Name: "edit_files", + Input: `{"files":[{"path":"` + chatPlanPath + `","edits":[{"search":"old","replace":"new"}]}]}`, + }) + require.NoError(t, err) + assert.False(t, resp.IsError) + assert.False(t, resolvePlanPathCalled) + }) + + t.Run("NestedPlanPathAllowedWhenResolverFails", func(t *testing.T) { + t.Parallel() + ctrl := gomock.NewController(t) + mockConn := agentconnmock.NewMockAgentConn(ctrl) + request := workspacesdk.FileEditRequest{Files: []workspacesdk.FileEdits{{ + Path: "/home/coder/myproject/plan.md", + Edits: []workspacesdk.FileEdit{{ + Search: "old", + Replace: "new", + }}, + }}} + mockConn.EXPECT().EditFiles(gomock.Any(), request).Return(nil) + + tool := chattool.EditFiles(chattool.EditFilesOptions{ + GetWorkspaceConn: func(context.Context) (workspacesdk.AgentConn, error) { + return mockConn, nil + }, + ResolvePlanPath: func(context.Context) (string, string, error) { + return "", "", xerrors.New("workspace unavailable") + }, + }) + + resp, err := tool.Run(context.Background(), fantasy.ToolCall{ + ID: "call-1", + Name: "edit_files", + Input: `{"files":[{"path":"/home/coder/myproject/plan.md","edits":[{"search":"old","replace":"new"}]}]}`, + }) + require.NoError(t, err) + assert.False(t, resp.IsError) + }) + + t.Run("NestedPlanPathUnderHomeIsAllowed", func(t *testing.T) { + t.Parallel() + ctrl := gomock.NewController(t) + mockConn := agentconnmock.NewMockAgentConn(ctrl) + request := workspacesdk.FileEditRequest{Files: []workspacesdk.FileEdits{{ + Path: "/home/coder/myproject/plan.md", + Edits: []workspacesdk.FileEdit{{ + Search: "old", + Replace: "new", + }}, + }}} + mockConn.EXPECT().EditFiles(gomock.Any(), request).Return(nil) + + planPathCalled := false + tool := chattool.EditFiles(chattool.EditFilesOptions{ + GetWorkspaceConn: func(context.Context) (workspacesdk.AgentConn, error) { + return mockConn, nil + }, + ResolvePlanPath: func(context.Context) (string, string, error) { + planPathCalled = true + return "/home/coder/.coder/plans/PLAN-chat.md", "/home/coder", nil + }, + }) + + resp, err := tool.Run(context.Background(), fantasy.ToolCall{ + ID: "call-1", + Name: "edit_files", + Input: `{"files":[{"path":"/home/coder/myproject/plan.md","edits":[{"search":"old","replace":"new"}]}]}`, + }) + require.NoError(t, err) + assert.False(t, resp.IsError) + assert.True(t, planPathCalled) + }) + + t.Run("AllowsNonSharedPath", func(t *testing.T) { + t.Parallel() + ctrl := gomock.NewController(t) + mockConn := agentconnmock.NewMockAgentConn(ctrl) + request := workspacesdk.FileEditRequest{Files: []workspacesdk.FileEdits{{ + Path: "/home/dev/my-plan.md", + Edits: []workspacesdk.FileEdit{{ + Search: "old", + Replace: "new", + }}, + }}} + mockConn.EXPECT().EditFiles(gomock.Any(), request).Return(nil) + + resolvePlanPathCalled := false + tool := chattool.EditFiles(chattool.EditFilesOptions{ + GetWorkspaceConn: func(context.Context) (workspacesdk.AgentConn, error) { + return mockConn, nil + }, + ResolvePlanPath: func(context.Context) (string, string, error) { + resolvePlanPathCalled = true + return "", "", xerrors.New("should not be called") + }, + }) + + resp, err := tool.Run(context.Background(), fantasy.ToolCall{ + ID: "call-1", + Name: "edit_files", + Input: `{"files":[{"path":"/home/dev/my-plan.md","edits":[{"search":"old","replace":"new"}]}]}`, + }) + require.NoError(t, err) + assert.False(t, resp.IsError) + assert.False(t, resolvePlanPathCalled) + }) + + t.Run("AllowsSharedPlanPathWhenResolvePlanPathIsNil", func(t *testing.T) { + t.Parallel() + ctrl := gomock.NewController(t) + mockConn := agentconnmock.NewMockAgentConn(ctrl) + request := workspacesdk.FileEditRequest{Files: []workspacesdk.FileEdits{{ + Path: chattool.LegacySharedPlanPath, + Edits: []workspacesdk.FileEdit{{ + Search: "old", + Replace: "new", + }}, + }}} + mockConn.EXPECT().EditFiles(gomock.Any(), request).Return(nil) + + tool := chattool.EditFiles(chattool.EditFilesOptions{ + GetWorkspaceConn: func(context.Context) (workspacesdk.AgentConn, error) { + return mockConn, nil + }, + }) + + resp, err := tool.Run(context.Background(), fantasy.ToolCall{ + ID: "call-1", + Name: "edit_files", + Input: `{"files":[{"path":"` + chattool.LegacySharedPlanPath + `","edits":[{"search":"old","replace":"new"}]}]}`, + }) + require.NoError(t, err) + assert.False(t, resp.IsError) + }) +} diff --git a/coderd/x/chatd/chattool/planpath.go b/coderd/x/chatd/chattool/planpath.go new file mode 100644 index 0000000000..964dde0d03 --- /dev/null +++ b/coderd/x/chatd/chattool/planpath.go @@ -0,0 +1,90 @@ +package chattool + +import ( + "context" + "path" + "strings" + + "github.com/google/uuid" + "golang.org/x/xerrors" + + "github.com/coder/coder/v2/codersdk/workspacesdk" +) + +const planFileNamePrefix = "PLAN-" + +// LegacySharedPlanPath is the original shared plan file path used by +// every chat in a workspace. +const LegacySharedPlanPath = "/home/coder/PLAN.md" + +// ResolveWorkspaceHome returns the workspace user's home directory. +func ResolveWorkspaceHome( + ctx context.Context, + conn workspacesdk.AgentConn, +) (string, error) { + if conn == nil { + return "", xerrors.New("workspace connection is required") + } + + resp, err := conn.LS(ctx, "", workspacesdk.LSRequest{ + Path: []string{}, + Relativity: workspacesdk.LSRelativityHome, + }) + if err != nil { + return "", xerrors.Errorf("resolve workspace home: %w", err) + } + + home := strings.TrimSpace(resp.AbsolutePathString) + if home == "" { + return "", xerrors.New("workspace home path is empty") + } + + return home, nil +} + +// PlanPathForChat returns the per-chat plan file path rooted in the +// workspace home directory. +func PlanPathForChat(home string, chatID uuid.UUID) string { + return path.Join( + home, + ".coder", + "plans", + planFileNamePrefix+chatID.String()+".md", + ) +} + +// chatd consumes agent-normalized POSIX paths. Workspace agents are +// expected to convert separators to forward slashes before these +// helpers run. + +// isAbsolutePath reports whether p is an absolute POSIX path. +func isAbsolutePath(p string) bool { + return path.IsAbs(p) +} + +// looksLikePlanFileName reports whether the base name of requestedPath +// is "plan.md" (case-insensitive), ignoring the directory component. +func looksLikePlanFileName(requestedPath string) bool { + cleaned := path.Clean(requestedPath) + return strings.EqualFold(path.Base(cleaned), "plan.md") +} + +// LooksLikeHomePlanFile reports whether requestedPath is a plan.md +// variant (case-insensitive) sitting directly in the workspace home +// directory. +// The filename is compared case-insensitively because LLM output varies. +func LooksLikeHomePlanFile(requestedPath, home string) bool { + normalized := path.Clean(requestedPath) + normalizedHome := path.Clean(home) + + return looksLikePlanFileName(normalized) && + strings.EqualFold(path.Dir(normalized), normalizedHome) +} + +// looksLikeLegacySharedPlanPath reports whether requestedPath +// matches the legacy shared plan path (case-insensitive). Used as a +// narrow fallback when the workspace home cannot be resolved. +func looksLikeLegacySharedPlanPath(requestedPath string) bool { + normalized := path.Clean(requestedPath) + return strings.EqualFold(normalized, LegacySharedPlanPath) +} diff --git a/coderd/x/chatd/chattool/planpath_internal_test.go b/coderd/x/chatd/chattool/planpath_internal_test.go new file mode 100644 index 0000000000..48f769dcd8 --- /dev/null +++ b/coderd/x/chatd/chattool/planpath_internal_test.go @@ -0,0 +1,132 @@ +package chattool + +import ( + "testing" + + "github.com/stretchr/testify/require" +) + +func TestIsAbsolutePath(t *testing.T) { + t.Parallel() + + tests := []struct { + path string + want bool + }{ + {"/home/coder/PLAN.md", true}, + {"/workspace/project/plan.md", true}, + {"plan.md", false}, + {"./plan.md", false}, + {"../plan.md", false}, + {"", false}, + } + + for _, tt := range tests { + t.Run(tt.path, func(t *testing.T) { + t.Parallel() + require.Equal(t, tt.want, isAbsolutePath(tt.path)) + }) + } +} + +func TestLooksLikePlanFileName(t *testing.T) { + t.Parallel() + + require.True(t, looksLikePlanFileName("plan.md")) + require.True(t, looksLikePlanFileName("./Plan.md")) + require.True(t, looksLikePlanFileName("/home/coder/PLAN.md")) + require.False(t, looksLikePlanFileName("/home/coder/README.md")) +} + +func TestLooksLikeLegacySharedPlanPath(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + requested string + want bool + }{ + { + name: "ExactMatch", + requested: "/home/coder/PLAN.md", + want: true, + }, + { + name: "CaseInsensitive", + requested: "/home/coder/plan.md", + want: true, + }, + { + name: "MixedCase", + requested: "/home/coder/Plan.md", + want: true, + }, + { + name: "NestedPath", + requested: "/home/coder/myproject/plan.md", + want: false, + }, + { + name: "DifferentHome", + requested: "/Users/dev/PLAN.md", + want: false, + }, + { + name: "PerChatPath", + requested: "/home/coder/.coder/plans/PLAN-123e4567-e89b-12d3-a456-426614174000.md", + want: false, + }, + { + name: "EmptyString", + requested: "", + want: false, + }, + } + + for _, testCase := range tests { + t.Run(testCase.name, func(t *testing.T) { + t.Parallel() + require.Equal(t, testCase.want, looksLikeLegacySharedPlanPath(testCase.requested)) + }) + } +} + +func TestRejectSharedPlanPath(t *testing.T) { + t.Parallel() + + resp, rejected := rejectSharedPlanPath( + LegacySharedPlanPath, + "/Users/dev", + "/Users/dev/.coder/plans/PLAN-chat.md", + nil, + ) + + require.True(t, rejected) + require.True(t, resp.IsError) + require.Equal( + t, + sharedPlanPathMessage( + LegacySharedPlanPath, + "/Users/dev/.coder/plans/PLAN-chat.md", + ), + resp.Content, + ) +} + +func TestSharedPlanPathMessage(t *testing.T) { + t.Parallel() + + require.Equal( + t, + "the plan path /home/coder/plan.md is no longer supported at the home root; use the chat-specific plan path: /home/coder/.coder/plans/PLAN-chat.md", + sharedPlanPathMessage( + "/home/coder/plan.md", + "/home/coder/.coder/plans/PLAN-chat.md", + ), + ) + require.Equal( + t, + "the plan path /home/coder/plan.md could not be verified because the workspace is currently unavailable to resolve the chat-specific plan path, try again shortly", + planPathVerificationMessage("/home/coder/plan.md"), + ) +} diff --git a/coderd/x/chatd/chattool/planpath_test.go b/coderd/x/chatd/chattool/planpath_test.go new file mode 100644 index 0000000000..3857dd0327 --- /dev/null +++ b/coderd/x/chatd/chattool/planpath_test.go @@ -0,0 +1,219 @@ +package chattool_test + +import ( + "context" + "strings" + "testing" + + "github.com/google/uuid" + "github.com/stretchr/testify/require" + "go.uber.org/mock/gomock" + "golang.org/x/xerrors" + + "github.com/coder/coder/v2/coderd/x/chatd/chattool" + "github.com/coder/coder/v2/codersdk/workspacesdk" + "github.com/coder/coder/v2/codersdk/workspacesdk/agentconnmock" +) + +func TestResolveWorkspaceHome(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + resp workspacesdk.LSResponse + lsErr error + want string + wantErr bool + errMatch string + }{ + { + name: "StandardLinuxHome", + resp: workspacesdk.LSResponse{AbsolutePathString: "/home/coder"}, + want: "/home/coder", + }, + { + name: "NonStandardHome", + resp: workspacesdk.LSResponse{AbsolutePathString: "/Users/dev"}, + want: "/Users/dev", + }, + { + name: "LSError", + lsErr: xerrors.New("list failed"), + wantErr: true, + errMatch: "list failed", + }, + { + name: "EmptyAbsolutePathString", + resp: workspacesdk.LSResponse{AbsolutePathString: ""}, + wantErr: true, + errMatch: "workspace home path is empty", + }, + { + name: "WhitespaceOnlyAbsolutePathString", + resp: workspacesdk.LSResponse{AbsolutePathString: " \t\n "}, + wantErr: true, + errMatch: "workspace home path is empty", + }, + } + + for _, testCase := range tests { + t.Run(testCase.name, func(t *testing.T) { + t.Parallel() + + ctrl := gomock.NewController(t) + conn := agentconnmock.NewMockAgentConn(ctrl) + + conn.EXPECT().LS( + gomock.Any(), + "", + workspacesdk.LSRequest{ + Path: []string{}, + Relativity: workspacesdk.LSRelativityHome, + }, + ).Return(testCase.resp, testCase.lsErr) + + got, err := chattool.ResolveWorkspaceHome(context.Background(), conn) + if testCase.wantErr { + require.Error(t, err) + require.ErrorContains(t, err, testCase.errMatch) + require.Empty(t, got) + return + } + + require.NoError(t, err) + require.Equal(t, testCase.want, got) + }) + } +} + +func TestPlanPathForChat(t *testing.T) { + t.Parallel() + + t.Run("StandardHome", func(t *testing.T) { + t.Parallel() + + chatID := uuid.MustParse("123e4567-e89b-12d3-a456-426614174000") + + got := chattool.PlanPathForChat("/home/coder", chatID) + + require.Equal( + t, + "/home/coder/.coder/plans/PLAN-123e4567-e89b-12d3-a456-426614174000.md", + got, + ) + }) + + t.Run("NonStandardHome", func(t *testing.T) { + t.Parallel() + + chatID := uuid.MustParse("aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee") + + got := chattool.PlanPathForChat("/Users/dev", chatID) + + require.Equal( + t, + "/Users/dev/.coder/plans/PLAN-aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee.md", + got, + ) + }) + + t.Run("MatchesExpectedFormat", func(t *testing.T) { + t.Parallel() + + home := "/workspace/home" + chatID := uuid.MustParse("f47ac10b-58cc-4372-a567-0e02b2c3d479") + + got := chattool.PlanPathForChat(home, chatID) + + require.True(t, strings.HasPrefix(got, home+"/.coder/plans/PLAN-")) + require.True(t, strings.HasSuffix(got, chatID.String()+".md")) + }) +} + +func TestLooksLikeHomePlanFile(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + requested string + home string + want bool + }{ + { + name: "UppercaseHomeRootPlan", + requested: "/home/coder/PLAN.md", + home: "/home/coder", + want: true, + }, + { + name: "LowercaseHomeRootPlan", + requested: "/home/coder/plan.md", + home: "/home/coder", + want: true, + }, + { + name: "MixedCaseHomeRootPlan", + requested: "/home/coder/Plan.md", + home: "/home/coder", + want: true, + }, + { + name: "UppercaseExtension", + requested: "/home/coder/PLAN.MD", + home: "/home/coder", + want: true, + }, + { + name: "CustomHomeRootPlan", + requested: "/Users/dev/plan.md", + home: "/Users/dev", + want: true, + }, + { + name: "NestedPlanUnderHome", + requested: "/home/coder/myproject/plan.md", + home: "/home/coder", + want: false, + }, + { + name: "PerChatPlanPath", + requested: "/home/coder/.coder/plans/PLAN-123e4567-e89b-12d3-a456-426614174000.md", + home: "/home/coder", + want: false, + }, + { + name: "DifferentFilename", + requested: "/home/coder/README.md", + home: "/home/coder", + want: false, + }, + { + name: "DifferentExtension", + requested: "/home/coder/plan.txt", + home: "/home/coder", + want: false, + }, + { + name: "EmptyPath", + requested: "", + home: "/home/coder", + want: false, + }, + { + name: "DifferentHomeMismatch", + requested: "/home/coder/plan.md", + home: "/Users/dev", + want: false, + }, + } + + for _, testCase := range tests { + t.Run(testCase.name, func(t *testing.T) { + t.Parallel() + + got := chattool.LooksLikeHomePlanFile(testCase.requested, testCase.home) + + require.Equal(t, testCase.want, got) + }) + } +} diff --git a/coderd/x/chatd/chattool/planpathmessage.go b/coderd/x/chatd/chattool/planpathmessage.go new file mode 100644 index 0000000000..a6845a82d2 --- /dev/null +++ b/coderd/x/chatd/chattool/planpathmessage.go @@ -0,0 +1,54 @@ +package chattool + +import ( + "fmt" + + "charm.land/fantasy" +) + +// rejectSharedPlanPath reports whether requestedPath targets the shared +// home-root plan file and, if so, returns a rejection response that +// points callers at the chat-specific plan path. +func rejectSharedPlanPath( + requestedPath string, + home string, + chatPath string, + planPathErr error, +) (fantasy.ToolResponse, bool) { + if planPathErr != nil { + // When the resolver fails, we cannot determine the actual + // home directory. Fall back to rejecting only the exact + // legacy shared path (case-insensitive) rather than every + // file named plan.md. + if !looksLikeLegacySharedPlanPath(requestedPath) { + return fantasy.ToolResponse{}, false + } + + return fantasy.NewTextErrorResponse( + planPathVerificationMessage(requestedPath), + ), true + } + + if !LooksLikeHomePlanFile(requestedPath, home) && !looksLikeLegacySharedPlanPath(requestedPath) { + return fantasy.ToolResponse{}, false + } + + return fantasy.NewTextErrorResponse( + sharedPlanPathMessage(requestedPath, chatPath), + ), true +} + +func sharedPlanPathMessage(requestedPath, chatPath string) string { + return fmt.Sprintf( + "the plan path %s is no longer supported at the home root; use the chat-specific plan path: %s", + requestedPath, + chatPath, + ) +} + +func planPathVerificationMessage(requestedPath string) string { + return fmt.Sprintf( + "the plan path %s could not be verified because the workspace is currently unavailable to resolve the chat-specific plan path, try again shortly", + requestedPath, + ) +} diff --git a/coderd/x/chatd/chattool/proposeplan.go b/coderd/x/chatd/chattool/proposeplan.go index 3704e53a43..bc46e7392c 100644 --- a/coderd/x/chatd/chattool/proposeplan.go +++ b/coderd/x/chatd/chattool/proposeplan.go @@ -17,6 +17,7 @@ const maxProposePlanSize = 32 * 1024 // 32 KiB // ProposePlanOptions configures the propose_plan tool. type ProposePlanOptions struct { GetWorkspaceConn func(context.Context) (workspacesdk.AgentConn, error) + ResolvePlanPath func(context.Context) (chatPath string, home string, err error) StoreFile func(ctx context.Context, name string, mediaType string, data []byte) (uuid.UUID, error) } @@ -31,8 +32,9 @@ func ProposePlan(options ProposePlanOptions) fantasy.AgentTool { return fantasy.NewAgentTool( "propose_plan", "Present a Markdown plan file from the workspace for user review. "+ - "The file must already exist with a .md extension — use write_file to create it or edit_files to refine it before calling this tool. "+ - "Pass the absolute file path (e.g. /home/coder/PLAN.md). The tool reads the content from the workspace.", + "The file must already exist with a .md extension. Use write_file to create it or edit_files to refine it before calling this tool. "+ + "Pass the absolute file path to the plan. Important: use the chat-specific absolute plan path, not a generic path like PLAN.md in the home directory. "+ + "The tool reads the content from the workspace.", func(ctx context.Context, args ProposePlanArgs, _ fantasy.ToolCall) (fantasy.ToolResponse, error) { if options.GetWorkspaceConn == nil { return fantasy.NewTextErrorResponse("workspace connection resolver is not configured"), nil @@ -44,7 +46,7 @@ func ProposePlan(options ProposePlanOptions) fantasy.AgentTool { if err != nil { return fantasy.NewTextErrorResponse(err.Error()), nil } - return executeProposePlanTool(ctx, conn, args, options.StoreFile) + return executeProposePlanTool(ctx, conn, args, options.ResolvePlanPath, options.StoreFile) }, ) } @@ -53,17 +55,32 @@ func executeProposePlanTool( ctx context.Context, conn workspacesdk.AgentConn, args ProposePlanArgs, + resolvePlanPath func(context.Context) (chatPath string, home string, err error), storeFile func(ctx context.Context, name string, mediaType string, data []byte) (uuid.UUID, error), ) (fantasy.ToolResponse, error) { - path := strings.TrimSpace(args.Path) - if path == "" { - return fantasy.NewTextErrorResponse("path is required (use an absolute path, e.g. /home/coder/PLAN.md)"), nil + requestedPath := strings.TrimSpace(args.Path) + if requestedPath == "" { + return fantasy.NewTextErrorResponse("path is required (use the chat-specific absolute plan path)"), nil } - if !strings.HasSuffix(path, ".md") { + if !strings.HasSuffix(requestedPath, ".md") { return fantasy.NewTextErrorResponse("path must end with .md"), nil } - rc, _, err := conn.ReadFile(ctx, path, 0, maxProposePlanSize+1) + hasPlanFileName := looksLikePlanFileName(requestedPath) + if hasPlanFileName && !isAbsolutePath(requestedPath) { + return fantasy.NewTextErrorResponse( + "plan files must use absolute paths; use the chat-specific absolute plan path", + ), nil + } + + if resolvePlanPath != nil && hasPlanFileName { + chatPath, home, err := resolvePlanPath(ctx) + if resp, rejected := rejectSharedPlanPath(requestedPath, home, chatPath, err); rejected { + return resp, nil + } + } + + rc, _, err := conn.ReadFile(ctx, requestedPath, 0, maxProposePlanSize+1) if err != nil { return fantasy.NewTextErrorResponse(err.Error()), nil } @@ -77,14 +94,14 @@ func executeProposePlanTool( return fantasy.NewTextErrorResponse("plan file exceeds 32 KiB size limit"), nil } - fileID, err := storeFile(ctx, filepath.Base(path), "text/markdown", data) + fileID, err := storeFile(ctx, filepath.Base(requestedPath), "text/markdown", data) if err != nil { return fantasy.NewTextErrorResponse("failed to store plan file: " + err.Error()), nil } return toolResponse(map[string]any{ "ok": true, - "path": path, + "path": requestedPath, "kind": "plan", "file_id": fileID.String(), "media_type": "text/markdown", diff --git a/coderd/x/chatd/chattool/proposeplan_test.go b/coderd/x/chatd/chattool/proposeplan_test.go index 1d8113d082..92b645818c 100644 --- a/coderd/x/chatd/chattool/proposeplan_test.go +++ b/coderd/x/chatd/chattool/proposeplan_test.go @@ -82,6 +82,34 @@ func TestProposePlan(t *testing.T) { assert.Contains(t, resp.Content, "path must end with .md") }) + t.Run("RelativePlanPathReturnsError", func(t *testing.T) { + t.Parallel() + ctrl := gomock.NewController(t) + mockConn := agentconnmock.NewMockAgentConn(ctrl) + + storeFile, _ := fakeStoreFile(t) + resolvePlanPathCalled := false + tool := newProposePlanToolWithPlanPath( + t, + mockConn, + storeFile, + func(context.Context) (string, string, error) { + resolvePlanPathCalled = true + return "/home/coder/.coder/plans/PLAN-chat.md", "/home/coder", nil + }, + ) + + resp, err := tool.Run(context.Background(), fantasy.ToolCall{ + ID: "call-1", + Name: "propose_plan", + Input: `{"path":"plan.md"}`, + }) + require.NoError(t, err) + assert.True(t, resp.IsError) + assert.False(t, resolvePlanPathCalled) + assert.Equal(t, relativePlanPathMessage(), resp.Content) + }) + t.Run("OversizedFileRejected", func(t *testing.T) { t.Parallel() ctrl := gomock.NewController(t) @@ -135,7 +163,16 @@ func TestProposePlan(t *testing.T) { Return(io.NopCloser(strings.NewReader("# Plan\n\nContent")), "text/markdown", nil) storeFile, stored := fakeStoreFile(t) - tool := newProposePlanTool(t, mockConn, storeFile) + planPathCalled := false + tool := newProposePlanToolWithPlanPath( + t, + mockConn, + storeFile, + func(context.Context) (string, string, error) { + planPathCalled = true + return "/home/coder/.coder/plans/PLAN-xxx.md", "/home/coder", nil + }, + ) resp, err := tool.Run(context.Background(), fantasy.ToolCall{ ID: "call-1", Name: "propose_plan", @@ -143,6 +180,7 @@ func TestProposePlan(t *testing.T) { }) require.NoError(t, err) assert.False(t, resp.IsError) + assert.True(t, planPathCalled) result := decodeProposePlanResponse(t, resp) assert.True(t, result.OK) @@ -154,6 +192,41 @@ func TestProposePlan(t *testing.T) { assert.NotContains(t, resp.Content, "content") }) + t.Run("NestedPlanPathUnderHomeIsAllowed", func(t *testing.T) { + t.Parallel() + ctrl := gomock.NewController(t) + mockConn := agentconnmock.NewMockAgentConn(ctrl) + + mockConn.EXPECT(). + ReadFile(gomock.Any(), "/home/coder/myproject/plan.md", int64(0), int64(32*1024+1)). + Return(io.NopCloser(strings.NewReader("# Nested Plan")), "text/markdown", nil) + + storeFile, stored := fakeStoreFile(t) + planPathCalled := false + tool := newProposePlanToolWithPlanPath( + t, + mockConn, + storeFile, + func(context.Context) (string, string, error) { + planPathCalled = true + return "/home/coder/.coder/plans/PLAN-chat.md", "/home/coder", nil + }, + ) + resp, err := tool.Run(context.Background(), fantasy.ToolCall{ + ID: "call-1", + Name: "propose_plan", + Input: `{"path":"/home/coder/myproject/plan.md"}`, + }) + require.NoError(t, err) + assert.False(t, resp.IsError) + assert.True(t, planPathCalled) + + result := decodeProposePlanResponse(t, resp) + assert.True(t, result.OK) + assert.Equal(t, "/home/coder/myproject/plan.md", result.Path) + assert.Equal(t, []byte("# Nested Plan"), *stored) + }) + t.Run("FileNotFound", func(t *testing.T) { t.Parallel() ctrl := gomock.NewController(t) @@ -218,6 +291,128 @@ func TestProposePlan(t *testing.T) { assert.Contains(t, resp.Content, "storage unavailable") }) + t.Run("RejectsSharedPlanPathWithResolvedPath", func(t *testing.T) { + t.Parallel() + ctrl := gomock.NewController(t) + mockConn := agentconnmock.NewMockAgentConn(ctrl) + + storeFile, _ := fakeStoreFile(t) + tool := newProposePlanToolWithPlanPath( + t, + mockConn, + storeFile, + func(context.Context) (string, string, error) { + return "/home/coder/.coder/plans/PLAN-chat.md", "/home/coder", nil + }, + ) + + resp, err := tool.Run(context.Background(), fantasy.ToolCall{ + ID: "call-1", + Name: "propose_plan", + Input: `{"path":"` + chattool.LegacySharedPlanPath + `"}`, + }) + require.NoError(t, err) + assert.True(t, resp.IsError) + assert.Equal( + t, + sharedPlanPathResolvedMessage(chattool.LegacySharedPlanPath, "/home/coder/.coder/plans/PLAN-chat.md"), + resp.Content, + ) + }) + + t.Run("RejectsSharedPlanPathWhenResolverFails", func(t *testing.T) { + t.Parallel() + ctrl := gomock.NewController(t) + mockConn := agentconnmock.NewMockAgentConn(ctrl) + + storeFile, _ := fakeStoreFile(t) + tool := newProposePlanToolWithPlanPath( + t, + mockConn, + storeFile, + func(context.Context) (string, string, error) { + return "", "", xerrors.New("workspace unavailable") + }, + ) + + resp, err := tool.Run(context.Background(), fantasy.ToolCall{ + ID: "call-1", + Name: "propose_plan", + Input: `{"path":"` + chattool.LegacySharedPlanPath + `"}`, + }) + require.NoError(t, err) + assert.True(t, resp.IsError) + assert.Equal(t, planPathVerificationMessage(chattool.LegacySharedPlanPath), resp.Content) + }) + + t.Run("PerChatPlanPathIsAllowed", func(t *testing.T) { + t.Parallel() + ctrl := gomock.NewController(t) + mockConn := agentconnmock.NewMockAgentConn(ctrl) + chatPlanPath := "/home/coder/.coder/plans/PLAN-123e4567-e89b-12d3-a456-426614174000.md" + + mockConn.EXPECT(). + ReadFile(gomock.Any(), chatPlanPath, int64(0), int64(32*1024+1)). + Return(io.NopCloser(strings.NewReader("# Per-Chat Plan")), "text/markdown", nil) + + storeFile, stored := fakeStoreFile(t) + resolvePlanPathCalled := false + tool := newProposePlanToolWithPlanPath( + t, + mockConn, + storeFile, + func(context.Context) (string, string, error) { + resolvePlanPathCalled = true + return chatPlanPath, "/home/coder", nil + }, + ) + resp, err := tool.Run(context.Background(), fantasy.ToolCall{ + ID: "call-1", + Name: "propose_plan", + Input: `{"path":"` + chatPlanPath + `"}`, + }) + require.NoError(t, err) + assert.False(t, resp.IsError) + assert.False(t, resolvePlanPathCalled) + + result := decodeProposePlanResponse(t, resp) + assert.True(t, result.OK) + assert.Equal(t, chatPlanPath, result.Path) + assert.Equal(t, []byte("# Per-Chat Plan"), *stored) + }) + + t.Run("NestedPlanPathAllowedWhenResolverFails", func(t *testing.T) { + t.Parallel() + ctrl := gomock.NewController(t) + mockConn := agentconnmock.NewMockAgentConn(ctrl) + + mockConn.EXPECT(). + ReadFile(gomock.Any(), "/home/coder/myproject/plan.md", int64(0), int64(32*1024+1)). + Return(io.NopCloser(strings.NewReader("# Nested Plan")), "text/markdown", nil) + + storeFile, stored := fakeStoreFile(t) + tool := newProposePlanToolWithPlanPath( + t, + mockConn, + storeFile, + func(context.Context) (string, string, error) { + return "", "", xerrors.New("workspace unavailable") + }, + ) + resp, err := tool.Run(context.Background(), fantasy.ToolCall{ + ID: "call-1", + Name: "propose_plan", + Input: `{"path":"/home/coder/myproject/plan.md"}`, + }) + require.NoError(t, err) + assert.False(t, resp.IsError) + + result := decodeProposePlanResponse(t, resp) + assert.True(t, result.OK) + assert.Equal(t, "/home/coder/myproject/plan.md", result.Path) + assert.Equal(t, []byte("# Nested Plan"), *stored) + }) + t.Run("WorkspaceConnectionError", func(t *testing.T) { t.Parallel() storeFile, _ := fakeStoreFile(t) @@ -278,16 +473,45 @@ func newProposePlanTool( t *testing.T, mockConn *agentconnmock.MockAgentConn, storeFile func(ctx context.Context, name string, mediaType string, data []byte) (uuid.UUID, error), +) fantasy.AgentTool { + t.Helper() + return newProposePlanToolWithPlanPath(t, mockConn, storeFile, nil) +} + +func newProposePlanToolWithPlanPath( + t *testing.T, + mockConn *agentconnmock.MockAgentConn, + storeFile func(ctx context.Context, name string, mediaType string, data []byte) (uuid.UUID, error), + resolvePlanPath func(context.Context) (string, string, error), ) fantasy.AgentTool { t.Helper() return chattool.ProposePlan(chattool.ProposePlanOptions{ GetWorkspaceConn: func(_ context.Context) (workspacesdk.AgentConn, error) { return mockConn, nil }, - StoreFile: storeFile, + ResolvePlanPath: resolvePlanPath, + StoreFile: storeFile, }) } +func sharedPlanPathResolvedMessage(requestedPath, planPath string) string { + return "the plan path " + requestedPath + + " is no longer supported at the home root; use the chat-specific plan path: " + planPath +} + +func planPathVerificationMessage(requestedPath string) string { + return "the plan path " + requestedPath + + " could not be verified because the workspace is currently unavailable to resolve the chat-specific plan path, try again shortly" +} + +func editFilesBatchRejectedMessage(message string) string { + return message + "; no files in this batch were applied" +} + +func relativePlanPathMessage() string { + return "plan files must use absolute paths; use the chat-specific absolute plan path" +} + func fakeStoreFile(t *testing.T) (func(ctx context.Context, name string, mediaType string, data []byte) (uuid.UUID, error), *[]byte) { t.Helper() diff --git a/coderd/x/chatd/chattool/writefile.go b/coderd/x/chatd/chattool/writefile.go index a9c372ca48..983b387209 100644 --- a/coderd/x/chatd/chattool/writefile.go +++ b/coderd/x/chatd/chattool/writefile.go @@ -11,6 +11,7 @@ import ( type WriteFileOptions struct { GetWorkspaceConn func(context.Context) (workspacesdk.AgentConn, error) + ResolvePlanPath func(context.Context) (chatPath string, home string, err error) } type WriteFileArgs struct { @@ -30,7 +31,7 @@ func WriteFile(options WriteFileOptions) fantasy.AgentTool { if err != nil { return fantasy.NewTextErrorResponse(err.Error()), nil } - return executeWriteFileTool(ctx, conn, args) + return executeWriteFileTool(ctx, conn, args, options.ResolvePlanPath) }, ) } @@ -39,12 +40,28 @@ func executeWriteFileTool( ctx context.Context, conn workspacesdk.AgentConn, args WriteFileArgs, + resolvePlanPath func(context.Context) (chatPath string, home string, err error), ) (fantasy.ToolResponse, error) { - if args.Path == "" { + requestedPath := strings.TrimSpace(args.Path) + if requestedPath == "" { return fantasy.NewTextErrorResponse("path is required"), nil } - if err := conn.WriteFile(ctx, args.Path, strings.NewReader(args.Content)); err != nil { + hasPlanFileName := looksLikePlanFileName(requestedPath) + if hasPlanFileName && !isAbsolutePath(requestedPath) { + return fantasy.NewTextErrorResponse( + "plan files must use absolute paths; use the chat-specific absolute plan path", + ), nil + } + + if resolvePlanPath != nil && hasPlanFileName { + chatPath, home, err := resolvePlanPath(ctx) + if resp, rejected := rejectSharedPlanPath(requestedPath, home, chatPath, err); rejected { + return resp, nil + } + } + + if err := conn.WriteFile(ctx, requestedPath, strings.NewReader(args.Content)); err != nil { return fantasy.NewTextErrorResponse(err.Error()), nil } return toolResponse(map[string]any{"ok": true}), nil diff --git a/coderd/x/chatd/chattool/writefile_test.go b/coderd/x/chatd/chattool/writefile_test.go new file mode 100644 index 0000000000..2fa563156e --- /dev/null +++ b/coderd/x/chatd/chattool/writefile_test.go @@ -0,0 +1,321 @@ +package chattool_test + +import ( + "context" + "io" + "strings" + "testing" + + "charm.land/fantasy" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "go.uber.org/mock/gomock" + "golang.org/x/xerrors" + + "github.com/coder/coder/v2/coderd/x/chatd/chattool" + "github.com/coder/coder/v2/codersdk/workspacesdk" + "github.com/coder/coder/v2/codersdk/workspacesdk/agentconnmock" +) + +func TestWriteFile(t *testing.T) { + t.Parallel() + + t.Run("RejectsHomeRootPlanVariantsWhenResolvePlanPathIsConfigured", func(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + requested string + home string + }{ + { + name: "ExactLegacyPath", + requested: chattool.LegacySharedPlanPath, + home: "/home/coder", + }, + { + name: "LowercasePlanAtHomeRoot", + requested: "/home/coder/plan.md", + home: "/home/coder", + }, + { + name: "MixedCasePlanAtHomeRoot", + requested: "/home/coder/Plan.md", + home: "/home/coder", + }, + } + + for _, testCase := range tests { + t.Run(testCase.name, func(t *testing.T) { + t.Parallel() + + ctrl := gomock.NewController(t) + mockConn := agentconnmock.NewMockAgentConn(ctrl) + tool := chattool.WriteFile(chattool.WriteFileOptions{ + GetWorkspaceConn: func(context.Context) (workspacesdk.AgentConn, error) { + return mockConn, nil + }, + ResolvePlanPath: func(context.Context) (string, string, error) { + return "/home/coder/.coder/plans/PLAN-chat.md", testCase.home, nil + }, + }) + + resp, err := tool.Run(context.Background(), fantasy.ToolCall{ + ID: "call-1", + Name: "write_file", + Input: `{"path":"` + testCase.requested + `","content":"# Plan"}`, + }) + require.NoError(t, err) + assert.True(t, resp.IsError) + assert.Equal( + t, + sharedPlanPathResolvedMessage( + testCase.requested, + "/home/coder/.coder/plans/PLAN-chat.md", + ), + resp.Content, + ) + }) + } + }) + + t.Run("RejectsRelativePlanPathsWhenResolvePlanPathIsConfigured", func(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + requested string + }{ + { + name: "PlainRelativePath", + requested: "plan.md", + }, + { + name: "DotSlashRelativePath", + requested: "./plan.md", + }, + } + + for _, testCase := range tests { + t.Run(testCase.name, func(t *testing.T) { + t.Parallel() + + ctrl := gomock.NewController(t) + mockConn := agentconnmock.NewMockAgentConn(ctrl) + resolvePlanPathCalled := false + tool := chattool.WriteFile(chattool.WriteFileOptions{ + GetWorkspaceConn: func(context.Context) (workspacesdk.AgentConn, error) { + return mockConn, nil + }, + ResolvePlanPath: func(context.Context) (string, string, error) { + resolvePlanPathCalled = true + return "/home/coder/.coder/plans/PLAN-chat.md", "/home/coder", nil + }, + }) + + resp, err := tool.Run(context.Background(), fantasy.ToolCall{ + ID: "call-1", + Name: "write_file", + Input: `{"path":"` + testCase.requested + `","content":"# Plan"}`, + }) + require.NoError(t, err) + assert.True(t, resp.IsError) + assert.False(t, resolvePlanPathCalled) + assert.Equal(t, relativePlanPathMessage(), resp.Content) + }) + } + }) + + t.Run("RejectsSharedPlanPathWhenResolverFails", func(t *testing.T) { + t.Parallel() + ctrl := gomock.NewController(t) + mockConn := agentconnmock.NewMockAgentConn(ctrl) + tool := chattool.WriteFile(chattool.WriteFileOptions{ + GetWorkspaceConn: func(context.Context) (workspacesdk.AgentConn, error) { + return mockConn, nil + }, + ResolvePlanPath: func(context.Context) (string, string, error) { + return "", "", xerrors.New("workspace unavailable") + }, + }) + + resp, err := tool.Run(context.Background(), fantasy.ToolCall{ + ID: "call-1", + Name: "write_file", + Input: `{"path":"/home/coder/plan.md","content":"# Plan"}`, + }) + require.NoError(t, err) + assert.True(t, resp.IsError) + assert.Equal(t, planPathVerificationMessage("/home/coder/plan.md"), resp.Content) + }) + + t.Run("PerChatPlanPathIsAllowed", func(t *testing.T) { + t.Parallel() + ctrl := gomock.NewController(t) + mockConn := agentconnmock.NewMockAgentConn(ctrl) + chatPlanPath := "/home/coder/.coder/plans/PLAN-123e4567-e89b-12d3-a456-426614174000.md" + mockConn.EXPECT(). + WriteFile(gomock.Any(), chatPlanPath, gomock.Any()). + DoAndReturn(func(_ context.Context, path string, reader io.Reader) error { + data, err := io.ReadAll(reader) + require.NoError(t, err) + require.Equal(t, chatPlanPath, path) + require.Equal(t, "# Plan", string(data)) + return nil + }) + + resolvePlanPathCalled := false + tool := chattool.WriteFile(chattool.WriteFileOptions{ + GetWorkspaceConn: func(context.Context) (workspacesdk.AgentConn, error) { + return mockConn, nil + }, + ResolvePlanPath: func(context.Context) (string, string, error) { + resolvePlanPathCalled = true + return chatPlanPath, "/home/coder", nil + }, + }) + + resp, err := tool.Run(context.Background(), fantasy.ToolCall{ + ID: "call-1", + Name: "write_file", + Input: `{"path":"` + chatPlanPath + `","content":"# Plan"}`, + }) + require.NoError(t, err) + assert.False(t, resp.IsError) + assert.False(t, resolvePlanPathCalled) + assert.Equal(t, `{"ok":true}`, strings.TrimSpace(resp.Content)) + }) + + t.Run("NestedPlanPathAllowedWhenResolverFails", func(t *testing.T) { + t.Parallel() + ctrl := gomock.NewController(t) + mockConn := agentconnmock.NewMockAgentConn(ctrl) + mockConn.EXPECT(). + WriteFile(gomock.Any(), "/home/coder/myproject/plan.md", gomock.Any()). + DoAndReturn(func(_ context.Context, path string, reader io.Reader) error { + data, err := io.ReadAll(reader) + require.NoError(t, err) + require.Equal(t, "/home/coder/myproject/plan.md", path) + require.Equal(t, "# Plan", string(data)) + return nil + }) + + tool := chattool.WriteFile(chattool.WriteFileOptions{ + GetWorkspaceConn: func(context.Context) (workspacesdk.AgentConn, error) { + return mockConn, nil + }, + ResolvePlanPath: func(context.Context) (string, string, error) { + return "", "", xerrors.New("workspace unavailable") + }, + }) + + resp, err := tool.Run(context.Background(), fantasy.ToolCall{ + ID: "call-1", + Name: "write_file", + Input: `{"path":"/home/coder/myproject/plan.md","content":"# Plan"}`, + }) + require.NoError(t, err) + assert.False(t, resp.IsError) + assert.Equal(t, `{"ok":true}`, strings.TrimSpace(resp.Content)) + }) + + t.Run("NestedPlanPathUnderHomeIsAllowed", func(t *testing.T) { + t.Parallel() + ctrl := gomock.NewController(t) + mockConn := agentconnmock.NewMockAgentConn(ctrl) + mockConn.EXPECT(). + WriteFile(gomock.Any(), "/home/coder/myproject/plan.md", gomock.Any()). + DoAndReturn(func(_ context.Context, path string, reader io.Reader) error { + data, err := io.ReadAll(reader) + require.NoError(t, err) + require.Equal(t, "/home/coder/myproject/plan.md", path) + require.Equal(t, "# Plan", string(data)) + return nil + }) + + planPathCalled := false + tool := chattool.WriteFile(chattool.WriteFileOptions{ + GetWorkspaceConn: func(context.Context) (workspacesdk.AgentConn, error) { + return mockConn, nil + }, + ResolvePlanPath: func(context.Context) (string, string, error) { + planPathCalled = true + return "/home/coder/.coder/plans/PLAN-chat.md", "/home/coder", nil + }, + }) + + resp, err := tool.Run(context.Background(), fantasy.ToolCall{ + ID: "call-1", + Name: "write_file", + Input: `{"path":"/home/coder/myproject/plan.md","content":"# Plan"}`, + }) + require.NoError(t, err) + assert.False(t, resp.IsError) + assert.True(t, planPathCalled) + assert.Equal(t, `{"ok":true}`, strings.TrimSpace(resp.Content)) + }) + + t.Run("AllowsNonSharedPath", func(t *testing.T) { + t.Parallel() + ctrl := gomock.NewController(t) + mockConn := agentconnmock.NewMockAgentConn(ctrl) + mockConn.EXPECT(). + WriteFile(gomock.Any(), "/home/dev/my-plan.md", gomock.Any()). + DoAndReturn(func(_ context.Context, path string, reader io.Reader) error { + data, err := io.ReadAll(reader) + require.NoError(t, err) + require.Equal(t, "/home/dev/my-plan.md", path) + require.Equal(t, "# Plan", string(data)) + return nil + }) + + resolvePlanPathCalled := false + tool := chattool.WriteFile(chattool.WriteFileOptions{ + GetWorkspaceConn: func(context.Context) (workspacesdk.AgentConn, error) { + return mockConn, nil + }, + ResolvePlanPath: func(context.Context) (string, string, error) { + resolvePlanPathCalled = true + return "", "", xerrors.New("should not be called") + }, + }) + + resp, err := tool.Run(context.Background(), fantasy.ToolCall{ + ID: "call-1", + Name: "write_file", + Input: `{"path":"/home/dev/my-plan.md","content":"# Plan"}`, + }) + require.NoError(t, err) + assert.False(t, resp.IsError) + assert.False(t, resolvePlanPathCalled) + assert.Equal(t, `{"ok":true}`, strings.TrimSpace(resp.Content)) + }) + + t.Run("AllowsSharedPlanPathWhenResolvePlanPathIsNil", func(t *testing.T) { + t.Parallel() + ctrl := gomock.NewController(t) + mockConn := agentconnmock.NewMockAgentConn(ctrl) + mockConn.EXPECT(). + WriteFile(gomock.Any(), chattool.LegacySharedPlanPath, gomock.Any()). + DoAndReturn(func(_ context.Context, _ string, reader io.Reader) error { + data, err := io.ReadAll(reader) + require.NoError(t, err) + require.Equal(t, "# Plan", string(data)) + return nil + }) + + tool := chattool.WriteFile(chattool.WriteFileOptions{ + GetWorkspaceConn: func(context.Context) (workspacesdk.AgentConn, error) { + return mockConn, nil + }, + }) + + resp, err := tool.Run(context.Background(), fantasy.ToolCall{ + ID: "call-1", + Name: "write_file", + Input: `{"path":"` + chattool.LegacySharedPlanPath + `","content":"# Plan"}`, + }) + require.NoError(t, err) + assert.False(t, resp.IsError) + }) +} diff --git a/coderd/x/chatd/instruction_test.go b/coderd/x/chatd/instruction_test.go index ffeba4db2d..9b8f3dfc10 100644 --- a/coderd/x/chatd/instruction_test.go +++ b/coderd/x/chatd/instruction_test.go @@ -8,9 +8,111 @@ import ( "github.com/stretchr/testify/require" "github.com/coder/coder/v2/coderd/x/chatd/chatprompt" + "github.com/coder/coder/v2/coderd/x/chatd/chattool" "github.com/coder/coder/v2/codersdk" ) +func TestRenderPlanPathPrompt(t *testing.T) { + t.Parallel() + + newPromptWithPlaceholder := func() []fantasy.Message { + return []fantasy.Message{ + { + Role: fantasy.MessageRoleSystem, + Content: []fantasy.MessagePart{ + fantasy.TextPart{Text: "\n" + defaultSystemPromptPlanPathBlockPlaceholder + "\n"}, + }, + }, + { + Role: fantasy.MessageRoleUser, + Content: []fantasy.MessagePart{ + fantasy.TextPart{Text: "hello"}, + }, + }, + } + } + + messageText := func(t *testing.T, message fantasy.Message) string { + t.Helper() + part, ok := fantasy.AsMessagePart[fantasy.TextPart](message.Content[0]) + require.True(t, ok) + return part.Text + } + + t.Run("ReplacesPlaceholderWithResolvedHome", func(t *testing.T) { + t.Parallel() + + prompt := newPromptWithPlaceholder() + got := renderPlanPathPrompt(prompt, formatPlanPathBlock( + "/Users/dev/.coder/plans/PLAN-chat.md", + "/Users/dev", + )) + + require.Len(t, got, len(prompt)) + text := messageText(t, got[0]) + require.Contains(t, text, "Your plan file path for this chat is: /Users/dev/.coder/plans/PLAN-chat.md") + require.Contains(t, text, "Do not use /Users/dev/PLAN.md.") + require.NotContains(t, text, defaultSystemPromptPlanPathBlockPlaceholder) + }) + + t.Run("FallsBackToLegacySharedPathWhenHomeIsEmpty", func(t *testing.T) { + t.Parallel() + + prompt := newPromptWithPlaceholder() + got := renderPlanPathPrompt(prompt, formatPlanPathBlock( + "/home/coder/.coder/plans/PLAN-chat.md", + "", + )) + + text := messageText(t, got[0]) + require.Contains(t, text, "Do not use "+chattool.LegacySharedPlanPath+".") + }) + + t.Run("LeavesPromptUnchangedWhenPlaceholderMissing", func(t *testing.T) { + t.Parallel() + + prompt := []fantasy.Message{ + { + Role: fantasy.MessageRoleSystem, + Content: []fantasy.MessagePart{ + fantasy.TextPart{Text: "base instructions"}, + }, + }, + { + Role: fantasy.MessageRoleSystem, + Content: []fantasy.MessagePart{ + fantasy.TextPart{Text: "workspace awareness"}, + }, + }, + { + Role: fantasy.MessageRoleUser, + Content: []fantasy.MessagePart{ + fantasy.TextPart{Text: "hello"}, + }, + }, + } + + got := renderPlanPathPrompt(prompt, formatPlanPathBlock( + "/home/coder/.coder/plans/PLAN-chat.md", + "/home/coder", + )) + + require.Equal(t, prompt, got) + }) + + t.Run("RemovesPlaceholderWhenPlanPathBlockIsEmpty", func(t *testing.T) { + t.Parallel() + + prompt := newPromptWithPlaceholder() + got := renderPlanPathPrompt(prompt, "") + + require.Len(t, got, len(prompt)) + text := messageText(t, got[0]) + require.NotContains(t, text, defaultSystemPromptPlanPathBlockPlaceholder) + require.NotContains(t, text, "") + }) +} + func TestInsertSystemInstructionAfterSystemMessages(t *testing.T) { t.Parallel() diff --git a/coderd/x/chatd/prompt.go b/coderd/x/chatd/prompt.go index 5f2e012544..d6764f8be7 100644 --- a/coderd/x/chatd/prompt.go +++ b/coderd/x/chatd/prompt.go @@ -1,5 +1,7 @@ package chatd +const defaultSystemPromptPlanPathBlockPlaceholder = "{{CODER_CHAT_PLAN_FILE_PATH_BLOCK}}" + // DefaultSystemPrompt is used for new chats when no deployment override is // configured. const DefaultSystemPrompt = `You are the Coder agent — an interactive chat tool that helps users with software-engineering tasks inside of the Coder product. @@ -88,11 +90,16 @@ Propose a plan when: If no workspace is attached to this chat yet, create and start one first using create_workspace and start_workspace. Once a workspace is available: 1. Use spawn_agent and wait_agent to research the codebase and gather context as needed. -2. Use write_file to create a Markdown plan file in the workspace (e.g. /home/coder/PLAN.md). +2. Use write_file to create a Markdown plan file at the absolute + chat-specific path from the block below when it is + available. 3. Iterate on the plan with edit_files if needed. -4. Call propose_plan with the absolute file path to present the plan to the user. +4. Call propose_plan with the same absolute plan file path from the + block below. 5. Wait for the user to review and approve the plan before starting implementation. -The propose_plan tool reads the file from the workspace — do not pass content directly. +The propose_plan tool reads the file from the workspace. Do not pass content directly. Write the file first, then present it. All file paths must be absolute. +When the block below is present, use that exact path. +` + defaultSystemPromptPlanPathBlockPlaceholder + ` ` diff --git a/site/src/pages/AgentsPage/components/ChatElements/tools/ProposePlanTool.stories.tsx b/site/src/pages/AgentsPage/components/ChatElements/tools/ProposePlanTool.stories.tsx index ecb13ad723..35bfd83f45 100644 --- a/site/src/pages/AgentsPage/components/ChatElements/tools/ProposePlanTool.stories.tsx +++ b/site/src/pages/AgentsPage/components/ChatElements/tools/ProposePlanTool.stories.tsx @@ -35,6 +35,10 @@ const samplePlan = [ "> **Note**: Based on [RFC 6749](https://tools.ietf.org/html/rfc6749).", ].join("\n"); +const defaultPlanPath = + "/home/coder/.coder/plans/PLAN-a1b2c3d4-e5f6-7890-abcd-ef1234567890.md"; +const defaultPlanFilename = defaultPlanPath.split("/").pop() ?? "PLAN.md"; + const meta: Meta = { title: "pages/AgentsPage/ChatElements/tools/ProposePlan", component: Tool, @@ -54,20 +58,22 @@ export default meta; type Story = StoryObj; export const Running: Story = { - args: { status: "running", args: { path: "/home/coder/PLAN.md" } }, + args: { status: "running", args: { path: defaultPlanPath } }, play: async ({ canvasElement }) => { const canvas = within(canvasElement); - expect(canvas.getByText(/Proposing PLAN\.md/)).toBeInTheDocument(); + expect( + canvas.getByText(`Proposing ${defaultPlanFilename}…`), + ).toBeInTheDocument(); }, }; export const Completed: Story = { args: { status: "completed", - args: { path: "/home/coder/PLAN.md" }, + args: { path: defaultPlanPath }, result: { ok: true, - path: "/home/coder/PLAN.md", + path: defaultPlanPath, kind: "plan", file_id: "test-file-id-completed", media_type: "text/markdown", @@ -138,12 +144,14 @@ export const ErrorState: Story = { args: { status: "completed", isError: true, - args: { path: "/home/coder/PLAN.md" }, + args: { path: defaultPlanPath }, result: "Failed to read file: file not found", }, play: async ({ canvasElement }) => { const canvas = within(canvasElement); - expect(canvas.getByText(/Proposed PLAN\.md/)).toBeInTheDocument(); + expect( + canvas.getByText(`Proposed ${defaultPlanFilename}`), + ).toBeInTheDocument(); expect(canvas.getByLabelText("Error")).toBeInTheDocument(); }, }; @@ -151,10 +159,10 @@ export const ErrorState: Story = { export const EmptyContent: Story = { args: { status: "completed", - args: { path: "/home/coder/PLAN.md" }, + args: { path: defaultPlanPath }, result: { ok: true, - path: "/home/coder/PLAN.md", + path: defaultPlanPath, kind: "plan", file_id: "test-file-id-empty-content", media_type: "text/markdown", @@ -172,10 +180,10 @@ export const EmptyContent: Story = { export const FileIDLoading: Story = { args: { status: "completed", - args: { path: "/home/coder/PLAN.md" }, + args: { path: defaultPlanPath }, result: { ok: true, - path: "/home/coder/PLAN.md", + path: defaultPlanPath, kind: "plan", file_id: "test-file-id-loading", media_type: "text/markdown", @@ -195,10 +203,10 @@ export const FileIDLoading: Story = { export const FileIDCompleted: Story = { args: { status: "completed", - args: { path: "/home/coder/PLAN.md" }, + args: { path: defaultPlanPath }, result: { ok: true, - path: "/home/coder/PLAN.md", + path: defaultPlanPath, kind: "plan", file_id: "test-file-id-success", media_type: "text/markdown", @@ -216,10 +224,10 @@ export const FileIDCompleted: Story = { export const FileIDFetchError: Story = { args: { status: "completed", - args: { path: "/home/coder/PLAN.md" }, + args: { path: defaultPlanPath }, result: { ok: true, - path: "/home/coder/PLAN.md", + path: defaultPlanPath, kind: "plan", file_id: "test-file-id-error", media_type: "text/markdown",