From 448fa70acb932a523b59b796ec316f26e1a6a56f Mon Sep 17 00:00:00 2001 From: Trung Nguyen Date: Tue, 8 Sep 2026 09:56:04 +0200 Subject: [PATCH] feat(plan)!: remove the session_plan toolset Remove the per-session plan toolset (write_session_plan, read_session_plan, exit_plan_mode) and every session-plan tendril: - pkg/tools/builtin/sessionplan and its runtime-owned handlers - the session_plan_updated runtime event and its client decoder - the session scope in pkg/plans (Service.List no longer takes options, UpdateSession and UnsupportedError are gone) - session-plan rows in the TUI /plans browser and detail dialog - the --session/--scope flags on the plans CLI - the session_plan schema enum value, example, and docs The shared plan toolset, /plans browser, and plans CLI are unchanged for shared plans. Downstream consumers have migrated off the per-session workflow. BREAKING CHANGE: the session_plan toolset type is no longer accepted in agent configs. --- agent-schema.json | 4 +- cmd/root/plans.go | 195 ++----- cmd/root/plans_test.go | 257 +++------- cmd/root/plans_unix_test.go | 6 +- docs/concepts/tools/index.md | 1 - docs/configuration/tools/index.md | 1 - docs/data/nav.yml | 2 - docs/features/cli/index.md | 27 +- docs/features/tui/index.md | 2 +- docs/tools/plan/index.md | 16 +- docs/tools/session_plan/index.md | 134 ----- examples/session_plan.yaml | 48 -- pkg/plans/errors.go | 24 +- pkg/plans/json_test.go | 34 +- pkg/plans/plans.go | 107 +--- pkg/plans/service.go | 241 +-------- pkg/plans/service_symlink_test.go | 31 +- pkg/plans/service_test.go | 478 ++---------------- pkg/runtime/client.go | 1 - pkg/runtime/event.go | 23 - pkg/runtime/loop.go | 4 - pkg/runtime/sessionplan_handlers.go | 66 --- pkg/teamloader/toolsets/catalog.go | 1 - pkg/teamloader/toolsets/toolsets.go | 2 - .../builtin/sessioncontext/sessioncontext.go | 5 +- pkg/tools/builtin/sessionplan/sessionplan.go | 202 -------- .../builtin/sessionplan/sessionplan_test.go | 160 ------ pkg/tui/dialog/plan_browser.go | 51 +- pkg/tui/dialog/plan_browser_test.go | 103 +--- pkg/tui/dialog/plan_detail.go | 44 +- pkg/tui/dialog/plan_detail_test.go | 70 --- pkg/tui/messages/plans.go | 14 +- pkg/tui/plans.go | 161 ++---- pkg/tui/plans_test.go | 387 ++------------ pkg/tui/plans_unix_test.go | 4 +- pkg/tui/tui.go | 17 +- 36 files changed, 415 insertions(+), 2508 deletions(-) delete mode 100644 docs/tools/session_plan/index.md delete mode 100644 examples/session_plan.yaml delete mode 100644 pkg/runtime/sessionplan_handlers.go delete mode 100644 pkg/tools/builtin/sessionplan/sessionplan.go delete mode 100644 pkg/tools/builtin/sessionplan/sessionplan_test.go diff --git a/agent-schema.json b/agent-schema.json index e30adc2be0..e8b7bf49b4 100644 --- a/agent-schema.json +++ b/agent-schema.json @@ -2152,7 +2152,6 @@ "background_jobs", "tasks", "plan", - "session_plan", "session_context", "todo", "fetch", @@ -2494,8 +2493,7 @@ "background_jobs", "tasks", "plan", - "session_plan", - "session_context", + "session_context", "todo", "fetch", "api", diff --git a/cmd/root/plans.go b/cmd/root/plans.go index 49b308b872..5c27d097bf 100644 --- a/cmd/root/plans.go +++ b/cmd/root/plans.go @@ -29,13 +29,12 @@ const plansConflictExitCode = 3 // Stable machine-readable error codes of the --json stderr contract. const ( - plansErrCodeConflict = "conflict" - plansErrCodeNotFound = "not_found" - plansErrCodeInvalid = "invalid_argument" - plansErrCodeUnsupported = "unsupported" - plansErrCodeCorrupt = "corrupt" - plansErrCodeStorage = "storage" - plansErrCodeUnknown = "error" + plansErrCodeConflict = "conflict" + plansErrCodeNotFound = "not_found" + plansErrCodeInvalid = "invalid_argument" + plansErrCodeCorrupt = "corrupt" + plansErrCodeStorage = "storage" + plansErrCodeUnknown = "error" ) // plansCmdOption customizes newPlansCmd; used by tests to inject the service. @@ -70,16 +69,9 @@ func newPlansCmd(opts ...plansCmdOption) *cobra.Command { cmd := &cobra.Command{ Use: "plans", - Short: "Manage shared and session plans", - Long: `Manage the plans agents collaborate on, from the host. - -Two kinds of plans exist: - - - shared plans: the named, versioned documents of the plan toolset, - collaborated on across sessions. Fully manageable here. - - session plans: the single per-session plan of the "draft, review, - execute" workflow. Read-only here (list, get, export); they belong to - their session and are changed from within it. + Short: "Manage shared plans", + Long: `Manage the plans agents collaborate on, from the host: the named, +versioned documents of the plan toolset, collaborated on across sessions. Mutations guard against concurrent edits: pass --expected-version (the version from a previous get or list) to fail with exit code 3 when the plan @@ -94,8 +86,7 @@ failures are then reported as a single JSON object on stderr.`, docker-agent plans update release --file ./plan.md --expected-version 1 docker-agent plans status release done --expected-version 2 docker-agent plans export release --output ./plan.md - docker-agent plans delete release --expected-version 3 - docker-agent plans get --session `, + docker-agent plans delete release --expected-version 3`, GroupID: "advanced", SilenceUsage: true, } @@ -127,8 +118,8 @@ failures are then reported as a single JSON object on stderr.`, func (o *plansOptions) runPlans(sub string, jsonOut *bool, handler func(cmd *cobra.Command, svc plans.Service, args []string) error) func(*cobra.Command, []string) error { return func(cmd *cobra.Command, args []string) error { ctx := cmd.Context() - // Telemetry carries the subcommand only: positional args are plan and - // session names, and raw errors embed names and filesystem paths, so + // Telemetry carries the subcommand only: positional args are plan + // names, and raw errors embed names and filesystem paths, so // failures are reduced to their stable code by plansTelemetryError. trackArgs := []string{sub} telemetry.TrackCommand(ctx, "plans", trackArgs) @@ -239,13 +230,12 @@ type plansErrorDocument struct { // error text. func plansErrorBodyFor(err error) plansErrorBody { var ( - conflict *plans.ConflictError - notFound *plans.NotFoundError - unsupported *plans.UnsupportedError - corrupt *plans.CorruptError - storageErr *plans.StorageError - validation *plans.ValidationError - usage *plansUsageError + conflict *plans.ConflictError + notFound *plans.NotFoundError + corrupt *plans.CorruptError + storageErr *plans.StorageError + validation *plans.ValidationError + usage *plansUsageError ) switch { case errors.As(err, &conflict): @@ -261,8 +251,6 @@ func plansErrorBodyFor(err error) plansErrorBody { } case errors.As(err, ¬Found): return plansErrorBody{Code: plansErrCodeNotFound, Message: err.Error(), Scope: notFound.Scope, Name: notFound.Name} - case errors.As(err, &unsupported): - return plansErrorBody{Code: plansErrCodeUnsupported, Message: err.Error(), Scope: unsupported.Scope, Op: unsupported.Op} case errors.As(err, &corrupt): return plansErrorBody{Code: plansErrCodeCorrupt, Message: err.Error(), Scope: corrupt.Scope, Name: corrupt.Name} case errors.As(err, &storageErr): @@ -276,8 +264,8 @@ func plansErrorBodyFor(err error) plansErrorBody { // plansTelemetryError sanitizes a failure for telemetry by reducing it to // its stable machine-readable code (plansErrorBodyFor): raw error text -// carries plan names, session IDs, and filesystem paths that must never be -// sent. Nil stays nil so a success is never tracked as an error. +// carries plan names and filesystem paths that must never be sent. Nil stays +// nil so a success is never tracked as an error. func plansTelemetryError(err error) error { if err == nil { return nil @@ -294,61 +282,12 @@ func printPlansError(w io.Writer, err error, jsonOut bool) { fmt.Fprintln(w, "Error:", err.Error()) } -// planRefFlags selects the plan a subcommand addresses: the shared plan named -// by the positional argument (the default), or a session's plan via -// --session. --scope disambiguates explicitly; --session alone implies -// session scope. -type planRefFlags struct { - scope string - session string -} - -func (f *planRefFlags) register(cmd *cobra.Command) { - cmd.Flags().StringVar(&f.scope, "scope", "", `Plan scope: "shared" or "session" (default "shared"; "session" is implied by --session)`) - cmd.Flags().StringVar(&f.session, "session", "", "Session ID whose plan to address (session scope)") -} - -func (f *planRefFlags) sessionSelected() bool { - return f.session != "" || f.scope == string(plans.ScopeSession) -} - -func (f *planRefFlags) resolve(name string) (plans.Ref, error) { - scope := plans.Scope(f.scope) - if f.scope == "" { - scope = plans.ScopeShared - if f.session != "" { - scope = plans.ScopeSession - } +// resolvePlanRef addresses the plan named by the positional argument. +func resolvePlanRef(name string) (plans.Ref, error) { + if name == "" { + return plans.Ref{}, plansUsagef("a plan name is required") } - switch scope { - case plans.ScopeShared: - if f.session != "" { - return plans.Ref{}, plansUsagef("--session selects a session plan: drop --scope shared or use --scope session") - } - if name == "" { - return plans.Ref{}, plansUsagef("a plan name is required for shared plans") - } - return plans.SharedRef(name), nil - case plans.ScopeSession: - if f.session == "" { - return plans.Ref{}, plansUsagef("--scope session requires --session ") - } - if name != "" { - return plans.Ref{}, plansUsagef("session plans are addressed by --session , not by name; drop %q", name) - } - return plans.SessionRef(f.session), nil - default: - return plans.Ref{}, plansUsagef("invalid --scope %q: use %q or %q", f.scope, plans.ScopeShared, plans.ScopeSession) - } -} - -// planRefName is the plan's identity within its scope, for messages and the -// delete document. -func planRefName(ref plans.Ref) string { - if ref.Scope == plans.ScopeSession { - return ref.SessionID - } - return ref.Name + return plans.SharedRef(name), nil } // planGuardFlags implements the mutation write-guard: --expected-version @@ -514,7 +453,7 @@ func printPlanMutation(cmd *cobra.Command, jsonOut bool, p plans.Plan, verb stri // sends it to stderr so stdout stays pure content while the metadata remains // visible. func printPlanMetadata(w io.Writer, p plans.Plan) { - details := make([]string, 0, 5) + details := make([]string, 0, 4) if p.Title != "" { details = append(details, "title: "+p.Title) } @@ -525,9 +464,6 @@ func printPlanMetadata(w io.Writer, p plans.Plan) { if !p.UpdatedAt.IsZero() { details = append(details, "updated: "+formatPlanTime(p.UpdatedAt)) } - if p.Path != "" { - details = append(details, "path: "+p.Path) - } fmt.Fprintf(w, "%s plan %q (%s)\n", p.Scope, p.Name, strings.Join(details, ", ")) } @@ -547,8 +483,7 @@ func printPlansTable(w io.Writer, list []plans.Plan) { func newPlansListCmd(o *plansOptions) *cobra.Command { var flags struct { - json bool - session string + json bool } cmd := &cobra.Command{ @@ -557,14 +492,12 @@ func newPlansListCmd(o *plansOptions) *cobra.Command { Short: "List plans", Long: `List every shared plan with its metadata (content is not included). -With --session , that session's plan is listed first when it exists; a -session without a plan is simply not listed. Plans that exist but cannot be -read are reported as warnings on stderr (in the "warnings" field with --json) -so they are never mistaken for missing.`, +Plans that exist but cannot be read are reported as warnings on stderr (in +the "warnings" field with --json) so they are never mistaken for missing.`, Args: cobra.NoArgs, } cmd.RunE = o.runPlans("list", &flags.json, func(cmd *cobra.Command, svc plans.Service, _ []string) error { - result, err := svc.List(cmd.Context(), plans.ListOptions{SessionID: flags.session}) + result, err := svc.List(cmd.Context()) if err != nil { return err } @@ -588,7 +521,6 @@ so they are never mistaken for missing.`, }) cmd.Flags().BoolVar(&flags.json, "json", false, "Output as JSON") - cmd.Flags().StringVar(&flags.session, "session", "", "Also include this session's plan when it exists") return cmd } @@ -596,26 +528,20 @@ so they are never mistaken for missing.`, func newPlansGetCmd(o *plansOptions) *cobra.Command { var flags struct { json bool - ref planRefFlags } cmd := &cobra.Command{ - Use: "get []", + Use: "get ", Short: "Print a plan's content and metadata", Long: `Print a plan: its content goes to stdout and a concise metadata line goes to stderr, so redirecting stdout captures the content alone (use export for a -byte-exact file copy). - -By default addresses a shared plan. Use --session to print that -session's plan instead; the name is then omitted.`, +byte-exact file copy).`, Example: ` docker-agent plans get release - docker-agent plans get release --json - docker-agent plans get --session - docker-agent plans get --scope session --session `, + docker-agent plans get release --json`, Args: cobra.MaximumNArgs(1), } cmd.RunE = o.runPlans("get", &flags.json, func(cmd *cobra.Command, svc plans.Service, args []string) error { - ref, err := flags.ref.resolve(firstArg(args)) + ref, err := resolvePlanRef(firstArg(args)) if err != nil { return err } @@ -635,7 +561,6 @@ session's plan instead; the name is then omitted.`, return nil }) - flags.ref.register(cmd) cmd.Flags().BoolVar(&flags.json, "json", false, "Output as JSON") return cmd @@ -697,7 +622,6 @@ func newPlansUpdateCmd(o *plansOptions) *cobra.Command { title string author string status string - ref planRefFlags guard planGuardFlags } @@ -712,13 +636,13 @@ Metadata flags that are omitted keep their previous value; passing them Exactly one of --expected-version or --force must be given: the former fails with exit code 3 when the plan changed since it was read, the latter -deliberately replaces it unconditionally. Session plans cannot be updated.`, +deliberately replaces it unconditionally.`, Example: ` docker-agent plans update release --file ./plan.md --expected-version 1 docker-agent plans update release --file ./plan.md --force --status in-progress`, Args: cobra.MaximumNArgs(1), } cmd.RunE = o.runPlans("update", &flags.json, func(cmd *cobra.Command, svc plans.Service, args []string) error { - ref, err := flags.ref.resolve(firstArg(args)) + ref, err := resolvePlanRef(firstArg(args)) if err != nil { return err } @@ -748,7 +672,6 @@ deliberately replaces it unconditionally. Session plans cannot be updated.`, return printPlanMutation(cmd, flags.json, p, "Updated") }) - flags.ref.register(cmd) flags.guard.register(cmd) cmd.Flags().StringVar(&flags.file, "file", "", `File with the new plan content ("-" reads stdin); required`) cmd.Flags().StringVar(&flags.title, "title", "", "New plan title (omit to preserve the current one)") @@ -763,7 +686,6 @@ deliberately replaces it unconditionally. Session plans cannot be updated.`, func newPlansStatusCmd(o *plansOptions) *cobra.Command { var flags struct { json bool - ref planRefFlags guard planGuardFlags } @@ -774,28 +696,17 @@ func newPlansStatusCmd(o *plansOptions) *cobra.Command { "done") without touching its body. Setting the status is a write and bumps the version. -Exactly one of --expected-version or --force must be given. Session plans -have no status.`, +Exactly one of --expected-version or --force must be given.`, Example: ` docker-agent plans status release done --expected-version 2 docker-agent plans status release blocked --force`, Args: cobra.RangeArgs(1, 2), } cmd.RunE = o.runPlans("status", &flags.json, func(cmd *cobra.Command, svc plans.Service, args []string) error { - var name, status string - if flags.ref.sessionSelected() { - // Session plans have no name, so the only positional is the - // status; the service then rejects the mutation as unsupported. - if len(args) != 1 { - return plansUsagef("session plans take only a status: plans status --session ") - } - status = args[0] - } else { - if len(args) != 2 { - return plansUsagef("shared plans take a name and a status: plans status ") - } - name, status = args[0], args[1] + if len(args) != 2 { + return plansUsagef("status takes a name and a status: plans status ") } - ref, err := flags.ref.resolve(name) + name, status := args[0], args[1] + ref, err := resolvePlanRef(name) if err != nil { return err } @@ -815,7 +726,6 @@ have no status.`, return nil }) - flags.ref.register(cmd) flags.guard.register(cmd) cmd.Flags().BoolVar(&flags.json, "json", false, "Output as JSON") @@ -827,25 +737,21 @@ func newPlansExportCmd(o *plansOptions) *cobra.Command { json bool output string force bool - ref planRefFlags } cmd := &cobra.Command{ - Use: "export []", + Use: "export ", Short: "Write a plan's content to a file", Long: `Write a plan's content, byte-exact, to --output (required). Parent directories are created and the write is atomic, so a reader never observes a partial export. An existing destination is refused and left untouched -unless --force is given, which replaces an existing regular file atomically. -Works for both scopes: shared plans by name, a session's plan via ---session .`, +unless --force is given, which replaces an existing regular file atomically.`, Example: ` docker-agent plans export release --output ./plan.md - docker-agent plans export release --output ./plan.md --force - docker-agent plans export --session --output ./plan.md`, + docker-agent plans export release --output ./plan.md --force`, Args: cobra.MaximumNArgs(1), } cmd.RunE = o.runPlans("export", &flags.json, func(cmd *cobra.Command, svc plans.Service, args []string) error { - ref, err := flags.ref.resolve(firstArg(args)) + ref, err := resolvePlanRef(firstArg(args)) if err != nil { return err } @@ -861,7 +767,6 @@ Works for both scopes: shared plans by name, a session's plan via return nil }) - flags.ref.register(cmd) cmd.Flags().StringVar(&flags.output, "output", "", "Destination file for the plan content; required") cmd.Flags().BoolVar(&flags.force, "force", false, "Replace the destination file when it already exists") cmd.Flags().BoolVar(&flags.json, "json", false, "Output as JSON") @@ -873,7 +778,6 @@ Works for both scopes: shared plans by name, a session's plan via func newPlansDeleteCmd(o *plansOptions) *cobra.Command { var flags struct { json bool - ref planRefFlags guard planGuardFlags } @@ -885,13 +789,13 @@ func newPlansDeleteCmd(o *plansOptions) *cobra.Command { --expected-version or --force must be given: the former fails with exit code 3 when the plan changed since it was read (leaving it in place), the latter deletes unconditionally — which is also how a corrupt plan is -recovered. Session plans cannot be deleted from the host.`, +recovered.`, Example: ` docker-agent plans delete release --expected-version 3 docker-agent plans delete release --force`, Args: cobra.MaximumNArgs(1), } cmd.RunE = o.runPlans("delete", &flags.json, func(cmd *cobra.Command, svc plans.Service, args []string) error { - ref, err := flags.ref.resolve(firstArg(args)) + ref, err := resolvePlanRef(firstArg(args)) if err != nil { return err } @@ -905,14 +809,13 @@ recovered. Session plans cannot be deleted from the host.`, if flags.json { return writePlansJSON(cmd.OutOrStdout(), plansDeletedDocument{ SchemaVersion: plansSchemaVersion, - Deleted: plansRefBody{Scope: ref.Scope, Name: planRefName(ref)}, + Deleted: plansRefBody{Scope: ref.Scope, Name: ref.Name}, }) } - fmt.Fprintf(cmd.OutOrStdout(), "Deleted %s plan %q\n", ref.Scope, planRefName(ref)) + fmt.Fprintf(cmd.OutOrStdout(), "Deleted %s plan %q\n", ref.Scope, ref.Name) return nil }) - flags.ref.register(cmd) flags.guard.register(cmd) cmd.Flags().BoolVar(&flags.json, "json", false, "Output as JSON") diff --git a/cmd/root/plans_test.go b/cmd/root/plans_test.go index 5c0bd234d2..0ce1471b0e 100644 --- a/cmd/root/plans_test.go +++ b/cmd/root/plans_test.go @@ -18,18 +18,16 @@ import ( "github.com/docker/docker-agent/pkg/plans" "github.com/docker/docker-agent/pkg/tools/builtin/plan" - "github.com/docker/docker-agent/pkg/tools/builtin/sessionplan" ) -// newPlansTestService builds a hermetic plans.Service over temp directories, -// returning both so tests can plant files directly. No test ever touches the +// newPlansTestService builds a hermetic plans.Service over a temp directory, +// returning it so tests can plant files directly. No test ever touches the // real user data directory. -func newPlansTestService(t *testing.T) (svc plans.Service, sharedDir, sessionDir string) { +func newPlansTestService(t *testing.T) (svc plans.Service, sharedDir string) { t.Helper() sharedDir = t.TempDir() - sessionDir = t.TempDir() - svc = plans.NewService(plan.NewFilesystemStorage(sharedDir), plans.WithSessionDir(sessionDir)) - return svc, sharedDir, sessionDir + svc = plans.NewService(plan.NewFilesystemStorage(sharedDir)) + return svc, sharedDir } func executePlansIn(t *testing.T, svc plans.Service, stdin io.Reader, args ...string) (stdout, stderr string, err error) { @@ -71,13 +69,6 @@ func writePlanContentFile(t *testing.T, content string) string { return path } -func writeSessionPlanFile(t *testing.T, dir, sessionID, content string) string { - t.Helper() - path, err := sessionplan.WriteContent(dir, sessionID, content) - require.NoError(t, err) - return path -} - func requirePlansStatusCode(t *testing.T, err error, want int) { t.Helper() require.Error(t, err) @@ -145,7 +136,7 @@ func TestPlansCommand_RegisteredOnRoot(t *testing.T) { func TestPlansList_EmptyJSON(t *testing.T) { t.Parallel() - svc, _, _ := newPlansTestService(t) + svc, _ := newPlansTestService(t) stdout, stderr, err := executePlans(t, svc, "list", "--json") require.NoError(t, err) @@ -162,21 +153,18 @@ func TestPlansList_EmptyJSON(t *testing.T) { func TestPlansList_HumanShowsMetadataAndSendsWarningsToStderr(t *testing.T) { t.Parallel() - svc, sharedDir, sessionDir := newPlansTestService(t) + svc, sharedDir := newPlansTestService(t) _, err := svc.Create(t.Context(), plans.CreateRequest{ Ref: plans.SharedRef("alpha"), Content: "body", Title: "Alpha plan", Status: "draft", }) require.NoError(t, err) - writeSessionPlanFile(t, sessionDir, "sess-1", "# session plan") require.NoError(t, os.WriteFile(filepath.Join(sharedDir, "bad.json"), []byte("{nope"), 0o600)) - stdout, stderr, err := executePlans(t, svc, "list", "--session", "sess-1") + stdout, stderr, err := executePlans(t, svc, "list") require.NoError(t, err) assert.Regexp(t, `SCOPE\s+NAME\s+STATUS\s+VERSION\s+UPDATED\s+TITLE`, stdout) assert.Regexp(t, `shared\s+alpha\s+draft\s+1\s+\S+\s+Alpha plan`, stdout) - // Session plans have no version or status: shown as "-". - assert.Regexp(t, `session\s+sess-1\s+-\s+-\s+\S+\s+-`, stdout) assert.NotContains(t, stdout, "\x1b[", "human output must be ANSI-free") // Human-mode warnings go to stderr, not stdout. @@ -187,30 +175,22 @@ func TestPlansList_HumanShowsMetadataAndSendsWarningsToStderr(t *testing.T) { func TestPlansList_JSONMetadataAndWarnings(t *testing.T) { t.Parallel() - svc, sharedDir, sessionDir := newPlansTestService(t) + svc, sharedDir := newPlansTestService(t) _, err := svc.Create(t.Context(), plans.CreateRequest{ Ref: plans.SharedRef("alpha"), Content: "body", Title: "Alpha plan", Author: "alice", Status: "draft", }) require.NoError(t, err) - writeSessionPlanFile(t, sessionDir, "sess-1", "# session plan") require.NoError(t, os.WriteFile(filepath.Join(sharedDir, "bad.json"), []byte("{nope"), 0o600)) - stdout, stderr, err := executePlans(t, svc, "list", "--session", "sess-1", "--json") + stdout, stderr, err := executePlans(t, svc, "list", "--json") require.NoError(t, err) assert.Empty(t, stderr, "JSON mode must not write warnings to stderr") var doc plansTestListDocument require.NoError(t, json.Unmarshal([]byte(stdout), &doc)) - require.Len(t, doc.Plans, 2) + require.Len(t, doc.Plans, 1) - sess := doc.Plans[0] - assert.Equal(t, plans.ScopeSession, sess.Scope) - assert.Equal(t, "sess-1", sess.Name) - assert.Equal(t, "sess-1", sess.SessionID) - assert.Nil(t, sess.Version) - assert.Empty(t, sess.Content, "list is metadata only") - - shared := doc.Plans[1] + shared := doc.Plans[0] assert.Equal(t, plans.ScopeShared, shared.Scope) assert.Equal(t, "alpha", shared.Name) assert.Equal(t, "Alpha plan", shared.Title) @@ -218,27 +198,17 @@ func TestPlansList_JSONMetadataAndWarnings(t *testing.T) { assert.Equal(t, "draft", shared.Status) require.NotNil(t, shared.Version) assert.Equal(t, 1, *shared.Version) + assert.Empty(t, shared.Content, "list is metadata only") require.Len(t, doc.Warnings, 1) assert.Contains(t, doc.Warnings[0], "bad") } -func TestPlansList_InvalidSessionID(t *testing.T) { - t.Parallel() - svc, _, _ := newPlansTestService(t) - - stdout, stderr, err := executePlans(t, svc, "list", "--session", "../escape", "--json") - requirePlansStatusCode(t, err, 1) - assert.Empty(t, stdout) - body := decodePlansError(t, stderr) - assert.Equal(t, "invalid_argument", body.Code) -} - // --- Get ----------------------------------------------------------------------- func TestPlansGet_SharedHuman(t *testing.T) { t.Parallel() - svc, _, _ := newPlansTestService(t) + svc, _ := newPlansTestService(t) _, err := svc.Create(t.Context(), plans.CreateRequest{ Ref: plans.SharedRef("release"), Content: "# release\n\nstep 1\n", Title: "Release", Status: "draft", }) @@ -256,7 +226,7 @@ func TestPlansGet_SharedHuman(t *testing.T) { func TestPlansGet_SharedJSON(t *testing.T) { t.Parallel() - svc, _, _ := newPlansTestService(t) + svc, _ := newPlansTestService(t) _, err := svc.Create(t.Context(), plans.CreateRequest{ Ref: plans.SharedRef("release"), Content: "the body", Title: "Release", Author: "alice", Status: "draft", }) @@ -285,63 +255,21 @@ func TestPlansGet_SharedJSON(t *testing.T) { assert.NotContains(t, stdout, `"updatedAt"`) } -func TestPlansGet_Session(t *testing.T) { - t.Parallel() - svc, _, sessionDir := newPlansTestService(t) - path := writeSessionPlanFile(t, sessionDir, "sess-1", "# session plan\n") - - // --session alone implies session scope; --scope session spells it out. - for _, args := range [][]string{ - {"get", "--session", "sess-1"}, - {"get", "--scope", "session", "--session", "sess-1"}, - } { - stdout, stderr, err := executePlans(t, svc, args...) - require.NoError(t, err, "args %v", args) - assert.Equal(t, "# session plan\n", stdout) - assert.Contains(t, stderr, `session plan "sess-1"`) - assert.Contains(t, stderr, "version: -", "session plans have no version") - } - - stdout, _, err := executePlans(t, svc, "get", "--session", "sess-1", "--json") - require.NoError(t, err) - var doc plansTestPlanDocument - require.NoError(t, json.Unmarshal([]byte(stdout), &doc)) - assert.Equal(t, plans.ScopeSession, doc.Plan.Scope) - assert.Equal(t, "sess-1", doc.Plan.SessionID) - assert.Equal(t, "# session plan\n", doc.Plan.Content) - assert.Equal(t, path, doc.Plan.Path) - assert.Nil(t, doc.Plan.Version) - assert.Contains(t, stdout, `"session_id"`) - assert.NotContains(t, stdout, `"sessionId"`) -} - -func TestPlansGet_RefValidation(t *testing.T) { +func TestPlansGet_RequiresName(t *testing.T) { t.Parallel() - svc, _, _ := newPlansTestService(t) + svc, _ := newPlansTestService(t) - tests := []struct { - args []string - wantMsg string - }{ - {[]string{"get"}, "a plan name is required"}, - {[]string{"get", "p", "--session", "sess-1"}, "addressed by --session"}, - {[]string{"get", "--scope", "session"}, "requires --session"}, - {[]string{"get", "--scope", "shared", "--session", "sess-1"}, "--session selects a session plan"}, - {[]string{"get", "p", "--scope", "bogus"}, "invalid --scope"}, - } - for _, tt := range tests { - stdout, stderr, err := executePlans(t, svc, append(tt.args, "--json")...) - requirePlansStatusCode(t, err, 1) - assert.Empty(t, stdout, "args %v", tt.args) - body := decodePlansError(t, stderr) - assert.Equal(t, "invalid_argument", body.Code, "args %v", tt.args) - assert.Contains(t, body.Message, tt.wantMsg, "args %v", tt.args) - } + stdout, stderr, err := executePlans(t, svc, "get", "--json") + requirePlansStatusCode(t, err, 1) + assert.Empty(t, stdout) + body := decodePlansError(t, stderr) + assert.Equal(t, "invalid_argument", body.Code) + assert.Contains(t, body.Message, "a plan name is required") } func TestPlansGet_NotFoundJSON(t *testing.T) { t.Parallel() - svc, _, _ := newPlansTestService(t) + svc, _ := newPlansTestService(t) stdout, stderr, err := executePlans(t, svc, "get", "missing", "--json") requirePlansStatusCode(t, err, 1) @@ -354,7 +282,7 @@ func TestPlansGet_NotFoundJSON(t *testing.T) { func TestPlansGet_NotFoundHuman(t *testing.T) { t.Parallel() - svc, _, _ := newPlansTestService(t) + svc, _ := newPlansTestService(t) stdout, stderr, err := executePlans(t, svc, "get", "missing") requirePlansStatusCode(t, err, 1) @@ -366,7 +294,7 @@ func TestPlansGet_NotFoundHuman(t *testing.T) { func TestPlansGet_InvalidName(t *testing.T) { t.Parallel() - svc, _, _ := newPlansTestService(t) + svc, _ := newPlansTestService(t) _, stderr, err := executePlans(t, svc, "get", "UPPER", "--json") requirePlansStatusCode(t, err, 1) @@ -377,7 +305,7 @@ func TestPlansGet_InvalidName(t *testing.T) { func TestPlansGet_Corrupt(t *testing.T) { t.Parallel() - svc, sharedDir, _ := newPlansTestService(t) + svc, sharedDir := newPlansTestService(t) require.NoError(t, os.WriteFile(filepath.Join(sharedDir, "broken.json"), []byte("{not json"), 0o600)) _, stderr, err := executePlans(t, svc, "get", "broken", "--json") @@ -392,7 +320,7 @@ func TestPlansGet_Corrupt(t *testing.T) { func TestPlansCreate_FromFile(t *testing.T) { t.Parallel() - svc, _, _ := newPlansTestService(t) + svc, _ := newPlansTestService(t) file := writePlanContentFile(t, "plan body") stdout, stderr, err := executePlans(t, svc, @@ -410,7 +338,7 @@ func TestPlansCreate_FromFile(t *testing.T) { func TestPlansCreate_FromStdin(t *testing.T) { t.Parallel() - svc, _, _ := newPlansTestService(t) + svc, _ := newPlansTestService(t) // Headless: content is piped through a non-TTY stdin via --file -. stdout, _, err := executePlansIn(t, svc, strings.NewReader("piped body"), "create", "p", "--file", "-", "--json") @@ -426,7 +354,7 @@ func TestPlansCreate_FromStdin(t *testing.T) { func TestPlansCreate_StdinAtSizeLimit(t *testing.T) { t.Parallel() - svc, _, _ := newPlansTestService(t) + svc, _ := newPlansTestService(t) // Exactly the cap is not "over" it: the bounded reader must pass the // content through (no off-by-one) and the real filesystem storage must @@ -447,7 +375,7 @@ func TestPlansCreate_StdinAtSizeLimit(t *testing.T) { func TestPlansCreate_FileAtSizeLimit(t *testing.T) { t.Parallel() - svc, _, _ := newPlansTestService(t) + svc, _ := newPlansTestService(t) file := writePlanContentFile(t, strings.Repeat("b", plan.MaxPlanContentSize)) _, stderr, err := executePlans(t, svc, "create", "p", "--file", file) @@ -457,7 +385,7 @@ func TestPlansCreate_FileAtSizeLimit(t *testing.T) { func TestPlansCreate_StdinOverSizeLimit(t *testing.T) { t.Parallel() - svc, _, _ := newPlansTestService(t) + svc, _ := newPlansTestService(t) // One byte over the cap must be detected and refused. content := strings.Repeat("a", plan.MaxPlanContentSize+1) @@ -475,7 +403,7 @@ func TestPlansCreate_StdinOverSizeLimit(t *testing.T) { func TestPlansCreate_EmptyStdin(t *testing.T) { t.Parallel() - svc, _, _ := newPlansTestService(t) + svc, _ := newPlansTestService(t) // Empty piped content reaches the service and is rejected there, like an // empty --file. @@ -488,7 +416,7 @@ func TestPlansCreate_EmptyStdin(t *testing.T) { func TestPlansCreate_OversizedFile(t *testing.T) { t.Parallel() - svc, _, _ := newPlansTestService(t) + svc, _ := newPlansTestService(t) big := filepath.Join(t.TempDir(), "big.md") require.NoError(t, os.WriteFile(big, make([]byte, plan.MaxPlanContentSize+1), 0o600)) @@ -501,7 +429,7 @@ func TestPlansCreate_OversizedFile(t *testing.T) { func TestPlansCreate_DirectoryAsFile(t *testing.T) { t.Parallel() - svc, _, _ := newPlansTestService(t) + svc, _ := newPlansTestService(t) _, stderr, err := executePlans(t, svc, "create", "p", "--file", t.TempDir(), "--json") requirePlansStatusCode(t, err, 1) @@ -521,7 +449,7 @@ func (r stdinMustNotBeRead) Read([]byte) (int, error) { func TestPlansCreate_RequiresFileWithoutPrompting(t *testing.T) { t.Parallel() - svc, _, _ := newPlansTestService(t) + svc, _ := newPlansTestService(t) // Without --file the command must fail immediately instead of waiting on // an interactive terminal for content. @@ -532,7 +460,7 @@ func TestPlansCreate_RequiresFileWithoutPrompting(t *testing.T) { func TestPlansCreate_ExistingNameConflicts(t *testing.T) { t.Parallel() - svc, _, _ := newPlansTestService(t) + svc, _ := newPlansTestService(t) mustCreatePlan(t, svc, "p", "original") file := writePlanContentFile(t, "clobber") @@ -556,7 +484,7 @@ func TestPlansCreate_ExistingNameConflicts(t *testing.T) { func TestPlansCreate_ExistingNameConflictHuman(t *testing.T) { t.Parallel() - svc, _, _ := newPlansTestService(t) + svc, _ := newPlansTestService(t) mustCreatePlan(t, svc, "p", "original") file := writePlanContentFile(t, "clobber") @@ -574,7 +502,7 @@ func TestPlansCreate_ExistingNameConflictHuman(t *testing.T) { // code 3 and stays byte-identical, and a fresh name keeps working. func TestPlansCreate_ExistingRevisionZeroFileConflicts(t *testing.T) { t.Parallel() - svc, sharedDir, _ := newPlansTestService(t) + svc, sharedDir := newPlansTestService(t) original := `{"name":"planted","content":"precious content"}` plantedPath := filepath.Join(sharedDir, "planted.json") @@ -605,7 +533,7 @@ func TestPlansCreate_ExistingRevisionZeroFileConflicts(t *testing.T) { func TestPlansCreate_Validation(t *testing.T) { t.Parallel() - svc, _, _ := newPlansTestService(t) + svc, _ := newPlansTestService(t) // Empty content is rejected by the service. empty := writePlanContentFile(t, "") @@ -635,7 +563,7 @@ func TestPlansCreate_Validation(t *testing.T) { func TestPlansUpdate_Success(t *testing.T) { t.Parallel() - svc, _, _ := newPlansTestService(t) + svc, _ := newPlansTestService(t) _, err := svc.Create(t.Context(), plans.CreateRequest{ Ref: plans.SharedRef("p"), Content: "v1", Title: "Original", Status: "draft", }) @@ -661,7 +589,7 @@ func TestPlansUpdate_Success(t *testing.T) { func TestPlansUpdate_StaleConflict(t *testing.T) { t.Parallel() - svc, _, _ := newPlansTestService(t) + svc, _ := newPlansTestService(t) mustCreatePlan(t, svc, "p", "v1") _, err := svc.Update(t.Context(), plans.UpdateRequest{Ref: plans.SharedRef("p"), Content: "v2", ExpectedVersion: new(1)}) require.NoError(t, err) @@ -691,7 +619,7 @@ func TestPlansUpdate_StaleConflict(t *testing.T) { func TestPlansUpdate_Force(t *testing.T) { t.Parallel() - svc, _, _ := newPlansTestService(t) + svc, _ := newPlansTestService(t) mustCreatePlan(t, svc, "p", "v1") _, err := svc.Update(t.Context(), plans.UpdateRequest{Ref: plans.SharedRef("p"), Content: "v2", ExpectedVersion: new(1)}) require.NoError(t, err) @@ -705,7 +633,7 @@ func TestPlansUpdate_Force(t *testing.T) { func TestPlansUpdate_RequiresGuard(t *testing.T) { t.Parallel() - svc, _, _ := newPlansTestService(t) + svc, _ := newPlansTestService(t) mustCreatePlan(t, svc, "p", "v1") file := writePlanContentFile(t, "v2") @@ -719,7 +647,7 @@ func TestPlansUpdate_RequiresGuard(t *testing.T) { func TestPlansUpdate_NotFound(t *testing.T) { t.Parallel() - svc, _, _ := newPlansTestService(t) + svc, _ := newPlansTestService(t) file := writePlanContentFile(t, "x") _, stderr, err := executePlans(t, svc, "update", "ghost", "--file", file, "--force", "--json") @@ -731,7 +659,7 @@ func TestPlansUpdate_NotFound(t *testing.T) { func TestPlansMutations_ExpectedVersionMustBePositive(t *testing.T) { t.Parallel() - svc, _, _ := newPlansTestService(t) + svc, _ := newPlansTestService(t) mustCreatePlan(t, svc, "p", "v1") file := writePlanContentFile(t, "v2") @@ -752,7 +680,7 @@ func TestPlansMutations_ExpectedVersionMustBePositive(t *testing.T) { func TestPlansStatus_Success(t *testing.T) { t.Parallel() - svc, _, _ := newPlansTestService(t) + svc, _ := newPlansTestService(t) mustCreatePlan(t, svc, "p", "body") stdout, _, err := executePlans(t, svc, "status", "p", "in-progress", "--expected-version", "1") @@ -766,7 +694,7 @@ func TestPlansStatus_Success(t *testing.T) { func TestPlansStatus_StaleConflict(t *testing.T) { t.Parallel() - svc, _, _ := newPlansTestService(t) + svc, _ := newPlansTestService(t) _, err := svc.Create(t.Context(), plans.CreateRequest{Ref: plans.SharedRef("p"), Content: "body", Status: "draft"}) require.NoError(t, err) _, err = svc.Update(t.Context(), plans.UpdateRequest{Ref: plans.SharedRef("p"), Content: "v2", ExpectedVersion: new(1)}) @@ -784,7 +712,7 @@ func TestPlansStatus_StaleConflict(t *testing.T) { func TestPlansStatus_Validation(t *testing.T) { t.Parallel() - svc, _, _ := newPlansTestService(t) + svc, _ := newPlansTestService(t) mustCreatePlan(t, svc, "p", "body") // An empty status string is rejected by the service. @@ -808,7 +736,7 @@ func TestPlansStatus_Validation(t *testing.T) { // misclassified as invalid_argument (issue #3844: labels are user-defined). func TestPlansMetadata_LargeLabelsAccepted(t *testing.T) { t.Parallel() - svc, _, _ := newPlansTestService(t) + svc, _ := newPlansTestService(t) bigTitle := strings.Repeat("t", 5<<10) bigAuthor := strings.Repeat("a", 5<<10) @@ -835,43 +763,11 @@ func TestPlansMetadata_LargeLabelsAccepted(t *testing.T) { assert.Equal(t, "new body", p.Content) } -// --- Session mutations are unsupported ------------------------------------------- - -func TestPlansMutations_SessionUnsupported(t *testing.T) { - t.Parallel() - svc, _, sessionDir := newPlansTestService(t) - writeSessionPlanFile(t, sessionDir, "sess-1", "# plan") - file := writePlanContentFile(t, "new content") - - tests := []struct { - args []string - wantOp string - }{ - {[]string{"update", "--session", "sess-1", "--file", file, "--force"}, "update"}, - {[]string{"status", "--session", "sess-1", "done", "--force"}, "set_status"}, - {[]string{"delete", "--session", "sess-1", "--force"}, "delete"}, - } - for _, tt := range tests { - stdout, stderr, err := executePlans(t, svc, append(tt.args, "--json")...) - requirePlansStatusCode(t, err, 1) - assert.Empty(t, stdout, "args %v", tt.args) - body := decodePlansError(t, stderr) - assert.Equal(t, "unsupported", body.Code, "args %v", tt.args) - assert.Equal(t, "session", body.Scope, "args %v", tt.args) - assert.Equal(t, tt.wantOp, body.Op, "args %v", tt.args) - assert.Contains(t, body.Message, "within its session", "the error must tell the caller what to do instead") - } - - // The refused mutations left the session plan untouched. - p := mustGetPlan(t, svc, plans.SessionRef("sess-1")) - assert.Equal(t, "# plan", p.Content) -} - // --- Export -------------------------------------------------------------------- func TestPlansExport_Shared(t *testing.T) { t.Parallel() - svc, _, _ := newPlansTestService(t) + svc, _ := newPlansTestService(t) mustCreatePlan(t, svc, "release", "the body") dest := filepath.Join(t.TempDir(), "nested", "plan.md") @@ -885,13 +781,13 @@ func TestPlansExport_Shared(t *testing.T) { assert.Equal(t, "the body", string(data)) } -func TestPlansExport_SessionJSON(t *testing.T) { +func TestPlansExport_JSON(t *testing.T) { t.Parallel() - svc, _, sessionDir := newPlansTestService(t) - writeSessionPlanFile(t, sessionDir, "sess-2", "# session plan") + svc, _ := newPlansTestService(t) + mustCreatePlan(t, svc, "release", "the body") dest := filepath.Join(t.TempDir(), "plan.md") - stdout, _, err := executePlans(t, svc, "export", "--session", "sess-2", "--output", dest, "--json") + stdout, _, err := executePlans(t, svc, "export", "release", "--output", dest, "--json") require.NoError(t, err) var doc struct { @@ -900,22 +796,23 @@ func TestPlansExport_SessionJSON(t *testing.T) { } require.NoError(t, json.Unmarshal([]byte(stdout), &doc)) assert.Equal(t, "1", doc.SchemaVersion) - assert.Equal(t, plans.ScopeSession, doc.Export.Scope) - assert.Equal(t, "sess-2", doc.Export.Name) + assert.Equal(t, plans.ScopeShared, doc.Export.Scope) + assert.Equal(t, "release", doc.Export.Name) assert.Equal(t, dest, doc.Export.Path) - assert.Nil(t, doc.Export.Version, "session plans have no version to export") - assert.Equal(t, len("# session plan"), doc.Export.BytesWritten) + require.NotNil(t, doc.Export.Version) + assert.Equal(t, 1, *doc.Export.Version) + assert.Equal(t, len("the body"), doc.Export.BytesWritten) assert.Contains(t, stdout, `"bytes_written"`) assert.NotContains(t, stdout, `"bytesWritten"`) data, err := os.ReadFile(dest) require.NoError(t, err) - assert.Equal(t, "# session plan", string(data)) + assert.Equal(t, "the body", string(data)) } func TestPlansExport_NotFound(t *testing.T) { t.Parallel() - svc, _, _ := newPlansTestService(t) + svc, _ := newPlansTestService(t) dest := filepath.Join(t.TempDir(), "plan.md") _, stderr, err := executePlans(t, svc, "export", "ghost", "--output", dest, "--json") @@ -927,7 +824,7 @@ func TestPlansExport_NotFound(t *testing.T) { func TestPlansExport_RequiresOutput(t *testing.T) { t.Parallel() - svc, _, _ := newPlansTestService(t) + svc, _ := newPlansTestService(t) mustCreatePlan(t, svc, "p", "body") _, _, err := executePlans(t, svc, "export", "p") @@ -937,7 +834,7 @@ func TestPlansExport_RequiresOutput(t *testing.T) { func TestPlansExport_RefusesExistingDestination(t *testing.T) { t.Parallel() - svc, _, _ := newPlansTestService(t) + svc, _ := newPlansTestService(t) mustCreatePlan(t, svc, "release", "new body") dest := filepath.Join(t.TempDir(), "plan.md") require.NoError(t, os.WriteFile(dest, []byte("precious"), 0o600)) @@ -956,7 +853,7 @@ func TestPlansExport_RefusesExistingDestination(t *testing.T) { func TestPlansExport_ForceReplacesExistingFile(t *testing.T) { t.Parallel() - svc, _, _ := newPlansTestService(t) + svc, _ := newPlansTestService(t) mustCreatePlan(t, svc, "release", "new body") dest := filepath.Join(t.TempDir(), "plan.md") require.NoError(t, os.WriteFile(dest, []byte("old"), 0o600)) @@ -975,7 +872,7 @@ func TestPlansExport_ForceReplacesExistingFile(t *testing.T) { func TestPlansDelete_WithExpectedVersion(t *testing.T) { t.Parallel() - svc, _, _ := newPlansTestService(t) + svc, _ := newPlansTestService(t) mustCreatePlan(t, svc, "p", "body") stdout, _, err := executePlans(t, svc, "delete", "p", "--expected-version", "1") @@ -990,7 +887,7 @@ func TestPlansDelete_WithExpectedVersion(t *testing.T) { func TestPlansDelete_JSON(t *testing.T) { t.Parallel() - svc, _, _ := newPlansTestService(t) + svc, _ := newPlansTestService(t) mustCreatePlan(t, svc, "p", "body") stdout, _, err := executePlans(t, svc, "delete", "p", "--force", "--json") @@ -1011,7 +908,7 @@ func TestPlansDelete_JSON(t *testing.T) { func TestPlansDelete_StaleConflictPreservesPlan(t *testing.T) { t.Parallel() - svc, _, _ := newPlansTestService(t) + svc, _ := newPlansTestService(t) mustCreatePlan(t, svc, "p", "v1") _, err := svc.Update(t.Context(), plans.UpdateRequest{Ref: plans.SharedRef("p"), Content: "v2", ExpectedVersion: new(1)}) require.NoError(t, err) @@ -1028,7 +925,7 @@ func TestPlansDelete_StaleConflictPreservesPlan(t *testing.T) { func TestPlansDelete_SafetyRequiresGuardOrForce(t *testing.T) { t.Parallel() - svc, _, _ := newPlansTestService(t) + svc, _ := newPlansTestService(t) mustCreatePlan(t, svc, "p", "body") // The CLI never prompts, so a bare delete is refused. @@ -1047,7 +944,7 @@ func TestPlansDelete_SafetyRequiresGuardOrForce(t *testing.T) { func TestPlansDelete_NotFound(t *testing.T) { t.Parallel() - svc, _, _ := newPlansTestService(t) + svc, _ := newPlansTestService(t) _, stderr, err := executePlans(t, svc, "delete", "ghost", "--force", "--json") requirePlansStatusCode(t, err, 1) @@ -1064,7 +961,7 @@ func TestPlansDelete_NotFound(t *testing.T) { // schema-versioned JSON object on stderr, nothing on stdout, exit code 1. func TestPlansValidation_JSONContract(t *testing.T) { t.Parallel() - svc, _, _ := newPlansTestService(t) + svc, _ := newPlansTestService(t) file := writePlanContentFile(t, "body") tests := []struct { @@ -1103,7 +1000,7 @@ func TestPlansValidation_JSONContract(t *testing.T) { // --json is only honoured when it was parsed before the failure. func TestPlansValidation_UnknownFlagBeforeJSONIsPlainText(t *testing.T) { t.Parallel() - svc, _, _ := newPlansTestService(t) + svc, _ := newPlansTestService(t) stdout, stderr, err := executePlans(t, svc, "list", "--frobnicate", "--json") require.Error(t, err) @@ -1117,7 +1014,7 @@ func TestPlansValidation_UnknownFlagBeforeJSONIsPlainText(t *testing.T) { // the caller and rendered once, never as JSON. func TestPlansValidation_HumanModeKeepsCobraRendering(t *testing.T) { t.Parallel() - svc, _, _ := newPlansTestService(t) + svc, _ := newPlansTestService(t) _, stderr, err := executePlans(t, svc, "get", "a", "b") require.Error(t, err) @@ -1265,8 +1162,8 @@ func TestHardenPlansValidation_PreRunEErrorHumanModePassesThrough(t *testing.T) // TestPlansTelemetryError_ReducesToStableCode pins the telemetry // sanitization: the tracked error is exactly the stable machine-readable -// code of the JSON error contract, so plan names, session IDs, and -// filesystem paths embedded in error text never leave the machine. +// code of the JSON error contract, so plan names and filesystem paths +// embedded in error text never leave the machine. func TestPlansTelemetryError_ReducesToStableCode(t *testing.T) { t.Parallel() @@ -1320,7 +1217,7 @@ func (f failingPlanStorage) Delete(context.Context, string, *int) (bool, error) func TestPlansList_StorageErrorJSON(t *testing.T) { t.Parallel() - svc := plans.NewService(failingPlanStorage{err: errors.New("backend boom")}, plans.WithSessionDir(t.TempDir())) + svc := plans.NewService(failingPlanStorage{err: errors.New("backend boom")}) stdout, stderr, err := executePlans(t, svc, "list", "--json") requirePlansStatusCode(t, err, 1) @@ -1334,7 +1231,7 @@ func TestPlansList_StorageErrorJSON(t *testing.T) { func TestPlansList_StorageErrorHuman(t *testing.T) { t.Parallel() - svc := plans.NewService(failingPlanStorage{err: errors.New("backend boom")}, plans.WithSessionDir(t.TempDir())) + svc := plans.NewService(failingPlanStorage{err: errors.New("backend boom")}) stdout, stderr, err := executePlans(t, svc, "list") requirePlansStatusCode(t, err, 1) diff --git a/cmd/root/plans_unix_test.go b/cmd/root/plans_unix_test.go index 118356bfd3..45e6021f65 100644 --- a/cmd/root/plans_unix_test.go +++ b/cmd/root/plans_unix_test.go @@ -21,7 +21,7 @@ import ( // blocking open. func TestPlansCreate_RejectsNamedPipe(t *testing.T) { t.Parallel() - svc, _, _ := newPlansTestService(t) + svc, _ := newPlansTestService(t) fifo := filepath.Join(t.TempDir(), "content.pipe") if err := syscall.Mkfifo(fifo, 0o600); err != nil { @@ -56,7 +56,7 @@ func TestPlansCreate_RejectsNamedPipe(t *testing.T) { // regular file" message shows no read happened. func TestPlansCreate_RejectsDevice(t *testing.T) { t.Parallel() - svc, _, _ := newPlansTestService(t) + svc, _ := newPlansTestService(t) if _, err := os.Stat("/dev/zero"); err != nil { t.Skipf("/dev/zero not available: %v", err) @@ -77,7 +77,7 @@ func TestPlansCreate_RejectsDevice(t *testing.T) { // a blocking open in the storage's load path. func TestPlansGetList_FIFOPlanFileFailsFast(t *testing.T) { t.Parallel() - svc, sharedDir, _ := newPlansTestService(t) + svc, sharedDir := newPlansTestService(t) mustCreatePlan(t, svc, "good", "content") if err := syscall.Mkfifo(filepath.Join(sharedDir, "wedged.json"), 0o600); err != nil { diff --git a/docs/concepts/tools/index.md b/docs/concepts/tools/index.md index 149e876d9a..0a01187435 100644 --- a/docs/concepts/tools/index.md +++ b/docs/concepts/tools/index.md @@ -53,7 +53,6 @@ Docker Agent ships with several built-in tools that require no external dependen | [Scheduler](../../tools/scheduler/index.md) | Schedule instructions to run at a time or on a recurring interval | | [Webhook](../../tools/webhook/index.md) | Outbound notifications to Slack, Discord, Telegram, IFTTT, and more | | [Plan](../../tools/plan/index.md) | Shared persistent scratchpad for multi-agent collaboration | -| [Session Plan](../../tools/session_plan/index.md) | Per-session plan tracker for the draft/review/execute workflow | | [Session Context](../../tools/session_context/index.md) | Reference a previous session as context | ## MCP Tools diff --git a/docs/configuration/tools/index.md b/docs/configuration/tools/index.md index d25e36968e..27cc0316ba 100644 --- a/docs/configuration/tools/index.md +++ b/docs/configuration/tools/index.md @@ -26,7 +26,6 @@ Built-in tools are included with Docker Agent and require no external dependenci | `environment` | Report the OS and resolved shell (read-only, no arguments, auto-approved) | [Environment](../../tools/environment/index.md) | | `think` | Reasoning scratchpad | [Think](../../tools/think/index.md) | | `plan` | Shared persistent scratchpad for multi-agent collaboration | [Plan](../../tools/plan/index.md) | -| `session_plan` | Per-session markdown plan for the draft-review-execute workflow | [Session Plan](../../tools/session_plan/index.md) | | `session_context` | Reference a previous session as context (read-only) | [Session Context](../../tools/session_context/index.md) | | `todo` | Task list management | [Todo](../../tools/todo/index.md) | | `memory` | Persistent key-value storage (SQLite) | [Memory](../../tools/memory/index.md) | diff --git a/docs/data/nav.yml b/docs/data/nav.yml index 5577526b8e..88e362a4db 100644 --- a/docs/data/nav.yml +++ b/docs/data/nav.yml @@ -103,8 +103,6 @@ url: /tools/think/ - title: Plan url: /tools/plan/ - - title: Session Plan - url: /tools/session_plan/ - title: Todo url: /tools/todo/ - title: Tasks diff --git a/docs/features/cli/index.md b/docs/features/cli/index.md index 4eab494462..04cc46b345 100644 --- a/docs/features/cli/index.md +++ b/docs/features/cli/index.md @@ -670,10 +670,7 @@ Entries are unioned with the gateway, the kit-resolved tool install hosts, and a ### `docker agent plans` -Manage the plans agents collaborate on, from the host — without starting a session. Two plan systems are covered: - -- **Shared plans** — the named, versioned documents of the [plan toolset](../../tools/plan/index.md). Fully manageable: create, update, set status, export, delete. -- **Session plans** — the single per-session plan of the "draft, review, execute" workflow. Read-only here (`list`, `get`, `export`); they belong to their session and are changed from within it. A mutation aimed at a session plan fails with an `unsupported` error explaining what to do instead. +Manage the plans agents collaborate on, from the host — without starting a session: the named, versioned documents of the [plan toolset](../../tools/plan/index.md). Fully manageable: create, update, set status, export, delete. ```bash $ docker agent plans [flags] @@ -681,13 +678,13 @@ $ docker agent plans [flags] | Subcommand | Description | | ---------- | ----------- | -| `list [--session ]` | List shared plans with scope, name, status, version, updated time, and title. With `--session`, that session's plan is listed first when it exists. Plans that exist but cannot be read are reported as warnings on stderr (in the `warnings` field with `--json`), so they are never mistaken for missing. | -| `get ` | Print a plan. Content goes to stdout and a concise metadata line goes to stderr, so `> file` captures the content alone (use `export` for a byte-exact copy). `get --session ` prints a session's plan; the name is then omitted (`--scope shared\|session` disambiguates explicitly, and `--session` alone implies session scope). | -| `create --file ` | Create a new shared plan with content from `--file` (required — the CLI never prompts; `--file -` reads stdin). Create-only: an existing name fails with a version conflict instead of overwriting. `--title`, `--author`, and `--status` set metadata. | -| `update --file ` | Replace the content of an existing shared plan (never creates). Omitted `--title`/`--author`/`--status` flags preserve the current values; passing them (even empty) overwrites. | -| `status ` | Set a shared plan's free-form status without touching its body (bumps the version). | -| `export --output ` | Write a plan's content, byte-exact, to a file (parents created, atomic write). An existing destination is refused (`invalid_argument`) and left untouched; add `--force` to replace an existing regular file atomically. Works for both scopes: `export --session --output `. | -| `delete ` | Delete a shared plan. A `--force` delete also recovers a corrupt plan. | +| `list` | List plans with scope, name, status, version, updated time, and title. Plans that exist but cannot be read are reported as warnings on stderr (in the `warnings` field with `--json`), so they are never mistaken for missing. | +| `get ` | Print a plan. Content goes to stdout and a concise metadata line goes to stderr, so `> file` captures the content alone (use `export` for a byte-exact copy). | +| `create --file ` | Create a new plan with content from `--file` (required — the CLI never prompts; `--file -` reads stdin). Create-only: an existing name fails with a version conflict instead of overwriting. `--title`, `--author`, and `--status` set metadata. | +| `update --file ` | Replace the content of an existing plan (never creates). Omitted `--title`/`--author`/`--status` flags preserve the current values; passing them (even empty) overwrites. | +| `status ` | Set a plan's free-form status without touching its body (bumps the version). | +| `export --output ` | Write a plan's content, byte-exact, to a file (parents created, atomic write). An existing destination is refused (`invalid_argument`) and left untouched; add `--force` to replace an existing regular file atomically. | +| `delete ` | Delete a plan. A `--force` delete also recovers a corrupt plan. | Plan content passed via `--file` (a regular file, or stdin with `--file -`) is capped at 10 MiB — the same limit the plan storage itself enforces — and a directory or non-regular file (device, named pipe) is rejected up front; violations fail with an `invalid_argument` error. @@ -698,13 +695,13 @@ Plan content passed via `--file` (a regular file, or stdin with `--file -`) is c `create` takes no guard: it is inherently create-only and conflicts (exit code 3) when the name already exists. -**JSON output:** every subcommand accepts `--json`. Success documents go to stdout with a top-level `"schema_version": "1"` marker and stable service-model keys (`plans`, `plan`, `export`, `deleted`) whose fields are snake_case (`updated_at`, `session_id`, `bytes_written`; a zero/unknown `updated_at` is omitted); empty plan lists encode as `[]`, and no prose or ANSI is mixed in. Failures print a single JSON object to stderr: +**JSON output:** every subcommand accepts `--json`. Success documents go to stdout with a top-level `"schema_version": "1"` marker and stable service-model keys (`plans`, `plan`, `export`, `deleted`) whose fields are snake_case (`updated_at`, `bytes_written`; a zero/unknown `updated_at` is omitted); empty plan lists encode as `[]`, and no prose or ANSI is mixed in. Failures print a single JSON object to stderr: ```json {"schema_version":"1","error":{"code":"conflict","message":"...","scope":"shared","name":"p","expected_version":1,"current_version":2}} ``` -with `code` one of `conflict` (including `expected_version` and `current_version`), `not_found`, `invalid_argument`, `unsupported`, `corrupt`, `storage`, or `error`; `scope`, `name`, and `op` are included where the failure carries them. Validation performed before a subcommand runs is covered too: a missing required flag, a violated `--expected-version`/`--force` group rule, and wrong positional arguments are reported as the same JSON object (code `invalid_argument`) whenever `--json` is present. One residual: flags are parsed left-to-right and parsing stops at the first unknown flag or invalid flag value, so such an error is reported as JSON only when `--json` appears before it on the command line; errors raised before a `plans` subcommand is resolved at all (e.g. an unknown subcommand) also remain plain text. +with `code` one of `conflict` (including `expected_version` and `current_version`), `not_found`, `invalid_argument`, `corrupt`, `storage`, or `error`; `scope`, `name`, and `op` are included where the failure carries them. Validation performed before a subcommand runs is covered too: a missing required flag, a violated `--expected-version`/`--force` group rule, and wrong positional arguments are reported as the same JSON object (code `invalid_argument`) whenever `--json` is present. One residual: flags are parsed left-to-right and parsing stops at the first unknown flag or invalid flag value, so such an error is reported as JSON only when `--json` appears before it on the command line; errors raised before a `plans` subcommand is resolved at all (e.g. an unknown subcommand) also remain plain text. ```bash # Examples @@ -719,11 +716,9 @@ $ docker agent plans export release --output ./plan.md $ docker agent plans export release --output ./plan.md --force # replace an existing file $ docker agent plans delete release --expected-version 3 $ docker agent plans delete scratch --force -$ docker agent plans get --session # a session's plan -$ docker agent plans export --session --output ./session-plan.md ``` -Plans live under the data directory (`~/.cagent/plans/` and `~/.cagent/session_plans/` by default), so `--data-dir` selects which store the commands operate on. +Plans live under the data directory (`~/.cagent/plans/` by default), so `--data-dir` selects which store the commands operate on. ### `docker agent debug` diff --git a/docs/features/tui/index.md b/docs/features/tui/index.md index f61d403caf..cb5a67e241 100644 --- a/docs/features/tui/index.md +++ b/docs/features/tui/index.md @@ -81,7 +81,7 @@ Type `/` during a session to see available commands, or press Ctrl+`, or `/effort` alone to pick from the supported levels; reasoning models only). Press Tab after `/effort` and a space to complete a level the current model supports | | `/settings` | Manage appearance, behavior, and notification preferences | diff --git a/docs/tools/plan/index.md b/docs/tools/plan/index.md index 3f27b01989..97a9d34e9d 100644 --- a/docs/tools/plan/index.md +++ b/docs/tools/plan/index.md @@ -132,7 +132,7 @@ See [`examples/shared_plan.yaml`](https://github.com/docker/docker-agent/blob/ma ## Managing plans from the host -Shared plans can also be inspected and managed outside a session with the [`docker agent plans`](../../features/cli/index.md#docker-agent-plans) command group: list, get, create, update, set status, export, and delete — with the same optimistic-locking semantics as the tools (`--expected-version` guards a write and a stale version fails with exit code 3; `--force` writes unconditionally). Session plans (the per-session "draft, review, execute" plan) can be listed, read, and exported through the same commands but stay owned by their session and cannot be mutated from the host. +Shared plans can also be inspected and managed outside a session with the [`docker agent plans`](../../features/cli/index.md#docker-agent-plans) command group: list, get, create, update, set status, export, and delete — with the same optimistic-locking semantics as the tools (`--expected-version` guards a write and a stale version fails with exit code 3; `--force` writes unconditionally). ```bash $ docker agent plans list @@ -142,7 +142,7 @@ $ docker agent plans update release --file ./plan.md --expected-version 1 ### The `/plans` browser in the TUI -Inside the full-screen TUI, the `/plans` slash command (also in the Ctrl+K command palette) opens a plan browser over the same store the agents use, so changes made by agents mid-session appear immediately. The list shows every shared plan plus the current session's [session plan](../session_plan/index.md), with each plan's scope, identity (name, or session ID for the session plan), status, version (`-` for the unversioned session plan), last update time, and title. +Inside the full-screen TUI, the `/plans` slash command (also in the Ctrl+K command palette) opens a plan browser over the same store the agents use, so changes made by agents mid-session appear immediately. The list shows every shared plan with its scope, name, status, version, last update time, and title. Keybindings: @@ -151,14 +151,14 @@ Keybindings: | ↑/↓, mouse | Navigate; Enter or double-click opens a detail view with the full metadata and scrollable markdown content | | / | Filter by name, title, status, or scope (Esc leaves filter mode) | | r | Refresh from storage | -| x | Export the selected plan to `.md` (shared) or `session-plan-.md` (session) in the session's working directory. An existing file is never overwritten — the export fails with a notification instead | -| s | Set a shared plan's free-form status via a small input dialog | -| e | Edit a shared plan's content in `$VISUAL`/`$EDITOR` | -| n | Create a new shared plan: pick a name, then draft the content in `$VISUAL`/`$EDITOR` (an empty draft aborts) | -| d | Delete a shared plan after a confirmation that names the plan and its version | +| x | Export the selected plan to `.md` in the session's working directory. An existing file is never overwritten — the export fails with a notification instead | +| s | Set a plan's free-form status via a small input dialog | +| e | Edit a plan's content in `$VISUAL`/`$EDITOR` | +| n | Create a new plan: pick a name, then draft the content in `$VISUAL`/`$EDITOR` (an empty draft aborts) | +| d | Delete a plan after a confirmation that names the plan and its version | | Esc | Close the detail view / the browser | -Every mutation is guarded by the version shown on screen (the same optimistic locking as `last_known_revision`): if an agent changed the plan in the meantime, the write is rejected, a notification reports the current version, the newer content is left intact and re-read into the browser, and an edit draft is kept in a temp file so nothing is lost. Session plans are read-only here — status, edit, and delete report why instead of attempting the write. The browser also refreshes live when agents in the same process write, re-status, or delete plans (and when this session's agent updates its session plan); in the lean TUI, which has no overlays, `/plans` is unavailable. +Every mutation is guarded by the version shown on screen (the same optimistic locking as `last_known_revision`): if an agent changed the plan in the meantime, the write is rejected, a notification reports the current version, the newer content is left intact and re-read into the browser, and an edit draft is kept in a temp file so nothing is lost. The browser also refreshes live when agents in the same process write, re-status, or delete plans; in the lean TUI, which has no overlays, `/plans` is unavailable. > [!TIP] > **Plan vs. Todo vs. Tasks** diff --git a/docs/tools/session_plan/index.md b/docs/tools/session_plan/index.md deleted file mode 100644 index ea4c8e4970..0000000000 --- a/docs/tools/session_plan/index.md +++ /dev/null @@ -1,134 +0,0 @@ ---- -title: "Session Plan Tool" -description: "Per-session plan tracker for the draft, review, execute workflow." -keywords: docker agent, ai agents, tools, toolsets, session plan tool -linkTitle: "Session Plan" -weight: 160 -canonical: https://docs.docker.com/ai/docker-agent/tools/session_plan/ ---- - -_Per-session plan tracker for the "draft, review, execute" workflow._ - -## Overview - -The `session_plan` toolset gives one agent a place to write a plan for the current session, signal that the plan is ready, and let the host route the next turn to an executing agent. - -Different from the [`plan` toolset](../plan/index.md) — `plan` is for shared, named plans multiple agents collaborate on over many sessions. `session_plan` is for one ephemeral plan per session, scoped to that session by ID. - -Plans live as Markdown files under: - -```text -~/.cagent/session_plans/.md -``` - -The tool surface is three tools: - -| Tool | Description | -| -------------------- | ---------------------------------------------------------------------------------------------------- | -| `write_session_plan` | Create or replace this session's plan as markdown. There's exactly one plan per session. | -| `read_session_plan` | Read the plan written for the current session and return it as markdown. | -| `exit_plan_mode` | Signal that the plan is ready for review. Does not switch agents on its own. | - -## Configuration - -```yaml -toolsets: - - type: session_plan -``` - -No configuration options. The plan path is derived from the session ID; the agent does not name plans. - -Restrict the toolset to a subset of tools the standard way: - -```yaml -# An agent that consumes a plan but should not be able to (re)write or finalize one. -toolsets: - - type: session_plan - tools: - - read_session_plan -``` - -## When to call exit_plan_mode - -Call `exit_plan_mode` once the plan is complete and you do not intend to change it on the next turn. It validates that a plan exists for the session and returns a "ready for review" tool result. It does **not** switch agents or solicit user approval on its own — the host application owns the next-turn routing (for example, by reading the tool result, by a UI affordance the user toggles, or by a `handoff` declared on the agent). - -This separation keeps the tool reusable across UIs: a CLI that prints tool results inline, a chat UI with a plan-mode toggle, and a server that auto-routes the next turn through a `handoff` can all consume the same signal without one stepping on another. - -## Storage and cleanup - -- Plans are markdown files written atomically (temp + rename), so concurrent readers — in this process or another — never observe a partial write. -- A best-effort sweep on first use of the toolset removes plan files older than 30 days under the plans directory. Stranded plans for long-gone sessions do not accumulate. -- The session ID identifies the file directly. There is no in-process mutex or revision counter, because two sessions cannot map to the same path. - -## Events - -A `session_plan_updated` event is emitted whenever `write_session_plan` succeeds: - -```json -{ - "type": "session_plan_updated", - "session_id": "...", - "path": "/Users/.../.cagent/session_plans/.md", - "content": "# my plan\n...", - "agent_name": "planner" -} -``` - -Embedders that render the plan inline can subscribe and update without re-reading the file. - -## Managing session plans from the host - -A session plan belongs to its session: hosts can read and export it, never change it. - -- **CLI** — the [`docker agent plans`](../../features/cli/index.md#docker-agent-plans) command group lists, reads (`get --session `), and exports session plans alongside shared plans. Mutations (`update`, `status`, `delete`) are refused with an `unsupported` error explaining the ownership rule. -- **TUI** — the `/plans` browser (see the [plan toolset docs](../plan/index.md#the-plans-browser-in-the-tui) for the full keybinding table) includes the **current session's** plan as the `session` scope row; plans of other sessions are never enumerated. Its identity is the session ID and its version column shows `-` — session plans have no versions. Enter opens the detail view (scope, session ID, update time, scrollable markdown) and x exports to `session-plan-.md` in the working directory (refusing to overwrite an existing file). e opens the plan body in your external editor (`$VISUAL` or `$EDITOR`) for editing — the write is unguarded and last-write-wins by design. Status and delete visibly report that session plans don't support them (session plans belong to their session and carry no shared-plan metadata). The browser refreshes live on the `session_plan_updated` event, so a plan the agent just wrote appears without reopening. - -## Example - -A two-agent workflow: `root` executes, `planner` plans. `/plan` hands off to the planner; `exit_plan_mode` signals "ready", and the host decides what happens next. - -```yaml -agents: - root: - model: anthropic/claude-sonnet-4-5 - description: Executes approved plans - instruction: | - You execute plans the planner has handed off. When you see a message - that a plan has been approved, read it with read_session_plan and work - through its steps in order. - toolsets: - - type: session_plan - tools: - - read_session_plan - - type: filesystem - - type: shell - commands: - plan: - description: "Switch to the planner" - agent: planner - - planner: - model: anthropic/claude-sonnet-4-5 - description: Investigates and writes plans for review - instruction: | - Investigate the user's request, then write the plan with - write_session_plan. Iterate with the user until the plan is complete, - then call exit_plan_mode to mark it ready for review. - toolsets: - - type: session_plan - - type: filesystem - readonly: true - - type: user_prompt -``` - -See [`examples/session_plan.yaml`](https://github.com/docker/docker-agent/blob/main/examples/session_plan.yaml) for a complete working example. - -## Error Handling - -- `read_session_plan` and `exit_plan_mode` return a "no plan written yet" error when called before `write_session_plan`. -- `write_session_plan` validates the session ID and refuses to write anything that could escape the plans directory; in practice the runtime generates UUIDs so this only triggers if an embedder supplies a hand-crafted ID. - -> [!TIP] -> **session_plan vs. plan vs. todo vs. tasks** -> -> Use **session_plan** when one agent drafts an approach for the user to review before another agent executes it (ephemeral, one per session). Use [plan](../plan/index.md) for shared, named plans multiple agents collaborate on over many sessions. Use [todo](../todo/index.md) for lightweight in-session task lists. Use [tasks](../tasks/index.md) for a structured, persistent task database with priorities and dependencies. diff --git a/examples/session_plan.yaml b/examples/session_plan.yaml deleted file mode 100644 index 309ea2b5e0..0000000000 --- a/examples/session_plan.yaml +++ /dev/null @@ -1,48 +0,0 @@ -# A two-agent "plan then execute" workflow. -# -# The user starts in `root`. Typing `/plan` switches to `planner`, which has -# read-only tools and the session_plan toolset. The planner investigates -# the request, writes the plan to /session_plans/.md, -# and calls exit_plan_mode when ready — that signal tells the host the plan -# is final. Switching back to `root` for execution is up to the host -# (toggle, slash command, an explicit handoff on the agent, …); the tool -# itself does not move the conversation. - -agents: - root: - model: anthropic/claude-sonnet-4-5 - description: Executes plans the planner has handed off - instruction: | - You execute plans the planner has prepared. When you see a message - saying a plan has been approved, read it with read_session_plan and - work through its steps in order. For ambiguous or multi-file changes - that have not been planned yet, type `/plan ` first instead of - starting work directly. - toolsets: - - type: session_plan - tools: - - read_session_plan - - type: filesystem - - type: shell - commands: - plan: - description: "Switch to the planner" - agent: planner - - planner: - model: anthropic/claude-sonnet-4-5 - description: Investigates the request and writes a plan for user review - instruction: | - Investigate the user's request with read-only tools, then draft the - plan with write_session_plan. Iterate — re-read code, ask clarifying - questions with user_prompt — until the plan is complete. When the - plan is final, call exit_plan_mode to mark it ready for review. - - Do not modify any files. The only write you may perform is - write_session_plan. - toolsets: - - type: session_plan - - type: filesystem - readonly: true - - type: fetch - - type: user_prompt diff --git a/pkg/plans/errors.go b/pkg/plans/errors.go index 8275cd6048..87d9deb77a 100644 --- a/pkg/plans/errors.go +++ b/pkg/plans/errors.go @@ -5,17 +5,15 @@ import "fmt" // NotFoundError reports that the addressed plan does not exist in its scope. type NotFoundError struct { Scope Scope - // Name is the plan name or the session ID, matching Scope. - Name string + Name string } func (e *NotFoundError) Error() string { return fmt.Sprintf("%s plan %q not found", e.Scope, e.Name) } -// ValidationError reports invalid caller input: a malformed plan name or -// session ID, an unknown scope, empty or oversized content, or an empty -// status. +// ValidationError reports invalid caller input: a malformed plan name, an +// unknown scope, empty or oversized content, or an empty status. type ValidationError struct { Message string } @@ -69,19 +67,3 @@ func (e *ConflictError) Error() string { } return fmt.Sprintf("version conflict on plan %q: expected version %d does not match current version %d; re-read the plan and retry, or force to overwrite", e.Name, e.Expected, e.Current) } - -// UnsupportedError reports an operation the plan's scope does not support, -// with Reason telling the caller what to do instead. -type UnsupportedError struct { - Scope Scope - Op string - Reason string -} - -func (e *UnsupportedError) Error() string { - msg := fmt.Sprintf("%s is not supported for %s plans", e.Op, e.Scope) - if e.Reason != "" { - msg += ": " + e.Reason - } - return msg -} diff --git a/pkg/plans/json_test.go b/pkg/plans/json_test.go index 2ae5e2eb26..33cf90c4e3 100644 --- a/pkg/plans/json_test.go +++ b/pkg/plans/json_test.go @@ -40,23 +40,6 @@ func TestPlanJSON_SharedShape(t *testing.T) { mustMarshal(t, p)) } -func TestPlanJSON_SessionShape(t *testing.T) { - t.Parallel() - p := Plan{ - Scope: ScopeSession, - Name: "sess-1", - Content: "# plan", - UpdatedAt: time.Date(2024, 5, 6, 7, 8, 9, 0, time.UTC), - SessionID: "sess-1", - Path: "/data/session_plans/sess-1.md", - } - - // No version, title, author, or status: session plans never carry them. - assert.JSONEq(t, - `{"scope":"session","name":"sess-1","content":"# plan","updated_at":"2024-05-06T07:08:09Z","session_id":"sess-1","path":"/data/session_plans/sess-1.md"}`, - mustMarshal(t, p)) -} - func TestPlanJSON_ZeroValuesOmitted(t *testing.T) { t.Parallel() @@ -70,11 +53,14 @@ func TestPlanJSON_ZeroValuesOmitted(t *testing.T) { func TestPlanJSON_RoundTrip(t *testing.T) { t.Parallel() p := Plan{ - Scope: ScopeSession, - Name: "sess-1", + Scope: ScopeShared, + Name: "release", + Title: "Release plan", + Author: "alice", + Status: "draft", + Content: "body", + Version: new(3), UpdatedAt: time.Date(2024, 5, 6, 7, 8, 9, 0, time.UTC), - SessionID: "sess-1", - Path: "/x/plan.md", } var got Plan @@ -89,8 +75,8 @@ func TestExportResultJSON_Shapes(t *testing.T) { `{"scope":"shared","name":"p","path":"/out/plan.md","version":2,"bytes_written":5}`, mustMarshal(t, ExportResult{Scope: ScopeShared, Name: "p", Path: "/out/plan.md", Version: new(2), BytesWritten: 5})) - // Session exports have no version to report. + // A nil version is omitted from the wire shape. assert.JSONEq(t, - `{"scope":"session","name":"sess-1","path":"/out/plan.md","bytes_written":0}`, - mustMarshal(t, ExportResult{Scope: ScopeSession, Name: "sess-1", Path: "/out/plan.md"})) + `{"scope":"shared","name":"p","path":"/out/plan.md","bytes_written":0}`, + mustMarshal(t, ExportResult{Scope: ScopeShared, Name: "p", Path: "/out/plan.md"})) } diff --git a/pkg/plans/plans.go b/pkg/plans/plans.go index 3628b9c735..4b8202f8f3 100644 --- a/pkg/plans/plans.go +++ b/pkg/plans/plans.go @@ -1,17 +1,11 @@ // Package plans is the host-facing contract for managing plans from a -// frontend such as a CLI or TUI. It unifies the two plan systems behind one -// model and one Service: the shared, named plans agents collaborate on -// (pkg/tools/builtin/plan) and the single per-session plan owned by the -// runtime (pkg/tools/builtin/sessionplan). +// frontend such as a CLI or TUI. It wraps the shared, named plans agents +// collaborate on (pkg/tools/builtin/plan) behind one model and one Service. // // The package wraps the existing storage rather than duplicating it. Shared // plans go through a caller-supplied plan.Storage — pass plan.SharedStorage() // to operate on the same store, and thus the same mutex, as the plan tools of -// agents running in this process. The session plan is read and written -// through the sessionplan helpers. Session plans have no revisions or -// optimistic locking: the version-guarded mutations reject them with a typed -// *UnsupportedError, and the one supported write is UpdateSession, which -// replaces the body of an existing plan last-write-wins. +// agents running in this process. package plans import ( @@ -19,28 +13,16 @@ import ( "time" ) -// Scope identifies which plan system a plan belongs to. +// Scope identifies which plan system a plan belongs to. It remains part of +// the wire contract for forward compatibility even though only one scope +// exists today. type Scope string -const ( - // ScopeShared is the cross-session store of named plans that agents - // collaborate on. Shared plans are versioned and fully mutable. - ScopeShared Scope = "shared" - // ScopeSession is the per-session plan of the "draft, review, execute" - // workflow. At most one exists per session; it has no versions, so the - // Service reads, exports, and replaces its body through UpdateSession - // but rejects the version-guarded mutations. - ScopeSession Scope = "session" -) - -// Mutable reports whether plans in this scope support the full set of -// version-guarded mutations (create, update, set-status, delete), so a -// frontend can disable those actions up front instead of provoking an -// *UnsupportedError. Session plans are not Mutable in this sense; their -// body is still replaceable through UpdateSession. -func (s Scope) Mutable() bool { return s == ScopeShared } +// ScopeShared is the cross-session store of named plans that agents +// collaborate on. Shared plans are versioned and fully mutable. +const ScopeShared Scope = "shared" -// Plan is the host-facing view of a plan from either scope. +// Plan is the host-facing view of a plan. // // The JSON tags are a stable, snake_case wire contract for host consumers // (e.g. the plans CLI --json output). It is deliberately independent of the @@ -49,56 +31,33 @@ func (s Scope) Mutable() bool { return s == ScopeShared } type Plan struct { // Scope tells which plan system the plan lives in. Scope Scope `json:"scope"` - // Name is the canonical identity within the scope: the validated plan - // name for shared plans, the session ID for session plans. + // Name is the validated canonical plan name. Name string `json:"name"` - // Title, Author, and Status are shared-plan metadata, empty for session - // plans. Status is a free-form lifecycle label with no fixed vocabulary. + // Title, Author, and Status are plan metadata. Status is a free-form + // lifecycle label with no fixed vocabulary. Title string `json:"title,omitempty"` Author string `json:"author,omitempty"` Status string `json:"status,omitempty"` // Content is the plan body. List returns metadata only, so Content is // empty there; Get populates it. Content string `json:"content,omitempty"` - // Version is the optimistic-lock revision of a shared plan. It is nil for - // session plans, which have no revisions: nil means version-guarded - // operations are unsupported, not "version zero". + // Version is the optimistic-lock revision of the plan. Version *int `json:"version,omitempty"` - // UpdatedAt is the time of the last write: the stored timestamp for - // shared plans, the plan file's modification time for session plans. - // Zero when unknown (and then omitted from JSON via omitzero, which - // consults time.Time.IsZero; omitempty would keep the zero struct). + // UpdatedAt is the stored time of the last write. Zero when unknown (and + // then omitted from JSON via omitzero, which consults time.Time.IsZero; + // omitempty would keep the zero struct). UpdatedAt time.Time `json:"updated_at,omitzero"` - // SessionID is the owning session of a session plan, empty for shared - // plans. - SessionID string `json:"session_id,omitempty"` - // Path is the backing file of a session plan. It is empty for shared - // plans, whose storage backend is pluggable and opaque. - Path string `json:"path,omitempty"` } -// Ref addresses a plan in either scope: Name addresses a shared plan, -// SessionID the plan of that session. The field matching Scope must be set. +// Ref addresses a plan by name within a scope. type Ref struct { - Scope Scope - Name string - SessionID string + Scope Scope + Name string } // SharedRef addresses the named shared plan. func SharedRef(name string) Ref { return Ref{Scope: ScopeShared, Name: name} } -// SessionRef addresses the plan of the given session. -func SessionRef(sessionID string) Ref { return Ref{Scope: ScopeSession, SessionID: sessionID} } - -// ListOptions controls List. -type ListOptions struct { - // SessionID, when non-empty, also includes that session's plan in the - // listing if one exists. Only the identified session is consulted; plan - // files left behind by other sessions are never enumerated. - SessionID string -} - // ListResult is the outcome of List. Warnings carries plans that exist but // could not be read, so a caller can tell "no plans" apart from "some plans // failed to load". @@ -151,8 +110,7 @@ type DeleteRequest struct { ExpectedVersion *int } -// ExportRequest writes a plan's content to a file on disk. Export works for -// both scopes. +// ExportRequest writes a plan's content to a file on disk. // // Force replaces an existing regular file at Path. Without it, Export // refuses any existing destination with a *ValidationError and leaves it @@ -163,8 +121,8 @@ type ExportRequest struct { Force bool } -// ExportResult reports a completed export. Version is the exported shared -// plan's version, nil for session plans. +// ExportResult reports a completed export. Version is the exported plan's +// version. type ExportResult struct { Scope Scope `json:"scope"` Name string `json:"name"` @@ -173,19 +131,15 @@ type ExportResult struct { BytesWritten int `json:"bytes_written"` } -// Service is the host-facing contract for managing plans across both scopes. -// The version-guarded mutations address shared plans only; one aimed at a -// session plan fails with a typed *UnsupportedError, and the session plan's -// body is replaced through the dedicated UpdateSession instead. Failures are +// Service is the host-facing contract for managing plans. Failures are // reported as the typed errors of this package so frontends never classify // by error text. type Service interface { - // List returns plan metadata (Content is left empty): every shared plan - // sorted by name and, when opts.SessionID is set, that session's plan - // first. A missing session plan is simply not included, never an error. - List(ctx context.Context, opts ListOptions) (ListResult, error) + // List returns plan metadata (Content is left empty) for every shared + // plan, sorted by name. + List(ctx context.Context) (ListResult, error) // Get returns the full plan, including content. A missing plan is a - // *NotFoundError in either scope. + // *NotFoundError. Get(ctx context.Context, ref Ref) (Plan, error) // Create adds a new shared plan; a name that already exists fails with a // *ConflictError carrying the current version. @@ -193,11 +147,6 @@ type Service interface { // Update replaces the content (and optionally metadata) of an existing // shared plan, honouring req.ExpectedVersion. Update(ctx context.Context, req UpdateRequest) (Plan, error) - // UpdateSession replaces the content of the session's existing plan. - // Session plans have no versions, so the write is unguarded and - // last-write-wins by design. A missing plan is a *NotFoundError: - // UpdateSession edits, it never creates. - UpdateSession(ctx context.Context, sessionID, content string) (Plan, error) // SetStatus sets the free-form status of an existing shared plan, // honouring req.ExpectedVersion. SetStatus(ctx context.Context, req SetStatusRequest) (Plan, error) diff --git a/pkg/plans/service.go b/pkg/plans/service.go index 3a8ef9f1da..40daf8b2bc 100644 --- a/pkg/plans/service.go +++ b/pkg/plans/service.go @@ -5,7 +5,6 @@ import ( "context" "errors" "fmt" - "io" "io/fs" "os" "path/filepath" @@ -15,61 +14,29 @@ import ( "github.com/docker/docker-agent/pkg/atomicfile" "github.com/docker/docker-agent/pkg/tools/builtin/plan" - "github.com/docker/docker-agent/pkg/tools/builtin/sessionplan" ) -// sessionMutationReason makes the unsupported-operation error actionable: -// it names the constraint and what to do instead. -const sessionMutationReason = "session plans have no versions and belong to their session; change the plan from within its session, or use a shared plan for cross-session collaboration" - type service struct { - storage plan.Storage - sessionDir string + storage plan.Storage } var _ Service = (*service)(nil) -// Option configures a Service built by NewService. -type Option func(*service) - -// WithSessionDir overrides the directory the session plan is read from, -// defaulting to sessionplan.DefaultDir(). -func WithSessionDir(dir string) Option { - return func(s *service) { s.sessionDir = dir } -} - // NewService returns a Service over the given shared-plan storage. Pass // plan.SharedStorage() to operate on the same store — and serialize on the // same mutex — as the plan tools of agents running in this process; any other // plan.Storage yields an isolated service. The storage must not be nil. -func NewService(storage plan.Storage, opts ...Option) Service { +func NewService(storage plan.Storage) Service { if storage == nil { panic("plans: storage must not be nil") } - s := &service{storage: storage, sessionDir: sessionplan.DefaultDir()} - for _, opt := range opts { - opt(s) - } - return s + return &service{storage: storage} } -func (s *service) List(ctx context.Context, opts ListOptions) (ListResult, error) { +func (s *service) List(ctx context.Context) (ListResult, error) { // Always a non-nil slice so an empty listing serializes as [] not null. result := ListResult{Plans: []Plan{}} - if opts.SessionID != "" { - p, ok, warning, err := s.statSessionPlan(opts.SessionID) - if err != nil { - return ListResult{}, err - } - if warning != "" { - result.Warnings = append(result.Warnings, warning) - } - if ok { - result.Plans = append(result.Plans, p) - } - } - summaries, warnings, err := s.storage.List(ctx) if err != nil { return ListResult{}, &StorageError{Scope: ScopeShared, Op: "list", Err: err} @@ -93,18 +60,21 @@ func (s *service) List(ctx context.Context, opts ListOptions) (ListResult, error } func (s *service) Get(ctx context.Context, ref Ref) (Plan, error) { - switch ref.Scope { - case ScopeShared: - return s.getShared(ctx, ref.Name) - case ScopeSession: - return s.getSession(ref.SessionID) - default: - return Plan{}, invalidScopeError(ref.Scope) + if err := checkRef(ref); err != nil { + return Plan{}, err + } + p, ok, err := s.storage.Get(ctx, ref.Name) + if err != nil { + return Plan{}, sharedError("get", ref.Name, err) } + if !ok { + return Plan{}, &NotFoundError{Scope: ScopeShared, Name: ref.Name} + } + return sharedPlan(p), nil } func (s *service) Create(ctx context.Context, req CreateRequest) (Plan, error) { - if err := checkSharedMutation("create", req.Ref); err != nil { + if err := checkRef(req.Ref); err != nil { return Plan{}, err } if err := validateContent(req.Content); err != nil { @@ -132,7 +102,7 @@ func (s *service) Create(ctx context.Context, req CreateRequest) (Plan, error) { } func (s *service) Update(ctx context.Context, req UpdateRequest) (Plan, error) { - if err := checkSharedMutation("update", req.Ref); err != nil { + if err := checkRef(req.Ref); err != nil { return Plan{}, err } if err := validateContent(req.Content); err != nil { @@ -153,50 +123,8 @@ func (s *service) Update(ctx context.Context, req UpdateRequest) (Plan, error) { return sharedPlan(p), nil } -// UpdateSession replaces the session plan's markdown through -// sessionplan.WriteContent, whose atomic rename means a reader observes the -// old or the new content, never a partial write, and an existing symlink -// entry is replaced rather than followed. Session plans have no revisions, -// so concurrent valid writers are last-write-wins by design. The pre-check -// enforces the edit-never-creates contract — a missing plan is a -// *NotFoundError — and, like every session-plan read, refuses to treat a -// non-regular file as a plan. -func (s *service) UpdateSession(ctx context.Context, sessionID, content string) (Plan, error) { - if err := validateContent(content); err != nil { - return Plan{}, err - } - path, err := sessionplan.Path(s.sessionDir, sessionID) - if err != nil { - return Plan{}, sessionError("update", sessionID, err) - } - info, err := os.Stat(path) - switch { - case errors.Is(err, fs.ErrNotExist): - return Plan{}, &NotFoundError{Scope: ScopeSession, Name: sessionID} - case err != nil: - return Plan{}, &StorageError{Scope: ScopeSession, Op: "update", Err: err} - case !info.Mode().IsRegular(): - return Plan{}, &CorruptError{Scope: ScopeSession, Name: sessionID, Err: fmt.Errorf("%s is not a regular file", path)} - } - // Observe cancellation before persisting, mirroring the shared storage: - // a caller whose deadline already expired must not mutate the plan. - if err := ctx.Err(); err != nil { - return Plan{}, &StorageError{Scope: ScopeSession, Op: "update", Err: err} - } - // An external deletion can still land between the pre-check and this - // write, which would then recreate the plan. That narrow race is - // accepted; closing it would take platform-specific no-create - // publication machinery for little practical gain. - if _, err := sessionplan.WriteContent(s.sessionDir, sessionID, content); err != nil { - return Plan{}, sessionError("update", sessionID, err) - } - // Read the plan back so the caller gets the stored bytes and the real - // file modification time. - return s.getSession(sessionID) -} - func (s *service) SetStatus(ctx context.Context, req SetStatusRequest) (Plan, error) { - if err := checkSharedMutation("set_status", req.Ref); err != nil { + if err := checkRef(req.Ref); err != nil { return Plan{}, err } if req.Status == "" { @@ -215,7 +143,7 @@ func (s *service) SetStatus(ctx context.Context, req SetStatusRequest) (Plan, er } func (s *service) Delete(ctx context.Context, req DeleteRequest) error { - if err := checkSharedMutation("delete", req.Ref); err != nil { + if err := checkRef(req.Ref); err != nil { return err } deleted, err := s.storage.Delete(ctx, req.Ref.Name, req.ExpectedVersion) @@ -248,97 +176,6 @@ func (s *service) Export(ctx context.Context, req ExportRequest) (ExportResult, }, nil } -func (s *service) getShared(ctx context.Context, name string) (Plan, error) { - if err := plan.ValidateName(name); err != nil { - return Plan{}, &ValidationError{Message: err.Error()} - } - p, ok, err := s.storage.Get(ctx, name) - if err != nil { - return Plan{}, sharedError("get", name, err) - } - if !ok { - return Plan{}, &NotFoundError{Scope: ScopeShared, Name: name} - } - return sharedPlan(p), nil -} - -func (s *service) getSession(sessionID string) (Plan, error) { - path, err := sessionplan.Path(s.sessionDir, sessionID) - if err != nil { - return Plan{}, sessionError("get", sessionID, err) - } - content, modTime, err := readSessionPlanFile(sessionID, path) - if err != nil { - return Plan{}, err - } - p := sessionPlan(sessionID, path, modTime) - p.Content = content - return p, nil -} - -// readSessionPlanFile reads a session plan's markdown bounded by the same -// content cap as shared plans, so a hostile or damaged file in the session -// plans directory can never cause unbounded allocation. Every check runs on -// the opened descriptor, never on the path, and the open itself is hang-safe -// (see plan.OpenContentFile). A plan that exists but is not a readable plan -// file — a directory, a device, or oversized content — is a *CorruptError so -// it is never mistaken for a missing plan; genuine I/O failures remain -// *StorageError. -func readSessionPlanFile(sessionID, path string) (content string, modTime time.Time, err error) { - f, err := plan.OpenContentFile(path) - if errors.Is(err, fs.ErrNotExist) { - return "", time.Time{}, &NotFoundError{Scope: ScopeSession, Name: sessionID} - } - if err != nil { - return "", time.Time{}, &StorageError{Scope: ScopeSession, Op: "get", Err: err} - } - defer f.Close() - - info, err := f.Stat() - if err != nil { - return "", time.Time{}, &StorageError{Scope: ScopeSession, Op: "get", Err: err} - } - if !info.Mode().IsRegular() { - return "", time.Time{}, &CorruptError{Scope: ScopeSession, Name: sessionID, Err: fmt.Errorf("%s is not a regular file", path)} - } - - // Read one byte past the cap so an over-cap file is detected without - // trusting a stat size that could change under us. - data, err := io.ReadAll(io.LimitReader(f, plan.MaxPlanContentSize+1)) - if err != nil { - return "", time.Time{}, &StorageError{Scope: ScopeSession, Op: "get", Err: err} - } - if len(data) > plan.MaxPlanContentSize { - return "", time.Time{}, &CorruptError{Scope: ScopeSession, Name: sessionID, Err: fmt.Errorf("plan file exceeds %d bytes", plan.MaxPlanContentSize)} - } - return string(data), info.ModTime().UTC(), nil -} - -// statSessionPlan returns list metadata for the session's plan without -// reading its content. A missing plan is (_, false, "", nil); an existing but -// unreadable one is surfaced as a warning, mirroring how List reports -// unreadable shared plans. -func (s *service) statSessionPlan(sessionID string) (p Plan, ok bool, warning string, err error) { - path, err := sessionplan.Path(s.sessionDir, sessionID) - if err != nil { - return Plan{}, false, "", sessionError("list", sessionID, err) - } - info, err := os.Stat(path) - switch { - case errors.Is(err, fs.ErrNotExist): - return Plan{}, false, "", nil - case err != nil: - return Plan{}, false, fmt.Sprintf("skipped session plan %q: %v", sessionID, err), nil - case info.IsDir(): - return Plan{}, false, fmt.Sprintf("skipped session plan %q: %s is a directory", sessionID, path), nil - case !info.Mode().IsRegular(): - return Plan{}, false, fmt.Sprintf("skipped session plan %q: %s is not a regular file", sessionID, path), nil - case info.Size() > plan.MaxPlanContentSize: - return Plan{}, false, fmt.Sprintf("skipped session plan %q: plan file exceeds %d bytes", sessionID, plan.MaxPlanContentSize), nil - } - return sessionPlan(sessionID, path, info.ModTime().UTC()), true, "", nil -} - // validateContent gates mutation content: it must be non-empty and within // the advertised content cap, refused as invalid input before the storage is // touched. Content of exactly the cap is accepted. @@ -352,21 +189,16 @@ func validateContent(content string) error { return nil } -// checkSharedMutation gates every mutation: session plans are refused with a -// typed, actionable error, unknown scopes are invalid input, and shared names +// checkRef gates every operation: unknown scopes are invalid input, and names // are validated with the storage's canonical rule before touching it. -func checkSharedMutation(op string, ref Ref) error { - switch ref.Scope { - case ScopeShared: - if err := plan.ValidateName(ref.Name); err != nil { - return &ValidationError{Message: err.Error()} - } - return nil - case ScopeSession: - return &UnsupportedError{Scope: ScopeSession, Op: op, Reason: sessionMutationReason} - default: +func checkRef(ref Ref) error { + if ref.Scope != ScopeShared { return invalidScopeError(ref.Scope) } + if err := plan.ValidateName(ref.Name); err != nil { + return &ValidationError{Message: err.Error()} + } + return nil } // sharedError maps a plan.Storage failure to this package's typed errors by @@ -386,19 +218,8 @@ func sharedError(op, name string, err error) error { return &StorageError{Scope: ScopeShared, Op: op, Err: err} } -func sessionError(op, sessionID string, err error) error { - switch { - case errors.Is(err, sessionplan.ErrPlanNotFound): - return &NotFoundError{Scope: ScopeSession, Name: sessionID} - case errors.Is(err, sessionplan.ErrInvalidSessionID): - return &ValidationError{Message: err.Error()} - default: - return &StorageError{Scope: ScopeSession, Op: op, Err: err} - } -} - func invalidScopeError(scope Scope) error { - return &ValidationError{Message: fmt.Sprintf("invalid plan scope %q: use %q or %q", scope, ScopeShared, ScopeSession)} + return &ValidationError{Message: fmt.Sprintf("invalid plan scope %q: use %q", scope, ScopeShared)} } func sharedPlan(p plan.Plan) Plan { @@ -414,16 +235,6 @@ func sharedPlan(p plan.Plan) Plan { } } -func sessionPlan(sessionID, path string, modTime time.Time) Plan { - return Plan{ - Scope: ScopeSession, - Name: sessionID, - SessionID: sessionID, - UpdatedAt: modTime, - Path: path, - } -} - // parseUpdatedAt tolerates a missing or malformed stored timestamp: it is // display metadata, so it degrades to the zero time instead of failing a read. func parseUpdatedAt(s string) time.Time { diff --git a/pkg/plans/service_symlink_test.go b/pkg/plans/service_symlink_test.go index 01b2cf871c..b191ae95e5 100644 --- a/pkg/plans/service_symlink_test.go +++ b/pkg/plans/service_symlink_test.go @@ -29,7 +29,7 @@ func symlinkedDestination(t *testing.T, targetContent string) (link, target stri // file it points to untouched. func TestService_ExportRefusesSymlinkDestination(t *testing.T) { t.Parallel() - svc, _, _ := newTestService(t) + svc, _ := newTestService(t) mustCreate(t, svc, "p", "new body") link, target := symlinkedDestination(t, "precious") @@ -57,7 +57,7 @@ func TestService_ExportRefusesSymlinkDestination(t *testing.T) { // left behind. func TestService_ExportRefusesDanglingSymlinkDestination(t *testing.T) { t.Parallel() - svc, _, _ := newTestService(t) + svc, _ := newTestService(t) mustCreate(t, svc, "p", "new body") dir := t.TempDir() @@ -83,7 +83,7 @@ func TestService_ExportRefusesDanglingSymlinkDestination(t *testing.T) { // the exported body, and the file the link pointed to is never modified. func TestService_ExportForceReplacesSymlinkEntryNotTarget(t *testing.T) { t.Parallel() - svc, _, _ := newTestService(t) + svc, _ := newTestService(t) mustCreate(t, svc, "p", "new body") link, target := symlinkedDestination(t, "precious") @@ -102,28 +102,3 @@ func TestService_ExportForceReplacesSymlinkEntryNotTarget(t *testing.T) { require.NoError(t, err) assert.Equal(t, "precious", string(data), "the symlink target must be untouched") } - -// TestService_UpdateSessionReplacesSymlinkEntryNotTarget proves the session -// edit publishes through the atomic rename of sessionplan.WriteContent: a -// symlink squatting on the plan path becomes a regular file holding the new -// body, and the file the link pointed to is never modified. -func TestService_UpdateSessionReplacesSymlinkEntryNotTarget(t *testing.T) { - t.Parallel() - svc, _, sessionDir := newTestService(t) - target := filepath.Join(t.TempDir(), "target.md") - require.NoError(t, os.WriteFile(target, []byte("precious"), 0o600)) - link := filepath.Join(sessionDir, "sess-1.md") - require.NoError(t, os.Symlink(target, link)) - - p, err := svc.UpdateSession(t.Context(), "sess-1", "new body") - require.NoError(t, err) - assert.Equal(t, "new body", p.Content) - - info, err := os.Lstat(link) - require.NoError(t, err) - assert.True(t, info.Mode().IsRegular(), "the edit must replace the symlink entry itself, not write through it") - - data, err := os.ReadFile(target) - require.NoError(t, err) - assert.Equal(t, "precious", string(data), "the symlink target must be untouched") -} diff --git a/pkg/plans/service_test.go b/pkg/plans/service_test.go index 417989f960..5c9a4bf180 100644 --- a/pkg/plans/service_test.go +++ b/pkg/plans/service_test.go @@ -17,18 +17,15 @@ import ( "github.com/stretchr/testify/require" "github.com/docker/docker-agent/pkg/tools/builtin/plan" - "github.com/docker/docker-agent/pkg/tools/builtin/sessionplan" ) // newTestService builds a Service over a fresh filesystem-backed shared -// storage and an isolated session-plans directory, returning both directories -// for tests that plant files directly. -func newTestService(t *testing.T) (svc Service, sharedDir, sessionDir string) { +// storage, returning the directory for tests that plant files directly. +func newTestService(t *testing.T) (svc Service, sharedDir string) { t.Helper() sharedDir = t.TempDir() - sessionDir = t.TempDir() - svc = NewService(plan.NewFilesystemStorage(sharedDir), WithSessionDir(sessionDir)) - return svc, sharedDir, sessionDir + svc = NewService(plan.NewFilesystemStorage(sharedDir)) + return svc, sharedDir } func mustCreate(t *testing.T, svc Service, name, content string) Plan { @@ -38,47 +35,29 @@ func mustCreate(t *testing.T, svc Service, name, content string) Plan { return p } -func writeSessionPlan(t *testing.T, dir, sessionID, content string) string { - t.Helper() - path, err := sessionplan.WriteContent(dir, sessionID, content) - require.NoError(t, err) - return path -} - // --- List -------------------------------------------------------------------- func TestService_ListEmpty(t *testing.T) { t.Parallel() - svc, _, _ := newTestService(t) + svc, _ := newTestService(t) - result, err := svc.List(t.Context(), ListOptions{}) + result, err := svc.List(t.Context()) require.NoError(t, err) assert.NotNil(t, result.Plans) assert.Empty(t, result.Plans) assert.Empty(t, result.Warnings) } -func TestService_ListEmptyWithSessionID(t *testing.T) { - t.Parallel() - svc, _, _ := newTestService(t) - - // A missing session plan is not an error during List. - result, err := svc.List(t.Context(), ListOptions{SessionID: "sess-1"}) - require.NoError(t, err) - assert.Empty(t, result.Plans) - assert.Empty(t, result.Warnings) -} - func TestService_ListSharedMetadataOnly(t *testing.T) { t.Parallel() - svc, _, _ := newTestService(t) + svc, _ := newTestService(t) _, err := svc.Create(t.Context(), CreateRequest{Ref: SharedRef("beta"), Content: "b", Status: "draft"}) require.NoError(t, err) _, err = svc.Create(t.Context(), CreateRequest{Ref: SharedRef("alpha"), Content: "a", Title: "Alpha", Author: "alice"}) require.NoError(t, err) - result, err := svc.List(t.Context(), ListOptions{}) + result, err := svc.List(t.Context()) require.NoError(t, err) require.Len(t, result.Plans, 2) @@ -98,61 +77,13 @@ func TestService_ListSharedMetadataOnly(t *testing.T) { assert.Equal(t, "draft", result.Plans[1].Status) } -func TestService_ListIncludesCurrentSessionPlanFirst(t *testing.T) { - t.Parallel() - svc, _, sessionDir := newTestService(t) - mustCreate(t, svc, "alpha", "a") - path := writeSessionPlan(t, sessionDir, "sess-1", "# session plan") - - result, err := svc.List(t.Context(), ListOptions{SessionID: "sess-1"}) - require.NoError(t, err) - require.Len(t, result.Plans, 2) - - sess := result.Plans[0] - assert.Equal(t, ScopeSession, sess.Scope) - assert.Equal(t, "sess-1", sess.Name) - assert.Equal(t, "sess-1", sess.SessionID) - assert.Equal(t, path, sess.Path) - assert.Nil(t, sess.Version, "session plans have no version") - assert.Empty(t, sess.Content, "List is metadata only") - - // The timestamp comes from file metadata. - info, err := os.Stat(path) - require.NoError(t, err) - assert.True(t, sess.UpdatedAt.Equal(info.ModTime().UTC())) - - assert.Equal(t, ScopeShared, result.Plans[1].Scope) - assert.Equal(t, "alpha", result.Plans[1].Name) -} - -func TestService_ListConsultsOnlySuppliedSession(t *testing.T) { - t.Parallel() - svc, _, sessionDir := newTestService(t) - writeSessionPlan(t, sessionDir, "current", "mine") - writeSessionPlan(t, sessionDir, "stale-other", "left behind") - - result, err := svc.List(t.Context(), ListOptions{SessionID: "current"}) - require.NoError(t, err) - require.Len(t, result.Plans, 1) - assert.Equal(t, "current", result.Plans[0].Name) -} - -func TestService_ListInvalidSessionID(t *testing.T) { - t.Parallel() - svc, _, _ := newTestService(t) - - _, err := svc.List(t.Context(), ListOptions{SessionID: "../escape"}) - var invalid *ValidationError - require.ErrorAs(t, err, &invalid) -} - func TestService_ListWarnsOnCorruptShared(t *testing.T) { t.Parallel() - svc, sharedDir, _ := newTestService(t) + svc, sharedDir := newTestService(t) mustCreate(t, svc, "good", "ok") require.NoError(t, os.WriteFile(filepath.Join(sharedDir, "bad.json"), []byte("{nope"), 0o600)) - result, err := svc.List(t.Context(), ListOptions{}) + result, err := svc.List(t.Context()) require.NoError(t, err) require.Len(t, result.Plans, 1) assert.Equal(t, "good", result.Plans[0].Name) @@ -163,9 +94,9 @@ func TestService_ListWarnsOnCorruptShared(t *testing.T) { func TestService_ListStorageFailure(t *testing.T) { t.Parallel() base := errors.New("backend boom") - svc := NewService(failingStorage{err: base}, WithSessionDir(t.TempDir())) + svc := NewService(failingStorage{err: base}) - _, err := svc.List(t.Context(), ListOptions{}) + _, err := svc.List(t.Context()) var storageErr *StorageError require.ErrorAs(t, err, &storageErr) assert.Equal(t, ScopeShared, storageErr.Scope) @@ -175,31 +106,28 @@ func TestService_ListStorageFailure(t *testing.T) { // TestService_ListSortsSharedPlansFromUnsortedStorage pins the documented // sort order independently of the backend: an injected Storage that lists in -// arbitrary order must still yield a name-sorted listing, with the session -// plan first. +// arbitrary order must still yield a name-sorted listing. func TestService_ListSortsSharedPlansFromUnsortedStorage(t *testing.T) { t.Parallel() - sessionDir := t.TempDir() - svc := NewService(unsortedStorage{names: []string{"zeta", "alpha", "mid"}}, WithSessionDir(sessionDir)) - writeSessionPlan(t, sessionDir, "sess-1", "# session plan") + svc := NewService(unsortedStorage{names: []string{"zeta", "alpha", "mid"}}) - result, err := svc.List(t.Context(), ListOptions{SessionID: "sess-1"}) + result, err := svc.List(t.Context()) require.NoError(t, err) - require.Len(t, result.Plans, 4) + require.Len(t, result.Plans, 3) names := make([]string, 0, len(result.Plans)) for _, p := range result.Plans { names = append(names, p.Name) } - assert.Equal(t, []string{"sess-1", "alpha", "mid", "zeta"}, names, - "the session plan comes first, shared plans sorted by name regardless of storage order") + assert.Equal(t, []string{"alpha", "mid", "zeta"}, names, + "shared plans sorted by name regardless of storage order") } // --- Get --------------------------------------------------------------------- func TestService_GetShared(t *testing.T) { t.Parallel() - svc, _, _ := newTestService(t) + svc, _ := newTestService(t) _, err := svc.Create(t.Context(), CreateRequest{ Ref: SharedRef("release"), Content: "the body", Title: "Release", Author: "alice", Status: "draft", }) @@ -216,13 +144,11 @@ func TestService_GetShared(t *testing.T) { require.NotNil(t, p.Version) assert.Equal(t, 1, *p.Version) assert.False(t, p.UpdatedAt.IsZero()) - assert.Empty(t, p.SessionID) - assert.Empty(t, p.Path) } func TestService_GetSharedNotFound(t *testing.T) { t.Parallel() - svc, _, _ := newTestService(t) + svc, _ := newTestService(t) _, err := svc.Get(t.Context(), SharedRef("missing")) var notFound *NotFoundError @@ -233,7 +159,7 @@ func TestService_GetSharedNotFound(t *testing.T) { func TestService_GetSharedInvalidName(t *testing.T) { t.Parallel() - svc, _, _ := newTestService(t) + svc, _ := newTestService(t) for _, name := range []string{"", "UPPER", "../escape", "a/b", "has space"} { _, err := svc.Get(t.Context(), SharedRef(name)) @@ -245,7 +171,7 @@ func TestService_GetSharedInvalidName(t *testing.T) { func TestService_GetSharedCorrupt(t *testing.T) { t.Parallel() - svc, sharedDir, _ := newTestService(t) + svc, sharedDir := newTestService(t) require.NoError(t, os.WriteFile(filepath.Join(sharedDir, "broken.json"), []byte("{not json"), 0o600)) _, err := svc.Get(t.Context(), SharedRef("broken")) @@ -261,120 +187,9 @@ func TestService_GetSharedCorrupt(t *testing.T) { require.NotErrorAs(t, err, ¬Found, "a corrupt plan must not read as missing") } -func TestService_GetSession(t *testing.T) { - t.Parallel() - svc, _, sessionDir := newTestService(t) - path := writeSessionPlan(t, sessionDir, "sess-1", "# my plan\nstep 1\n") - - p, err := svc.Get(t.Context(), SessionRef("sess-1")) - require.NoError(t, err) - assert.Equal(t, ScopeSession, p.Scope) - assert.Equal(t, "sess-1", p.Name) - assert.Equal(t, "sess-1", p.SessionID) - assert.Equal(t, "# my plan\nstep 1\n", p.Content) - assert.Equal(t, path, p.Path) - assert.Nil(t, p.Version, "session plans must not expose a version") - assert.Empty(t, p.Status) - - info, err := os.Stat(path) - require.NoError(t, err) - assert.True(t, p.UpdatedAt.Equal(info.ModTime().UTC())) -} - -func TestService_GetSessionNotFound(t *testing.T) { - t.Parallel() - svc, _, _ := newTestService(t) - - // Missing is not-found during Get, unlike List where it is skipped. - _, err := svc.Get(t.Context(), SessionRef("ghost")) - var notFound *NotFoundError - require.ErrorAs(t, err, ¬Found) - assert.Equal(t, ScopeSession, notFound.Scope) - assert.Equal(t, "ghost", notFound.Name) -} - -func TestService_GetSessionInvalidID(t *testing.T) { - t.Parallel() - svc, _, _ := newTestService(t) - - for _, id := range []string{"", "../escape", "a/b"} { - _, err := svc.Get(t.Context(), SessionRef(id)) - var invalid *ValidationError - require.ErrorAs(t, err, &invalid, "session ID %q should be invalid", id) - } -} - -// TestService_GetSessionOversized proves the host read of session-plan -// markdown is bounded: a file past the shared content cap is a typed -// *CorruptError — the plan exists but cannot be treated as a plan — never an -// unbounded read or a not-found. -func TestService_GetSessionOversized(t *testing.T) { - t.Parallel() - svc, _, sessionDir := newTestService(t) - require.NoError(t, os.MkdirAll(sessionDir, 0o700)) - require.NoError(t, os.WriteFile(filepath.Join(sessionDir, "sess-1.md"), make([]byte, plan.MaxPlanContentSize+1), 0o600)) - - _, err := svc.Get(t.Context(), SessionRef("sess-1")) - var corrupt *CorruptError - require.ErrorAs(t, err, &corrupt) - assert.Equal(t, ScopeSession, corrupt.Scope) - assert.Equal(t, "sess-1", corrupt.Name) - assert.Contains(t, err.Error(), "exceeds") - - var notFound *NotFoundError - require.NotErrorAs(t, err, ¬Found, "an oversized plan must not read as missing") - - // Export goes through Get and must refuse the same way, writing nothing. - dest := filepath.Join(t.TempDir(), "export.md") - _, err = svc.Export(t.Context(), ExportRequest{Ref: SessionRef("sess-1"), Path: dest}) - require.ErrorAs(t, err, &corrupt) - assert.NoFileExists(t, dest) - - // List skips it with a warning, mirroring unreadable shared plans. - result, err := svc.List(t.Context(), ListOptions{SessionID: "sess-1"}) - require.NoError(t, err) - assert.Empty(t, result.Plans) - require.Len(t, result.Warnings, 1) - assert.Contains(t, result.Warnings[0], "sess-1") - assert.Contains(t, result.Warnings[0], "exceeds") -} - -// TestService_GetSessionAtSizeCap proves the bound is exact: a session plan -// of exactly the content cap reads back whole. -func TestService_GetSessionAtSizeCap(t *testing.T) { - t.Parallel() - svc, _, sessionDir := newTestService(t) - content := strings.Repeat("a", plan.MaxPlanContentSize) - writeSessionPlan(t, sessionDir, "sess-1", content) - - p, err := svc.Get(t.Context(), SessionRef("sess-1")) - require.NoError(t, err) - assert.Len(t, p.Content, plan.MaxPlanContentSize) - assert.False(t, p.UpdatedAt.IsZero()) -} - -// TestService_GetSessionNotRegularFile proves a directory squatting on the -// session plan path is a *CorruptError on Get and a warning on List. -func TestService_GetSessionNotRegularFile(t *testing.T) { - t.Parallel() - svc, _, sessionDir := newTestService(t) - require.NoError(t, os.MkdirAll(filepath.Join(sessionDir, "sess-1.md"), 0o700)) - - _, err := svc.Get(t.Context(), SessionRef("sess-1")) - var corrupt *CorruptError - require.ErrorAs(t, err, &corrupt) - assert.Equal(t, ScopeSession, corrupt.Scope) - - result, err := svc.List(t.Context(), ListOptions{SessionID: "sess-1"}) - require.NoError(t, err) - assert.Empty(t, result.Plans) - require.Len(t, result.Warnings, 1) - assert.Contains(t, result.Warnings[0], "directory") -} - func TestService_GetUnknownScope(t *testing.T) { t.Parallel() - svc, _, _ := newTestService(t) + svc, _ := newTestService(t) _, err := svc.Get(t.Context(), Ref{Scope: "bogus", Name: "p"}) var invalid *ValidationError @@ -386,7 +201,7 @@ func TestService_GetUnknownScope(t *testing.T) { func TestService_Create(t *testing.T) { t.Parallel() - svc, _, _ := newTestService(t) + svc, _ := newTestService(t) p, err := svc.Create(t.Context(), CreateRequest{ Ref: SharedRef("release"), Content: "v1", Title: "T", Author: "alice", Status: "draft", @@ -404,7 +219,7 @@ func TestService_Create(t *testing.T) { func TestService_CreateIsCreateOnly(t *testing.T) { t.Parallel() - svc, _, _ := newTestService(t) + svc, _ := newTestService(t) mustCreate(t, svc, "p", "original") // A second create conflicts instead of overwriting, exposing the @@ -433,7 +248,7 @@ func TestService_CreateIsCreateOnly(t *testing.T) { // byte-identical, and creating under a fresh name keeps working. func TestService_CreateConflictsWithExistingRevisionZeroFile(t *testing.T) { t.Parallel() - svc, sharedDir, _ := newTestService(t) + svc, sharedDir := newTestService(t) original := `{"name":"planted","content":"precious content"}` path := filepath.Join(sharedDir, "planted.json") @@ -459,7 +274,7 @@ func TestService_CreateConflictsWithExistingRevisionZeroFile(t *testing.T) { func TestService_CreateValidation(t *testing.T) { t.Parallel() - svc, _, _ := newTestService(t) + svc, _ := newTestService(t) _, err := svc.Create(t.Context(), CreateRequest{Ref: SharedRef("p"), Content: ""}) var invalid *ValidationError @@ -478,7 +293,7 @@ func TestService_CreateValidation(t *testing.T) { // typed *ValidationError before the storage is touched. func TestService_ContentSizeCap(t *testing.T) { t.Parallel() - svc, _, _ := newTestService(t) + svc, _ := newTestService(t) atCap := strings.Repeat("a", plan.MaxPlanContentSize) p, err := svc.Create(t.Context(), CreateRequest{Ref: SharedRef("big"), Content: atCap}) @@ -509,7 +324,7 @@ func TestService_ContentSizeCap(t *testing.T) { // is size-capped (issue #3844: labels have no fixed vocabulary or size). func TestService_LargeMetadataAccepted(t *testing.T) { t.Parallel() - svc, _, _ := newTestService(t) + svc, _ := newTestService(t) bigTitle := strings.Repeat("t", 5<<10) bigAuthor := strings.Repeat("a", 5<<10) @@ -543,7 +358,7 @@ func TestService_LargeMetadataAccepted(t *testing.T) { func TestService_UpdatePreservesOmittedMetadata(t *testing.T) { t.Parallel() - svc, _, _ := newTestService(t) + svc, _ := newTestService(t) _, err := svc.Create(t.Context(), CreateRequest{ Ref: SharedRef("p"), Content: "v1", Title: "Original", Author: "alice", Status: "draft", }) @@ -570,7 +385,7 @@ func TestService_UpdatePreservesOmittedMetadata(t *testing.T) { func TestService_UpdateStaleConflictPreservesNewerContent(t *testing.T) { t.Parallel() - svc, _, _ := newTestService(t) + svc, _ := newTestService(t) mustCreate(t, svc, "p", "v1") _, err := svc.Update(t.Context(), UpdateRequest{Ref: SharedRef("p"), Content: "v2", ExpectedVersion: new(1)}) require.NoError(t, err) @@ -589,7 +404,7 @@ func TestService_UpdateStaleConflictPreservesNewerContent(t *testing.T) { func TestService_UpdateForceReplacesUnconditionally(t *testing.T) { t.Parallel() - svc, _, _ := newTestService(t) + svc, _ := newTestService(t) mustCreate(t, svc, "p", "v1") _, err := svc.Update(t.Context(), UpdateRequest{Ref: SharedRef("p"), Content: "v2", ExpectedVersion: new(1)}) require.NoError(t, err) @@ -603,7 +418,7 @@ func TestService_UpdateForceReplacesUnconditionally(t *testing.T) { func TestService_UpdateNotFound(t *testing.T) { t.Parallel() - svc, _, _ := newTestService(t) + svc, _ := newTestService(t) var notFound *NotFoundError @@ -620,7 +435,7 @@ func TestService_UpdateNotFound(t *testing.T) { func TestService_UpdateEmptyContent(t *testing.T) { t.Parallel() - svc, _, _ := newTestService(t) + svc, _ := newTestService(t) mustCreate(t, svc, "p", "v1") _, err := svc.Update(t.Context(), UpdateRequest{Ref: SharedRef("p"), Content: ""}) @@ -629,128 +444,11 @@ func TestService_UpdateEmptyContent(t *testing.T) { assert.Contains(t, invalid.Message, "content must not be empty") } -// --- UpdateSession ------------------------------------------------------------- - -func TestService_UpdateSession(t *testing.T) { - t.Parallel() - svc, _, sessionDir := newTestService(t) - path := writeSessionPlan(t, sessionDir, "sess-1", "# old plan") - - p, err := svc.UpdateSession(t.Context(), "sess-1", "# new plan\nstep 1\n") - require.NoError(t, err) - assert.Equal(t, ScopeSession, p.Scope) - assert.Equal(t, "sess-1", p.Name) - assert.Equal(t, "sess-1", p.SessionID) - assert.Equal(t, "# new plan\nstep 1\n", p.Content) - assert.Equal(t, path, p.Path) - assert.Nil(t, p.Version, "session plans must not expose a version") - assert.Empty(t, p.Status) - assert.False(t, p.UpdatedAt.IsZero()) - - got, err := svc.Get(t.Context(), SessionRef("sess-1")) - require.NoError(t, err) - assert.Equal(t, "# new plan\nstep 1\n", got.Content) -} - -func TestService_UpdateSessionLastWriteWins(t *testing.T) { - t.Parallel() - svc, _, sessionDir := newTestService(t) - writeSessionPlan(t, sessionDir, "sess-1", "v1") - - // Session plans have no versions: repeated writes simply replace. - _, err := svc.UpdateSession(t.Context(), "sess-1", "v2") - require.NoError(t, err) - p, err := svc.UpdateSession(t.Context(), "sess-1", "v3") - require.NoError(t, err) - assert.Equal(t, "v3", p.Content) - assert.Nil(t, p.Version) -} - -func TestService_UpdateSessionInvalidID(t *testing.T) { - t.Parallel() - svc, _, _ := newTestService(t) - - for _, id := range []string{"", "../escape", "a/b"} { - _, err := svc.UpdateSession(t.Context(), id, "content") - var invalid *ValidationError - require.ErrorAs(t, err, &invalid, "session ID %q should be invalid", id) - } -} - -func TestService_UpdateSessionNeverCreates(t *testing.T) { - t.Parallel() - svc, _, sessionDir := newTestService(t) - - _, err := svc.UpdateSession(t.Context(), "ghost", "content") - var notFound *NotFoundError - require.ErrorAs(t, err, ¬Found) - assert.Equal(t, ScopeSession, notFound.Scope) - assert.Equal(t, "ghost", notFound.Name) - - _, err = svc.Get(t.Context(), SessionRef("ghost")) - require.ErrorAs(t, err, ¬Found, "the refused update must not have created the plan") - assert.NoFileExists(t, filepath.Join(sessionDir, "ghost.md")) -} - -func TestService_UpdateSessionValidation(t *testing.T) { - t.Parallel() - svc, _, sessionDir := newTestService(t) - writeSessionPlan(t, sessionDir, "sess-1", "# old plan") - var invalid *ValidationError - - _, err := svc.UpdateSession(t.Context(), "sess-1", "") - require.ErrorAs(t, err, &invalid) - assert.Contains(t, invalid.Message, "content must not be empty") - - _, err = svc.UpdateSession(t.Context(), "sess-1", strings.Repeat("a", plan.MaxPlanContentSize+1)) - require.ErrorAs(t, err, &invalid) - assert.Contains(t, invalid.Message, "maximum plan size") - - got, err := svc.Get(t.Context(), SessionRef("sess-1")) - require.NoError(t, err) - assert.Equal(t, "# old plan", got.Content, "a refused update must leave the plan untouched") -} - -// TestService_UpdateSessionNotRegularFile proves a directory squatting on the -// session plan path refuses the update as a *CorruptError, mirroring Get. -func TestService_UpdateSessionNotRegularFile(t *testing.T) { - t.Parallel() - svc, _, sessionDir := newTestService(t) - require.NoError(t, os.MkdirAll(filepath.Join(sessionDir, "sess-1.md"), 0o700)) - - _, err := svc.UpdateSession(t.Context(), "sess-1", "content") - var corrupt *CorruptError - require.ErrorAs(t, err, &corrupt) - assert.Equal(t, ScopeSession, corrupt.Scope) - assert.Equal(t, "sess-1", corrupt.Name) -} - -// TestService_UpdateSessionExpiredContext proves cancellation is observed -// before persistence: an already-expired context never mutates the plan. -func TestService_UpdateSessionExpiredContext(t *testing.T) { - t.Parallel() - svc, _, sessionDir := newTestService(t) - writeSessionPlan(t, sessionDir, "sess-1", "# old plan") - - ctx, cancel := context.WithCancel(t.Context()) - cancel() - _, err := svc.UpdateSession(ctx, "sess-1", "new content") - var storageErr *StorageError - require.ErrorAs(t, err, &storageErr) - assert.Equal(t, ScopeSession, storageErr.Scope) - assert.Equal(t, "update", storageErr.Op) - require.ErrorIs(t, err, context.Canceled) - - got, err := svc.Get(t.Context(), SessionRef("sess-1")) - require.NoError(t, err) - assert.Equal(t, "# old plan", got.Content, "an expired context must not mutate the plan") -} - // --- SetStatus --------------------------------------------------------------- func TestService_SetStatusFreeForm(t *testing.T) { t.Parallel() - svc, _, _ := newTestService(t) + svc, _ := newTestService(t) mustCreate(t, svc, "p", "body") // Status is free-form: any non-empty string is accepted. @@ -765,7 +463,7 @@ func TestService_SetStatusFreeForm(t *testing.T) { func TestService_SetStatusStaleConflict(t *testing.T) { t.Parallel() - svc, _, _ := newTestService(t) + svc, _ := newTestService(t) _, err := svc.Create(t.Context(), CreateRequest{Ref: SharedRef("p"), Content: "body", Status: "draft"}) require.NoError(t, err) _, err = svc.Update(t.Context(), UpdateRequest{Ref: SharedRef("p"), Content: "v2", ExpectedVersion: new(1)}) @@ -783,7 +481,7 @@ func TestService_SetStatusStaleConflict(t *testing.T) { func TestService_SetStatusValidation(t *testing.T) { t.Parallel() - svc, _, _ := newTestService(t) + svc, _ := newTestService(t) mustCreate(t, svc, "p", "body") _, err := svc.SetStatus(t.Context(), SetStatusRequest{Ref: SharedRef("p"), Status: ""}) @@ -794,7 +492,7 @@ func TestService_SetStatusValidation(t *testing.T) { func TestService_SetStatusNotFound(t *testing.T) { t.Parallel() - svc, _, _ := newTestService(t) + svc, _ := newTestService(t) _, err := svc.SetStatus(t.Context(), SetStatusRequest{Ref: SharedRef("ghost"), Status: "done"}) var notFound *NotFoundError @@ -805,7 +503,7 @@ func TestService_SetStatusNotFound(t *testing.T) { func TestService_DeleteWithMatchingVersion(t *testing.T) { t.Parallel() - svc, _, _ := newTestService(t) + svc, _ := newTestService(t) mustCreate(t, svc, "p", "body") require.NoError(t, svc.Delete(t.Context(), DeleteRequest{Ref: SharedRef("p"), ExpectedVersion: new(1)})) @@ -817,7 +515,7 @@ func TestService_DeleteWithMatchingVersion(t *testing.T) { func TestService_DeleteStaleConflictPreservesPlan(t *testing.T) { t.Parallel() - svc, _, _ := newTestService(t) + svc, _ := newTestService(t) mustCreate(t, svc, "p", "v1") _, err := svc.Update(t.Context(), UpdateRequest{Ref: SharedRef("p"), Content: "v2", ExpectedVersion: new(1)}) require.NoError(t, err) @@ -834,7 +532,7 @@ func TestService_DeleteStaleConflictPreservesPlan(t *testing.T) { func TestService_DeleteForce(t *testing.T) { t.Parallel() - svc, _, _ := newTestService(t) + svc, _ := newTestService(t) mustCreate(t, svc, "p", "v1") // nil expected version deletes unconditionally. @@ -843,7 +541,7 @@ func TestService_DeleteForce(t *testing.T) { func TestService_DeleteNotFound(t *testing.T) { t.Parallel() - svc, _, _ := newTestService(t) + svc, _ := newTestService(t) err := svc.Delete(t.Context(), DeleteRequest{Ref: SharedRef("ghost")}) var notFound *NotFoundError @@ -852,7 +550,7 @@ func TestService_DeleteNotFound(t *testing.T) { func TestService_DeleteCorrupt(t *testing.T) { t.Parallel() - svc, sharedDir, _ := newTestService(t) + svc, sharedDir := newTestService(t) require.NoError(t, os.WriteFile(filepath.Join(sharedDir, "broken.json"), []byte("{nope"), 0o600)) // A guarded delete cannot verify the revision of a corrupt plan. @@ -865,53 +563,11 @@ func TestService_DeleteCorrupt(t *testing.T) { assert.NoFileExists(t, filepath.Join(sharedDir, "broken.json")) } -// --- Session mutations are unsupported ---------------------------------------- - -func TestService_SessionMutationsUnsupported(t *testing.T) { - t.Parallel() - svc, _, sessionDir := newTestService(t) - writeSessionPlan(t, sessionDir, "sess-1", "# plan") - ref := SessionRef("sess-1") - ctx := t.Context() - - calls := map[string]func() error{ - "create": func() error { - _, err := svc.Create(ctx, CreateRequest{Ref: ref, Content: "x"}) - return err - }, - "update": func() error { - _, err := svc.Update(ctx, UpdateRequest{Ref: ref, Content: "x"}) - return err - }, - "set_status": func() error { - _, err := svc.SetStatus(ctx, SetStatusRequest{Ref: ref, Status: "done"}) - return err - }, - "delete": func() error { - return svc.Delete(ctx, DeleteRequest{Ref: ref}) - }, - } - - for op, call := range calls { - err := call() - var unsupported *UnsupportedError - require.ErrorAs(t, err, &unsupported, "op %s", op) - assert.Equal(t, ScopeSession, unsupported.Scope) - assert.Equal(t, op, unsupported.Op) - assert.NotEmpty(t, unsupported.Reason, "the error must tell the caller what to do instead") - } - - // The session plan is untouched by the refused mutations. - p, err := svc.Get(ctx, ref) - require.NoError(t, err) - assert.Equal(t, "# plan", p.Content) -} - // --- Export ------------------------------------------------------------------ func TestService_ExportShared(t *testing.T) { t.Parallel() - svc, _, _ := newTestService(t) + svc, _ := newTestService(t) mustCreate(t, svc, "p", "the body") dest := filepath.Join(t.TempDir(), "nested", "export.md") @@ -929,42 +585,20 @@ func TestService_ExportShared(t *testing.T) { assert.Equal(t, "the body", string(data)) } -func TestService_ExportSession(t *testing.T) { - t.Parallel() - svc, _, sessionDir := newTestService(t) - writeSessionPlan(t, sessionDir, "sess-1", "# session plan") - - dest := filepath.Join(t.TempDir(), "export.md") - result, err := svc.Export(t.Context(), ExportRequest{Ref: SessionRef("sess-1"), Path: dest}) - require.NoError(t, err) - assert.Equal(t, ScopeSession, result.Scope) - assert.Equal(t, "sess-1", result.Name) - assert.Nil(t, result.Version, "session plans have no version to export") - assert.Equal(t, len("# session plan"), result.BytesWritten) - - data, err := os.ReadFile(dest) - require.NoError(t, err) - assert.Equal(t, "# session plan", string(data)) -} - func TestService_ExportNotFound(t *testing.T) { t.Parallel() - svc, _, _ := newTestService(t) + svc, _ := newTestService(t) var notFound *NotFoundError dest := filepath.Join(t.TempDir(), "export.md") _, err := svc.Export(t.Context(), ExportRequest{Ref: SharedRef("ghost"), Path: dest}) require.ErrorAs(t, err, ¬Found) assert.NoFileExists(t, dest) - - _, err = svc.Export(t.Context(), ExportRequest{Ref: SessionRef("ghost"), Path: dest}) - require.ErrorAs(t, err, ¬Found) - assert.NoFileExists(t, dest) } func TestService_ExportValidation(t *testing.T) { t.Parallel() - svc, _, _ := newTestService(t) + svc, _ := newTestService(t) mustCreate(t, svc, "p", "body") var invalid *ValidationError @@ -979,7 +613,7 @@ func TestService_ExportValidation(t *testing.T) { func TestService_ExportRefusesExistingDestination(t *testing.T) { t.Parallel() - svc, _, _ := newTestService(t) + svc, _ := newTestService(t) mustCreate(t, svc, "p", "new body") dest := filepath.Join(t.TempDir(), "export.md") @@ -997,7 +631,7 @@ func TestService_ExportRefusesExistingDestination(t *testing.T) { func TestService_ExportForceReplacesExistingFile(t *testing.T) { t.Parallel() - svc, _, _ := newTestService(t) + svc, _ := newTestService(t) mustCreate(t, svc, "p", "new body") dest := filepath.Join(t.TempDir(), "export.md") @@ -1014,7 +648,7 @@ func TestService_ExportForceReplacesExistingFile(t *testing.T) { func TestService_ExportForceStillRefusesDirectory(t *testing.T) { t.Parallel() - svc, _, _ := newTestService(t) + svc, _ := newTestService(t) mustCreate(t, svc, "p", "body") var invalid *ValidationError @@ -1031,7 +665,7 @@ func TestService_ExportForceStillRefusesDirectory(t *testing.T) { // split is not. func TestService_ExportConcurrentNonForceSingleWinner(t *testing.T) { t.Parallel() - svc, _, _ := newTestService(t) + svc, _ := newTestService(t) const body = "# plan\nthe full body\n" mustCreate(t, svc, "p", body) @@ -1127,7 +761,7 @@ func TestPublishExportNoReplaceRefusesExistingDestination(t *testing.T) { func TestService_StorageFailuresAreTyped(t *testing.T) { t.Parallel() base := errors.New("backend boom") - svc := NewService(failingStorage{err: base}, WithSessionDir(t.TempDir())) + svc := NewService(failingStorage{err: base}) ctx := t.Context() calls := map[string]func() error{ @@ -1169,7 +803,7 @@ func TestService_StorageFailuresAreTyped(t *testing.T) { // plan.Storage, not just the filesystem default. func TestService_InjectedInMemoryStorage(t *testing.T) { t.Parallel() - svc := NewService(newMemStorage(), WithSessionDir(t.TempDir())) + svc := NewService(newMemStorage()) ctx := t.Context() p, err := svc.Create(ctx, CreateRequest{Ref: SharedRef("p"), Content: "v1", Status: "draft"}) @@ -1185,7 +819,7 @@ func TestService_InjectedInMemoryStorage(t *testing.T) { assert.Equal(t, 2, *p.Version) assert.Equal(t, "draft", p.Status) - list, err := svc.List(ctx, ListOptions{}) + list, err := svc.List(ctx) require.NoError(t, err) require.Len(t, list.Plans, 1) assert.Equal(t, "p", list.Plans[0].Name) @@ -1203,12 +837,6 @@ func TestNewService_NilStoragePanics(t *testing.T) { }) } -func TestScope_Mutable(t *testing.T) { - t.Parallel() - assert.True(t, ScopeShared.Mutable()) - assert.False(t, ScopeSession.Mutable()) -} - // --- Test doubles -------------------------------------------------------------- // failingStorage is a plan.Storage whose every method fails, to verify the diff --git a/pkg/runtime/client.go b/pkg/runtime/client.go index 9d5449a3b5..6d831a1c0b 100644 --- a/pkg/runtime/client.go +++ b/pkg/runtime/client.go @@ -88,7 +88,6 @@ func NewClient(baseURL string, opts ...ClientOption) (*Client, error) { "stream_started": func() Event { return &StreamStartedEvent{} }, "shell": func() Event { return &ShellOutputEvent{} }, "session_title": func() Event { return &SessionTitleEvent{} }, - "session_plan_updated": func() Event { return &SessionPlanUpdatedEvent{} }, "plan_changed": func() Event { return &PlanChangedEvent{} }, "session_summary": func() Event { return &SessionSummaryEvent{} }, "session_compaction": func() Event { return &SessionCompactionEvent{} }, diff --git a/pkg/runtime/event.go b/pkg/runtime/event.go index 3c50ce01c1..5f557591b5 100644 --- a/pkg/runtime/event.go +++ b/pkg/runtime/event.go @@ -445,29 +445,6 @@ func SessionTitle(sessionID, title string) Event { func (e *SessionTitleEvent) GetSessionID() string { return e.SessionID } -// SessionPlanUpdatedEvent fires when the session_plan toolset writes a plan. -// Content and Path let a UI render the plan inline without re-reading the file. -type SessionPlanUpdatedEvent struct { - AgentContext - - Type string `json:"type"` - SessionID string `json:"session_id"` - Content string `json:"content,omitempty"` - Path string `json:"path,omitempty"` -} - -func SessionPlanUpdated(sessionID, content, path, agentName string) Event { - return &SessionPlanUpdatedEvent{ - Type: "session_plan_updated", - SessionID: sessionID, - Content: content, - Path: path, - AgentContext: newAgentContext(agentName), - } -} - -func (e *SessionPlanUpdatedEvent) GetSessionID() string { return e.SessionID } - // PlanChangedEvent fires after an agent successfully mutates a shared plan // (write, status change, or delete) through the plan toolset. It carries // identity and version only — no content — so a UI refreshes through its own diff --git a/pkg/runtime/loop.go b/pkg/runtime/loop.go index 50dbe00443..3124534668 100644 --- a/pkg/runtime/loop.go +++ b/pkg/runtime/loop.go @@ -33,7 +33,6 @@ import ( "github.com/docker/docker-agent/pkg/tools/builtin/modelpicker" "github.com/docker/docker-agent/pkg/tools/builtin/plan" "github.com/docker/docker-agent/pkg/tools/builtin/sessioncontext" - "github.com/docker/docker-agent/pkg/tools/builtin/sessionplan" "github.com/docker/docker-agent/pkg/tools/builtin/skills" "github.com/docker/docker-agent/pkg/tools/builtin/transfertask" "github.com/docker/docker-agent/pkg/userconfig" @@ -47,9 +46,6 @@ func (r *LocalRuntime) registerDefaultTools() { r.toolMap[modelpicker.ToolNameChangeModel] = r.handleChangeModel r.toolMap[modelpicker.ToolNameRevertModel] = r.handleRevertModel r.toolMap[skills.ToolNameRunSkill] = r.handleRunSkill - r.toolMap[sessionplan.ToolNameWriteSessionPlan] = r.handleWriteSessionPlan - r.toolMap[sessionplan.ToolNameReadSessionPlan] = r.handleReadSessionPlan - r.toolMap[sessionplan.ToolNameExitPlanMode] = r.handleExitPlanMode r.toolMap[sessioncontext.ToolNameListSessions] = r.handleListSessions r.toolMap[sessioncontext.ToolNameReadSession] = r.handleReadSession diff --git a/pkg/runtime/sessionplan_handlers.go b/pkg/runtime/sessionplan_handlers.go deleted file mode 100644 index b5ed3c87d8..0000000000 --- a/pkg/runtime/sessionplan_handlers.go +++ /dev/null @@ -1,66 +0,0 @@ -package runtime - -import ( - "context" - "encoding/json" - "errors" - "fmt" - "strings" - - "github.com/docker/docker-agent/pkg/session" - "github.com/docker/docker-agent/pkg/tools" - "github.com/docker/docker-agent/pkg/tools/builtin/sessionplan" -) - -func (r *LocalRuntime) handleWriteSessionPlan(ctx context.Context, sess *session.Session, toolCall tools.ToolCall, events EventSink, _ tools.Runtime) (*tools.ToolCallResult, error) { - var args sessionplan.WriteSessionPlanArgs - if err := json.Unmarshal([]byte(toolCall.Function.Arguments), &args); err != nil { - return nil, fmt.Errorf("invalid arguments: %w", err) - } - if strings.TrimSpace(args.Content) == "" { - return tools.ResultError("content must not be empty"), nil - } - - path, err := sessionplan.WriteContent(sessionplan.DefaultDir(), sess.ID, args.Content) - if err != nil { - if errors.Is(err, sessionplan.ErrInvalidSessionID) { - return tools.ResultError(err.Error()), nil - } - return nil, err - } - - events.Emit(SessionPlanUpdated(sess.ID, args.Content, path, r.CurrentAgentName(ctx))) - return tools.ResultSuccess("Plan saved to " + path), nil -} - -func (r *LocalRuntime) handleReadSessionPlan(_ context.Context, sess *session.Session, _ tools.ToolCall, _ EventSink, _ tools.Runtime) (*tools.ToolCallResult, error) { - content, _, err := sessionplan.ReadContent(sessionplan.DefaultDir(), sess.ID) - if errors.Is(err, sessionplan.ErrPlanNotFound) { - return tools.ResultError("no plan written yet for this session; call write_session_plan first"), nil - } - if err != nil { - if errors.Is(err, sessionplan.ErrInvalidSessionID) { - return tools.ResultError(err.Error()), nil - } - return nil, err - } - return tools.ResultSuccess(content), nil -} - -// handleExitPlanMode marks the session's plan as ready and returns control to -// the host. Switching agents is the host's decision — the runtime does not -// call setCurrentAgent here so a CLI that prints results inline, a chat UI -// with a mode toggle, and a server with a configured handoff can all consume -// the same marker without one stepping on the other. -func (r *LocalRuntime) handleExitPlanMode(_ context.Context, sess *session.Session, _ tools.ToolCall, _ EventSink, _ tools.Runtime) (*tools.ToolCallResult, error) { - if _, _, err := sessionplan.ReadContent(sessionplan.DefaultDir(), sess.ID); err != nil { - if errors.Is(err, sessionplan.ErrPlanNotFound) { - return tools.ResultError("no plan to mark ready; call write_session_plan before exit_plan_mode"), nil - } - if errors.Is(err, sessionplan.ErrInvalidSessionID) { - return tools.ResultError(err.Error()), nil - } - return nil, err - } - return tools.ResultSuccess("Plan ready for review."), nil -} diff --git a/pkg/teamloader/toolsets/catalog.go b/pkg/teamloader/toolsets/catalog.go index 87f7ba686d..9fc7a7e600 100644 --- a/pkg/teamloader/toolsets/catalog.go +++ b/pkg/teamloader/toolsets/catalog.go @@ -30,7 +30,6 @@ var BuiltinToolsets = []BuiltinToolsetInfo{ builtinToolset("scheduler", "scheduler", "Schedule instructions to run at a time or on a recurring interval"), builtinToolset("script", "script", "Define custom shell scripts as named tools with typed parameters"), builtinToolset("session_context", "session_context", "Reference a previous session as context in the current one"), - builtinToolset("session_plan", "session_plan", "Per-session plan tracker for the draft/review/execute workflow"), builtinToolset("shell", "shell", "Execute shell commands in the user's environment"), builtinToolset("tasks", "tasks", "Persistent task database with priorities and dependencies"), builtinToolset("think", "think", "Step-by-step reasoning scratchpad for planning"), diff --git a/pkg/teamloader/toolsets/toolsets.go b/pkg/teamloader/toolsets/toolsets.go index 60fa3a8ebe..901007d468 100644 --- a/pkg/teamloader/toolsets/toolsets.go +++ b/pkg/teamloader/toolsets/toolsets.go @@ -25,7 +25,6 @@ import ( "github.com/docker/docker-agent/pkg/tools/builtin/rag" "github.com/docker/docker-agent/pkg/tools/builtin/scheduler" "github.com/docker/docker-agent/pkg/tools/builtin/sessioncontext" - "github.com/docker/docker-agent/pkg/tools/builtin/sessionplan" "github.com/docker/docker-agent/pkg/tools/builtin/shell" "github.com/docker/docker-agent/pkg/tools/builtin/tasks" "github.com/docker/docker-agent/pkg/tools/builtin/think" @@ -64,7 +63,6 @@ func DefaultToolsetCreators() map[string]teamloader.ToolsetCreator { "scheduler": teamloader.Creator(scheduler.CreateToolSet), "script": shell.ScriptCreator, "session_context": teamloader.Creator(sessioncontext.CreateToolSet), - "session_plan": teamloader.Creator(sessionplan.CreateToolSet), "shell": shell.Creator, "tasks": tasks.Creator, "think": teamloader.Creator(think.CreateToolSet), diff --git a/pkg/tools/builtin/sessioncontext/sessioncontext.go b/pkg/tools/builtin/sessioncontext/sessioncontext.go index 6c336e1590..10b3ba734f 100644 --- a/pkg/tools/builtin/sessioncontext/sessioncontext.go +++ b/pkg/tools/builtin/sessioncontext/sessioncontext.go @@ -4,8 +4,7 @@ // // The toolset is a metadata stub — the runtime owns the handlers // (pkg/runtime/sessioncontext_handlers.go) so they can reach the live session -// store and exclude the session that is currently running. This mirrors the -// session_plan toolset, which is wired the same way. +// store and exclude the session that is currently running. package sessioncontext import ( @@ -73,7 +72,7 @@ type ReadSessionArgs struct { } // Tools advertises the metadata only; Handler is intentionally nil so the -// runtime's toolMap takes over (same pattern as session_plan and handoff). +// runtime's toolMap takes over (same pattern as handoff). func (t *ToolSet) Tools(context.Context) ([]tools.Tool, error) { return []tools.Tool{ { diff --git a/pkg/tools/builtin/sessionplan/sessionplan.go b/pkg/tools/builtin/sessionplan/sessionplan.go deleted file mode 100644 index 154275a141..0000000000 --- a/pkg/tools/builtin/sessionplan/sessionplan.go +++ /dev/null @@ -1,202 +0,0 @@ -// Package sessionplan provides a per-session plan tracker for the -// "draft, review, execute" workflow. One markdown plan per session, stored -// at /session_plans/.md. The toolset is a metadata -// stub — the runtime owns the handlers (pkg/runtime/sessionplan_handlers.go) -// so they can reach the live session. -// -// Complementary to pkg/tools/builtin/plan, which models shared, named plans -// multiple agents collaborate on. Tool names are deliberately distinct -// (write_session_plan / read_session_plan vs. write_plan / read_plan) so -// the two toolsets can coexist on the same agent without colliding. -package sessionplan - -import ( - "bytes" - "context" - "errors" - "fmt" - "io/fs" - "log/slog" - "os" - "path/filepath" - "regexp" - "sync" - "time" - - "github.com/docker/docker-agent/pkg/atomicfile" - "github.com/docker/docker-agent/pkg/paths" - "github.com/docker/docker-agent/pkg/tools" -) - -const ( - ToolNameWriteSessionPlan = "write_session_plan" - ToolNameReadSessionPlan = "read_session_plan" - ToolNameExitPlanMode = "exit_plan_mode" -) - -// sessionIDPattern rejects anything that could escape the plans directory -// ('/', '\\', '..'). 128 chars is well above any realistic ID. -var sessionIDPattern = regexp.MustCompile(`^[A-Za-z0-9][A-Za-z0-9._-]{0,127}$`) - -const maxPlanAge = 30 * 24 * time.Hour - -var ( - ErrInvalidSessionID = errors.New("invalid session ID") - ErrPlanNotFound = errors.New("session plan not found") -) - -func DefaultDir() string { - return filepath.Join(paths.GetDataDir(), "session_plans") -} - -type ToolSet struct{} - -var ( - _ tools.ToolSet = (*ToolSet)(nil) - _ tools.Instructable = (*ToolSet)(nil) -) - -func CreateToolSet() (tools.ToolSet, error) { - runStartupSweep() - return &ToolSet{}, nil -} - -// New builds a toolset without running the startup sweep, for tests and -// embedders that want predictable filesystem behaviour. -func New() *ToolSet { - return &ToolSet{} -} - -func (t *ToolSet) Instructions() string { - return `## Session Plan Tools - -Use this toolset to draft a plan for the current session, then mark it ready for review. - -- ` + "`write_session_plan(content)`" + ` creates or replaces the plan for this session. There is exactly one plan per session and ` + "`content`" + ` is the full markdown — calling it again replaces the previous version. -- ` + "`read_session_plan()`" + ` returns the plan you (or an earlier turn) wrote. It errors when no plan has been written yet. -- ` + "`exit_plan_mode()`" + ` signals that the plan is ready for review. Call this once the plan is complete and you do not intend to change it on the next turn. It does not switch agents on its own — the host application decides what happens next based on the user's reply.` -} - -type WriteSessionPlanArgs struct { - Content string `json:"content" jsonschema:"The full plan content as markdown. Replaces the existing plan for this session."` -} - -// Tools advertises the metadata only; Handler is intentionally nil so the -// runtime's toolMap takes over (same pattern as handoff and transfer_task). -func (t *ToolSet) Tools(context.Context) ([]tools.Tool, error) { - return []tools.Tool{ - { - Name: ToolNameWriteSessionPlan, - Category: "session_plan", - Description: "Create or replace the plan for this session as markdown. There is exactly one plan per session, addressed by session ID.", - Parameters: tools.MustSchemaFor[WriteSessionPlanArgs](), - Annotations: tools.ToolAnnotations{ - Title: "Write Session Plan", - }, - }, - { - Name: ToolNameReadSessionPlan, - Category: "session_plan", - Description: "Read the plan written for this session and return it as markdown. Errors if no plan has been written yet.", - Annotations: tools.ToolAnnotations{ - Title: "Read Session Plan", - ReadOnlyHint: true, - }, - }, - { - Name: ToolNameExitPlanMode, - Category: "session_plan", - Description: "Signal that the plan written for this session is ready for review. Only call this once the plan is complete and you do not intend to change it on the next turn. It does not switch agents.", - Annotations: tools.ToolAnnotations{ - Title: "Exit Plan Mode", - ReadOnlyHint: true, - }, - }, - }, nil -} - -func Path(dir, sessionID string) (string, error) { - if !sessionIDPattern.MatchString(sessionID) { - return "", fmt.Errorf("%w: %q", ErrInvalidSessionID, sessionID) - } - return filepath.Join(dir, sessionID+".md"), nil -} - -// WriteContent uses atomicfile.Write so a concurrent reader — in this process -// or another — sees either the old or the new file, never a partial one, and -// so an existing symlink is replaced rather than followed. -func WriteContent(dir, sessionID, content string) (string, error) { - path, err := Path(dir, sessionID) - if err != nil { - return "", err - } - if err := os.MkdirAll(dir, 0o700); err != nil { - return "", fmt.Errorf("create plans dir: %w", err) - } - if err := atomicfile.Write(path, bytes.NewReader([]byte(content)), 0o600); err != nil { - return "", fmt.Errorf("write plan: %w", err) - } - return path, nil -} - -// ReadContent returns ErrPlanNotFound (with the path so callers can include -// it in user-facing messages) on ENOENT, distinguishing "plan missing" from a -// real read failure such as a permission error. -func ReadContent(dir, sessionID string) (content, path string, err error) { - path, err = Path(dir, sessionID) - if err != nil { - return "", "", err - } - data, err := os.ReadFile(path) - if errors.Is(err, fs.ErrNotExist) { - return "", path, ErrPlanNotFound - } - if err != nil { - return "", path, fmt.Errorf("read plan: %w", err) - } - return string(data), path, nil -} - -// Sweep is best-effort: a permission glitch on one file should not block -// cleaning the rest, but the first error encountered is returned so callers -// can surface it. -func Sweep(dir string, now time.Time, maxAge time.Duration) error { - entries, err := os.ReadDir(dir) - if errors.Is(err, fs.ErrNotExist) { - return nil - } - if err != nil { - return fmt.Errorf("scan plans dir: %w", err) - } - cutoff := now.Add(-maxAge) - var firstErr error - for _, e := range entries { - if e.IsDir() || filepath.Ext(e.Name()) != ".md" { - continue - } - info, err := e.Info() - if err != nil { - if firstErr == nil { - firstErr = err - } - continue - } - if !info.ModTime().Before(cutoff) { - continue - } - if err := os.Remove(filepath.Join(dir, e.Name())); err != nil && !errors.Is(err, fs.ErrNotExist) { - if firstErr == nil { - firstErr = err - } - } - } - return firstErr -} - -// runStartupSweep swallows the sweep error so a read-only data dir does not -// block the toolset from being created. -var runStartupSweep = sync.OnceFunc(func() { - if err := Sweep(DefaultDir(), time.Now(), maxPlanAge); err != nil { - slog.Warn("sessionplan: sweep of stale plan files failed", "error", err, "dir", DefaultDir()) - } -}) diff --git a/pkg/tools/builtin/sessionplan/sessionplan_test.go b/pkg/tools/builtin/sessionplan/sessionplan_test.go deleted file mode 100644 index c859955e17..0000000000 --- a/pkg/tools/builtin/sessionplan/sessionplan_test.go +++ /dev/null @@ -1,160 +0,0 @@ -package sessionplan - -import ( - "os" - "path/filepath" - "strings" - "testing" - "time" - - "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" -) - -func TestPath(t *testing.T) { - t.Parallel() - t.Run("accepts UUID-shaped IDs", func(t *testing.T) { - dir := t.TempDir() - path, err := Path(dir, "7c2d8f0a-1234-4abc-9def-1234567890ab") - require.NoError(t, err) - assert.Equal(t, filepath.Join(dir, "7c2d8f0a-1234-4abc-9def-1234567890ab.md"), path) - }) - - // Path-traversal defence: the regex is the only thing standing between an - // adversarial session ID and arbitrary disk writes. - for _, name := range []string{ - "", - "../escape", - "a/b", - `a\b`, - "-leading-dash", - "_leading-underscore", - ".leading-dot", - strings.Repeat("a", 200), - } { - t.Run("rejects "+name, func(t *testing.T) { - _, err := Path("/plans", name) - require.Error(t, err) - assert.ErrorIs(t, err, ErrInvalidSessionID) - }) - } -} - -func TestWriteReadRoundTrip(t *testing.T) { - t.Parallel() - dir := t.TempDir() - - path, err := WriteContent(dir, "session-1", "# my plan\nstep 1\n") - require.NoError(t, err) - assert.Equal(t, filepath.Join(dir, "session-1.md"), path) - - got, gotPath, err := ReadContent(dir, "session-1") - require.NoError(t, err) - assert.Equal(t, path, gotPath) - assert.Equal(t, "# my plan\nstep 1\n", got) -} - -func TestWriteContentOverwrites(t *testing.T) { - t.Parallel() - dir := t.TempDir() - - _, err := WriteContent(dir, "s", "v1") - require.NoError(t, err) - _, err = WriteContent(dir, "s", "v2") - require.NoError(t, err) - - got, _, err := ReadContent(dir, "s") - require.NoError(t, err) - assert.Equal(t, "v2", got) -} - -func TestWriteContentCreatesDir(t *testing.T) { - t.Parallel() - base := t.TempDir() - dir := filepath.Join(base, "nested", "plans") - - _, err := WriteContent(dir, "s", "hello") - require.NoError(t, err) - - info, err := os.Stat(dir) - require.NoError(t, err) - assert.True(t, info.IsDir()) -} - -func TestReadContentNotFound(t *testing.T) { - t.Parallel() - dir := t.TempDir() - - content, path, err := ReadContent(dir, "ghost") - assert.Empty(t, content) - assert.Equal(t, filepath.Join(dir, "ghost.md"), path) - assert.ErrorIs(t, err, ErrPlanNotFound) -} - -func TestReadContentInvalidSessionID(t *testing.T) { - t.Parallel() - _, _, err := ReadContent(t.TempDir(), "../escape") - assert.ErrorIs(t, err, ErrInvalidSessionID) -} - -func TestSweepRemovesOldPlans(t *testing.T) { - t.Parallel() - dir := t.TempDir() - now := time.Now() - - makeFile := func(name string, mtime time.Time) string { - path := filepath.Join(dir, name) - require.NoError(t, os.WriteFile(path, []byte("x"), 0o600)) - require.NoError(t, os.Chtimes(path, mtime, mtime)) - return path - } - - fresh := makeFile("fresh.md", now.Add(-time.Hour)) - stale := makeFile("stale.md", now.Add(-maxPlanAge-time.Hour)) - // Sweep targets *.md only so we don't poke at unrelated files a future - // version of the toolset might drop in the dir. - other := makeFile("notes.txt", now.Add(-maxPlanAge*2)) - subdir := filepath.Join(dir, "subdir") - require.NoError(t, os.Mkdir(subdir, 0o700)) - - require.NoError(t, Sweep(dir, now, maxPlanAge)) - - _, err := os.Stat(stale) - require.ErrorIs(t, err, os.ErrNotExist, "stale plan should have been removed") - - _, err = os.Stat(fresh) - require.NoError(t, err, "fresh plan should survive") - - _, err = os.Stat(other) - require.NoError(t, err, "non-md file should survive") - - _, err = os.Stat(subdir) - require.NoError(t, err, "subdir should survive") -} - -func TestSweepMissingDirIsNoOp(t *testing.T) { - t.Parallel() - err := Sweep(filepath.Join(t.TempDir(), "does-not-exist"), time.Now(), maxPlanAge) - assert.NoError(t, err) -} - -func TestTools(t *testing.T) { - t.Parallel() - ts := New() - got, err := ts.Tools(t.Context()) - require.NoError(t, err) - - names := make([]string, 0, len(got)) - for _, tool := range got { - names = append(names, tool.Name) - // Handlers must be nil so the runtime's toolMap takes precedence; a - // non-nil handler here would silently bypass the runtime path. - assert.Nil(t, tool.Handler, "tool %q should not declare a handler", tool.Name) - } - assert.ElementsMatch(t, []string{ToolNameWriteSessionPlan, ToolNameReadSessionPlan, ToolNameExitPlanMode}, names) -} - -func TestInstructionsNonEmpty(t *testing.T) { - t.Parallel() - assert.NotEmpty(t, (&ToolSet{}).Instructions()) -} diff --git a/pkg/tui/dialog/plan_browser.go b/pkg/tui/dialog/plan_browser.go index 4f7f80d6ce..5aca223577 100644 --- a/pkg/tui/dialog/plan_browser.go +++ b/pkg/tui/dialog/plan_browser.go @@ -215,34 +215,15 @@ func (d *planBrowserDialog) selectedPlan() (plans.Plan, bool) { // planRef derives the service address of a listed plan. func planRef(p plans.Plan) plans.Ref { - if p.Scope == plans.ScopeSession { - return plans.SessionRef(p.SessionID) - } return plans.SharedRef(p.Name) } -// planCurrentSessionLabel is the browser-row identity of the listed session -// plan. The service only ever lists the active session's plan, so labelling -// it beats showing a bare session ID that means nothing at a glance; the -// full ID stays visible in the footer and the detail dialog. -const planCurrentSessionLabel = "current session" - -// planDisplayName is the identity a browser row shows: the shared plan's -// name, or the current-session label for the session plan. -func planDisplayName(p plans.Plan) string { - if p.Scope == plans.ScopeSession { - return planCurrentSessionLabel - } - return p.Name -} - func (d *planBrowserDialog) applyFilter() { query := strings.ToLower(strings.TrimSpace(d.filterInput.Value())) d.filtered = d.filtered[:0] for _, p := range d.all { if query == "" || strings.Contains(strings.ToLower(p.Name), query) || - strings.Contains(strings.ToLower(planDisplayName(p)), query) || strings.Contains(strings.ToLower(p.Title), query) || strings.Contains(strings.ToLower(p.Status), query) || strings.Contains(string(p.Scope), query) { @@ -405,9 +386,8 @@ func (d *planBrowserDialog) openDetailCmd() tea.Cmd { } // guardedPlan returns the selected plan when the given action applies to it: -// session plans support only edit, and shared plans must carry a displayed -// version. A refused action yields an explanatory notification instead of a -// failed service call. +// plans must carry a displayed version. A refused action yields an +// explanatory notification instead of a failed service call. func (d *planBrowserDialog) guardedPlan(action string) (plans.Plan, tea.Cmd, bool) { p, ok := d.selectedPlan() if !ok { @@ -444,19 +424,9 @@ func (d *planBrowserDialog) editCmd() tea.Cmd { } // planMutationGuard returns an explanatory notification when the plan does -// not support the action from the host: session plans support only edit — -// they belong to their session and carry no shared-plan metadata — and a -// shared plan without a version (which the service always provides) is -// refused rather than mutated unguarded. +// not support the action from the host: a plan without a version (which the +// service always provides) is refused rather than mutated unguarded. func planMutationGuard(p plans.Plan, action string) tea.Cmd { - if p.Scope == plans.ScopeSession { - if action == "edit" { - return nil - } - return notification.InfoCmd(fmt.Sprintf( - "Session plans don't support %s: they belong to their session and carry no shared-plan metadata. Press e to edit the plan body, or use a shared plan.", action, - )) - } if p.Version == nil { return notification.ErrorCmd(fmt.Sprintf("Cannot %s %q: no version is known; refresh (r) and retry.", action, p.Name)) } @@ -554,7 +524,7 @@ func (d *planBrowserDialog) View() string { } // footerLine shows load warnings when present, otherwise the identity of the -// selected plan (useful for truncated names such as session IDs). +// selected plan (useful for truncated names). func (d *planBrowserDialog) footerLine(contentWidth int) string { if len(d.warnings) > 0 { text := fmt.Sprintf("⚠ %d plan(s) could not be read: %s", len(d.warnings), d.warnings[0]) @@ -581,9 +551,6 @@ func (d *planBrowserDialog) SetSize(width, height int) tea.Cmd { func (d *planBrowserDialog) renderPlan(p plans.Plan, selected bool, maxWidth int) string { mainStyle, metaStyle := styles.PaletteUnselectedActionStyle, styles.PaletteUnselectedDescStyle scopeStyle := styles.MutedStyle - if p.Scope == plans.ScopeSession { - scopeStyle = styles.WarningStyle - } if selected { mainStyle, metaStyle = styles.PaletteSelectedActionStyle, styles.PaletteSelectedDescStyle scopeStyle = metaStyle @@ -594,7 +561,7 @@ func (d *planBrowserDialog) renderPlan(p plans.Plan, selected bool, maxWidth int titleWidth := max(0, maxWidth-fixed) row := scopeStyle.Render(planCell(string(p.Scope), planColScope)) + gap + - mainStyle.Render(planCell(planDisplayName(p), planColName)) + gap + + mainStyle.Render(planCell(p.Name, planColName)) + gap + metaStyle.Render(planCell(planLabel(p.Status), planColStatus)) + gap + metaStyle.Render(planCell(planVersionLabel(p.Version), planColVersion)) + gap + metaStyle.Render(planCell(planTimeAgo(d.now(), p.UpdatedAt), planColUpdated)) + gap + @@ -618,8 +585,8 @@ func planLabel(s string) string { return s } -// planVersionLabel renders a shared plan's version and "-" for session -// plans, which have none. +// planVersionLabel renders a plan's version, defensively substituting "-" +// when none is known. func planVersionLabel(version *int) string { if version == nil { return "-" @@ -628,7 +595,7 @@ func planVersionLabel(version *int) string { } // planVersionOrZero reads a plan's displayed version, with 0 as the -// no-version sentinel for session plans (shared versions start at 1). +// no-version sentinel (versions start at 1). func planVersionOrZero(p plans.Plan) int { if p.Version == nil { return 0 diff --git a/pkg/tui/dialog/plan_browser_test.go b/pkg/tui/dialog/plan_browser_test.go index d7274139c2..ba9603f9f2 100644 --- a/pkg/tui/dialog/plan_browser_test.go +++ b/pkg/tui/dialog/plan_browser_test.go @@ -20,18 +20,11 @@ func letterKey(r rune) tea.KeyPressMsg { return tea.KeyPressMsg{Code: r, Text: string(r)} } -// testPlanListing builds a listing with the current session's plan first -// (the service's ordering) and two shared plans. +// testPlanListing builds a listing with two shared plans. func testPlanListing() plans.ListResult { now := time.Now().UTC() return plans.ListResult{ Plans: []plans.Plan{ - { - Scope: plans.ScopeSession, - Name: "11112222-3333-4444-5555-666677778888", - SessionID: "11112222-3333-4444-5555-666677778888", - UpdatedAt: now.Add(-5 * time.Minute), - }, { Scope: plans.ScopeShared, Name: "release", @@ -93,20 +86,12 @@ func TestPlanBrowserRendersScopeIdentityStatusVersionTimeTitle(t *testing.T) { d := newTestPlanBrowser(t, testPlanListing()) view := d.View() - assert.Contains(t, view, "session", "scope column must name the session scope") assert.Contains(t, view, "shared", "scope column must name the shared scope") - assert.Contains(t, view, "current session", "the session plan row is labelled as the current session's") - assert.Contains(t, view, "11112222-3333-4444-5555-666677778888", - "the footer keeps the full session ID of the selected session plan") assert.Contains(t, view, "release") assert.Contains(t, view, "in-progress") - assert.Contains(t, view, "v3", "shared plan version must be shown") + assert.Contains(t, view, "v3", "plan version must be shown") assert.Contains(t, view, "2h ago", "updated time must be shown") assert.Contains(t, view, "Release plan", "title must be shown") - - // The session plan row renders "-" for its nonexistent version. - sessionRow := d.renderPlan(d.filtered[0], false, 90) - assert.Contains(t, sessionRow, "-") } func TestPlanBrowserRowsTruncateSafely(t *testing.T) { @@ -138,11 +123,9 @@ func TestPlanBrowserNavigation(t *testing.T) { d.Update(tea.KeyPressMsg{Code: tea.KeyDown}) assert.Equal(t, 1, d.selected) d.Update(tea.KeyPressMsg{Code: tea.KeyDown}) - assert.Equal(t, 2, d.selected) - d.Update(tea.KeyPressMsg{Code: tea.KeyDown}) - assert.Equal(t, 2, d.selected, "selection stays at the end of the list") + assert.Equal(t, 1, d.selected, "selection stays at the end of the list") d.Update(tea.KeyPressMsg{Code: tea.KeyUp}) - assert.Equal(t, 1, d.selected) + assert.Equal(t, 0, d.selected) } func TestPlanBrowserFilter(t *testing.T) { @@ -181,42 +164,20 @@ func TestPlanBrowserFilterNoMatches(t *testing.T) { assert.Contains(t, d.View(), "No plans match the filter") } -// TestPlanBrowserSessionRowSearchable proves the session row matches its -// "current session" label as well as its session ID. -func TestPlanBrowserSessionRowSearchable(t *testing.T) { - t.Parallel() - d := newTestPlanBrowser(t, testPlanListing()) - - d.Update(letterKey('/')) - for _, r := range "current" { - d.Update(letterKey(r)) - } - require.Len(t, d.filtered, 1, "filtering by the label must keep only the session row") - assert.Equal(t, plans.ScopeSession, d.filtered[0].Scope) - - // The session ID itself stays searchable too. - d.filterInput.SetValue("11112222") - d.applyFilter() - require.Len(t, d.filtered, 1) - assert.Equal(t, plans.ScopeSession, d.filtered[0].Scope) -} - func TestPlanBrowserEnterOpensDetail(t *testing.T) { t.Parallel() d := newTestPlanBrowser(t, testPlanListing()) - // Session plan (first row): detail is addressed by session ref. _, cmd := d.Update(tea.KeyPressMsg{Code: tea.KeyEnter}) msg, ok := firstMsgOfType[messages.OpenPlanDetailMsg](collectMsgs(cmd)) require.True(t, ok, "enter must open the detail dialog, not invoke agent tools") - assert.Equal(t, plans.SessionRef("11112222-3333-4444-5555-666677778888"), msg.Ref) + assert.Equal(t, plans.SharedRef("release"), msg.Ref) - // Shared plan. d.Update(tea.KeyPressMsg{Code: tea.KeyDown}) _, cmd = d.Update(tea.KeyPressMsg{Code: tea.KeyEnter}) msg, ok = firstMsgOfType[messages.OpenPlanDetailMsg](collectMsgs(cmd)) require.True(t, ok) - assert.Equal(t, plans.SharedRef("release"), msg.Ref) + assert.Equal(t, plans.SharedRef("db-migration"), msg.Ref) } func TestPlanBrowserRefreshAndExportKeys(t *testing.T) { @@ -230,13 +191,12 @@ func TestPlanBrowserRefreshAndExportKeys(t *testing.T) { _, cmd = d.Update(letterKey('x')) exportMsg, ok := firstMsgOfType[messages.ExportPlanMsg](collectMsgs(cmd)) require.True(t, ok, "x must request an export") - assert.Equal(t, plans.SessionRef("11112222-3333-4444-5555-666677778888"), exportMsg.Ref) + assert.Equal(t, plans.SharedRef("release"), exportMsg.Ref) } func TestPlanBrowserStatusFlow(t *testing.T) { t.Parallel() - d := newTestPlanBrowser(t, testPlanListing()) - d.Update(tea.KeyPressMsg{Code: tea.KeyDown}) // select release, v3 + d := newTestPlanBrowser(t, testPlanListing()) // release, v3, selected _, cmd := d.Update(letterKey('s')) openMsg, ok := firstMsgOfType[OpenDialogMsg](collectMsgs(cmd)) @@ -274,19 +234,23 @@ func TestPlanBrowserStatusEmptyRejected(t *testing.T) { assert.Equal(t, notification.TypeError, note.Type) } -func TestPlanBrowserSessionMutationsUnsupported(t *testing.T) { +// TestPlanBrowserVersionlessMutationRefused proves a plan without a known +// version (which the service always provides) is refused rather than +// mutated unguarded. +func TestPlanBrowserVersionlessMutationRefused(t *testing.T) { t.Parallel() - d := newTestPlanBrowser(t, testPlanListing()) // session plan selected + d := newTestPlanBrowser(t, plans.ListResult{Plans: []plans.Plan{ + {Scope: plans.ScopeShared, Name: "release"}, + }}) - for _, r := range []rune{'s', 'd'} { + for _, r := range []rune{'s', 'd', 'e'} { _, cmd := d.Update(letterKey(r)) msgs := collectMsgs(cmd) note, ok := firstMsgOfType[notification.ShowMsg](msgs) - require.True(t, ok, "%c on a session plan must show an explanatory notification", r) - assert.Contains(t, note.Text, "Session plans") - assert.Contains(t, note.Text, "edit", "the notification must point at the supported edit action") + require.True(t, ok, "%c on a versionless plan must show an explanatory notification", r) + assert.Contains(t, note.Text, "no version is known") _, opened := firstMsgOfType[OpenDialogMsg](msgs) - assert.False(t, opened, "%c must not open an action dialog for session plans", r) + assert.False(t, opened, "%c must not open an action dialog for a versionless plan", r) _, statusEmitted := firstMsgOfType[messages.SetPlanStatusMsg](msgs) assert.False(t, statusEmitted) _, deleteEmitted := firstMsgOfType[messages.DeletePlanMsg](msgs) @@ -296,26 +260,9 @@ func TestPlanBrowserSessionMutationsUnsupported(t *testing.T) { } } -// TestPlanBrowserSessionEditEmitsIntent proves e on the session row edits the -// current session plan with the no-version sentinel 0 instead of refusing. -func TestPlanBrowserSessionEditEmitsIntent(t *testing.T) { - t.Parallel() - d := newTestPlanBrowser(t, testPlanListing()) // session plan selected - - _, cmd := d.Update(letterKey('e')) - msgs := collectMsgs(cmd) - editMsg, ok := firstMsgOfType[messages.EditPlanMsg](msgs) - require.True(t, ok, "e must edit the current session plan") - assert.Equal(t, plans.SessionRef("11112222-3333-4444-5555-666677778888"), editMsg.Ref) - assert.Equal(t, 0, editMsg.ExpectedVersion, "session plans have no versions; 0 is the sentinel") - _, notified := firstMsgOfType[notification.ShowMsg](msgs) - assert.False(t, notified, "a supported edit must not produce an unsupported notification") -} - func TestPlanBrowserDeleteFlow(t *testing.T) { t.Parallel() - d := newTestPlanBrowser(t, testPlanListing()) - d.Update(tea.KeyPressMsg{Code: tea.KeyDown}) // select release, v3 + d := newTestPlanBrowser(t, testPlanListing()) // release, v3, selected _, cmd := d.Update(letterKey('d')) openMsg, ok := firstMsgOfType[OpenDialogMsg](collectMsgs(cmd)) @@ -366,8 +313,7 @@ func TestPlanBrowserNewFlow(t *testing.T) { func TestPlanBrowserEditKey(t *testing.T) { t.Parallel() - d := newTestPlanBrowser(t, testPlanListing()) - d.Update(tea.KeyPressMsg{Code: tea.KeyDown}) // select release, v3 + d := newTestPlanBrowser(t, testPlanListing()) // release, v3, selected _, cmd := d.Update(letterKey('e')) editMsg, ok := firstMsgOfType[messages.EditPlanMsg](collectMsgs(cmd)) @@ -378,11 +324,10 @@ func TestPlanBrowserEditKey(t *testing.T) { func TestPlanBrowserDataMsgReplacesRowsAndKeepsSelection(t *testing.T) { t.Parallel() - d := newTestPlanBrowser(t, testPlanListing()) - d.Update(tea.KeyPressMsg{Code: tea.KeyDown}) // select release + d := newTestPlanBrowser(t, testPlanListing()) // release selected updated := plans.ListResult{Plans: []plans.Plan{ - {Scope: plans.ScopeShared, Name: "db-migration", Version: new(1)}, + {Scope: plans.ScopeShared, Name: "another", Version: new(1)}, {Scope: plans.ScopeShared, Name: "release", Status: "done", Version: new(4)}, }} d.Update(PlanBrowserDataMsg{Result: updated}) @@ -392,7 +337,7 @@ func TestPlanBrowserDataMsgReplacesRowsAndKeepsSelection(t *testing.T) { require.True(t, ok) assert.Equal(t, "release", p.Name, "selection follows the plan identity across refreshes") assert.Equal(t, 4, *p.Version) - assert.NotContains(t, d.View(), "11112222", "removed rows must disappear") + assert.NotContains(t, d.View(), "db-migration", "removed rows must disappear") } // manyPlansListing builds n shared plans named plan-00, plan-01, … so tests diff --git a/pkg/tui/dialog/plan_detail.go b/pkg/tui/dialog/plan_detail.go index 14422b9529..68e14a5c50 100644 --- a/pkg/tui/dialog/plan_detail.go +++ b/pkg/tui/dialog/plan_detail.go @@ -167,9 +167,6 @@ func (d *planDetailDialog) headerLines(contentWidth int) []string { p := d.plan title := "Plan: " + p.Name - if p.Scope == plans.ScopeSession { - title = "Session plan" - } lines := []string{ RenderTitle(toolcommon.TruncateText(title, contentWidth), contentWidth, styles.DialogTitleStyle), @@ -181,27 +178,20 @@ func (d *planDetailDialog) headerLines(contentWidth int) []string { return l + styles.DialogContentStyle.Render(toolcommon.TruncateText(value, max(1, contentWidth-10))) } - if p.Scope == plans.ScopeSession { - lines = append(lines, - field("Scope", "session — owned by its session, body editable here"), - field("Session", p.SessionID), - field("Version", "- (session plans have no versions)"), - ) - } else { - lines = append(lines, - field("Scope", "shared — collaborative, versioned"), - field("Name", p.Name), - ) - if p.Title != "" { - lines = append(lines, field("Title", p.Title)) - } - lines = append(lines, - field("Status", planLabel(p.Status)), - field("Version", planVersionLabel(p.Version)), - field("Author", planLabel(p.Author)), - ) + lines = append(lines, + field("Scope", "shared — collaborative, versioned"), + field("Name", p.Name), + ) + if p.Title != "" { + lines = append(lines, field("Title", p.Title)) } - lines = append(lines, field("Updated", d.updatedLabel()), RenderSeparator(contentWidth)) + lines = append(lines, + field("Status", planLabel(p.Status)), + field("Version", planVersionLabel(p.Version)), + field("Author", planLabel(p.Author)), + field("Updated", d.updatedLabel()), + RenderSeparator(contentWidth), + ) return lines } @@ -245,13 +235,7 @@ func (d *planDetailDialog) renderContent(contentWidth int) []string { func (d *planDetailDialog) helpKeys() []string { keys := []string{"↑/↓", "scroll", "r", "refresh", "x", "export"} - switch { - case d.plan.Scope.Mutable(): - keys = append(keys, "s", "status", "e", "edit", "d", "delete") - case d.plan.Scope == plans.ScopeSession: - // Session plans support editing the body only. - keys = append(keys, "e", "edit") - } + keys = append(keys, "s", "status", "e", "edit", "d", "delete") return append(keys, "esc", "close") } diff --git a/pkg/tui/dialog/plan_detail_test.go b/pkg/tui/dialog/plan_detail_test.go index c71c302be6..e3a353d666 100644 --- a/pkg/tui/dialog/plan_detail_test.go +++ b/pkg/tui/dialog/plan_detail_test.go @@ -11,7 +11,6 @@ import ( "github.com/stretchr/testify/require" "github.com/docker/docker-agent/pkg/plans" - "github.com/docker/docker-agent/pkg/tui/components/notification" "github.com/docker/docker-agent/pkg/tui/messages" ) @@ -50,31 +49,6 @@ func TestPlanDetailRendersSharedMetadata(t *testing.T) { assert.Contains(t, view, "Step one.") } -func TestPlanDetailRendersSessionMetadata(t *testing.T) { - t.Parallel() - p := plans.Plan{ - Scope: plans.ScopeSession, - Name: "11112222-3333-4444-5555-666677778888", - SessionID: "11112222-3333-4444-5555-666677778888", - Content: "session plan body", - UpdatedAt: time.Now(), - } - d := newTestPlanDetail(t, p) - - view := d.View() - assert.Contains(t, view, "body editable here", "session scope must advertise the body edit") - assert.NotContains(t, view, "read-only", "session plans are no longer presented as wholly read-only") - assert.Contains(t, view, "11112222-3333-4444-5555-666677778888") - assert.Contains(t, view, "session plans have no versions") - assert.NotContains(t, view, "status", "session detail must not advertise unsupported actions") - assert.Contains(t, view, "session plan body") - - keys := strings.Join(d.helpKeys(), " ") - assert.Contains(t, keys, "edit", "the help must advertise e edit for session plans") - assert.NotContains(t, keys, "status") - assert.NotContains(t, keys, "delete") -} - func TestPlanDetailScrollsLongContent(t *testing.T) { t.Parallel() p := sharedDetailPlan() @@ -133,50 +107,6 @@ func TestPlanDetailKeysEmitIntents(t *testing.T) { assert.True(t, closed) } -func TestPlanDetailSessionActionsUnsupported(t *testing.T) { - t.Parallel() - p := plans.Plan{ - Scope: plans.ScopeSession, - Name: "sess-1", - SessionID: "sess-1", - Content: "body", - } - d := newTestPlanDetail(t, p) - - for _, r := range []rune{'s', 'd'} { - _, cmd := d.Update(letterKey(r)) - msgs := collectMsgs(cmd) - note, ok := firstMsgOfType[notification.ShowMsg](msgs) - require.True(t, ok, "%c must explain that session plans don't support the action", r) - assert.Contains(t, note.Text, "Session plans") - assert.Contains(t, note.Text, "edit", "the notification must point at the supported edit action") - _, opened := firstMsgOfType[OpenDialogMsg](msgs) - assert.False(t, opened) - } -} - -// TestPlanDetailSessionEditEmitsIntent proves e edits the session plan body -// with the no-version sentinel 0 instead of refusing. -func TestPlanDetailSessionEditEmitsIntent(t *testing.T) { - t.Parallel() - p := plans.Plan{ - Scope: plans.ScopeSession, - Name: "sess-1", - SessionID: "sess-1", - Content: "body", - } - d := newTestPlanDetail(t, p) - - _, cmd := d.Update(letterKey('e')) - msgs := collectMsgs(cmd) - editMsg, ok := firstMsgOfType[messages.EditPlanMsg](msgs) - require.True(t, ok, "e must edit the session plan body") - assert.Equal(t, plans.SessionRef("sess-1"), editMsg.Ref) - assert.Equal(t, 0, editMsg.ExpectedVersion, "session plans have no versions; 0 is the sentinel") - _, notified := firstMsgOfType[notification.ShowMsg](msgs) - assert.False(t, notified, "a supported edit must not produce an unsupported notification") -} - func TestPlanDetailDataMsgAppliesOnlyMatchingPlan(t *testing.T) { t.Parallel() d := newTestPlanDetail(t, sharedDetailPlan()) diff --git a/pkg/tui/messages/plans.go b/pkg/tui/messages/plans.go index 29e7f43a57..c94a22cef3 100644 --- a/pkg/tui/messages/plans.go +++ b/pkg/tui/messages/plans.go @@ -6,12 +6,10 @@ import "github.com/docker/docker-agent/pkg/plans" // intents; the app model services them through the pkg/plans host service and // pushes fresh data back into the open dialogs. Dialogs never touch storage. // -// Shared-plan mutation messages carry the version that was displayed when -// the user chose the action (never nil), so every shared write is guarded by -// optimistic locking and a concurrent change surfaces as an actionable -// conflict instead of a silent overwrite. The session plan has no versions: -// its only mutation is an EditPlanMsg carrying the sentinel ExpectedVersion -// 0, and the write is last-write-wins by design. +// Mutation messages carry the version that was displayed when the user chose +// the action (never nil), so every write is guarded by optimistic locking +// and a concurrent change surfaces as an actionable conflict instead of a +// silent overwrite. type ( // ShowPlanBrowserMsg opens the /plans browser dialog. ShowPlanBrowserMsg struct{} @@ -46,9 +44,7 @@ type ( CreatePlanMsg struct{ Name string } // EditPlanMsg edits a plan's content in the external $VISUAL/$EDITOR. - // For shared plans ExpectedVersion is the displayed version guarding - // the write; for the session plan — which has no versions — it is the - // sentinel 0 and the write is unguarded. + // ExpectedVersion is the displayed version guarding the write. EditPlanMsg struct { Ref plans.Ref ExpectedVersion int diff --git a/pkg/tui/plans.go b/pkg/tui/plans.go index 8b10dc6e9e..4efd9d8cb2 100644 --- a/pkg/tui/plans.go +++ b/pkg/tui/plans.go @@ -75,25 +75,6 @@ func (m *appModel) plansService() plans.Service { return m.plansSvc } -// currentPlanSessionID identifies the active session whose plan is included -// in listings. Only this session is ever consulted; plan files left behind -// by other sessions are never enumerated. -func (m *appModel) currentPlanSessionID() string { - if m.application == nil { - return "" - } - if sess := m.application.Session(); sess != nil { - return sess.ID - } - return "" -} - -// planListOptions snapshots the listing options from model state; commands -// run off the event loop and must not touch the model. -func (m *appModel) planListOptions() plans.ListOptions { - return plans.ListOptions{SessionID: m.currentPlanSessionID()} -} - // planDialogOpen reports whether any dialog of the /plans flow is on the // stack — topmost or buried under another dialog — i.e. plan data is on // screen and worth live-refreshing. @@ -124,43 +105,28 @@ func (m *appModel) planDetailOpen(ref plans.Ref) bool { func (m *appModel) handleShowPlanBrowser() (tea.Model, tea.Cmd) { // One browser only: with a browser already on the stack (even buried) or - // its opening read already in flight for this session, a repeated /plans - // must not start a second List or stack a duplicate browser. A request - // for a different session may launch — the superseded read's result is - // dropped as stale in handlePlanBrowserLoaded. - if m.planBrowserOpen() { - return m, nil - } - svc, ctx, opts := m.plansService(), m.ctx(), m.planListOptions() - if m.planBrowserLoadInFlight && m.planBrowserLoadSessionID == opts.SessionID { + // its opening read already in flight, a repeated /plans must not start a + // second List or stack a duplicate browser. + if m.planBrowserOpen() || m.planBrowserLoadInFlight { return m, nil } m.planBrowserLoadInFlight = true - m.planBrowserLoadSessionID = opts.SessionID + svc, ctx := m.plansService(), m.ctx() timeout := m.planReadTimeoutOrDefault() return m, func() tea.Msg { ctx, cancel := context.WithTimeout(ctx, timeout) defer cancel() - result, err := svc.List(ctx, opts) - return planBrowserLoadedMsg{sessionID: opts.SessionID, result: result, err: err} + result, err := svc.List(ctx) + return planBrowserLoadedMsg{result: result, err: err} } } // handlePlanBrowserLoaded opens the /plans browser with the completed -// listing. A result whose session no longer matches the active one is -// dropped: the user switched tabs while the read was in flight, and popping -// a browser that lists the previous tab's session plan would mislead. +// listing. func (m *appModel) handlePlanBrowserLoaded(msg planBrowserLoadedMsg) (tea.Model, tea.Cmd) { - // Clear the guard first, whatever the outcome, so a failed or stale open - // never wedges /plans. A result whose session differs from the guard's - // belongs to a superseded launch; the guard keeps tracking the newer - // in-flight read. - if msg.sessionID == m.planBrowserLoadSessionID { - m.planBrowserLoadInFlight = false - } - if msg.sessionID != m.currentPlanSessionID() { - return m, nil - } + // Clear the guard first, whatever the outcome, so a failed open never + // wedges /plans. + m.planBrowserLoadInFlight = false if msg.err != nil { cmd := m.planReadFailureCmd(msg.err) return m, cmd @@ -196,7 +162,7 @@ func (m *appModel) planRefreshCmd(notifyWarnings bool) tea.Cmd { return nil } m.planRefreshInFlight = true - svc, ctx, opts := m.plansService(), m.ctx(), m.planListOptions() + svc, ctx := m.plansService(), m.ctx() timeout := m.planReadTimeoutOrDefault() refs := m.openPlanDetailRefs() return func() tea.Msg { @@ -205,8 +171,8 @@ func (m *appModel) planRefreshCmd(notifyWarnings bool) tea.Cmd { // refresh pipeline can never get stuck. ctx, cancel := context.WithTimeout(ctx, timeout) defer cancel() - msg := planRefreshedMsg{sessionID: opts.SessionID, notifyWarnings: notifyWarnings} - msg.list, msg.listErr = svc.List(ctx, opts) + msg := planRefreshedMsg{notifyWarnings: notifyWarnings} + msg.list, msg.listErr = svc.List(ctx) for _, ref := range refs { p, err := svc.Get(ctx, ref) msg.details = append(msg.details, planDetailFetch{ref: ref, plan: p, err: err}) @@ -229,10 +195,7 @@ func (m *appModel) appendPlanRefreshCmd(cmds []tea.Cmd) []tea.Cmd { // refresh pipeline can never wedge. With no plan dialog left the result — // data, errors, and warnings alike — is dropped along with any queued // follow-up: it has nowhere to land and a notification would reference -// nothing on screen. A result read for another session's listing (the user -// switched tabs while it was in flight) is dropped too, and exactly one -// fresh reload for the current session replaces it, folding in the queued -// intent. Otherwise the list data (or its failure) surfaces +// nothing on screen. Otherwise the list data (or its failure) surfaces // unconditionally. Detail read failures surface only for a detail that is // the topmost dialog now, when the result is applied: a plan that // disappeared closes it — through the targeted ClosePlanDetailMsg, so @@ -250,13 +213,6 @@ func (m *appModel) handlePlanRefreshed(msg planRefreshedMsg) (tea.Model, tea.Cmd m.planRefreshQueuedWarnings = false return m, nil } - if msg.sessionID != m.currentPlanSessionID() { - notify := msg.notifyWarnings || m.planRefreshQueuedWarnings - m.planRefreshQueued = false - m.planRefreshQueuedWarnings = false - cmd := m.planRefreshCmd(notify) - return m, cmd - } var cmds []tea.Cmd if msg.listErr != nil { @@ -318,21 +274,16 @@ type planDetailFetch struct { } // planBrowserLoadedMsg reports the listing read that backs opening the -// /plans browser. sessionID is the session the listing was requested for, -// so a result that raced a tab switch can be told apart and dropped. +// /plans browser. type planBrowserLoadedMsg struct { - sessionID string - result plans.ListResult - err error + result plans.ListResult + err error } // planRefreshedMsg reports a completed asynchronous reload of the open plan // dialogs: the browser listing plus the full plan of every detail dialog -// that was open when the reload started. sessionID is the session the -// listing was read for, so a reload that raced a tab switch can be told -// apart, dropped, and replaced by a fresh one. +// that was open when the reload started. type planRefreshedMsg struct { - sessionID string list plans.ListResult listErr error details []planDetailFetch @@ -459,22 +410,12 @@ func (m *appModel) handlePlanExportResult(msg planExportResultMsg) (tea.Model, t return m, notification.SuccessCmd(fmt.Sprintf("Exported %s plan to %s (%d bytes)", msg.result.Scope, msg.result.Path, msg.result.BytesWritten)) } -// planExportFilename is the deterministic default export target: the plan -// name for shared plans, a short session marker for session plans. +// planExportFilename is the deterministic default export target, named after +// the plan. func planExportFilename(ref plans.Ref) string { - if ref.Scope == plans.ScopeSession { - return "session-plan-" + planShortSessionID(ref.SessionID) + ".md" - } return ref.Name + ".md" } -func planShortSessionID(id string) string { - if r := []rune(id); len(r) > 8 { - return string(r[:8]) - } - return id -} - // activeWorkingDir is the working directory of the active tab's runtime, // falling back to the process working directory. func (m *appModel) activeWorkingDir() string { @@ -616,8 +557,7 @@ func (m *appModel) handleEditPlan(msg messages.EditPlanMsg) (tea.Model, tea.Cmd) ready.currentVersion = planVersionOf(p) if ready.currentVersion != msg.ExpectedVersion { // No draft for a drifted base; handlePlanEditReady refreshes - // instead of editing. Session plans never take this branch: they - // have no versions, so both sides are always 0. + // instead of editing. return ready } ready.draftPath, ready.draftErr = planDraftFile(planDraftPattern(msg.Ref), p.Content) @@ -685,13 +625,9 @@ type planEditorClosedMsg struct { } // planDraftPattern names the temp draft of an editor-driven edit after the -// plan's identity: the shared plan name, or a short session marker mirroring -// planExportFilename. Both are service-validated identifiers by the time a -// draft is created, so the pattern is filename-safe. +// plan's name, a service-validated identifier by the time a draft is +// created, so the pattern is filename-safe. func planDraftPattern(ref plans.Ref) string { - if ref.Scope == plans.ScopeSession { - return "cagent-plan-session-" + planShortSessionID(ref.SessionID) + "-*.md" - } return "cagent-plan-" + ref.Name + "-*.md" } @@ -755,14 +691,9 @@ func (m *appModel) handlePlanEditorClosed(msg planEditorClosedMsg) (tea.Model, t } ctx, cancel := context.WithTimeout(ctx, timeout) defer cancel() - switch { - case msg.create: + if msg.create { result.plan, result.err = svc.Create(ctx, plans.CreateRequest{Ref: msg.ref, Content: content}) - case msg.ref.Scope == plans.ScopeSession: - // Session plans have no versions: the replace is deliberately - // unguarded, last-write-wins. - result.plan, result.err = svc.UpdateSession(ctx, msg.ref.SessionID, content) - default: + } else { expected := msg.expectedVersion result.plan, result.err = svc.Update(ctx, plans.UpdateRequest{Ref: msg.ref, Content: content, ExpectedVersion: &expected}) } @@ -815,14 +746,10 @@ func (m *appModel) handlePlanWriteResult(msg planWriteResultMsg) (tea.Model, tea notification.InfoCmd("Your draft is kept at "+msg.draftPath), ) case msg.emptyDraft: - switch { - case msg.create: + if msg.create { return m, notification.InfoCmd(fmt.Sprintf("Plan %q not created: the draft was empty.", msg.ref.Name)) - case msg.ref.Scope == plans.ScopeSession: - return m, notification.InfoCmd("Session plan left unchanged: an empty draft is never committed.") - default: - return m, notification.InfoCmd(fmt.Sprintf("Plan %q left unchanged: an empty draft is never committed.", msg.ref.Name)) } + return m, notification.InfoCmd(fmt.Sprintf("Plan %q left unchanged: an empty draft is never committed.", msg.ref.Name)) case msg.err != nil: cmd := m.planEditorFailureCmd(msg.err, msg.draftPath) return m, cmd @@ -830,13 +757,9 @@ func (m *appModel) handlePlanWriteResult(msg planWriteResultMsg) (tea.Model, tea _ = os.Remove(msg.draftPath) var text string - switch { - case msg.create: + if msg.create { text = fmt.Sprintf("Created shared plan %q (now v%d)", msg.plan.Name, planVersionOf(msg.plan)) - case msg.ref.Scope == plans.ScopeSession: - // Session plans have no version to report. - text = "Updated the current session plan." - default: + } else { text = fmt.Sprintf("Updated shared plan %q (now v%d)", msg.plan.Name, planVersionOf(msg.plan)) } cmds := []tea.Cmd{notification.SuccessCmd(text)} @@ -930,11 +853,10 @@ func planVersionOf(p plans.Plan) int { // user-facing notifications, never classifying by error text. func planErrorCmd(err error) tea.Cmd { var ( - conflict *plans.ConflictError - notFound *plans.NotFoundError - validation *plans.ValidationError - corrupt *plans.CorruptError - unsupported *plans.UnsupportedError + conflict *plans.ConflictError + notFound *plans.NotFoundError + validation *plans.ValidationError + corrupt *plans.CorruptError ) switch { case errors.As(err, &conflict): @@ -948,8 +870,6 @@ func planErrorCmd(err error) tea.Cmd { return notification.ErrorCmd("Invalid input: " + validation.Message) case errors.As(err, &corrupt): return notification.ErrorCmd(fmt.Sprintf("Plan %q is corrupt and cannot be read (%v). Delete it to recover.", corrupt.Name, corrupt.Err)) - case errors.As(err, &unsupported): - return notification.InfoCmd("Unsupported: " + unsupported.Error()) default: return notification.ErrorCmd("Plan storage failure: " + err.Error()) } @@ -964,21 +884,6 @@ func planWarningsCmds(warnings []string) []tea.Cmd { ))} } -// handleSessionPlanUpdatedEvent forwards the event to the chat page like any -// runtime event and live-refreshes open plan dialogs when the active -// session's plan changed. The refresh reads run in a command, never here. -func (m *appModel) handleSessionPlanUpdatedEvent(msg *runtime.SessionPlanUpdatedEvent) (tea.Model, tea.Cmd) { - if name := msg.GetAgentName(); name != "" { - m.sessionState.SetCurrentAgentName(name) - } - chatCmd := m.updateChatCmd(msg) - var refresh tea.Cmd - if m.planDialogOpen() && msg.SessionID == m.currentPlanSessionID() { - refresh = m.planRefreshCmd(false) - } - return m, tea.Batch(chatCmd, refresh) -} - // handlePlanChangedEvent live-refreshes open plan dialogs after an agent // mutated a shared plan. Shared plans are scope-global, so the refresh does // not depend on which session emitted the event. diff --git a/pkg/tui/plans_test.go b/pkg/tui/plans_test.go index 60d374cbea..ddbe6f69f8 100644 --- a/pkg/tui/plans_test.go +++ b/pkg/tui/plans_test.go @@ -19,7 +19,6 @@ import ( "github.com/docker/docker-agent/pkg/runtime" "github.com/docker/docker-agent/pkg/session" "github.com/docker/docker-agent/pkg/tools/builtin/plan" - "github.com/docker/docker-agent/pkg/tools/builtin/sessionplan" "github.com/docker/docker-agent/pkg/tui/components/notification" "github.com/docker/docker-agent/pkg/tui/dialog" "github.com/docker/docker-agent/pkg/tui/messages" @@ -28,20 +27,18 @@ import ( ) // newPlansTestModel wires an appModel around a temp-backed plans service and -// a real session, returning the model, the service, the active session, and -// the session-plans directory for planting files. -func newPlansTestModel(t *testing.T) (*appModel, plans.Service, *session.Session, string) { +// a real session, returning the model and the service. +func newPlansTestModel(t *testing.T) (*appModel, plans.Service) { t.Helper() m, _ := newTestModel(t) - sessionDir := t.TempDir() - svc := plans.NewService(plan.NewFilesystemStorage(t.TempDir()), plans.WithSessionDir(sessionDir)) + svc := plans.NewService(plan.NewFilesystemStorage(t.TempDir())) WithPlansService(svc)(m) sess := session.New() m.application = app.New(t.Context(), stubRuntime{}, sess) m.sessionState = service.NewSessionState(sess) - return m, svc, sess, sessionDir + return m, svc } func mustCreatePlan(t *testing.T, svc plans.Service, name, content string) plans.Plan { @@ -51,16 +48,6 @@ func mustCreatePlan(t *testing.T, svc plans.Service, name, content string) plans return p } -// switchPlansTestSession replaces the model's active session with a fresh -// one, as a tab switch would, and returns it. -func switchPlansTestSession(t *testing.T, m *appModel) *session.Session { - t.Helper() - sess := session.New() - m.application = app.New(t.Context(), stubRuntime{}, sess) - m.sessionState = service.NewSessionState(sess) - return sess -} - // openPlanBrowser puts a plan browser dialog on the model's dialog stack, as // if /plans had been run. func openPlanBrowser(t *testing.T, m *appModel, result plans.ListResult) { @@ -148,7 +135,7 @@ func notificationTexts(msgs []tea.Msg) []string { func TestHandleShowPlanBrowser_OpensDialog(t *testing.T) { t.Parallel() - m, svc, _, _ := newPlansTestModel(t) + m, svc := newPlansTestModel(t) mustCreatePlan(t, svc, "release", "content") msgs := runPlanFlow(t, m, messages.ShowPlanBrowserMsg{}) @@ -159,34 +146,9 @@ func TestHandleShowPlanBrowser_OpensDialog(t *testing.T) { assert.Contains(t, openMsg.Model.View(), "release") } -func TestPlanList_IncludesOnlyActiveSessionPlan(t *testing.T) { - t.Parallel() - m, svc, sess, sessionDir := newPlansTestModel(t) - mustCreatePlan(t, svc, "shared-one", "content") - openPlanBrowser(t, m, plans.ListResult{Plans: []plans.Plan{}}) - - // Current session's plan plus a stale plan from another session that - // must never be enumerated. - _, err := sessionplan.WriteContent(sessionDir, sess.ID, "my plan") - require.NoError(t, err) - staleSess := session.New() - _, err = sessionplan.WriteContent(sessionDir, staleSess.ID, "stale plan") - require.NoError(t, err) - - dataMsg, ok := firstOfType[dialog.PlanBrowserDataMsg](runPlanFlow(t, m, messages.RefreshPlansMsg{})) - require.True(t, ok) - - names := make([]string, 0, len(dataMsg.Result.Plans)) - for _, p := range dataMsg.Result.Plans { - names = append(names, p.Name) - } - assert.ElementsMatch(t, []string{sess.ID, "shared-one"}, names, - "the listing includes the active session's plan and shared plans, never stale session plans") -} - func TestHandleSetPlanStatus_RefreshesAfterWrite(t *testing.T) { t.Parallel() - m, svc, _, _ := newPlansTestModel(t) + m, svc := newPlansTestModel(t) created := mustCreatePlan(t, svc, "release", "content") require.Equal(t, 1, *created.Version) openPlanBrowser(t, m, plans.ListResult{Plans: []plans.Plan{created}}) @@ -214,7 +176,7 @@ func TestHandleSetPlanStatus_RefreshesAfterWrite(t *testing.T) { func TestHandleSetPlanStatus_StaleConflictPreservesNewerData(t *testing.T) { t.Parallel() - m, svc, _, _ := newPlansTestModel(t) + m, svc := newPlansTestModel(t) mustCreatePlan(t, svc, "release", "content") openPlanBrowser(t, m, plans.ListResult{Plans: []plans.Plan{}}) @@ -249,7 +211,7 @@ func TestHandleSetPlanStatus_StaleConflictPreservesNewerData(t *testing.T) { func TestHandleDeletePlan_Semantics(t *testing.T) { t.Parallel() - m, svc, _, _ := newPlansTestModel(t) + m, svc := newPlansTestModel(t) mustCreatePlan(t, svc, "release", "content") openPlanBrowser(t, m, plans.ListResult{Plans: []plans.Plan{}}) @@ -282,7 +244,7 @@ func TestHandleDeletePlan_Semantics(t *testing.T) { } func TestHandleExportPlan_RefusesOverwrite(t *testing.T) { - m, svc, _, _ := newPlansTestModel(t) + m, svc := newPlansTestModel(t) mustCreatePlan(t, svc, "release", "the content") workDir := t.TempDir() @@ -311,28 +273,9 @@ func TestHandleExportPlan_RefusesOverwrite(t *testing.T) { assert.Equal(t, "precious local edits", string(data), "an existing file must never be overwritten") } -func TestHandleExportPlan_SessionDefaultFilename(t *testing.T) { - m, _, sess, sessionDir := newPlansTestModel(t) - _, err := sessionplan.WriteContent(sessionDir, sess.ID, "session body") - require.NoError(t, err) - - workDir := t.TempDir() - t.Chdir(workDir) - - msgs := runPlanFlow(t, m, messages.ExportPlanMsg{Ref: plans.SessionRef(sess.ID)}) - note, ok := firstOfType[notification.ShowMsg](msgs) - require.True(t, ok) - assert.Equal(t, notification.TypeSuccess, note.Type) - - path := filepath.Join(workDir, "session-plan-"+sess.ID[:8]+".md") - data, err := os.ReadFile(path) - require.NoError(t, err) - assert.Equal(t, "session body", string(data)) -} - func TestHandlePlanEditorClosed_CreatesPlan(t *testing.T) { t.Parallel() - m, svc, _, _ := newPlansTestModel(t) + m, svc := newPlansTestModel(t) draft := filepath.Join(t.TempDir(), "draft.md") require.NoError(t, os.WriteFile(draft, []byte("# fresh plan"), 0o600)) @@ -352,7 +295,7 @@ func TestHandlePlanEditorClosed_CreatesPlan(t *testing.T) { func TestHandlePlanEditorClosed_EmptyDraftAborts(t *testing.T) { t.Parallel() - m, svc, _, _ := newPlansTestModel(t) + m, svc := newPlansTestModel(t) draft := filepath.Join(t.TempDir(), "draft.md") require.NoError(t, os.WriteFile(draft, []byte(" \n \n"), 0o600)) @@ -375,7 +318,7 @@ func TestHandlePlanEditorClosed_EmptyDraftAborts(t *testing.T) { // slurping the file whole — and the draft is preserved for the user to trim. func TestHandlePlanEditorClosed_OversizedDraftRefusedBounded(t *testing.T) { t.Parallel() - m, svc, _, _ := newPlansTestModel(t) + m, svc := newPlansTestModel(t) draft := filepath.Join(t.TempDir(), "draft.md") require.NoError(t, os.WriteFile(draft, make([]byte, plan.MaxPlanContentSize+1), 0o600)) @@ -397,7 +340,7 @@ func TestHandlePlanEditorClosed_OversizedDraftRefusedBounded(t *testing.T) { // descriptor instead of read, with the path preserved. func TestHandlePlanEditorClosed_NonRegularDraftRejected(t *testing.T) { t.Parallel() - m, svc, _, _ := newPlansTestModel(t) + m, svc := newPlansTestModel(t) draftDir := t.TempDir() msgs := runPlanFlow(t, m, planEditorClosedMsg{ref: plans.SharedRef("fresh"), create: true, path: draftDir}) @@ -413,7 +356,7 @@ func TestHandlePlanEditorClosed_NonRegularDraftRejected(t *testing.T) { func TestHandlePlanEditorClosed_ConflictKeepsDraftAndNewerContent(t *testing.T) { t.Parallel() - m, svc, _, _ := newPlansTestModel(t) + m, svc := newPlansTestModel(t) mustCreatePlan(t, svc, "release", "v1 content") // The plan moved to v2 while the user was editing v1. @@ -443,7 +386,7 @@ func TestHandlePlanEditorClosed_ConflictKeepsDraftAndNewerContent(t *testing.T) func TestHandleEditPlan_VersionDriftRefreshesInsteadOfEditing(t *testing.T) { t.Parallel() - m, svc, _, _ := newPlansTestModel(t) + m, svc := newPlansTestModel(t) mustCreatePlan(t, svc, "release", "v1 content") openPlanBrowser(t, m, plans.ListResult{Plans: []plans.Plan{}}) v1 := 1 @@ -462,135 +405,9 @@ func TestHandleEditPlan_VersionDriftRefreshesInsteadOfEditing(t *testing.T) { assert.True(t, refreshed, "version drift must refresh the data on screen") } -// TestSessionPlanEdit_PersistsAndRefreshes drives the whole session edit: -// the preparation reads the plan and seeds a session-named draft without any -// drift warning (session plans have no versions), dispatching the prepared -// edit launches the editor, and the closed editor's draft is persisted -// last-write-wins, confirmed with a session-appropriate notification, and -// refreshed into the open browser. -func TestSessionPlanEdit_PersistsAndRefreshes(t *testing.T) { - t.Parallel() - m, svc, sess, sessionDir := newPlansTestModel(t) - _, err := sessionplan.WriteContent(sessionDir, sess.ID, "# session plan v1") - require.NoError(t, err) - openPlanBrowser(t, m, plans.ListResult{Plans: []plans.Plan{}}) - - _, cmd := m.Update(messages.EditPlanMsg{Ref: plans.SessionRef(sess.ID), ExpectedVersion: 0}) - require.NotNil(t, cmd) - result := cmd() - ready, ok := result.(planEditReadyMsg) - require.True(t, ok, "got %T", result) - require.NoError(t, ready.err) - require.NoError(t, ready.draftErr) - require.NotEmpty(t, ready.draftPath, "a session edit must draft; there is no version to drift") - t.Cleanup(func() { _ = os.Remove(ready.draftPath) }) - assert.Zero(t, ready.currentVersion, "session plans have no versions") - assert.Contains(t, filepath.Base(ready.draftPath), sess.ID[:8], "the draft is named after the session") - - data, err := os.ReadFile(ready.draftPath) - require.NoError(t, err) - assert.Equal(t, "# session plan v1", string(data), "the draft must be seeded with the current body") - - // Dispatching the prepared edit launches the editor — an exec command, - // not a drift or failure notification. - _, editorCmd := m.Update(result) - require.NotNil(t, editorCmd, "the prepared edit must launch the editor") - assert.Empty(t, notificationTexts(collectMsgs(editorCmd)), "the launch must not be a notification") - - // The editor closed with new content: the plan is replaced and the - // browser refreshed. - require.NoError(t, os.WriteFile(ready.draftPath, []byte("# edited in the editor"), 0o600)) - msgs := runPlanFlow(t, m, planEditorClosedMsg{ref: plans.SessionRef(sess.ID), path: ready.draftPath}) - texts := notificationTexts(msgs) - require.NotEmpty(t, texts) - assert.Contains(t, texts[0], "session plan") - assert.NotContains(t, texts[0], "v0", "a session edit must not claim a shared-plan version") - assert.NotContains(t, texts[0], "shared") - - stored, err := svc.Get(t.Context(), plans.SessionRef(sess.ID)) - require.NoError(t, err) - assert.Equal(t, "# edited in the editor", stored.Content) - - dataMsg, ok := firstOfType[dialog.PlanBrowserDataMsg](msgs) - require.True(t, ok, "a successful session edit must refresh the browser") - require.Len(t, dataMsg.Result.Plans, 1) - assert.Equal(t, sess.ID, dataMsg.Result.Plans[0].SessionID) - - _, err = os.Stat(ready.draftPath) - assert.True(t, os.IsNotExist(err), "the draft is removed after a successful write") -} - -func TestHandlePlanEditorClosed_SessionEmptyDraftLeavesPlan(t *testing.T) { - t.Parallel() - m, svc, sess, sessionDir := newPlansTestModel(t) - _, err := sessionplan.WriteContent(sessionDir, sess.ID, "# keep me") - require.NoError(t, err) - - draft := filepath.Join(t.TempDir(), "draft.md") - require.NoError(t, os.WriteFile(draft, []byte(" \n \n"), 0o600)) - - msgs := runPlanFlow(t, m, planEditorClosedMsg{ref: plans.SessionRef(sess.ID), path: draft}) - texts := notificationTexts(msgs) - require.NotEmpty(t, texts) - assert.Contains(t, texts[0], "Session plan left unchanged") - assert.NotContains(t, texts[0], `""`, "the message must not render the empty shared-plan name") - - stored, err := svc.Get(t.Context(), plans.SessionRef(sess.ID)) - require.NoError(t, err) - assert.Equal(t, "# keep me", stored.Content, "an empty draft must never be committed") -} - -// TestHandlePlanEditorClosed_SessionPlanVanishedKeepsDraft proves a session -// edit whose plan disappeared while the editor was open never turns into a -// create: the write is refused as not-found, the plan stays missing, and the -// draft is kept. -func TestHandlePlanEditorClosed_SessionPlanVanishedKeepsDraft(t *testing.T) { - t.Parallel() - m, svc, sess, _ := newPlansTestModel(t) // no session plan on disk - - draft := filepath.Join(t.TempDir(), "draft.md") - require.NoError(t, os.WriteFile(draft, []byte("edited content"), 0o600)) - - msgs := runPlanFlow(t, m, planEditorClosedMsg{ref: plans.SessionRef(sess.ID), path: draft}) - texts := notificationTexts(msgs) - require.NotEmpty(t, texts) - assert.Contains(t, texts[0], "No session plan") - assert.Contains(t, strings.Join(texts, " "), draft, "the notification must point at the kept draft") - - _, err := svc.Get(t.Context(), plans.SessionRef(sess.ID)) - require.Error(t, err, "the refused edit must not create a session plan") - _, err = os.Stat(draft) - require.NoError(t, err, "the draft must be kept when the write is refused") -} - -func TestSessionPlanUpdatedEvent_RefreshesOpenPlanDialogs(t *testing.T) { - t.Parallel() - m, _, sess, sessionDir := newPlansTestModel(t) - openPlanBrowser(t, m, plans.ListResult{Plans: []plans.Plan{}}) - - // The agent writes the session plan; the browser must pick it up. - _, err := sessionplan.WriteContent(sessionDir, sess.ID, "plan body") - require.NoError(t, err) - - msgs := runPlanFlow(t, m, runtime.SessionPlanUpdated(sess.ID, "plan body", "", "root")) - dataMsg, ok := firstOfType[dialog.PlanBrowserDataMsg](msgs) - require.True(t, ok, "an open plan browser must live-refresh on session plan writes") - require.Len(t, dataMsg.Result.Plans, 1) - assert.Equal(t, sess.ID, dataMsg.Result.Plans[0].SessionID) -} - -func TestSessionPlanUpdatedEvent_NoRefreshWithoutPlanDialog(t *testing.T) { - t.Parallel() - m, _, sess, _ := newPlansTestModel(t) - - msgs := runPlanFlow(t, m, runtime.SessionPlanUpdated(sess.ID, "plan body", "", "root")) - _, refreshed := firstOfType[dialog.PlanBrowserDataMsg](msgs) - assert.False(t, refreshed, "no plan dialog open, nothing to refresh") -} - func TestPlanChangedEvent_RefreshesOpenPlanDialogs(t *testing.T) { t.Parallel() - m, svc, _, _ := newPlansTestModel(t) + m, svc := newPlansTestModel(t) mustCreatePlan(t, svc, "release", "content") openPlanBrowser(t, m, plans.ListResult{Plans: []plans.Plan{}}) @@ -603,7 +420,7 @@ func TestPlanChangedEvent_RefreshesOpenPlanDialogs(t *testing.T) { func TestPlanChangedEvent_BackgroundSessionStillRefreshes(t *testing.T) { t.Parallel() - m, svc, _, _ := newPlansTestModel(t) + m, svc := newPlansTestModel(t) mustCreatePlan(t, svc, "release", "content") sv := supervisor.New(nil) @@ -674,7 +491,7 @@ func (s *blockingPlansService) Delete(ctx context.Context, _ plans.DeleteRequest // timeout notification instead of a freeze. func TestHandleSetPlanStatus_WedgedLockTimesOutAsynchronously(t *testing.T) { t.Parallel() - m, svc, _, _ := newPlansTestModel(t) + m, svc := newPlansTestModel(t) mustCreatePlan(t, svc, "release", "content") openPlanBrowser(t, m, plans.ListResult{Plans: []plans.Plan{}}) blocking := &blockingPlansService{Service: svc} @@ -718,7 +535,7 @@ func TestHandleSetPlanStatus_WedgedLockTimesOutAsynchronously(t *testing.T) { func TestHandleDeletePlan_WedgedLockTimesOutAsynchronously(t *testing.T) { t.Parallel() - m, svc, _, _ := newPlansTestModel(t) + m, svc := newPlansTestModel(t) mustCreatePlan(t, svc, "release", "content") blocking := &blockingPlansService{Service: svc} WithPlansService(blocking)(m) @@ -749,7 +566,7 @@ func TestHandleDeletePlan_WedgedLockTimesOutAsynchronously(t *testing.T) { // survives and the notification points at it. func TestHandlePlanEditorClosed_TimeoutKeepsDraft(t *testing.T) { t.Parallel() - m, svc, _, _ := newPlansTestModel(t) + m, svc := newPlansTestModel(t) blocking := &blockingPlansService{Service: svc} WithPlansService(blocking)(m) m.planMutationTimeout = 50 * time.Millisecond @@ -785,7 +602,7 @@ func TestHandlePlanEditorClosed_TimeoutKeepsDraft(t *testing.T) { // commands are applied. func TestPlanChangedEvent_RefreshesBuriedPlanDialogs(t *testing.T) { t.Parallel() - m, svc, _, _ := newPlansTestModel(t) + m, svc := newPlansTestModel(t) mustCreatePlan(t, svc, "release", "v1 content") sizeDialogs(t, m) @@ -822,28 +639,6 @@ func TestPlanChangedEvent_RefreshesBuriedPlanDialogs(t *testing.T) { assert.Contains(t, detail.View(), "v2 content", "the buried detail must render the refreshed plan") } -func TestSessionPlanUpdatedEvent_RefreshesBuriedBrowser(t *testing.T) { - t.Parallel() - m, _, sess, sessionDir := newPlansTestModel(t) - - sizeDialogs(t, m) - browser := dialog.NewPlanBrowserDialog(plans.ListResult{Plans: []plans.Plan{}}) - openDialog(t, m, browser) - openDialog(t, m, dialog.NewHelpDialog(nil)) // real non-plan dialog on top - - _, err := sessionplan.WriteContent(sessionDir, sess.ID, "plan body") - require.NoError(t, err) - - msgs := runPlanFlow(t, m, runtime.SessionPlanUpdated(sess.ID, "plan body", "", "root")) - dataMsg, ok := firstOfType[dialog.PlanBrowserDataMsg](msgs) - require.True(t, ok, "a buried plan browser must still live-refresh on session plan writes") - require.Len(t, dataMsg.Result.Plans, 1) - - // Apply the broadcast: the buried browser instance receives the rows. - _, _ = m.Update(dataMsg) - assert.Contains(t, browser.View(), sess.ID[:8], "the buried browser must render the refreshed rows") -} - // failingGetPlansService wraps a real service and fails Get for one exact // ref with a configured error, so tests drive detail-refresh failures // deterministically instead of through fragile filesystem state. @@ -897,7 +692,7 @@ func TestPlanRefresh_BuriedDetailSuppressesErrorsUntilSurfaced(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { t.Parallel() - m, svc, _, _ := newPlansTestModel(t) + m, svc := newPlansTestModel(t) p := mustCreatePlan(t, svc, "release", "content") WithPlansService(&failingGetPlansService{ Service: svc, @@ -946,7 +741,7 @@ func TestPlanRefresh_BuriedDetailSuppressesErrorsUntilSurfaced(t *testing.T) { // underneath. Notifications may repeat; the dialog stack must stay correct. func TestPlanRefresh_DuplicateVanishedDetailClosesOnlyDetail(t *testing.T) { t.Parallel() - m, svc, _, _ := newPlansTestModel(t) + m, svc := newPlansTestModel(t) p := mustCreatePlan(t, svc, "release", "content") WithPlansService(&failingGetPlansService{ Service: svc, @@ -999,7 +794,7 @@ func TestPlanRefresh_DuplicateVanishedDetailClosesOnlyDetail(t *testing.T) { // while a reload is in flight collapse into exactly one follow-up reload. func TestPlanRefresh_CoalescesInFlightRequests(t *testing.T) { t.Parallel() - m, svc, _, _ := newPlansTestModel(t) + m, svc := newPlansTestModel(t) mustCreatePlan(t, svc, "release", "content") openPlanBrowser(t, m, plans.ListResult{Plans: []plans.Plan{}}) @@ -1025,7 +820,7 @@ func TestPlanRefresh_CoalescesInFlightRequests(t *testing.T) { // the kept path. func TestHandlePlanEditorClosed_EditorErrorKeepsDraft(t *testing.T) { t.Parallel() - m, svc, _, _ := newPlansTestModel(t) + m, svc := newPlansTestModel(t) draft := filepath.Join(t.TempDir(), "draft.md") require.NoError(t, os.WriteFile(draft, []byte("# saved before the editor failed"), 0o600)) @@ -1080,11 +875,11 @@ func (s *blockingReadPlansService) await(ctx context.Context, op string) error { } } -func (s *blockingReadPlansService) List(ctx context.Context, opts plans.ListOptions) (plans.ListResult, error) { +func (s *blockingReadPlansService) List(ctx context.Context) (plans.ListResult, error) { if err := s.await(ctx, "list"); err != nil { return plans.ListResult{}, err } - return s.Service.List(ctx, opts) + return s.Service.List(ctx) } func (s *blockingReadPlansService) Get(ctx context.Context, ref plans.Ref) (plans.Plan, error) { @@ -1116,7 +911,7 @@ func requireDeferredRead(t *testing.T, m *appModel, blocking *blockingReadPlansS func TestHandleShowPlanBrowser_ReadsAsynchronously(t *testing.T) { t.Parallel() - m, svc, _, _ := newPlansTestModel(t) + m, svc := newPlansTestModel(t) mustCreatePlan(t, svc, "release", "content") blocking := newBlockingReadPlansService(svc) WithPlansService(blocking)(m) @@ -1128,7 +923,7 @@ func TestHandleShowPlanBrowser_ReadsAsynchronously(t *testing.T) { func TestHandleRefreshPlans_ReadsAsynchronously(t *testing.T) { t.Parallel() - m, svc, _, _ := newPlansTestModel(t) + m, svc := newPlansTestModel(t) p := mustCreatePlan(t, svc, "release", "content") blocking := newBlockingReadPlansService(svc) WithPlansService(blocking)(m) @@ -1148,7 +943,7 @@ func TestHandleRefreshPlans_ReadsAsynchronously(t *testing.T) { func TestHandleOpenPlanDetail_ReadsAsynchronously(t *testing.T) { t.Parallel() - m, svc, _, _ := newPlansTestModel(t) + m, svc := newPlansTestModel(t) mustCreatePlan(t, svc, "release", "content") blocking := newBlockingReadPlansService(svc) WithPlansService(blocking)(m) @@ -1167,7 +962,7 @@ func TestHandleOpenPlanDetail_ReadsAsynchronously(t *testing.T) { } func TestHandleExportPlan_ExportsAsynchronously(t *testing.T) { - m, svc, _, _ := newPlansTestModel(t) + m, svc := newPlansTestModel(t) mustCreatePlan(t, svc, "release", "the content") blocking := newBlockingReadPlansService(svc) WithPlansService(blocking)(m) @@ -1195,7 +990,7 @@ func TestHandleExportPlan_ExportsAsynchronously(t *testing.T) { func TestPlanChangedEvent_RefreshReadsAsynchronously(t *testing.T) { t.Parallel() - m, svc, _, _ := newPlansTestModel(t) + m, svc := newPlansTestModel(t) mustCreatePlan(t, svc, "release", "content") blocking := newBlockingReadPlansService(svc) WithPlansService(blocking)(m) @@ -1218,7 +1013,7 @@ func TestHandleEditPlan_PreparesAsynchronously(t *testing.T) { t.Run("matching version drafts and launches the editor", func(t *testing.T) { t.Parallel() - m, svc, _, _ := newPlansTestModel(t) + m, svc := newPlansTestModel(t) p := mustCreatePlan(t, svc, "release", "v1 content") blocking := newBlockingReadPlansService(svc) WithPlansService(blocking)(m) @@ -1254,7 +1049,7 @@ func TestHandleEditPlan_PreparesAsynchronously(t *testing.T) { t.Run("version drift refreshes instead of editing", func(t *testing.T) { t.Parallel() - m, svc, _, _ := newPlansTestModel(t) + m, svc := newPlansTestModel(t) mustCreatePlan(t, svc, "release", "v1 content") openPlanBrowser(t, m, plans.ListResult{Plans: []plans.Plan{}}) v1 := 1 @@ -1295,7 +1090,7 @@ func TestHandleEditPlan_PreparesAsynchronously(t *testing.T) { // one write happens, and once it completed the pre-check refuses the // existing file. func TestHandleExportPlan_DuplicateInFlightRefused(t *testing.T) { - m, svc, _, _ := newPlansTestModel(t) + m, svc := newPlansTestModel(t) mustCreatePlan(t, svc, "release", "the content") blocking := newBlockingReadPlansService(svc) WithPlansService(blocking)(m) @@ -1352,7 +1147,7 @@ func TestHandleExportPlan_DuplicateInFlightRefused(t *testing.T) { // stack exactly one browser. func TestHandleShowPlanBrowser_DuplicateRequestsDropped(t *testing.T) { t.Parallel() - m, svc, _, _ := newPlansTestModel(t) + m, svc := newPlansTestModel(t) mustCreatePlan(t, svc, "release", "content") blocking := newBlockingReadPlansService(svc) WithPlansService(blocking)(m) @@ -1374,7 +1169,7 @@ func TestHandleShowPlanBrowser_DuplicateRequestsDropped(t *testing.T) { // browser. func TestHandleShowPlanBrowser_RefusedWhenBrowserAlreadyOpen(t *testing.T) { t.Parallel() - m, svc, _, _ := newPlansTestModel(t) + m, svc := newPlansTestModel(t) blocking := newBlockingReadPlansService(svc) WithPlansService(blocking)(m) openPlanBrowser(t, m, plans.ListResult{Plans: []plans.Plan{}}) @@ -1384,38 +1179,12 @@ func TestHandleShowPlanBrowser_RefusedWhenBrowserAlreadyOpen(t *testing.T) { assert.Zero(t, blocking.readsStarted.Load()) } -// TestHandleShowPlanBrowser_SessionSwitchAllowsFreshLaunch proves the -// browser-load guard tracks session identity: /plans for the new session -// launches while the previous session's read is still in flight, the stale -// result opens nothing, and the fresh one opens exactly one browser. -func TestHandleShowPlanBrowser_SessionSwitchAllowsFreshLaunch(t *testing.T) { - t.Parallel() - m, svc, _, _ := newPlansTestModel(t) - mustCreatePlan(t, svc, "release", "content") - blocking := newBlockingReadPlansService(svc) - WithPlansService(blocking)(m) - - _, cmdA := m.Update(messages.ShowPlanBrowserMsg{}) - require.NotNil(t, cmdA) - - switchPlansTestSession(t, m) - _, cmdB := m.Update(messages.ShowPlanBrowserMsg{}) - require.NotNil(t, cmdB, "/plans for the new session must launch despite the stale in-flight read") - - close(blocking.release) - msgsA := drainPlanFlow(t, m, cmdA) - assert.Zero(t, countOfType[dialog.OpenDialogMsg](msgsA), "the stale session's listing must not open a browser") - msgsB := drainPlanFlow(t, m, cmdB) - assert.Equal(t, 1, countOfType[dialog.OpenDialogMsg](msgsB), "the fresh session's listing opens the browser") - assert.False(t, m.planBrowserLoadInFlight) -} - // TestHandleOpenPlanDetail_DuplicateRequestsDropped proves two open requests // for the same plan racing one in-flight read start exactly one Get and // stack exactly one detail dialog. func TestHandleOpenPlanDetail_DuplicateRequestsDropped(t *testing.T) { t.Parallel() - m, svc, _, _ := newPlansTestModel(t) + m, svc := newPlansTestModel(t) mustCreatePlan(t, svc, "release", "content") blocking := newBlockingReadPlansService(svc) WithPlansService(blocking)(m) @@ -1440,7 +1209,7 @@ func TestHandleOpenPlanDetail_DuplicateRequestsDropped(t *testing.T) { // duplicate, while a different plan still loads. func TestHandleOpenPlanDetail_RefusedWhenDetailAlreadyOpen(t *testing.T) { t.Parallel() - m, svc, _, _ := newPlansTestModel(t) + m, svc := newPlansTestModel(t) p := mustCreatePlan(t, svc, "release", "content") mustCreatePlan(t, svc, "other", "content") blocking := newBlockingReadPlansService(svc) @@ -1463,7 +1232,7 @@ func TestHandleOpenPlanDetail_RefusedWhenDetailAlreadyOpen(t *testing.T) { // copy. func TestHandlePlanDetailLoaded_DuplicateOpenRefused(t *testing.T) { t.Parallel() - m, svc, _, _ := newPlansTestModel(t) + m, svc := newPlansTestModel(t) p := mustCreatePlan(t, svc, "release", "content") sizeDialogs(t, m) @@ -1574,7 +1343,7 @@ func TestPlanReads_WedgedStorageTimesOut(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - m, svc, _, _ := newPlansTestModel(t) + m, svc := newPlansTestModel(t) mustCreatePlan(t, svc, "release", "content") blocking := newBlockingReadPlansService(svc) // never released: reads answer only via ctx WithPlansService(blocking)(m) @@ -1621,7 +1390,7 @@ func TestPlanReads_WedgedStorageTimesOut(t *testing.T) { // the follow-up reload. func TestPlanRefresh_TimeoutClearsInFlightAndRunsQueued(t *testing.T) { t.Parallel() - m, svc, _, _ := newPlansTestModel(t) + m, svc := newPlansTestModel(t) mustCreatePlan(t, svc, "release", "content") openPlanBrowser(t, m, plans.ListResult{Plans: []plans.Plan{}}) blocking := newBlockingReadPlansService(svc) // never released @@ -1658,14 +1427,14 @@ func TestPlanRefresh_TimeoutClearsInFlightAndRunsQueued(t *testing.T) { // TestStalePlanResults_Dropped proves slow read results cannot disrupt a // user who moved on: a detail result opens no dialog after /plans was -// closed, a prepared edit launches no editor (and leaves no draft behind), -// and a listing read for a previous tab's session opens no browser. +// closed, and a prepared edit launches no editor (and leaves no draft +// behind). func TestStalePlanResults_Dropped(t *testing.T) { t.Parallel() t.Run("detail result after /plans closed opens no dialog", func(t *testing.T) { t.Parallel() - m, svc, _, _ := newPlansTestModel(t) + m, svc := newPlansTestModel(t) p := mustCreatePlan(t, svc, "release", "content") // No plan dialog is open anymore when the read lands. @@ -1675,7 +1444,7 @@ func TestStalePlanResults_Dropped(t *testing.T) { t.Run("edit result after /plans closed launches no editor", func(t *testing.T) { t.Parallel() - m, _, _, _ := newPlansTestModel(t) + m, _ := newPlansTestModel(t) draft := filepath.Join(t.TempDir(), "draft.md") require.NoError(t, os.WriteFile(draft, []byte("stored content"), 0o600)) @@ -1689,66 +1458,6 @@ func TestStalePlanResults_Dropped(t *testing.T) { _, err := os.Stat(draft) assert.True(t, os.IsNotExist(err), "the unused draft holds no user edits and is removed") }) - - t.Run("browser listing for a switched session opens no dialog", func(t *testing.T) { - t.Parallel() - m, svc, _, _ := newPlansTestModel(t) - p := mustCreatePlan(t, svc, "release", "content") - - _, cmd := m.Update(planBrowserLoadedMsg{ - sessionID: "previous-tab-session", - result: plans.ListResult{Plans: []plans.Plan{p}}, - }) - assert.Nil(t, cmd, "a listing read for another session must not open the browser") - }) -} - -// --- Stale refresh across session switches --------------------------------- - -// TestPlanRefresh_StaleSessionResultDroppedAndRelaunched reproduces the -// verifier proof: a reload launched for session A lands after the user -// switched to session B. A's listing — naming A's session plan — must not -// reach the dialogs, and exactly one fresh reload for B must replace it. -func TestPlanRefresh_StaleSessionResultDroppedAndRelaunched(t *testing.T) { - t.Parallel() - m, svc, sessA, sessionDir := newPlansTestModel(t) - mustCreatePlan(t, svc, "release", "content") - _, err := sessionplan.WriteContent(sessionDir, sessA.ID, "session A plan") - require.NoError(t, err) - - openPlanBrowser(t, m, plans.ListResult{Plans: []plans.Plan{}}) - - // The reload launches against session A's listing. - _, cmd := m.Update(messages.RefreshPlansMsg{}) - require.NotNil(t, cmd) - result := cmd() - refreshed, ok := result.(planRefreshedMsg) - require.True(t, ok, "got %T", result) - require.Equal(t, sessA.ID, refreshed.sessionID) - - // The user switches to session B before the result lands. - sessB := switchPlansTestSession(t, m) - _, err = sessionplan.WriteContent(sessionDir, sessB.ID, "session B plan") - require.NoError(t, err) - - // Dispatching A's stale result broadcasts nothing and relaunches once. - _, cmd = m.Update(result) - require.NotNil(t, cmd, "a fresh reload for the current session must launch") - assert.True(t, m.planRefreshInFlight, "the relaunched reload must be in flight") - - msgs := drainPlanFlow(t, m, cmd) - require.Equal(t, 1, countOfType[dialog.PlanBrowserDataMsg](msgs), - "exactly one fresh reload broadcasts; the stale one never does") - dataMsg, ok := firstOfType[dialog.PlanBrowserDataMsg](msgs) - require.True(t, ok) - var sessionRows []string - for _, p := range dataMsg.Result.Plans { - if p.Scope == plans.ScopeSession { - sessionRows = append(sessionRows, p.SessionID) - } - } - assert.Equal(t, []string{sessB.ID}, sessionRows, "only the current session's plan may be applied") - assert.False(t, m.planRefreshInFlight, "the pipeline must settle") } // TestPlanRefresh_ResultWithoutDialogsDropsSilently proves a reload result @@ -1756,7 +1465,7 @@ func TestPlanRefresh_StaleSessionResultDroppedAndRelaunched(t *testing.T) { // orphan notification and leaves the pipeline clean, queued intent included. func TestPlanRefresh_ResultWithoutDialogsDropsSilently(t *testing.T) { t.Parallel() - m, svc, _, _ := newPlansTestModel(t) + m, svc := newPlansTestModel(t) mustCreatePlan(t, svc, "release", "content") // A reload was in flight (with a queued follow-up) when the user closed @@ -1765,7 +1474,7 @@ func TestPlanRefresh_ResultWithoutDialogsDropsSilently(t *testing.T) { m.planRefreshQueued = true m.planRefreshQueuedWarnings = true - _, cmd := m.Update(planRefreshedMsg{sessionID: m.currentPlanSessionID(), listErr: errors.New("boom")}) + _, cmd := m.Update(planRefreshedMsg{listErr: errors.New("boom")}) assert.Nil(t, cmd, "no notification and no follow-up may be produced") assert.False(t, m.planRefreshInFlight, "the pipeline must be idle") assert.False(t, m.planRefreshQueued, "the queued follow-up must be dropped") diff --git a/pkg/tui/plans_unix_test.go b/pkg/tui/plans_unix_test.go index 5da94890b9..323af899f9 100644 --- a/pkg/tui/plans_unix_test.go +++ b/pkg/tui/plans_unix_test.go @@ -30,7 +30,7 @@ import ( // leaves the path in place. func TestHandlePlanEditorClosed_NoInlineDraftRead(t *testing.T) { t.Parallel() - m, svc, _, _ := newPlansTestModel(t) + m, svc := newPlansTestModel(t) fifo := filepath.Join(t.TempDir(), "draft.md") if err := syscall.Mkfifo(fifo, 0o600); err != nil { @@ -70,7 +70,7 @@ func TestShowPlanBrowser_FIFOPlanFileDoesNotHang(t *testing.T) { t.Parallel() m, _ := newTestModel(t) sharedDir := t.TempDir() - svc := plans.NewService(plan.NewFilesystemStorage(sharedDir), plans.WithSessionDir(t.TempDir())) + svc := plans.NewService(plan.NewFilesystemStorage(sharedDir)) WithPlansService(svc)(m) sess := session.New() m.application = app.New(t.Context(), stubRuntime{}, sess) diff --git a/pkg/tui/tui.go b/pkg/tui/tui.go index 9f9ca5380c..b189462ce0 100644 --- a/pkg/tui/tui.go +++ b/pkg/tui/tui.go @@ -114,15 +114,11 @@ type appModel struct { planRefreshQueued bool planRefreshQueuedWarnings bool - // planBrowserLoadInFlight and planBrowserLoadSessionID guard the /plans - // browser-opening read: a repeated request for the same session while its - // List is in flight is dropped, so duplicate browsers can never stack and - // no redundant read starts. A request for a different session (the user - // switched tabs) may launch; the superseded result is dropped as stale by - // its session stamp and only the matching result clears the guard. Both - // fields are touched exclusively from Update. - planBrowserLoadInFlight bool - planBrowserLoadSessionID string + // planBrowserLoadInFlight guards the /plans browser-opening read: a + // repeated request while its List is in flight is dropped, so duplicate + // browsers can never stack and no redundant read starts. Touched + // exclusively from Update. + planBrowserLoadInFlight bool // planDetailLoadsInFlight tracks the refs of running detail-opening // reads, so repeated open requests for the same plan cannot pile up @@ -1199,9 +1195,6 @@ func (m *appModel) update(msg tea.Msg) (tea.Model, tea.Cmd) { m.sessionState.SetSessionTitle(msg.Title) return m.forwardChat(msg) - case *runtime.SessionPlanUpdatedEvent: - return m.handleSessionPlanUpdatedEvent(msg) - case *runtime.PlanChangedEvent: return m.handlePlanChangedEvent(msg)