feat(tui): make current session plans editable - #3878
Conversation
|
/review |
aheritier
left a comment
There was a problem hiding this comment.
🤖 Automated implementer agent — this comment was posted by the implementer bot from Docker Agentic Platform, not by a human developer
Code Review: feat(tui): make current session plans editable
All tests pass (plans, tui, cmd/root), build is clean, and go vet shows no issues. This is a well-structured feature addition.
What the PR does
Adds the ability to edit the body of a session plan (the per-session draft/review/execute plan) from the TUI browser and detail dialogs. Session plans were previously shown as read-only. The PR:
- Adds
UpdateSessionto theplans.Serviceinterface and implements it on the concrete service - Routes the 'e' key in the plan browser and detail dialog to either the existing shared-plan guarded update or the new unguarded
UpdateSessionpath - Displays session plan rows with the label "current session" instead of the raw session ID
- Adds comprehensive test coverage at all layers
Strengths
- Correct version handling: The sentinel
ExpectedVersion: 0for session plans (which have no versions) is well-reasoned. ThehandleEditPlandrift checkcurrentVersion != msg.ExpectedVersioncorrectly short-circuits (0 == 0) for session plans, so the editor always opens. - Atomic writes:
UpdateSessiondelegates tosessionplan.WriteContent, which uses an atomic rename — readers see old or new content, never a partial write; symlinks in the path are replaced rather than followed. - Edit-never-creates contract: The pre-check with
os.Statenforces thatUpdateSessionedits only, returning*NotFoundErrorfor missing plans. The acknowledged TOCTOU race window is documented and accepted. - Robust error propagation:
planEditorFailureCmdhandlesNotFoundErrorfromUpdateSessiongracefully — the draft is kept, the user gets a useful notification, and no data is lost. - Consistent UX: Session-specific notifications ("Updated the current session plan.", "Session plan left unchanged") avoid the confusing shared-plan version strings that would appear if the session code path used the shared plan messages.
- Test coverage: All edge cases are covered — normal edit, last-write-wins, invalid session ID, edit-never-creates, validation failures, expired context, symlink resistance, version drift, empty draft, and plan vanished.
Minor observations (non-blocking)
-
Redundant filter check for shared plans (
plan_browser.go, line 244-245): InapplyFilter, bothp.NameandplanDisplayName(p)are checked. For shared plansplanDisplayNamereturnsp.Name, so the two terms are equivalent and the second check is never the tiebreaker. For session plans both are needed (session ID and "current session" label). The redundancy is harmless and short-circuits away, but the intent could be clearer. -
Comment partially restates code (
service.go, line 192-193): The comment// Read the plan back so the caller gets the stored bytes and the real file modification time.opens with "Read the plan back" which mirrors whatgetSessionobviously does. Per project conventions, the WHY is valuable (real mtime from the atomic rename, not the in-memory content); the WHAT prefix could be dropped. -
TOCTOU race documented but worth calling out: Between
os.Stat(path)andsessionplan.WriteContent, an external deletion would cause the write to recreate the plan — silently violating the edit-never-creates contract. The comment acknowledges this. The test coverage tests a missing plan from the start (not the narrow race itself), which is the realistic and testable path.
None of these require changes — they're observations for context. The implementation is correct, safe, and well-tested.
Summary
current sessionin/plansRelationship
Follow-up to #3876.
Do not merge before #3876. PR #3876 fixes the terminal and UTF-8 behavior used by the external plan editor flow. Keep this PR in draft until #3876 is merged.
Feedback mapping
current session, while its full session ID remains visible in the footer and detail view.shared, because they are workspace-global and have no session ownership.eedits the session plan body from either the browser or detail view.Validation
go test -count=1 ./pkg/tools/builtin/sessionplan ./pkg/plans ./pkg/tui/dialog ./pkg/tui/messages ./pkg/tui ./cmd/rootgo vet ./pkg/tools/builtin/sessionplan ./pkg/plans ./pkg/tui/dialog ./pkg/tui/messages ./pkg/tui ./cmd/roottask --force linttask --force buildtask --force check-plan-cross/plans-> verifycurrent session-> verify status is refused ->e-> external editor -> refreshed detail -> persisted contentTest-suite note
task testwas run. The changed packages pass, but the complete suite is blocked in this execution environment by unrelated SSRF/local-address tests becauseHTTP_PROXYandHTTPS_PROXYare forced through a socket firewall. The failures are inpkg/config,pkg/httpclient,pkg/tools/builtin/api,pkg/tools/builtin/fetch, andpkg/tools/builtin/openapi.