fix(coderd/x/chatd): broaden plan path detection to catch case-insensitive variants

This commit is contained in:
Michael Suchacz
2026-04-10 22:05:12 +00:00
parent ba45e3bee1
commit a9d6fcaad0
10 changed files with 260 additions and 67 deletions
+5 -5
View File
@@ -4471,16 +4471,16 @@ func (p *Server) runChat(
}
defer workspaceCtx.close()
planPathFn := func(ctx context.Context) (string, error) {
planPathFn := func(ctx context.Context) (string, string, error) {
conn, err := workspaceCtx.getWorkspaceConn(ctx)
if err != nil {
return "", err
return "", "", err
}
home, err := chattool.ResolveWorkspaceHome(ctx, conn)
if err != nil {
return "", err
return "", "", err
}
return chattool.PlanPathForChat(home, chat.ID), nil
return chattool.PlanPathForChat(home, chat.ID), home, nil
}
resolvePlanPathInstruction := func(resolveCtx context.Context) string {
if chat.ParentChatID.Valid {
@@ -4494,7 +4494,7 @@ func (p *Server) runChat(
return ""
}
planPath, err := planPathFn(planCtx)
planPath, _, err := planPathFn(planCtx)
if err != nil {
return ""
}
+16 -3
View File
@@ -10,7 +10,7 @@ import (
type EditFilesOptions struct {
GetWorkspaceConn func(context.Context) (workspacesdk.AgentConn, error)
PlanPath func(context.Context) (string, error)
PlanPath func(context.Context) (chatPath string, home string, err error)
}
type EditFilesArgs struct {
@@ -39,14 +39,27 @@ func executeEditFilesTool(
ctx context.Context,
conn workspacesdk.AgentConn,
args EditFilesArgs,
resolvePlanPath func(context.Context) (string, error),
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 _, file := range args.Files {
if resp, rejected := rejectSharedPlanPath(ctx, file.Path, resolvePlanPath); rejected {
if resolvePlanPath == nil || !looksLikePlanFileName(file.Path) {
continue
}
if !planPathLoaded {
chatPath, home, planPathErr = resolvePlanPath(ctx)
planPathLoaded = true
}
if resp, rejected := rejectSharedPlanPath(file.Path, home, chatPath, planPathErr); rejected {
return resp, nil
}
}
+7 -7
View File
@@ -18,7 +18,7 @@ import (
func TestEditFiles(t *testing.T) {
t.Parallel()
t.Run("RejectsSharedPlanPathWhenPlanPathIsConfigured", func(t *testing.T) {
t.Run("RejectsHomeRootPlanPathsWhenPlanPathIsConfigured", func(t *testing.T) {
t.Parallel()
ctrl := gomock.NewController(t)
mockConn := agentconnmock.NewMockAgentConn(ctrl)
@@ -26,21 +26,21 @@ func TestEditFiles(t *testing.T) {
GetWorkspaceConn: func(context.Context) (workspacesdk.AgentConn, error) {
return mockConn, nil
},
PlanPath: func(context.Context) (string, error) {
return "/home/coder/.coder/plans/PLAN-chat.md", nil
PlanPath: func(context.Context) (string, string, error) {
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: `{"files":[{"path":"` + chattool.LegacySharedPlanPath + `","edits":[{"search":"old","replace":"new"}]}]}`,
Input: `{"files":[{"path":"/Users/dev/plan.md","edits":[{"search":"old","replace":"new"}]}]}`,
})
require.NoError(t, err)
assert.True(t, resp.IsError)
assert.Equal(
t,
sharedPlanPathResolvedMessage("/home/coder/.coder/plans/PLAN-chat.md"),
sharedPlanPathResolvedMessage("/Users/dev/.coder/plans/PLAN-chat.md"),
resp.Content,
)
})
@@ -63,9 +63,9 @@ func TestEditFiles(t *testing.T) {
GetWorkspaceConn: func(context.Context) (workspacesdk.AgentConn, error) {
return mockConn, nil
},
PlanPath: func(context.Context) (string, error) {
PlanPath: func(context.Context) (string, string, error) {
planPathCalled = true
return "", xerrors.New("should not be called")
return "", "", xerrors.New("should not be called")
},
})
+16
View File
@@ -53,6 +53,22 @@ func PlanPathForChat(home string, chatID uuid.UUID) string {
)
}
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.
func LooksLikeHomePlanFile(requestedPath, home string) bool {
cleaned := path.Clean(requestedPath)
cleanedHome := path.Clean(home)
return looksLikePlanFileName(cleaned) &&
path.Dir(cleaned) == cleanedHome
}
// IsLegacySharedPlanPath reports whether requested is the exact legacy
// shared plan file path.
func IsLegacySharedPlanPath(requested string) bool {
+89
View File
@@ -182,3 +182,92 @@ func TestIsLegacySharedPlanPath(t *testing.T) {
})
}
}
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 {
testCase := testCase
t.Run(testCase.name, func(t *testing.T) {
t.Parallel()
got := chattool.LooksLikeHomePlanFile(testCase.requested, testCase.home)
require.Equal(t, testCase.want, got)
})
}
}
+14 -10
View File
@@ -1,7 +1,6 @@
package chattool
import (
"context"
"fmt"
"strings"
@@ -9,22 +8,27 @@ import (
)
func rejectSharedPlanPath(
ctx context.Context,
requestedPath string,
resolvePlanPath func(context.Context) (string, error),
home string,
planPath string,
planPathErr error,
) (fantasy.ToolResponse, bool) {
if resolvePlanPath == nil || !IsLegacySharedPlanPath(requestedPath) {
if !looksLikeLegacyHomePlanPath(requestedPath, home) {
return fantasy.ToolResponse{}, false
}
return fantasy.NewTextErrorResponse(sharedPlanPathMessage(ctx, resolvePlanPath)), true
return fantasy.NewTextErrorResponse(sharedPlanPathMessage(planPath, planPathErr)), true
}
func sharedPlanPathMessage(
ctx context.Context,
resolvePlanPath func(context.Context) (string, error),
) string {
planPath, err := resolvePlanPath(ctx)
func looksLikeLegacyHomePlanPath(requestedPath, home string) bool {
if strings.TrimSpace(home) == "" {
return IsLegacySharedPlanPath(requestedPath)
}
return LooksLikeHomePlanFile(requestedPath, home)
}
func sharedPlanPathMessage(planPath string, err error) string {
if err == nil && strings.TrimSpace(planPath) != "" {
return fmt.Sprintf(
"the shared plan path %s is no longer supported; use the chat-specific plan path: %s",
+9 -5
View File
@@ -17,7 +17,7 @@ const maxProposePlanSize = 32 * 1024 // 32 KiB
// ProposePlanOptions configures the propose_plan tool.
type ProposePlanOptions struct {
GetWorkspaceConn func(context.Context) (workspacesdk.AgentConn, error)
PlanPath func(context.Context) (string, error)
PlanPath func(context.Context) (chatPath string, home string, err error)
StoreFile func(ctx context.Context, name string, mediaType string, data []byte) (uuid.UUID, error)
}
@@ -33,7 +33,8 @@ func ProposePlan(options ProposePlanOptions) fantasy.AgentTool {
"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 to the plan. Use the chat-specific plan path provided in your instructions. The tool reads the content from the workspace.",
"Pass the absolute file path to the plan. Important: use the chat-specific plan path from your runtime instructions, 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
@@ -54,7 +55,7 @@ func executeProposePlanTool(
ctx context.Context,
conn workspacesdk.AgentConn,
args ProposePlanArgs,
resolvePlanPath func(context.Context) (string, 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),
) (fantasy.ToolResponse, error) {
path := strings.TrimSpace(args.Path)
@@ -65,8 +66,11 @@ func executeProposePlanTool(
return fantasy.NewTextErrorResponse("path must end with .md"), nil
}
if resp, rejected := rejectSharedPlanPath(ctx, path, resolvePlanPath); rejected {
return resp, nil
if resolvePlanPath != nil && looksLikePlanFileName(path) {
chatPath, home, err := resolvePlanPath(ctx)
if resp, rejected := rejectSharedPlanPath(path, home, chatPath, err); rejected {
return resp, nil
}
}
rc, _, err := conn.ReadFile(ctx, path, 0, maxProposePlanSize+1)
+42 -8
View File
@@ -140,9 +140,9 @@ func TestProposePlan(t *testing.T) {
t,
mockConn,
storeFile,
func(context.Context) (string, error) {
func(context.Context) (string, string, error) {
planPathCalled = true
return "", xerrors.New("should not be called")
return "", "", xerrors.New("should not be called")
},
)
resp, err := tool.Run(context.Background(), fantasy.ToolCall{
@@ -152,7 +152,7 @@ func TestProposePlan(t *testing.T) {
})
require.NoError(t, err)
assert.False(t, resp.IsError)
assert.False(t, planPathCalled)
assert.True(t, planPathCalled)
result := decodeProposePlanResponse(t, resp)
assert.True(t, result.OK)
@@ -164,6 +164,40 @@ 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)
@@ -238,8 +272,8 @@ func TestProposePlan(t *testing.T) {
t,
mockConn,
storeFile,
func(context.Context) (string, error) {
return "/home/coder/.coder/plans/PLAN-chat.md", nil
func(context.Context) (string, string, error) {
return "/home/coder/.coder/plans/PLAN-chat.md", "/home/coder", nil
},
)
@@ -267,8 +301,8 @@ func TestProposePlan(t *testing.T) {
t,
mockConn,
storeFile,
func(context.Context) (string, error) {
return "", xerrors.New("workspace unavailable")
func(context.Context) (string, string, error) {
return "", "", xerrors.New("workspace unavailable")
},
)
@@ -351,7 +385,7 @@ func newProposePlanToolWithPlanPath(
t *testing.T,
mockConn *agentconnmock.MockAgentConn,
storeFile func(ctx context.Context, name string, mediaType string, data []byte) (uuid.UUID, error),
planPath func(context.Context) (string, error),
planPath func(context.Context) (string, string, error),
) fantasy.AgentTool {
t.Helper()
return chattool.ProposePlan(chattool.ProposePlanOptions{
+7 -4
View File
@@ -11,7 +11,7 @@ import (
type WriteFileOptions struct {
GetWorkspaceConn func(context.Context) (workspacesdk.AgentConn, error)
PlanPath func(context.Context) (string, error)
PlanPath func(context.Context) (chatPath string, home string, err error)
}
type WriteFileArgs struct {
@@ -40,14 +40,17 @@ func executeWriteFileTool(
ctx context.Context,
conn workspacesdk.AgentConn,
args WriteFileArgs,
resolvePlanPath func(context.Context) (string, error),
resolvePlanPath func(context.Context) (chatPath string, home string, err error),
) (fantasy.ToolResponse, error) {
if args.Path == "" {
return fantasy.NewTextErrorResponse("path is required"), nil
}
if resp, rejected := rejectSharedPlanPath(ctx, args.Path, resolvePlanPath); rejected {
return resp, nil
if resolvePlanPath != nil && looksLikePlanFileName(args.Path) {
chatPath, home, err := resolvePlanPath(ctx)
if resp, rejected := rejectSharedPlanPath(args.Path, home, chatPath, err); rejected {
return resp, nil
}
}
if err := conn.WriteFile(ctx, args.Path, strings.NewReader(args.Content)); err != nil {
+55 -25
View File
@@ -20,31 +20,61 @@ import (
func TestWriteFile(t *testing.T) {
t.Parallel()
t.Run("RejectsSharedPlanPathWhenPlanPathIsConfigured", func(t *testing.T) {
t.Run("RejectsHomeRootPlanVariantsWhenPlanPathIsConfigured", 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
},
PlanPath: func(context.Context) (string, error) {
return "/home/coder/.coder/plans/PLAN-chat.md", 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.True(t, resp.IsError)
assert.Equal(
t,
sharedPlanPathResolvedMessage("/home/coder/.coder/plans/PLAN-chat.md"),
resp.Content,
)
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 {
testCase := testCase
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
},
PlanPath: 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("/home/coder/.coder/plans/PLAN-chat.md"),
resp.Content,
)
})
}
})
t.Run("AllowsNonSharedPath", func(t *testing.T) {
@@ -66,9 +96,9 @@ func TestWriteFile(t *testing.T) {
GetWorkspaceConn: func(context.Context) (workspacesdk.AgentConn, error) {
return mockConn, nil
},
PlanPath: func(context.Context) (string, error) {
PlanPath: func(context.Context) (string, string, error) {
planPathCalled = true
return "", xerrors.New("should not be called")
return "", "", xerrors.New("should not be called")
},
})