feat: add first-class host UX for plan management (#3844) - #3853
Conversation
|
not finished yet don't approve or merge |
aheritier
added
area/cli
… ChangeNotifier - ValidateName: canonical name rule now exported so pkg/plans validates identically to the storage without duplicating the regexp. - CorruptPlanError: typed error replacing the anonymous fmt.Errorf in load(); same message, cause wrapped, distinguishable via errors.As. - SharedStorage(): returns the storage behind the process-wide singleton so a TUI and agent tools in one process share one mutex. - ChangeNotifier interface + SetChangeCallback on ToolSet: fires after every successful write/status/delete, never on failures; stored as an atomic.Pointer so per-turn re-configuration from concurrent sessions is race-free. Discoverable through toolset wrappers via tools.As.
New pkg/plans wraps the existing plan.Storage and sessionplan helpers behind a single Service interface consumed by both the CLI and the TUI, so neither surface ever reads plan files directly. - Unified Plan model with explicit Scope (shared | session) and nil Version for session plans (no revisions). - Six typed host errors: NotFoundError, ValidationError, CorruptError, StorageError, ConflictError (with Expected/Current), UnsupportedError. - Service operations: List, Get, Create, Update, SetStatus, Delete, Export — all with ExpectedVersion / force semantics. - Create is create-only (expected revision 0); Update is update-only (MustExist); session mutations return UnsupportedError immediately. - List with SessionID includes only that session's plan; stale session files are never enumerated globally. - Export is atomic (atomicfile.Write + parent mkdir) for both scopes. - 34 unit tests covering all error kinds, create-only conflict, stale update/status/delete preserving newer content, session scope, injected in-memory storage, and NewService nil-storage panic.
Implements the CLI surface from #3844: list, get, create, update, status, export, delete — all built over pkg/plans.Service to avoid any direct storage access. Key behaviors: - Mutations require exactly one of --expected-version <n> or --force (Cobra MarkFlagsOneRequired + MarkFlagsMutuallyExclusive); nil expected version is never sent by accident. - --expected-version validated >= 1; create is guard-free (service enforces create-only via expected revision 0). - Session plans (--session <id> / --scope session): list/get/export work; mutations surface the service's UnsupportedError with actionable reason. - --json: stable schema_version:"1" envelope on stdout; single-line JSON error object on stderr with code/scope/name/op/expected_version/ current_version, never mixing prose. All codes are typed, not text-parsed. - Exit code 3 for conflicts, 1 for all other failures; Cobra SilenceErrors so stderr stays one object in --json mode. - Service is built lazily inside RunE so plan.SharedStorage() does not resolve the data directory before --data-dir is applied. - 30 hermetic CLI tests with injected temp-backed service.
Adds PlanChangedEvent (type plan_changed) carrying scope/name/action/ version but never content, so a UI refreshes through its own plan service rather than trusting event payloads. - configureToolsetHandlers wires a non-blocking callback via tools.As[plan.ChangeNotifier] — same pattern as the RAG event forwarder — so a stale sink can never hang a tool call. - Event registered in the client registry so remote runtimes decode it as *PlanChangedEvent rather than dropping it. - 3 tests: serialization (no content field), client decode over SSE, and end-to-end write/status/delete emit vs. failed-write no-emit through configureToolsetHandlers.
Implements the TUI host-management surface from #3844. Slash command: - /plans opens the plan browser (command palette discoverable, immediate, silently dropped in lean mode like /settings). Browser dialog (pkg/tui/dialog/plan_browser.go): - Lists shared plans + active session plan (no global session sweep), showing scope, identity, status, version (- for session), relative updated time, and title; all columns safely truncated. - Keyboard: ↑/↓ navigate, / filter (Esc exits filter), Enter → detail, r refresh, x export, s set status, d delete (confirmation), n new plan (name dialog → $EDITOR), e edit ($EDITOR with pre-seeded content). - Session plan rows show an explanatory notification instead of an action dialog for s/d/e. - Double-click opens detail; warnings shown in footer. - Mouse click navigates, double-click opens detail. Detail dialog (pkg/tui/dialog/plan_detail.go): - Explicit scope line ("shared — collaborative, versioned" / "session — owned by its session, read-only here"), all metadata, scrollable markdown-rendered content with width-cap fallback. - Same action keys as browser; session scope hides s/e/d from help and shows an explanation instead. - Implements PlanDetailViewer so the app can re-fetch the exact plan on refresh. App handlers (pkg/tui/plans.go): - Plans service built lazily (plan.SharedStorage() only resolved after --data-dir is applied); injectable for tests. - All mutations version-guarded (never nil by accident); conflicts surface current version, keep newer content, refresh dialogs. - Export: deterministic filename (<name>.md / session-plan-<8chars>.md), refuses to overwrite an existing file. - Create/edit: temp file + tea.ExecProcess, drift detected before editor opens, draft kept on conflict. - SessionPlanUpdatedEvent: refresh open dialogs only for the current session, forward to chat page. - PlanChangedEvent: refresh open dialogs regardless of which session emitted it; background-tab mutations also refresh via handleRoutedMsg. Broadcastable mechanism (pkg/tui/dialog/dialog.go): - Messages implementing Broadcastable are delivered to all dialogs in the stack so a browser buried under the detail stays in sync. 19 browser/detail tests + 17 app-handler tests.
c2a1f97 to
352d124
Compare
aheritier
left a comment
aheritier
left a comment
There was a problem hiding this comment.
Review — first-class host UX for plan management
Head: 352d124a · CI: green (21 check-runs, all completed — 15 success incl. lint, build-and-test, license-check, CodeQL ×3, build-image linux/amd64+arm64 and the docs checks; 6 skipped push/merge jobs) · no commit statuses, nothing pending or failing · MERGEABLE, mergeStateStatus: BLOCKED.
Recommendation: do not merge or approve yet — the author states "not finished yet don't approve or merge". This is a COMMENT-only review; the findings below are for the author's next pass, not gates I'm claiming beyond that.
The shape of this is strong: the file-lock build-tag split (unix / windows / !unix && !windows) with an honest doc comment about where cross-process exclusion genuinely degrades, the never-deleted sentinel inode rationale, atomic temp+rename writes, optimistic locking mirrored consistently across tools/CLI/TUI, the non-blocking event sink with an explicit release path, the pointer-identity dedup and its panic-safety justification, and a genuinely dense test suite (subprocess lock contention, TUI scroll/selection preservation, JSON error contract). Comments explain why, per AGENTS.md.
Four points are inline. The rest:
1. Windows locking is compile-verified only (moderate). pkg/tools/builtin/plan/lock_windows.go implements the LockFileEx/UnlockFileEx path, and plan_lock_test.go is — correctly — untagged, so it would exercise cross-process locking on Windows. But no CI job runs on a Windows runner: .github/workflows/ci.yml has lint, build-and-test, license-check on ubuntu-latest and build-image on linux/amd64 + linux/arm64 only. task check-plan-cross proves the Windows path compiles, not that it locks — and locking correctness is the crux of this feature. A minimal windows-latest job running go test ./pkg/tools/builtin/plan/ ./pkg/plans/ would close the gap cheaply.
2. check-plan-cross covers the build tags but not the new callers (minor). All three lock variants are covered for ./pkg/tools/builtin/plan/ and ./pkg/plans/ (linux for unix, windows, and js/wasip1/plan9 for !unix && !windows) — good. Not covered: the new host code that also branches on GOOS, i.e. pkg/tui/plans.go (notepad vs vi) and cmd/root/plans.go; both packages ship //go:build !windows test files, so a Windows-only build break there would pass CI silently. Adding ./cmd/root/ ./pkg/tui/ to at least the GOOS=windows line would catch it.
3. plan_changed isn't documented on the API/A2A surfaces (nit). The event is registered in the runtime event registry (pkg/runtime/event.go) and decodes off the SSE client, so it reaches API and A2A consumers — but the docs updated here are TUI/CLI/tool pages only, and docs/features/api-server/index.md's "Event types include" list isn't extended. That list is already non-exhaustive, so this is discretionary; a new externally visible event type still seems like a good candidate for it.
4. Telemetry carries raw positional args (nit / privacy minimisation). runPlans builds trackArgs := append([]string{sub}, args...), so plan names and --session values reach TrackCommand/TrackCommandError, and the error variant also ships err.Error() (which can embed --output/--file paths). This matches the existing convention in cmd/root/{models,alias,pull,a2a,debug}.go, so it is not a regression introduced here. But plan names are user-authored content; tracking only sub — as cmd/root/sandbox_cmd.go already does — would be strictly better for a brand-new command group.
5. plan_detail.go has no dedicated test file (nit). Coverage is genuinely there — seven TestPlanDetail* tests plus the newTestPlanDetail helper — but they live in pkg/tui/dialog/plan_browser_test.go. AGENTS.md asks for tests alongside source files; splitting them into plan_detail_test.go would make the ~300-line dialog's coverage discoverable without changing a single assertion.
| func (m *appModel) execPlanEditor(result planEditorClosedMsg) tea.Cmd { | ||
| parts := strings.Fields(cmp.Or(os.Getenv("VISUAL"), os.Getenv("EDITOR"))) | ||
| if len(parts) == 0 { | ||
| if goruntime.GOOS == "windows" { | ||
| parts = []string{"notepad"} | ||
| } else { | ||
| parts = []string{"vi"} | ||
| } | ||
| } | ||
| args := append(parts[1:], result.path) | ||
| // The editor process is owned by tea.ExecProcess, so exec.Command is intentional. | ||
| cmd := exec.Command(parts[0], args...) //nolint:noctx // owned by tea.ExecProcess | ||
| return tea.ExecProcess(cmd, func(err error) tea.Msg { | ||
| result.err = err | ||
| return result | ||
| }) | ||
| } |
There was a problem hiding this comment.
Minor (refactor): this editor-resolution + launch block duplicates pkg/tui/tui.go:3141-3153 almost line for line — cmp.Or(os.Getenv("VISUAL"), os.Getenv("EDITOR")) → strings.Fields → notepad/vi GOOS fallback → append(parts[1:], path) → exec.Command with the same //nolint:noctx // owned by tea.ExecProcess rationale.
There's already a pkg/tui/internal/editorname package that centralises the display name half of exactly this resolution (and duplicates the same Windows/Vi fallback decision a third time), so the launch half is the odd one out. Extracting one helper — e.g. editorname.Command(path) *exec.Cmd, or a small externalEditorCmd(path) in pkg/tui — would keep the sites from drifting; the Windows fallback and any future argument/flag handling are the likely divergence points.
| func hardenPlansValidation(cmd *cobra.Command) { | ||
| if inner := cmd.Args; inner != nil { | ||
| cmd.Args = func(cmd *cobra.Command, args []string) error { | ||
| return plansValidationError(cmd, inner(cmd, args)) | ||
| } | ||
| } | ||
| cmd.PreRunE = func(cmd *cobra.Command, _ []string) error { | ||
| if err := cmd.ValidateRequiredFlags(); err != nil { | ||
| return plansValidationError(cmd, err) | ||
| } | ||
| return plansValidationError(cmd, cmd.ValidateFlagGroups()) | ||
| } | ||
| } |
There was a problem hiding this comment.
Minor (latent footgun): cmd.Args is chained defensively just above (if inner := cmd.Args; inner != nil), but cmd.PreRunE is assigned unconditionally — so any PreRunE already set on a plans subcommand is silently dropped. No subcommand in this file sets one today, so this isn't a live bug, and the root command's PersistentPreRunE is a separate hook that stays intact.
The concern is the asymmetry: the next person adding a PreRunE to a plans subcommand gets a silently skipped hook, not a compile error. cmd/root/root.go:244-246 already uses the capture-then-chain pattern for this exact reason:
inner := cmd.PreRunE
cmd.PreRunE = func(cmd *cobra.Command, args []string) error {
if inner != nil {
if err := inner(cmd, args); err != nil {
return plansValidationError(cmd, err)
}
}
if err := cmd.ValidateRequiredFlags(); err != nil {
return plansValidationError(cmd, err)
}
return plansValidationError(cmd, cmd.ValidateFlagGroups())
}| func writeExportFile(scope Scope, path, content string) error { | ||
| clean := filepath.Clean(path) | ||
| if info, err := os.Stat(clean); err == nil && info.IsDir() { | ||
| return &ValidationError{Message: fmt.Sprintf("path %q is a directory, not a file", path)} | ||
| } | ||
| if err := os.MkdirAll(filepath.Dir(clean), 0o700); err != nil { | ||
| return &StorageError{Scope: scope, Op: "export", Err: err} | ||
| } | ||
| if err := atomicfile.Write(clean, strings.NewReader(content), 0o600); err != nil { | ||
| return &StorageError{Scope: scope, Op: "export", Err: err} | ||
| } | ||
| return nil |
There was a problem hiding this comment.
Moderate (behaviour / docs mismatch): writeExportFile only rejects a directory target; an existing file is silently overwritten by atomicfile.Write. The TUI export path deliberately does the opposite — it pre-checks and refuses, and its own error message points the user at this command as the escape hatch:
<path> already exists — move it away, or export to a custom path with 'docker agent plans export'.
and docs/tools/plan/index.md / docs/features/tui/index.md both document "An existing file is never overwritten" / "refusing to overwrite an existing file" for that path.
So the divergence looks deliberate, but it is undocumented where it matters: newPlansExportCmd's Long promises only that "parent directories are created and the write is atomic, so a reader never observes a partial export", and --output's help is just "Destination file for the plan content; required". A reader coming from the TUI's no-overwrite guarantee will reasonably assume the CLI is equally safe. Either state the overwrite explicitly in Long/--output, or apply the same guard here and add --force to opt out — which would also match the --expected-version/--force idiom the rest of the group already uses.
| // Non-blocking sink like the RAG forwarder: the callback fires from | ||
| // another session's tool call, and a blocking send into this stream's | ||
| // events channel could hang that session's tool call. | ||
| func (r *LocalRuntime) subscribePlanChanges(sess *session.Session, events EventSink) (release func()) { |
There was a problem hiding this comment.
Question (design): the fan-out here is per-stream across every agent's toolsets plus sess.ExtraToolSets, deduped by pointer identity on what the doc comment describes as a process-wide *plan.ToolSet singleton. The consequence is that a mutation performed by session A's tool call is delivered to every open stream in the process, with a hardcoded "shared" scope and empty agent attribution.
For the TUI (one process, one user, one /plans browser to refresh) that is exactly the desired behaviour, and the comment justifies the dedup well. For a long-lived serve api / serve mcp / serve a2a process serving many concurrent sessions, though, every session's SSE stream also observes every other session's plan mutations — plan names included. Is that intended on the server surfaces, or should the fan-out be scoped there (opt-in per stream, or filtered by scope/session)? Either way it'd be worth one line in this doc comment: it currently explains the dedup and the non-blocking sink, but not the cross-session visibility that follows from a shared notifier.
|
See my comments for followups @Sayt-0 |
3 participants
Summary
Closes #3844.
Adds a host-facing plan management contract used by a scriptable CLI and the TUI while preserving the existing
planandsession_plantoolsets.What changed
Shared plan storage and notifications
plan.Storagebackend throughplan.SharedStorage().flockon Unix;LockFileExon Windows;ChangeNotifier, registered once per runtime stream and released before its event channel closes.pkg/planshost contractA shared service interface for CLI and TUI:
sharedandsessionscopes;snake_case.docker agent plans3, generic failure exit code1;--jsonhas been parsed.TUI preview
The TUI is rendered with the active theme; the schematic below shows the information hierarchy and controls without depending on terminal colors.
Plan browser
Plan detail
Session plans use the same browser/detail flow but are explicitly labeled read-only and unversioned. Destructive shared-plan actions name the plan and require confirmation.
TUI
/plansRuntime event
PlanChangedEventcarries scope, name, action, and version without content. Every active runtime stream subscribed to the shared toolset receives the event; subscriptions are deduplicated and released with stream lifecycle.Issue expectations
/plansbrowser and detail dialogschema_version: "1", snake_case fields, structured errorsunsupportedValidation
task buildtask testtask test-binarygolangci-lint runand repository lintReview history
The dedicated adversarial review team was run repeatedly against published PR heads. Its confirmed findings were corrected in follow-up commits, including cross-process races, notifier fan-out, bounded I/O, TUI lock waits, stale async results, duplicate dialogs/exports, and create-only revision-zero handling.
One unrelated pre-existing race remains in
TestLoadSessionThenClickEditLabel(App.ReplaceSessionversus session startup); the plan-focused race suites pass.Known limitation
The TUI's default no-overwrite export remains a
statplus atomic write sequence. Duplicate in-process exports are prevented, but a separate external process can still create the destination during the narrow race window.