diff --git a/pkg/plans/plans.go b/pkg/plans/plans.go index 803e795268..3628b9c735 100644 --- a/pkg/plans/plans.go +++ b/pkg/plans/plans.go @@ -7,10 +7,11 @@ // 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 through the -// sessionplan helpers. Session plans have no revisions or optimistic locking -// and belong to their session, so the Service exposes them read/export-only -// and rejects mutations with a typed *UnsupportedError. +// 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. package plans import ( @@ -26,14 +27,17 @@ const ( // 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 and is - // read/export-only through the Service. + // 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 can be created, updated, and -// deleted through the Service, so a frontend can disable editing up front -// instead of provoking an *UnsupportedError. +// 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 } // Plan is the host-facing view of a plan from either scope. @@ -170,9 +174,11 @@ type ExportResult struct { } // Service is the host-facing contract for managing plans across both scopes. -// Mutations address shared plans only; a mutation aimed at a session plan -// fails with a typed *UnsupportedError. Failures are reported as the typed -// errors of this package so frontends never classify by error text. +// 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 +// 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 @@ -187,6 +193,11 @@ 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 fa95ca8300..c7d3e2ca52 100644 --- a/pkg/plans/service.go +++ b/pkg/plans/service.go @@ -152,6 +152,48 @@ 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 { return Plan{}, err diff --git a/pkg/plans/service_symlink_test.go b/pkg/plans/service_symlink_test.go index 57fbf388da..01b2cf871c 100644 --- a/pkg/plans/service_symlink_test.go +++ b/pkg/plans/service_symlink_test.go @@ -102,3 +102,28 @@ 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 c49639195b..417989f960 100644 --- a/pkg/plans/service_test.go +++ b/pkg/plans/service_test.go @@ -629,6 +629,123 @@ 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) { diff --git a/pkg/tui/dialog/plan_browser.go b/pkg/tui/dialog/plan_browser.go index bfc9203f83..af7b01ce60 100644 --- a/pkg/tui/dialog/plan_browser.go +++ b/pkg/tui/dialog/plan_browser.go @@ -221,12 +221,28 @@ func planRef(p plans.Plan) plans.Ref { 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) { @@ -388,10 +404,11 @@ func (d *planBrowserDialog) openDetailCmd() tea.Cmd { return core.CmdHandler(messages.OpenPlanDetailMsg{Ref: planRef(p)}) } -// guardedSharedPlan returns the selected plan when the given mutation applies -// to it: it must be a shared plan with a displayed version. Session plans get -// an explanatory notification instead of a failed service call. -func (d *planBrowserDialog) guardedSharedPlan(action string) (plans.Plan, tea.Cmd, bool) { +// 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. +func (d *planBrowserDialog) guardedPlan(action string) (plans.Plan, tea.Cmd, bool) { p, ok := d.selectedPlan() if !ok { return plans.Plan{}, nil, false @@ -403,7 +420,7 @@ func (d *planBrowserDialog) guardedSharedPlan(action string) (plans.Plan, tea.Cm } func (d *planBrowserDialog) statusCmd() tea.Cmd { - p, cmd, ok := d.guardedSharedPlan("status") + p, cmd, ok := d.guardedPlan("status") if !ok { return cmd } @@ -411,7 +428,7 @@ func (d *planBrowserDialog) statusCmd() tea.Cmd { } func (d *planBrowserDialog) deleteCmd() tea.Cmd { - p, cmd, ok := d.guardedSharedPlan("delete") + p, cmd, ok := d.guardedPlan("delete") if !ok { return cmd } @@ -419,21 +436,25 @@ func (d *planBrowserDialog) deleteCmd() tea.Cmd { } func (d *planBrowserDialog) editCmd() tea.Cmd { - p, cmd, ok := d.guardedSharedPlan("edit") + p, cmd, ok := d.guardedPlan("edit") if !ok { return cmd } - return core.CmdHandler(messages.EditPlanMsg{Ref: planRef(p), ExpectedVersion: *p.Version}) + return core.CmdHandler(messages.EditPlanMsg{Ref: planRef(p), ExpectedVersion: planVersionOrZero(p)}) } -// planMutationGuard returns an explanatory notification when the plan cannot -// be mutated from the host: session plans are read-only here, and a shared -// plan without a version (which the service always provides) is refused -// rather than mutated unguarded. +// 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. 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. Change the plan from within its session, or use a shared plan.", action)) + "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)) @@ -572,7 +593,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(p.Name, planColName)) + gap + + mainStyle.Render(planCell(planDisplayName(p), 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 + @@ -605,6 +626,15 @@ func planVersionLabel(version *int) string { return "v" + strconv.Itoa(*version) } +// planVersionOrZero reads a plan's displayed version, with 0 as the +// no-version sentinel for session plans (shared versions start at 1). +func planVersionOrZero(p plans.Plan) int { + if p.Version == nil { + return 0 + } + return *p.Version +} + func planTimeAgo(now, t time.Time) string { if t.IsZero() { return "-" diff --git a/pkg/tui/dialog/plan_browser_test.go b/pkg/tui/dialog/plan_browser_test.go index b9acc85915..d7274139c2 100644 --- a/pkg/tui/dialog/plan_browser_test.go +++ b/pkg/tui/dialog/plan_browser_test.go @@ -95,7 +95,9 @@ func TestPlanBrowserRendersScopeIdentityStatusVersionTimeTitle(t *testing.T) { 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, "11112222-3333-4444-55", "session plan identity is its session ID") + 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") @@ -179,6 +181,26 @@ 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()) @@ -256,12 +278,13 @@ func TestPlanBrowserSessionMutationsUnsupported(t *testing.T) { t.Parallel() d := newTestPlanBrowser(t, testPlanListing()) // session plan selected - for _, r := range []rune{'s', 'd', 'e'} { + 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 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") _, opened := firstMsgOfType[OpenDialogMsg](msgs) assert.False(t, opened, "%c must not open an action dialog for session plans", r) _, statusEmitted := firstMsgOfType[messages.SetPlanStatusMsg](msgs) @@ -273,6 +296,22 @@ 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()) diff --git a/pkg/tui/dialog/plan_detail.go b/pkg/tui/dialog/plan_detail.go index 0692deab33..14422b9529 100644 --- a/pkg/tui/dialog/plan_detail.go +++ b/pkg/tui/dialog/plan_detail.go @@ -149,7 +149,7 @@ func (d *planDetailDialog) handleKeyPress(msg tea.KeyPressMsg) (layout.Model, te if cmd := planMutationGuard(d.plan, "edit"); cmd != nil { return d, cmd } - return d, core.CmdHandler(messages.EditPlanMsg{Ref: d.PlanRef(), ExpectedVersion: *d.plan.Version}) + return d, core.CmdHandler(messages.EditPlanMsg{Ref: d.PlanRef(), ExpectedVersion: planVersionOrZero(d.plan)}) } return d, nil @@ -183,7 +183,7 @@ func (d *planDetailDialog) headerLines(contentWidth int) []string { if p.Scope == plans.ScopeSession { lines = append(lines, - field("Scope", "session — owned by its session, read-only here"), + field("Scope", "session — owned by its session, body editable here"), field("Session", p.SessionID), field("Version", "- (session plans have no versions)"), ) @@ -245,8 +245,12 @@ func (d *planDetailDialog) renderContent(contentWidth int) []string { func (d *planDetailDialog) helpKeys() []string { keys := []string{"↑/↓", "scroll", "r", "refresh", "x", "export"} - if d.plan.Scope.Mutable() { + 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") } return append(keys, "esc", "close") } diff --git a/pkg/tui/dialog/plan_detail_test.go b/pkg/tui/dialog/plan_detail_test.go index fc8a766de5..c71c302be6 100644 --- a/pkg/tui/dialog/plan_detail_test.go +++ b/pkg/tui/dialog/plan_detail_test.go @@ -62,11 +62,17 @@ func TestPlanDetailRendersSessionMetadata(t *testing.T) { d := newTestPlanDetail(t, p) view := d.View() - assert.Contains(t, view, "read-only here", "session scope must be explicit") + 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) { @@ -137,17 +143,40 @@ func TestPlanDetailSessionActionsUnsupported(t *testing.T) { } d := newTestPlanDetail(t, p) - for _, r := range []rune{'s', 'd', 'e'} { + 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 are read-only", r) + 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 f237b16e06..29e7f43a57 100644 --- a/pkg/tui/messages/plans.go +++ b/pkg/tui/messages/plans.go @@ -6,10 +6,12 @@ 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. // -// 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. +// 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. type ( // ShowPlanBrowserMsg opens the /plans browser dialog. ShowPlanBrowserMsg struct{} @@ -43,8 +45,10 @@ type ( // the external $VISUAL/$EDITOR. CreatePlanMsg struct{ Name string } - // EditPlanMsg edits a shared plan's content in the external - // $VISUAL/$EDITOR. + // 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. EditPlanMsg struct { Ref plans.Ref ExpectedVersion int diff --git a/pkg/tui/plans.go b/pkg/tui/plans.go index 00ac1e996b..0e1b8f7c34 100644 --- a/pkg/tui/plans.go +++ b/pkg/tui/plans.go @@ -615,10 +615,11 @@ 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. + // instead of editing. Session plans never take this branch: they + // have no versions, so both sides are always 0. return ready } - ready.draftPath, ready.draftErr = planDraftFile("cagent-plan-"+msg.Ref.Name+"-*.md", p.Content) + ready.draftPath, ready.draftErr = planDraftFile(planDraftPattern(msg.Ref), p.Content) return ready } } @@ -681,6 +682,17 @@ type planEditorClosedMsg struct { err error } +// 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. +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" +} + // planDraftFile writes content to a fresh temp markdown file and returns its // path. func planDraftFile(pattern, content string) (string, error) { @@ -719,8 +731,8 @@ func (m *appModel) handlePlanEditorClosed(msg planEditorClosedMsg) (tea.Model, t notification.InfoCmd("Your draft is kept at "+msg.path)) } - // Both the draft read and the guarded write run in a command: reading in - // Update would stall the event loop on a draft path swapped for a FIFO + // Both the draft read and the persistence call run in a command: reading + // in Update would stall the event loop on a draft path swapped for a FIFO // or device and allocate unbounded memory for a runaway file, and a // contended plans lock could freeze it just the same. The draft file // outlives the command until the service confirms the write. @@ -740,9 +752,14 @@ func (m *appModel) handlePlanEditorClosed(msg planEditorClosedMsg) (tea.Model, t } ctx, cancel := context.WithTimeout(ctx, timeout) defer cancel() - if msg.create { + switch { + case msg.create: result.plan, result.err = svc.Create(ctx, plans.CreateRequest{Ref: msg.ref, Content: content}) - } else { + 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: expected := msg.expectedVersion result.plan, result.err = svc.Update(ctx, plans.UpdateRequest{Ref: msg.ref, Content: content, ExpectedVersion: &expected}) } @@ -794,21 +811,31 @@ func (m *appModel) handlePlanWriteResult(msg planWriteResultMsg) (tea.Model, tea notification.ErrorCmd(fmt.Sprintf("Failed to read edited plan: %v", msg.readErr)), notification.InfoCmd("Your draft is kept at "+msg.draftPath)) case msg.emptyDraft: - if msg.create { + switch { + case 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 } _ = os.Remove(msg.draftPath) - verb := "Updated" - if msg.create { - verb = "Created" + var text string + switch { + case 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: + text = fmt.Sprintf("Updated shared plan %q (now v%d)", msg.plan.Name, planVersionOf(msg.plan)) } - cmds := []tea.Cmd{notification.SuccessCmd(fmt.Sprintf("%s shared plan %q (now v%d)", verb, msg.plan.Name, planVersionOf(msg.plan)))} + cmds := []tea.Cmd{notification.SuccessCmd(text)} cmds = m.appendPlanRefreshCmd(cmds) return m, tea.Sequence(cmds...) } diff --git a/pkg/tui/plans_test.go b/pkg/tui/plans_test.go index 6189835330..60d374cbea 100644 --- a/pkg/tui/plans_test.go +++ b/pkg/tui/plans_test.go @@ -462,6 +462,107 @@ 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)