feat(tui): add plan mode command and fix plan file editing#643
feat(tui): add plan mode command and fix plan file editing#643euxaristia wants to merge 4 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
WalkthroughChangesPlan mode workflow
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant handlePlanCommand
participant openPlanInEditor
participant tea.ExecProcess
participant Editor
User->>handlePlanCommand: Submit /plan open
handlePlanCommand->>openPlanInEditor: Resolve or create session plan
openPlanInEditor->>tea.ExecProcess: Launch configured editor
tea.ExecProcess->>Editor: Attach stdio and execute
Editor-->>tea.ExecProcess: Exit
tea.ExecProcess-->>handlePlanCommand: Return planEditorFinishedMsg
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
internal/tui/plan_command.go (3)
154-160: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
fileExistsreturning(struct{}, bool)is dead weight.The
struct{}value is never used at the call site (Line 77). Return a plainbool. Also noteos.Staterrors other than "not exist" (e.g. permission) are collapsed into "missing", which would then trigger an unnecessaryWritePlan; considererrors.Is(err, os.ErrNotExist)if that distinction matters.♻️ Simplify
-func fileExists(path string) (struct{}, bool) { - _, err := os.Stat(path) - if err != nil { - return struct{}{}, false - } - return struct{}{}, true -} +func fileExists(path string) bool { + _, err := os.Stat(path) + return err == nil +}- if _, ok := fileExists(path); !ok { + if !fileExists(path) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/tui/plan_command.go` around lines 154 - 160, Update fileExists to return only a bool, and adjust its call site to use the simplified result. Use errors.Is(err, os.ErrNotExist) to distinguish a missing file from other os.Stat failures, preserving non-not-found errors instead of treating them as absent.
116-126: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRedundant path resolution in
planText.
PlanFilePathis called to obtainpath, thenReadPlanre-resolves it internally; theif path != ""at Line 121 is also already implied by the Line 118 guard. Minor, but the double resolution can be collapsed.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/tui/plan_command.go` around lines 116 - 126, Refactor model.planText to avoid resolving the plan path twice: use a single planmode.PlanFilePath result to determine and read the plan, or update the read flow to pass the already-resolved path where supported. Remove the redundant if path != "" check inside the successful branch while preserving the existing header and content behavior.
101-103: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winEditor failures are silently swallowed.
The
tea.ExecProcesscallback discardserr, so a missing/misconfigured editor or a non-zero exit leaves the user with no feedback — the TUI just resumes as if nothing happened. Surface it via a transcript message.♻️ Report editor errors back to the transcript
- return m, tea.ExecProcess(cmd, func(err error) tea.Msg { - return nil - }) + return m, tea.ExecProcess(cmd, func(err error) tea.Msg { + if err != nil { + return editorFinishedMsg{err: err} + } + return nil + })Handle
editorFinishedMsginUpdateto append an error row. Please also confirm thetea.ExecProcess(cmd, func(error) tea.Msg)signature for bubbletea v2.0.7.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/tui/plan_command.go` around lines 101 - 103, Editor failures are discarded in the tea.ExecProcess callback. In the editor-launching method, return the received error as an editorFinishedMsg (confirming Bubble Tea v2.0.7’s callback signature), then handle editorFinishedMsg in Update by appending a transcript error row; preserve successful completion without adding an error.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/tui/plan_command.go`:
- Line 39: Add the missing PermissionModePlan constant to the agent package
alongside PermissionModeAuto and PermissionModeUnsafe, using the existing
permission-mode type and value conventions; verify all references in
plan_command.go compile successfully.
- Line 12: The import of internal/planmode in plan_command.go cannot resolve
because the package is missing or has an invalid declaration. Ensure the
planmode package files are included and declare a valid package name matching
the import, then verify references from plan_command.go compile successfully.
---
Nitpick comments:
In `@internal/tui/plan_command.go`:
- Around line 154-160: Update fileExists to return only a bool, and adjust its
call site to use the simplified result. Use errors.Is(err, os.ErrNotExist) to
distinguish a missing file from other os.Stat failures, preserving non-not-found
errors instead of treating them as absent.
- Around line 116-126: Refactor model.planText to avoid resolving the plan path
twice: use a single planmode.PlanFilePath result to determine and read the plan,
or update the read flow to pass the already-resolved path where supported.
Remove the redundant if path != "" check inside the successful branch while
preserving the existing header and content behavior.
- Around line 101-103: Editor failures are discarded in the tea.ExecProcess
callback. In the editor-launching method, return the received error as an
editorFinishedMsg (confirming Bubble Tea v2.0.7’s callback signature), then
handle editorFinishedMsg in Update by appending a transcript error row; preserve
successful completion without adding an error.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 328b2d08-881f-4c6a-ba61-2065b0525cc0
📒 Files selected for processing (3)
internal/tui/model.gointernal/tui/plan_command.gointernal/tui/run.go
| tea "charm.land/bubbletea/v2" | ||
|
|
||
| "github.com/Gitlawb/zero/internal/agent" | ||
| "github.com/Gitlawb/zero/internal/planmode" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Build is broken: internal/planmode isn't resolvable.
CI reports the package cannot be imported (invalid package name: "" / no module provides it). Either the package/files aren't committed in this PR or its package declaration is empty. This blocks the whole build.
🧰 Tools
🪛 GitHub Check: Security & code health
[failure] 12-12:
could not import github.com/Gitlawb/zero/internal/planmode (invalid package name: "")
[failure] 12-12:
no required module provides package github.com/Gitlawb/zero/internal/planmode; to add it:
🪛 GitHub Check: Smoke (ubuntu-latest)
[failure] 12-12:
no required module provides package github.com/Gitlawb/zero/internal/planmode; to add it:
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/tui/plan_command.go` at line 12, The import of internal/planmode in
plan_command.go cannot resolve because the package is missing or has an invalid
declaration. Ensure the planmode package files are included and declare a valid
package name matching the import, then verify references from plan_command.go
compile successfully.
Source: Pipeline failures
| arg := strings.ToLower(strings.TrimSpace(text)) | ||
| switch arg { | ||
| case "off", "exit": | ||
| if m.permissionMode != agent.PermissionModePlan { |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
agent.PermissionModePlan is undefined — compilation fails.
CI flags all four references (Lines 39, 51, 59, 68) as undefined: agent.PermissionModePlan. The new plan permission mode must be added to the agent package (alongside PermissionModeAuto/PermissionModeUnsafe) before this compiles.
Also applies to: 51-51, 59-59, 68-68
🧰 Tools
🪛 GitHub Check: Security & code health
[failure] 39-39:
undefined: agent.PermissionModePlan
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/tui/plan_command.go` at line 39, Add the missing PermissionModePlan
constant to the agent package alongside PermissionModeAuto and
PermissionModeUnsafe, using the existing permission-mode type and value
conventions; verify all references in plan_command.go compile successfully.
Source: Pipeline failures
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/planmode/planmode.go`:
- Around line 81-84: Use owner-only permissions for plan storage: change the
directory mode in the plan-writing function to 0o700 and the file mode passed to
os.WriteFile to 0o600, ensuring existing plan paths are also not left broadly
accessible.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 403fa48e-d25a-4818-9899-41686bac7419
📒 Files selected for processing (1)
internal/planmode/planmode.go
- restrict plan file permissions to owner only (0o700 dir, 0o600 file) - surface editor failures from /plan open in the transcript - simplify fileExists to return a bool - collapse redundant plan path resolution in planText
Vasanthdev2004
left a comment
Vasanthdev2004
left a comment
There was a problem hiding this comment.
The scoping on plan-file editing is sound — /plan open only opens the single deterministic .zero/plans/<session>.md (path contained via filepath.Rel checks, never clobbered if it exists) as a user-initiated editor session, not an agent write grant, so it doesn't punch a hole in the read-only gate. The on/off toggle and the run-active guard look right.
The blocker: this PR doesn't build against main. plan_command.go references agent.PermissionModePlan (lines 39/51/59/68), which is introduced by the still-open #642 and isn't in this diff — go build ./... and the tui tests fail with undefined: agent.PermissionModePlan. Rebase onto #642's branch (or wait for #642 to land) and I'll take another pass.
Minor: the doc comment on handlePlanCommand credits an external tool as the design source — worth dropping so the behavior stands on its own.
|
@gnanam1990 flagging this one — it depends on #642 landing first (it doesn't build against main until PermissionModePlan exists). No rush, but once #642 merges a review here would help move it forward. |
2 participants
This PR adds the /plan slash command to toggle read-only plan mode in the TUI, view the plan, and edit the plan file in /, and migrates editor spawning to tea.ExecProcess under Bubbletea v2 to resolve compilation and runner hangs.
Summary by CodeRabbit
/plan,/plan off,/plan exit, and/plan open./plan openopens the plan in the configured editor, temporarily suspends the TUI, then resumes afterward.