Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 23 additions & 12 deletions pkg/plans/plans.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 (
Expand All @@ -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.
Expand Down Expand Up @@ -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
Expand All @@ -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)
Expand Down
42 changes: 42 additions & 0 deletions pkg/plans/service.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
25 changes: 25 additions & 0 deletions pkg/plans/service_symlink_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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")
}
117 changes: 117 additions & 0 deletions pkg/plans/service_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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, &notFound)
assert.Equal(t, ScopeSession, notFound.Scope)
assert.Equal(t, "ghost", notFound.Name)

_, err = svc.Get(t.Context(), SessionRef("ghost"))
require.ErrorAs(t, err, &notFound, "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) {
Expand Down
58 changes: 44 additions & 14 deletions pkg/tui/dialog/plan_browser.go
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down Expand Up @@ -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
Expand All @@ -403,37 +420,41 @@ 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
}
return core.CmdHandler(OpenDialogMsg{Model: newPlanStatusDialog(p.Name, p.Status, *p.Version)})
}

func (d *planBrowserDialog) deleteCmd() tea.Cmd {
p, cmd, ok := d.guardedSharedPlan("delete")
p, cmd, ok := d.guardedPlan("delete")
if !ok {
return cmd
}
return core.CmdHandler(OpenDialogMsg{Model: newPlanDeleteConfirmDialog(p.Name, *p.Version)})
}

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))
Expand Down Expand Up @@ -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 +
Expand Down Expand Up @@ -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 "-"
Expand Down
Loading
Loading