From a002c88b88af166a41115f5516e7ecdc5a872cfb Mon Sep 17 00:00:00 2001 From: David Gageot Date: Thu, 25 Jun 2026 14:20:04 +0200 Subject: [PATCH 1/3] feat: add plan builtin toolset for shared multi-agent collaboration Add a 'plan' builtin toolset that lets multiple agents collaborate on named plans stored in a global shared folder under the docker-agent data directory. Exposes write_plan, read_plan, list_plans, and delete_plan. Wires the toolset into the default registry and adds the 'plan' type to the agent config JSON schema. Assisted-By: claude-sonnet-4-5 --- agent-schema.json | 4 +- examples/shared_plan.yaml | 48 +++++ pkg/teamloader/toolsets/toolsets.go | 4 + pkg/tools/builtin/plan/plan.go | 320 ++++++++++++++++++++++++++++ pkg/tools/builtin/plan/plan_test.go | 235 ++++++++++++++++++++ 5 files changed, 610 insertions(+), 1 deletion(-) create mode 100644 examples/shared_plan.yaml create mode 100644 pkg/tools/builtin/plan/plan.go create mode 100644 pkg/tools/builtin/plan/plan_test.go diff --git a/agent-schema.json b/agent-schema.json index 932677fdd1..d4a28ebc42 100644 --- a/agent-schema.json +++ b/agent-schema.json @@ -1832,6 +1832,7 @@ "filesystem", "shell", "tasks", + "plan", "todo", "fetch", "api", @@ -1898,7 +1899,7 @@ }, "path": { "type": "string", - "description": "Path for memory tool" + "description": "Storage path for the memory or tasks toolset" }, "shell": { "type": "object", @@ -2153,6 +2154,7 @@ "filesystem", "shell", "tasks", + "plan", "todo", "fetch", "api", diff --git a/examples/shared_plan.yaml b/examples/shared_plan.yaml new file mode 100644 index 0000000000..9809269eee --- /dev/null +++ b/examples/shared_plan.yaml @@ -0,0 +1,48 @@ +# Two agents collaborate on shared, named plans stored in a global folder +# under the docker-agent data directory. +# +# The architect writes a plan; the builder reads it and refines it. Both load +# the `plan` toolset, so they see the same shared plans (addressed by name) and +# the plans persist across runs. + +agents: + root: + model: anthropic/claude-sonnet-4-5 + description: Coordinator that routes work between the architect and the builder + instruction: | + You coordinate a planning session between two specialists. + - Hand off to the architect to draft or revise a high-level plan. + - Hand off to the builder to review the plan and add concrete + implementation steps. + Keep iterating until the plan is complete, then summarize it for the user. + handoffs: + - architect + - builder + + architect: + model: anthropic/claude-sonnet-4-5 + description: Drafts and revises high-level plans + instruction: | + You are a software architect. Use list_plans to see existing plans and + read_plan to inspect one, then use write_plan to create or revise a plan + by name. Always read before writing so you preserve content you want to + keep. Set the title and use "architect" as the author. When the + high-level plan is ready, hand off to the builder. + toolsets: + - type: plan + handoffs: + - builder + + builder: + model: openai/gpt-4o + description: Reviews plans and adds concrete implementation steps + instruction: | + You are an implementation engineer. Use read_plan to read the architect's + plan, then use write_plan to append concrete, actionable implementation + steps. Always read before writing so you don't lose the architect's work. + Use "builder" as the author. Delete obsolete plans with delete_plan. When + done, hand off back to root. + toolsets: + - type: plan + handoffs: + - root diff --git a/pkg/teamloader/toolsets/toolsets.go b/pkg/teamloader/toolsets/toolsets.go index 697fdac342..e9a6f9b136 100644 --- a/pkg/teamloader/toolsets/toolsets.go +++ b/pkg/teamloader/toolsets/toolsets.go @@ -17,6 +17,7 @@ import ( "github.com/docker/docker-agent/pkg/tools/builtin/memory" "github.com/docker/docker-agent/pkg/tools/builtin/modelpicker" "github.com/docker/docker-agent/pkg/tools/builtin/openapi" + "github.com/docker/docker-agent/pkg/tools/builtin/plan" "github.com/docker/docker-agent/pkg/tools/builtin/rag" "github.com/docker/docker-agent/pkg/tools/builtin/shell" "github.com/docker/docker-agent/pkg/tools/builtin/tasks" @@ -38,6 +39,9 @@ func DefaultToolsetCreators() map[string]teamloader.ToolsetCreator { "tasks": func(_ context.Context, toolset latest.Toolset, parentDir string, runConfig *config.RuntimeConfig, _ string) (tools.ToolSet, error) { return tasks.CreateToolSet(toolset, parentDir, runConfig) }, + "plan": func(_ context.Context, _ latest.Toolset, _ string, _ *config.RuntimeConfig, _ string) (tools.ToolSet, error) { + return plan.CreateToolSet() + }, "memory": func(_ context.Context, toolset latest.Toolset, parentDir string, runConfig *config.RuntimeConfig, configName string) (tools.ToolSet, error) { return memory.CreateToolSet(toolset, parentDir, runConfig, configName) }, diff --git a/pkg/tools/builtin/plan/plan.go b/pkg/tools/builtin/plan/plan.go new file mode 100644 index 0000000000..f8d8151cce --- /dev/null +++ b/pkg/tools/builtin/plan/plan.go @@ -0,0 +1,320 @@ +// Package plan provides a toolset that lets two or more agents collaborate on +// plans stored in a global shared folder under the docker-agent data +// directory. Plans are addressed by name, so any agent that loads this toolset +// can write, read, list, and delete the same shared plans, and they persist +// across sessions. +package plan + +import ( + "context" + "encoding/json" + "fmt" + "os" + "path/filepath" + "sort" + "strings" + "sync" + "time" + + "github.com/docker/docker-agent/pkg/paths" + "github.com/docker/docker-agent/pkg/tools" +) + +const ( + ToolNameWritePlan = "write_plan" + ToolNameReadPlan = "read_plan" + ToolNameListPlans = "list_plans" + ToolNameDeletePlan = "delete_plan" +) + +// Plan is a shared document collaborated on by the agents. +type Plan struct { + Name string `json:"name"` + Title string `json:"title,omitempty"` + Content string `json:"content"` + // Author is a free-form label identifying who last wrote the plan + // (typically the agent name). It helps collaborators see who made the + // most recent change. + Author string `json:"author,omitempty"` + Revision int `json:"revision"` + UpdatedAt string `json:"updatedAt"` +} + +// Summary is a lightweight view of a plan returned by list_plans. +type Summary struct { + Name string `json:"name"` + Title string `json:"title,omitempty"` + Author string `json:"author,omitempty"` + Revision int `json:"revision"` + UpdatedAt string `json:"updatedAt"` +} + +type ToolSet struct { + mu sync.Mutex + dir string +} + +var ( + _ tools.ToolSet = (*ToolSet)(nil) + _ tools.Instructable = (*ToolSet)(nil) + _ tools.Describer = (*ToolSet)(nil) +) + +// CreateToolSet is used by the tools registry. +func CreateToolSet() (tools.ToolSet, error) { + dir := DefaultDir() + if err := os.MkdirAll(dir, 0o700); err != nil { + return nil, fmt.Errorf("failed to create plans directory: %w", err) + } + return New(dir), nil +} + +// DefaultDir is the global shared folder where plans are stored, under the +// docker-agent data directory. +func DefaultDir() string { + return filepath.Join(paths.GetDataDir(), "plans") +} + +func New(dir string) *ToolSet { + return &ToolSet{dir: dir} +} + +func (t *ToolSet) Describe() string { + return "plan(dir=" + t.dir + ")" +} + +func (t *ToolSet) Instructions() string { + return `## Plan Tools + +Collaborate on shared, named plans with other agents. Plans are stored in a +global shared folder, so every agent using this toolset sees the same plans. + +- Use list_plans to discover existing plans. +- Use read_plan to inspect a plan before acting on or changing it. +- Use write_plan to create or update a plan by name. Writing replaces the whole + document, so read it first and preserve any content you want to keep. Set the + title and author (your agent name) so collaborators can see who made the + latest change. Each write bumps the revision number. +- Use delete_plan to remove a plan once it is no longer needed.` +} + +// sanitizeName maps a user-supplied plan name to a safe single-segment file +// name. It lowercases, replaces path separators and other illegal characters +// with '-', and collapses to a stable slug so the same name always resolves to +// the same file. +func sanitizeName(name string) string { + name = strings.TrimSpace(strings.ToLower(name)) + mapped := strings.Map(func(r rune) rune { + switch { + case r >= 'a' && r <= 'z', r >= '0' && r <= '9': + return r + case r == '-' || r == '_': + return r + default: + return '-' + } + }, name) + // Collapse runs of '-' and trim leading/trailing separators. + for strings.Contains(mapped, "--") { + mapped = strings.ReplaceAll(mapped, "--", "-") + } + return strings.Trim(mapped, "-") +} + +func (t *ToolSet) planPath(name string) (string, error) { + slug := sanitizeName(name) + if slug == "" { + return "", fmt.Errorf("invalid plan name %q", name) + } + return filepath.Join(t.dir, slug+".json"), nil +} + +func (t *ToolSet) load(path string) (Plan, bool) { + data, err := os.ReadFile(path) + if err != nil { + return Plan{}, false + } + var p Plan + if err := json.Unmarshal(data, &p); err != nil { + return Plan{}, false + } + return p, true +} + +func (t *ToolSet) save(path string, p Plan) error { + if err := os.MkdirAll(t.dir, 0o700); err != nil { + return fmt.Errorf("creating plans directory: %w", err) + } + data, err := json.MarshalIndent(p, "", " ") + if err != nil { + return fmt.Errorf("marshaling plan: %w", err) + } + return os.WriteFile(path, data, 0o600) +} + +type WritePlanArgs struct { + Name string `json:"name" jsonschema:"The plan name (used to address the plan; e.g. 'release', 'migration')."` + Content string `json:"content" jsonschema:"The full plan content (markdown). Replaces the existing plan."` + Title string `json:"title,omitempty" jsonschema:"Optional human-readable title. Preserved from the previous revision when omitted."` + Author string `json:"author,omitempty" jsonschema:"Optional label identifying who is writing the plan (typically the agent name)."` +} + +func (t *ToolSet) writePlan(_ context.Context, params WritePlanArgs) (*tools.ToolCallResult, error) { + if params.Content == "" { + return tools.ResultError("content must not be empty"), nil + } + path, err := t.planPath(params.Name) + if err != nil { + return tools.ResultError(err.Error()), nil + } + + t.mu.Lock() + defer t.mu.Unlock() + + plan, _ := t.load(path) + plan.Name = sanitizeName(params.Name) + plan.Content = params.Content + if params.Title != "" { + plan.Title = params.Title + } + plan.Author = params.Author + plan.Revision++ + plan.UpdatedAt = time.Now().UTC().Format(time.RFC3339) + + if err := t.save(path, plan); err != nil { + return tools.ResultError(err.Error()), nil + } + + return tools.ResultJSON(plan), nil +} + +type ReadPlanArgs struct { + Name string `json:"name" jsonschema:"The name of the plan to read."` +} + +func (t *ToolSet) readPlan(_ context.Context, params ReadPlanArgs) (*tools.ToolCallResult, error) { + path, err := t.planPath(params.Name) + if err != nil { + return tools.ResultError(err.Error()), nil + } + + t.mu.Lock() + defer t.mu.Unlock() + + plan, ok := t.load(path) + if !ok { + return tools.ResultError(fmt.Sprintf("plan %q not found", params.Name)), nil + } + + return tools.ResultJSON(plan), nil +} + +func (t *ToolSet) listPlans(_ context.Context, _ tools.ToolCall) (*tools.ToolCallResult, error) { + t.mu.Lock() + defer t.mu.Unlock() + + entries, err := os.ReadDir(t.dir) + if err != nil { + if os.IsNotExist(err) { + return tools.ResultJSON([]Summary{}), nil + } + return tools.ResultError(err.Error()), nil + } + + summaries := make([]Summary, 0, len(entries)) + for _, entry := range entries { + if entry.IsDir() || filepath.Ext(entry.Name()) != ".json" { + continue + } + plan, ok := t.load(filepath.Join(t.dir, entry.Name())) + if !ok { + continue + } + summaries = append(summaries, Summary{ + Name: plan.Name, + Title: plan.Title, + Author: plan.Author, + Revision: plan.Revision, + UpdatedAt: plan.UpdatedAt, + }) + } + + sort.Slice(summaries, func(i, j int) bool { + return summaries[i].Name < summaries[j].Name + }) + + return tools.ResultJSON(summaries), nil +} + +type DeletePlanArgs struct { + Name string `json:"name" jsonschema:"The name of the plan to delete."` +} + +func (t *ToolSet) deletePlan(_ context.Context, params DeletePlanArgs) (*tools.ToolCallResult, error) { + path, err := t.planPath(params.Name) + if err != nil { + return tools.ResultError(err.Error()), nil + } + + t.mu.Lock() + defer t.mu.Unlock() + + if _, ok := t.load(path); !ok { + return tools.ResultError(fmt.Sprintf("plan %q not found", params.Name)), nil + } + if err := os.Remove(path); err != nil { + return tools.ResultError(err.Error()), nil + } + + return tools.ResultJSON(map[string]string{"deleted": sanitizeName(params.Name)}), nil +} + +func (t *ToolSet) Tools(context.Context) ([]tools.Tool, error) { + return []tools.Tool{ + { + Name: ToolNameWritePlan, + Category: "plan", + Description: "Create or update a shared plan by name. Replaces the entire plan content, so read it first to preserve anything you want to keep. Each write bumps the revision number.", + Parameters: tools.MustSchemaFor[WritePlanArgs](), + OutputSchema: tools.MustSchemaFor[Plan](), + Handler: tools.NewHandler(t.writePlan), + Annotations: tools.ToolAnnotations{ + Title: "Write Plan", + }, + }, + { + Name: ToolNameReadPlan, + Category: "plan", + Description: "Read a shared plan by name, including its title, content, author, and revision number.", + Parameters: tools.MustSchemaFor[ReadPlanArgs](), + OutputSchema: tools.MustSchemaFor[Plan](), + Handler: tools.NewHandler(t.readPlan), + Annotations: tools.ToolAnnotations{ + Title: "Read Plan", + ReadOnlyHint: true, + }, + }, + { + Name: ToolNameListPlans, + Category: "plan", + Description: "List all shared plans with their name, title, author, and revision.", + OutputSchema: tools.MustSchemaFor[[]Summary](), + Handler: t.listPlans, + Annotations: tools.ToolAnnotations{ + Title: "List Plans", + ReadOnlyHint: true, + }, + }, + { + Name: ToolNameDeletePlan, + Category: "plan", + Description: "Delete a shared plan by name.", + Parameters: tools.MustSchemaFor[DeletePlanArgs](), + Handler: tools.NewHandler(t.deletePlan), + Annotations: tools.ToolAnnotations{ + Title: "Delete Plan", + DestructiveHint: new(true), + }, + }, + }, nil +} diff --git a/pkg/tools/builtin/plan/plan_test.go b/pkg/tools/builtin/plan/plan_test.go new file mode 100644 index 0000000000..a6aef53964 --- /dev/null +++ b/pkg/tools/builtin/plan/plan_test.go @@ -0,0 +1,235 @@ +package plan + +import ( + "encoding/json" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/docker/docker-agent/pkg/tools" +) + +func newTestPlanTool(t *testing.T) *ToolSet { + t.Helper() + return New(t.TempDir()) +} + +func TestPlanTool_DisplayNames(t *testing.T) { + tool := newTestPlanTool(t) + + all, err := tool.Tools(t.Context()) + require.NoError(t, err) + + for _, tl := range all { + assert.NotEmpty(t, tl.DisplayName()) + assert.NotEqual(t, tl.Name, tl.DisplayName()) + } +} + +func TestPlanTool_Instructions(t *testing.T) { + tool := newTestPlanTool(t) + assert.NotEmpty(t, tool.Instructions()) +} + +func TestPlanTool_Describe(t *testing.T) { + tool := newTestPlanTool(t) + assert.Contains(t, tool.Describe(), "plan(dir=") +} + +func TestPlanTool_Write(t *testing.T) { + tool := newTestPlanTool(t) + + result, err := tool.writePlan(t.Context(), WritePlanArgs{ + Name: "Release", + Content: "Step 1: do the thing", + Title: "Release plan", + Author: "planner", + }) + require.NoError(t, err) + assert.False(t, result.IsError) + + var plan Plan + require.NoError(t, json.Unmarshal([]byte(result.Output), &plan)) + assert.Equal(t, "release", plan.Name) + assert.Equal(t, "Release plan", plan.Title) + assert.Equal(t, "Step 1: do the thing", plan.Content) + assert.Equal(t, "planner", plan.Author) + assert.Equal(t, 1, plan.Revision) + assert.NotEmpty(t, plan.UpdatedAt) +} + +func TestPlanTool_WriteEmptyContent(t *testing.T) { + tool := newTestPlanTool(t) + + result, err := tool.writePlan(t.Context(), WritePlanArgs{Name: "p", Content: ""}) + require.NoError(t, err) + assert.True(t, result.IsError) + assert.Contains(t, result.Output, "content must not be empty") +} + +func TestPlanTool_WriteInvalidName(t *testing.T) { + tool := newTestPlanTool(t) + + result, err := tool.writePlan(t.Context(), WritePlanArgs{Name: "///", Content: "x"}) + require.NoError(t, err) + assert.True(t, result.IsError) + assert.Contains(t, result.Output, "invalid plan name") +} + +func TestPlanTool_ReadNotFound(t *testing.T) { + tool := newTestPlanTool(t) + + result, err := tool.readPlan(t.Context(), ReadPlanArgs{Name: "missing"}) + require.NoError(t, err) + assert.True(t, result.IsError) + assert.Contains(t, result.Output, "not found") +} + +func TestPlanTool_WriteThenRead(t *testing.T) { + tool := newTestPlanTool(t) + + _, err := tool.writePlan(t.Context(), WritePlanArgs{Name: "migration", Content: "the plan", Title: "T"}) + require.NoError(t, err) + + result, err := tool.readPlan(t.Context(), ReadPlanArgs{Name: "migration"}) + require.NoError(t, err) + assert.False(t, result.IsError) + + var plan Plan + require.NoError(t, json.Unmarshal([]byte(result.Output), &plan)) + assert.Equal(t, "the plan", plan.Content) + assert.Equal(t, "T", plan.Title) +} + +func TestPlanTool_RevisionIncrementsAndTitlePreserved(t *testing.T) { + tool := newTestPlanTool(t) + + _, err := tool.writePlan(t.Context(), WritePlanArgs{Name: "p", Content: "v1", Title: "Original"}) + require.NoError(t, err) + + // Second write omits the title; it should be preserved. + result, err := tool.writePlan(t.Context(), WritePlanArgs{Name: "p", Content: "v2"}) + require.NoError(t, err) + + var plan Plan + require.NoError(t, json.Unmarshal([]byte(result.Output), &plan)) + assert.Equal(t, "v2", plan.Content) + assert.Equal(t, "Original", plan.Title) + assert.Equal(t, 2, plan.Revision) +} + +func TestPlanTool_ListPlans(t *testing.T) { + tool := newTestPlanTool(t) + + _, err := tool.writePlan(t.Context(), WritePlanArgs{Name: "beta", Content: "b", Author: "x"}) + require.NoError(t, err) + _, err = tool.writePlan(t.Context(), WritePlanArgs{Name: "alpha", Content: "a", Author: "y"}) + require.NoError(t, err) + + result, err := tool.listPlans(t.Context(), tools.ToolCall{}) + require.NoError(t, err) + assert.False(t, result.IsError) + + var summaries []Summary + require.NoError(t, json.Unmarshal([]byte(result.Output), &summaries)) + require.Len(t, summaries, 2) + // Sorted by name. + assert.Equal(t, "alpha", summaries[0].Name) + assert.Equal(t, "beta", summaries[1].Name) +} + +func TestPlanTool_ListEmpty(t *testing.T) { + tool := newTestPlanTool(t) + + result, err := tool.listPlans(t.Context(), tools.ToolCall{}) + require.NoError(t, err) + assert.False(t, result.IsError) + + var summaries []Summary + require.NoError(t, json.Unmarshal([]byte(result.Output), &summaries)) + assert.Empty(t, summaries) +} + +func TestPlanTool_DeletePlan(t *testing.T) { + tool := newTestPlanTool(t) + + _, err := tool.writePlan(t.Context(), WritePlanArgs{Name: "temp", Content: "x"}) + require.NoError(t, err) + + result, err := tool.deletePlan(t.Context(), DeletePlanArgs{Name: "temp"}) + require.NoError(t, err) + assert.False(t, result.IsError) + assert.Contains(t, result.Output, "temp") + + // Verify it's gone. + readResult, err := tool.readPlan(t.Context(), ReadPlanArgs{Name: "temp"}) + require.NoError(t, err) + assert.True(t, readResult.IsError) +} + +func TestPlanTool_DeleteNotFound(t *testing.T) { + tool := newTestPlanTool(t) + + result, err := tool.deletePlan(t.Context(), DeletePlanArgs{Name: "nope"}) + require.NoError(t, err) + assert.True(t, result.IsError) + assert.Contains(t, result.Output, "not found") +} + +func TestPlanTool_SharedAcrossInstances(t *testing.T) { + dir := t.TempDir() + + // One agent writes the plan. + writer := New(dir) + _, err := writer.writePlan(t.Context(), WritePlanArgs{ + Name: "collab", + Content: "collaborative plan", + Author: "agent-a", + }) + require.NoError(t, err) + + // Another agent, sharing the same folder, reads it. + reader := New(dir) + result, err := reader.readPlan(t.Context(), ReadPlanArgs{Name: "collab"}) + require.NoError(t, err) + assert.False(t, result.IsError) + + var plan Plan + require.NoError(t, json.Unmarshal([]byte(result.Output), &plan)) + assert.Equal(t, "collaborative plan", plan.Content) + assert.Equal(t, "agent-a", plan.Author) +} + +func TestPlanTool_NameSanitizationIsStable(t *testing.T) { + tool := newTestPlanTool(t) + + _, err := tool.writePlan(t.Context(), WritePlanArgs{Name: "My Plan!", Content: "v1"}) + require.NoError(t, err) + + // A differently-formatted but equivalent name resolves to the same plan. + result, err := tool.readPlan(t.Context(), ReadPlanArgs{Name: "my plan"}) + require.NoError(t, err) + assert.False(t, result.IsError) + + var plan Plan + require.NoError(t, json.Unmarshal([]byte(result.Output), &plan)) + assert.Equal(t, "my-plan", plan.Name) +} + +func TestPlanTool_ParametersAreObjects(t *testing.T) { + tool := newTestPlanTool(t) + + allTools, err := tool.Tools(t.Context()) + require.NoError(t, err) + require.NotEmpty(t, allTools) + + for _, tl := range allTools { + if tl.Parameters == nil { + continue + } + m, err := tools.SchemaToMap(tl.Parameters) + require.NoError(t, err) + assert.Equal(t, "object", m["type"]) + } +} From 068fbfb76207d30e3585103508749806dc542dce Mon Sep 17 00:00:00 2001 From: David Gageot Date: Thu, 25 Jun 2026 14:32:54 +0200 Subject: [PATCH 2/3] fix(plan): harden shared plan storage after review - Share one ToolSet per process (singleton) so all collaborating agents serialize plan operations on a single mutex instead of each agent holding its own, which previously allowed lost updates. - Write plans atomically (temp file + rename) via pkg/atomicfile so readers never observe a partial file and an existing symlink at the target is replaced rather than followed. - Validate plan names strictly against a slug pattern instead of lossy sanitization, eliminating silent collisions (e.g. 'a/b' and 'a b' mapping onto 'a-b') and path traversal. - Distinguish 'not found' from corrupt/unreadable plan files: read_plan and list surface real errors instead of masking them, list skips corrupt files, and delete_plan removes corrupt plans too. - Preserve author (like title) across revisions when omitted. Assisted-By: claude-sonnet-4-5 --- pkg/tools/builtin/plan/plan.go | 134 ++++++++++++++++++---------- pkg/tools/builtin/plan/plan_test.go | 123 ++++++++++++++++++++----- 2 files changed, 185 insertions(+), 72 deletions(-) diff --git a/pkg/tools/builtin/plan/plan.go b/pkg/tools/builtin/plan/plan.go index f8d8151cce..cf7e83f1ed 100644 --- a/pkg/tools/builtin/plan/plan.go +++ b/pkg/tools/builtin/plan/plan.go @@ -3,19 +3,30 @@ // directory. Plans are addressed by name, so any agent that loads this toolset // can write, read, list, and delete the same shared plans, and they persist // across sessions. +// +// Concurrency: all agents in a single process share one ToolSet instance (see +// CreateToolSet), so their reads and writes are serialized by a single mutex. +// On-disk writes are atomic (write-to-temp + rename), so a reader — including +// a separate docker-agent process — never observes a partially written plan. +// Two distinct processes writing the *same* plan at the very same instant can +// still race on the read-modify-write revision bump (last writer wins); this +// is acceptable for the intended in-process multi-agent collaboration. package plan import ( + "bytes" "context" "encoding/json" + "errors" "fmt" "os" "path/filepath" + "regexp" "sort" - "strings" "sync" "time" + "github.com/docker/docker-agent/pkg/atomicfile" "github.com/docker/docker-agent/pkg/paths" "github.com/docker/docker-agent/pkg/tools" ) @@ -27,6 +38,13 @@ const ( ToolNameDeletePlan = "delete_plan" ) +// namePattern defines the accepted plan-name format: a lowercase slug made of +// alphanumerics, '-' and '_'. Names are validated against it rather than being +// silently rewritten, so two different inputs can never collapse onto the same +// file (which would let one plan clobber another) and no input can escape the +// plans directory via path separators or "..". +var namePattern = regexp.MustCompile(`^[a-z0-9][a-z0-9_-]*$`) + // Plan is a shared document collaborated on by the agents. type Plan struct { Name string `json:"name"` @@ -60,13 +78,22 @@ var ( _ tools.Describer = (*ToolSet)(nil) ) -// CreateToolSet is used by the tools registry. -func CreateToolSet() (tools.ToolSet, error) { +// sharedToolSet returns the one ToolSet shared by every agent in this process, +// built once on first use. Sharing a single instance means all collaborating +// agents serialize their plan operations on the same mutex. +var sharedToolSet = sync.OnceValues(func() (*ToolSet, error) { dir := DefaultDir() if err := os.MkdirAll(dir, 0o700); err != nil { return nil, fmt.Errorf("failed to create plans directory: %w", err) } return New(dir), nil +}) + +// CreateToolSet is used by the tools registry. It returns a process-wide +// singleton so that all agents collaborating in the same process share one +// lock over the global plans folder. +func CreateToolSet() (tools.ToolSet, error) { + return sharedToolSet() } // DefaultDir is the global shared folder where plans are stored, under the @@ -95,50 +122,40 @@ global shared folder, so every agent using this toolset sees the same plans. document, so read it first and preserve any content you want to keep. Set the title and author (your agent name) so collaborators can see who made the latest change. Each write bumps the revision number. -- Use delete_plan to remove a plan once it is no longer needed.` -} +- Use delete_plan to remove a plan once it is no longer needed. -// sanitizeName maps a user-supplied plan name to a safe single-segment file -// name. It lowercases, replaces path separators and other illegal characters -// with '-', and collapses to a stable slug so the same name always resolves to -// the same file. -func sanitizeName(name string) string { - name = strings.TrimSpace(strings.ToLower(name)) - mapped := strings.Map(func(r rune) rune { - switch { - case r >= 'a' && r <= 'z', r >= '0' && r <= '9': - return r - case r == '-' || r == '_': - return r - default: - return '-' - } - }, name) - // Collapse runs of '-' and trim leading/trailing separators. - for strings.Contains(mapped, "--") { - mapped = strings.ReplaceAll(mapped, "--", "-") - } - return strings.Trim(mapped, "-") +Plan names must be lowercase and may contain only letters, digits, '-' and '_' +(for example "release-2025" or "db_migration").` } +// planPath validates name and returns the absolute path of its plan file. The +// name is rejected (rather than rewritten) when it does not match namePattern, +// which guarantees a one-to-one mapping between names and files and prevents +// path traversal. func (t *ToolSet) planPath(name string) (string, error) { - slug := sanitizeName(name) - if slug == "" { - return "", fmt.Errorf("invalid plan name %q", name) + if !namePattern.MatchString(name) { + return "", fmt.Errorf("invalid plan name %q: use only lowercase letters, digits, '-' and '_', starting with a letter or digit", name) } - return filepath.Join(t.dir, slug+".json"), nil + return filepath.Join(t.dir, name+".json"), nil } -func (t *ToolSet) load(path string) (Plan, bool) { +// load reads and decodes the plan at path. It distinguishes a missing plan +// (false, nil) from a real failure such as a permission error or corrupt JSON +// (false, err), so callers can report the latter instead of masking it as +// "not found". +func (t *ToolSet) load(path string) (Plan, bool, error) { data, err := os.ReadFile(path) + if errors.Is(err, os.ErrNotExist) { + return Plan{}, false, nil + } if err != nil { - return Plan{}, false + return Plan{}, false, fmt.Errorf("reading plan: %w", err) } var p Plan if err := json.Unmarshal(data, &p); err != nil { - return Plan{}, false + return Plan{}, false, fmt.Errorf("plan file %s is corrupt: %w", filepath.Base(path), err) } - return p, true + return p, true, nil } func (t *ToolSet) save(path string, p Plan) error { @@ -149,14 +166,20 @@ func (t *ToolSet) save(path string, p Plan) error { if err != nil { return fmt.Errorf("marshaling plan: %w", err) } - return os.WriteFile(path, data, 0o600) + // Atomic write (temp file + rename): readers in other agents or processes + // see either the old or the new content, never a partial file, and an + // existing symlink at path is replaced rather than followed. + if err := atomicfile.Write(path, bytes.NewReader(data), 0o600); err != nil { + return fmt.Errorf("writing plan: %w", err) + } + return nil } type WritePlanArgs struct { - Name string `json:"name" jsonschema:"The plan name (used to address the plan; e.g. 'release', 'migration')."` + Name string `json:"name" jsonschema:"The plan name. Lowercase letters, digits, '-' and '_' only (e.g. 'release', 'db-migration')."` Content string `json:"content" jsonschema:"The full plan content (markdown). Replaces the existing plan."` Title string `json:"title,omitempty" jsonschema:"Optional human-readable title. Preserved from the previous revision when omitted."` - Author string `json:"author,omitempty" jsonschema:"Optional label identifying who is writing the plan (typically the agent name)."` + Author string `json:"author,omitempty" jsonschema:"Optional label identifying who is writing the plan (typically the agent name). Preserved from the previous revision when omitted."` } func (t *ToolSet) writePlan(_ context.Context, params WritePlanArgs) (*tools.ToolCallResult, error) { @@ -171,13 +194,21 @@ func (t *ToolSet) writePlan(_ context.Context, params WritePlanArgs) (*tools.Too t.mu.Lock() defer t.mu.Unlock() - plan, _ := t.load(path) - plan.Name = sanitizeName(params.Name) + plan, _, err := t.load(path) + if err != nil { + return tools.ResultError(err.Error()), nil + } + plan.Name = params.Name plan.Content = params.Content + // Title and author are preserved across revisions when omitted, so an + // agent updating only the content does not wipe the collaboration metadata + // set by a previous writer. if params.Title != "" { plan.Title = params.Title } - plan.Author = params.Author + if params.Author != "" { + plan.Author = params.Author + } plan.Revision++ plan.UpdatedAt = time.Now().UTC().Format(time.RFC3339) @@ -201,7 +232,10 @@ func (t *ToolSet) readPlan(_ context.Context, params ReadPlanArgs) (*tools.ToolC t.mu.Lock() defer t.mu.Unlock() - plan, ok := t.load(path) + plan, ok, err := t.load(path) + if err != nil { + return tools.ResultError(err.Error()), nil + } if !ok { return tools.ResultError(fmt.Sprintf("plan %q not found", params.Name)), nil } @@ -215,7 +249,7 @@ func (t *ToolSet) listPlans(_ context.Context, _ tools.ToolCall) (*tools.ToolCal entries, err := os.ReadDir(t.dir) if err != nil { - if os.IsNotExist(err) { + if errors.Is(err, os.ErrNotExist) { return tools.ResultJSON([]Summary{}), nil } return tools.ResultError(err.Error()), nil @@ -226,8 +260,10 @@ func (t *ToolSet) listPlans(_ context.Context, _ tools.ToolCall) (*tools.ToolCal if entry.IsDir() || filepath.Ext(entry.Name()) != ".json" { continue } - plan, ok := t.load(filepath.Join(t.dir, entry.Name())) - if !ok { + plan, ok, err := t.load(filepath.Join(t.dir, entry.Name())) + if err != nil || !ok { + // Skip unreadable or corrupt files so one bad plan doesn't break + // listing the rest; read_plan surfaces the specific error. continue } summaries = append(summaries, Summary{ @@ -259,14 +295,16 @@ func (t *ToolSet) deletePlan(_ context.Context, params DeletePlanArgs) (*tools.T t.mu.Lock() defer t.mu.Unlock() - if _, ok := t.load(path); !ok { - return tools.ResultError(fmt.Sprintf("plan %q not found", params.Name)), nil - } + // Remove the file directly and treat a missing file as "not found". We + // don't pre-load the plan: a corrupt plan should still be deletable. if err := os.Remove(path); err != nil { + if errors.Is(err, os.ErrNotExist) { + return tools.ResultError(fmt.Sprintf("plan %q not found", params.Name)), nil + } return tools.ResultError(err.Error()), nil } - return tools.ResultJSON(map[string]string{"deleted": sanitizeName(params.Name)}), nil + return tools.ResultJSON(map[string]string{"deleted": params.Name}), nil } func (t *ToolSet) Tools(context.Context) ([]tools.Tool, error) { diff --git a/pkg/tools/builtin/plan/plan_test.go b/pkg/tools/builtin/plan/plan_test.go index a6aef53964..f54c35ec68 100644 --- a/pkg/tools/builtin/plan/plan_test.go +++ b/pkg/tools/builtin/plan/plan_test.go @@ -2,6 +2,8 @@ package plan import ( "encoding/json" + "os" + "path/filepath" "testing" "github.com/stretchr/testify/assert" @@ -41,7 +43,7 @@ func TestPlanTool_Write(t *testing.T) { tool := newTestPlanTool(t) result, err := tool.writePlan(t.Context(), WritePlanArgs{ - Name: "Release", + Name: "release", Content: "Step 1: do the thing", Title: "Release plan", Author: "planner", @@ -68,13 +70,47 @@ func TestPlanTool_WriteEmptyContent(t *testing.T) { assert.Contains(t, result.Output, "content must not be empty") } -func TestPlanTool_WriteInvalidName(t *testing.T) { +func TestPlanTool_InvalidNames(t *testing.T) { tool := newTestPlanTool(t) - result, err := tool.writePlan(t.Context(), WritePlanArgs{Name: "///", Content: "x"}) + for _, name := range []string{"", "///", "Has Space", "UPPER", "../escape", "a/b", "-leading", "with.dot"} { + result, err := tool.writePlan(t.Context(), WritePlanArgs{Name: name, Content: "x"}) + require.NoError(t, err) + assert.True(t, result.IsError, "name %q should be rejected", name) + assert.Contains(t, result.Output, "invalid plan name") + } +} + +func TestPlanTool_ValidNames(t *testing.T) { + tool := newTestPlanTool(t) + + for _, name := range []string{"release", "release-2025", "db_migration", "a", "1plan"} { + result, err := tool.writePlan(t.Context(), WritePlanArgs{Name: name, Content: "x"}) + require.NoError(t, err) + assert.False(t, result.IsError, "name %q should be accepted: %s", name, result.Output) + } +} + +func TestPlanTool_NoSilentCollision(t *testing.T) { + tool := newTestPlanTool(t) + + // "a-b" is valid; "a/b" and "a b" are rejected outright rather than + // being silently mapped onto "a-b" and clobbering it. + _, err := tool.writePlan(t.Context(), WritePlanArgs{Name: "a-b", Content: "original"}) require.NoError(t, err) - assert.True(t, result.IsError) - assert.Contains(t, result.Output, "invalid plan name") + + for _, colliding := range []string{"a/b", "a b", "a!b"} { + result, err := tool.writePlan(t.Context(), WritePlanArgs{Name: colliding, Content: "evil"}) + require.NoError(t, err) + assert.True(t, result.IsError, "name %q must not be accepted", colliding) + } + + // The original plan is untouched. + result, err := tool.readPlan(t.Context(), ReadPlanArgs{Name: "a-b"}) + require.NoError(t, err) + var plan Plan + require.NoError(t, json.Unmarshal([]byte(result.Output), &plan)) + assert.Equal(t, "original", plan.Content) } func TestPlanTool_ReadNotFound(t *testing.T) { @@ -86,6 +122,19 @@ func TestPlanTool_ReadNotFound(t *testing.T) { assert.Contains(t, result.Output, "not found") } +func TestPlanTool_ReadCorruptReportsError(t *testing.T) { + tool := newTestPlanTool(t) + + // Write a corrupt plan file directly. + require.NoError(t, os.WriteFile(filepath.Join(tool.dir, "broken.json"), []byte("{not json"), 0o600)) + + result, err := tool.readPlan(t.Context(), ReadPlanArgs{Name: "broken"}) + require.NoError(t, err) + assert.True(t, result.IsError) + assert.Contains(t, result.Output, "corrupt") + assert.NotContains(t, result.Output, "not found") +} + func TestPlanTool_WriteThenRead(t *testing.T) { tool := newTestPlanTool(t) @@ -102,13 +151,13 @@ func TestPlanTool_WriteThenRead(t *testing.T) { assert.Equal(t, "T", plan.Title) } -func TestPlanTool_RevisionIncrementsAndTitlePreserved(t *testing.T) { +func TestPlanTool_RevisionIncrementsAndMetadataPreserved(t *testing.T) { tool := newTestPlanTool(t) - _, err := tool.writePlan(t.Context(), WritePlanArgs{Name: "p", Content: "v1", Title: "Original"}) + _, err := tool.writePlan(t.Context(), WritePlanArgs{Name: "p", Content: "v1", Title: "Original", Author: "alice"}) require.NoError(t, err) - // Second write omits the title; it should be preserved. + // Second write omits the title and author; both should be preserved. result, err := tool.writePlan(t.Context(), WritePlanArgs{Name: "p", Content: "v2"}) require.NoError(t, err) @@ -116,9 +165,24 @@ func TestPlanTool_RevisionIncrementsAndTitlePreserved(t *testing.T) { require.NoError(t, json.Unmarshal([]byte(result.Output), &plan)) assert.Equal(t, "v2", plan.Content) assert.Equal(t, "Original", plan.Title) + assert.Equal(t, "alice", plan.Author) assert.Equal(t, 2, plan.Revision) } +func TestPlanTool_AuthorCanBeUpdated(t *testing.T) { + tool := newTestPlanTool(t) + + _, err := tool.writePlan(t.Context(), WritePlanArgs{Name: "p", Content: "v1", Author: "alice"}) + require.NoError(t, err) + + result, err := tool.writePlan(t.Context(), WritePlanArgs{Name: "p", Content: "v2", Author: "bob"}) + require.NoError(t, err) + + var plan Plan + require.NoError(t, json.Unmarshal([]byte(result.Output), &plan)) + assert.Equal(t, "bob", plan.Author) +} + func TestPlanTool_ListPlans(t *testing.T) { tool := newTestPlanTool(t) @@ -151,6 +215,23 @@ func TestPlanTool_ListEmpty(t *testing.T) { assert.Empty(t, summaries) } +func TestPlanTool_ListSkipsCorrupt(t *testing.T) { + tool := newTestPlanTool(t) + + _, err := tool.writePlan(t.Context(), WritePlanArgs{Name: "good", Content: "ok"}) + require.NoError(t, err) + require.NoError(t, os.WriteFile(filepath.Join(tool.dir, "bad.json"), []byte("{nope"), 0o600)) + + result, err := tool.listPlans(t.Context(), tools.ToolCall{}) + require.NoError(t, err) + assert.False(t, result.IsError) + + var summaries []Summary + require.NoError(t, json.Unmarshal([]byte(result.Output), &summaries)) + require.Len(t, summaries, 1) + assert.Equal(t, "good", summaries[0].Name) +} + func TestPlanTool_DeletePlan(t *testing.T) { tool := newTestPlanTool(t) @@ -168,6 +249,16 @@ func TestPlanTool_DeletePlan(t *testing.T) { assert.True(t, readResult.IsError) } +func TestPlanTool_DeleteCorruptSucceeds(t *testing.T) { + tool := newTestPlanTool(t) + + require.NoError(t, os.WriteFile(filepath.Join(tool.dir, "broken.json"), []byte("{nope"), 0o600)) + + result, err := tool.deletePlan(t.Context(), DeletePlanArgs{Name: "broken"}) + require.NoError(t, err) + assert.False(t, result.IsError, "a corrupt plan should still be deletable") +} + func TestPlanTool_DeleteNotFound(t *testing.T) { tool := newTestPlanTool(t) @@ -201,22 +292,6 @@ func TestPlanTool_SharedAcrossInstances(t *testing.T) { assert.Equal(t, "agent-a", plan.Author) } -func TestPlanTool_NameSanitizationIsStable(t *testing.T) { - tool := newTestPlanTool(t) - - _, err := tool.writePlan(t.Context(), WritePlanArgs{Name: "My Plan!", Content: "v1"}) - require.NoError(t, err) - - // A differently-formatted but equivalent name resolves to the same plan. - result, err := tool.readPlan(t.Context(), ReadPlanArgs{Name: "my plan"}) - require.NoError(t, err) - assert.False(t, result.IsError) - - var plan Plan - require.NoError(t, json.Unmarshal([]byte(result.Output), &plan)) - assert.Equal(t, "my-plan", plan.Name) -} - func TestPlanTool_ParametersAreObjects(t *testing.T) { tool := newTestPlanTool(t) From c4d898b4ee2c291e26834d381a58dd16b2f446c1 Mon Sep 17 00:00:00 2001 From: David Gageot Date: Thu, 25 Jun 2026 15:11:59 +0200 Subject: [PATCH 3/3] fix(plan): address PR review feedback - list_plans now derives each plan's name from its filename rather than the name field stored inside the JSON, keeping list output consistent with the storage key that read_plan/delete_plan use. - list_plans surfaces unreadable/corrupt entries as warnings (new ListResult output type) instead of silently dropping them, so an agent can't mistake a temporarily unreadable plan for a missing one and clobber it. - CreateToolSet no longer memoizes a directory-creation failure via sync.OnceValues: it builds the (infallible) ToolSet struct once with sync.OnceValue and relies on save()'s own MkdirAll, so a late-arriving plans directory is recovered from automatically. Assisted-By: claude-sonnet-4-5 --- pkg/tools/builtin/plan/plan.go | 56 ++++++++++++++++++++--------- pkg/tools/builtin/plan/plan_test.go | 54 +++++++++++++++++++++------- 2 files changed, 81 insertions(+), 29 deletions(-) diff --git a/pkg/tools/builtin/plan/plan.go b/pkg/tools/builtin/plan/plan.go index cf7e83f1ed..7117a01b1a 100644 --- a/pkg/tools/builtin/plan/plan.go +++ b/pkg/tools/builtin/plan/plan.go @@ -23,6 +23,7 @@ import ( "path/filepath" "regexp" "sort" + "strings" "sync" "time" @@ -67,6 +68,15 @@ type Summary struct { UpdatedAt string `json:"updatedAt"` } +// ListResult is the output of list_plans. Warnings lists plan files that could +// not be read or decoded, so a caller can tell "no plans exist" apart from +// "some plans failed to load" — important because an agent that mistakes a +// temporarily unreadable plan for a missing one could recreate and clobber it. +type ListResult struct { + Plans []Summary `json:"plans"` + Warnings []string `json:"warnings,omitempty"` +} + type ToolSet struct { mu sync.Mutex dir string @@ -81,19 +91,21 @@ var ( // sharedToolSet returns the one ToolSet shared by every agent in this process, // built once on first use. Sharing a single instance means all collaborating // agents serialize their plan operations on the same mutex. -var sharedToolSet = sync.OnceValues(func() (*ToolSet, error) { - dir := DefaultDir() - if err := os.MkdirAll(dir, 0o700); err != nil { - return nil, fmt.Errorf("failed to create plans directory: %w", err) - } - return New(dir), nil +// +// Building the struct cannot fail, so the directory is *not* created here: +// save() runs os.MkdirAll on every write, which means a directory that is +// momentarily unavailable at startup (e.g. a not-yet-mounted parent) is +// recovered from automatically instead of being permanently memoized as an +// error. +var sharedToolSet = sync.OnceValue(func() *ToolSet { + return New(DefaultDir()) }) // CreateToolSet is used by the tools registry. It returns a process-wide // singleton so that all agents collaborating in the same process share one // lock over the global plans folder. func CreateToolSet() (tools.ToolSet, error) { - return sharedToolSet() + return sharedToolSet(), nil } // DefaultDir is the global shared folder where plans are stored, under the @@ -250,24 +262,34 @@ func (t *ToolSet) listPlans(_ context.Context, _ tools.ToolCall) (*tools.ToolCal entries, err := os.ReadDir(t.dir) if err != nil { if errors.Is(err, os.ErrNotExist) { - return tools.ResultJSON([]Summary{}), nil + return tools.ResultJSON(ListResult{Plans: []Summary{}}), nil } return tools.ResultError(err.Error()), nil } - summaries := make([]Summary, 0, len(entries)) + result := ListResult{Plans: []Summary{}} for _, entry := range entries { if entry.IsDir() || filepath.Ext(entry.Name()) != ".json" { continue } + // The filename is the authoritative key: read_plan and delete_plan + // resolve a name to .json, so the listed name must match the + // filename, not the (possibly drifted) name field inside the JSON. + name := strings.TrimSuffix(entry.Name(), ".json") plan, ok, err := t.load(filepath.Join(t.dir, entry.Name())) if err != nil || !ok { - // Skip unreadable or corrupt files so one bad plan doesn't break - // listing the rest; read_plan surfaces the specific error. + // Don't abort the whole listing on one bad file, but surface it as + // a warning so the caller doesn't mistake an unreadable plan for a + // missing one. + msg := "unreadable" + if err != nil { + msg = err.Error() + } + result.Warnings = append(result.Warnings, fmt.Sprintf("skipped %q: %s", name, msg)) continue } - summaries = append(summaries, Summary{ - Name: plan.Name, + result.Plans = append(result.Plans, Summary{ + Name: name, Title: plan.Title, Author: plan.Author, Revision: plan.Revision, @@ -275,11 +297,11 @@ func (t *ToolSet) listPlans(_ context.Context, _ tools.ToolCall) (*tools.ToolCal }) } - sort.Slice(summaries, func(i, j int) bool { - return summaries[i].Name < summaries[j].Name + sort.Slice(result.Plans, func(i, j int) bool { + return result.Plans[i].Name < result.Plans[j].Name }) - return tools.ResultJSON(summaries), nil + return tools.ResultJSON(result), nil } type DeletePlanArgs struct { @@ -336,7 +358,7 @@ func (t *ToolSet) Tools(context.Context) ([]tools.Tool, error) { Name: ToolNameListPlans, Category: "plan", Description: "List all shared plans with their name, title, author, and revision.", - OutputSchema: tools.MustSchemaFor[[]Summary](), + OutputSchema: tools.MustSchemaFor[ListResult](), Handler: t.listPlans, Annotations: tools.ToolAnnotations{ Title: "List Plans", diff --git a/pkg/tools/builtin/plan/plan_test.go b/pkg/tools/builtin/plan/plan_test.go index f54c35ec68..08693438cb 100644 --- a/pkg/tools/builtin/plan/plan_test.go +++ b/pkg/tools/builtin/plan/plan_test.go @@ -195,12 +195,13 @@ func TestPlanTool_ListPlans(t *testing.T) { require.NoError(t, err) assert.False(t, result.IsError) - var summaries []Summary - require.NoError(t, json.Unmarshal([]byte(result.Output), &summaries)) - require.Len(t, summaries, 2) + var list ListResult + require.NoError(t, json.Unmarshal([]byte(result.Output), &list)) + require.Len(t, list.Plans, 2) // Sorted by name. - assert.Equal(t, "alpha", summaries[0].Name) - assert.Equal(t, "beta", summaries[1].Name) + assert.Equal(t, "alpha", list.Plans[0].Name) + assert.Equal(t, "beta", list.Plans[1].Name) + assert.Empty(t, list.Warnings) } func TestPlanTool_ListEmpty(t *testing.T) { @@ -210,9 +211,10 @@ func TestPlanTool_ListEmpty(t *testing.T) { require.NoError(t, err) assert.False(t, result.IsError) - var summaries []Summary - require.NoError(t, json.Unmarshal([]byte(result.Output), &summaries)) - assert.Empty(t, summaries) + var list ListResult + require.NoError(t, json.Unmarshal([]byte(result.Output), &list)) + assert.Empty(t, list.Plans) + assert.Empty(t, list.Warnings) } func TestPlanTool_ListSkipsCorrupt(t *testing.T) { @@ -226,10 +228,38 @@ func TestPlanTool_ListSkipsCorrupt(t *testing.T) { require.NoError(t, err) assert.False(t, result.IsError) - var summaries []Summary - require.NoError(t, json.Unmarshal([]byte(result.Output), &summaries)) - require.Len(t, summaries, 1) - assert.Equal(t, "good", summaries[0].Name) + var list ListResult + require.NoError(t, json.Unmarshal([]byte(result.Output), &list)) + require.Len(t, list.Plans, 1) + assert.Equal(t, "good", list.Plans[0].Name) + // The corrupt file is surfaced as a warning rather than silently dropped. + require.Len(t, list.Warnings, 1) + assert.Contains(t, list.Warnings[0], "bad") +} + +func TestPlanTool_ListNameFromFilename(t *testing.T) { + tool := newTestPlanTool(t) + + // A plan file whose stored name field disagrees with its filename. The + // filename is authoritative because read_plan/delete_plan key off it. + drifted := Plan{Name: "wrong-name", Content: "x", Revision: 1} + data, err := json.Marshal(drifted) + require.NoError(t, err) + require.NoError(t, os.WriteFile(filepath.Join(tool.dir, "real-name.json"), data, 0o600)) + + result, err := tool.listPlans(t.Context(), tools.ToolCall{}) + require.NoError(t, err) + assert.False(t, result.IsError) + + var list ListResult + require.NoError(t, json.Unmarshal([]byte(result.Output), &list)) + require.Len(t, list.Plans, 1) + // The name returned matches the filename, so a follow-up read_plan works. + assert.Equal(t, "real-name", list.Plans[0].Name) + + read, err := tool.readPlan(t.Context(), ReadPlanArgs{Name: list.Plans[0].Name}) + require.NoError(t, err) + assert.False(t, read.IsError) } func TestPlanTool_DeletePlan(t *testing.T) {