feat: TUI, MCP server, session manager & profiles rename #23
Conversation
- Interactive TUI with bubbletea: Projects/Profiles/Settings tabs, left-right split panels, project detail with git status - MCP server over stdio with 6 tools (list_projects, add_project, remove_project, list_profiles, switch_profile, get_project_info) - Session manager: launch Claude in separate terminal windows, multi-instance per project, PID tracking, configurable terminal - JSON output mode via --json flag - Rename "Configs" to "Profiles" throughout codebase with backward compatible JSON deserialization (old "configs" field auto-migrates) - New Settings tab for Terminal, Default Behavior, Skip Permissions - CI workflows updated to Go 1.24 Closes #21, closes #19, closes #16
📝 WalkthroughWalkthroughThis PR upgrades Go to 1.24, migrates configuration from "configs" to "profiles" (with migration support), adds an interactive Bubble Tea TUI, multi-session terminal session manager (platform-specific launchers), an MCP stdio server with tools, JSON output mode, and extensive README updates. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant TUI as TUI Model
participant Config as Config Manager
participant Session as Session Manager
participant Terminal as Terminal Launcher
User->>TUI: launch (TTY)
TUI->>Config: LoadProfiles()
Config-->>TUI: profiles list
User->>TUI: select project + profile + Start
TUI->>Config: SetDefault(profile) / Save
TUI->>Session: StartSession(project, path, args, env)
Session->>Terminal: openInTerminal(sessionID, dir, args, env, terminal)
Terminal-->>Session: PID
Session->>Session: monitor process (async)
Session-->>TUI: session started (status)
User->>TUI: Focus / Kill
TUI->>Session: FocusSession() / KillSession(id)
Session->>Terminal: focusTerminalWindow() / killProcess(pid)
sequenceDiagram
actor Client
participant MCP as MCP Server (stdio)
participant Tools as Tool Handlers
participant Config as Config Manager
Client->>MCP: call list_projects
MCP->>Tools: listProjectsHandler()
Tools->>Config: ListProjects()
Config-->>Tools: []ProjectInfo
Tools-->>MCP: result JSON
MCP-->>Client: tool result
Client->>MCP: call switch_profile(target)
MCP->>Tools: switchProfileHandler(target)
Tools->>Config: LoadConfig()
Tools->>Config: SetDefault(target) / Save
Tools-->>MCP: success + new default
MCP-->>Client: tool result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Rewrite README to document new features: interactive TUI with three tabs, MCP server tools, session manager, terminal configuration, profiles rename, and settings tab. Update config format from "configs" to "profiles" with backward compat note. Update Go requirement to 1.24 and project structure.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/commands/cobra.go (1)
46-55:⚠️ Potential issue | 🟡 MinorUsage string still says
[config-name]— should be[profile-name].The PR renames "configs" to "profiles" throughout, but
TestCmd.Usestill reads"test [config-name]".Proposed fix
var TestCmd = &cobra.Command{ - Use: "test [config-name]", + Use: "test [profile-name]", Short: "Test API configuration", - Long: "Test API connectivity for all configurations or a specific one", + Long: "Test API connectivity for all profiles or a specific one",
🤖 Fix all issues with AI agents
In `@internal/commands/commands.go`:
- Around line 1487-1496: In RunServe, avoid fragile string comparison on
err.Error() when calling mcpserver.RunServer(); instead detect the expected
EOF/closing condition by using errors.Is or errors.As with the SDK's typed error
(or if none exist, use strings.Contains(err.Error(), "server is closing") or
strings.Contains(err.Error(), "EOF")) and only suppress exit for that condition;
update the error check in RunServe to use errors.Is/As against the SDK's
sentinel error (or fallback to strings.Contains) before calling
ui.ShowError/os.Exit so the logic remains robust to message changes.
In `@internal/config/config.go`:
- Around line 398-423: BuildClaudeCmd currently calls
SetEnvironmentVarsWithConfig(&selected) which mutates the host process
environment; instead have BuildClaudeCmd construct the subprocess environment
(like ClaudeCmdSpec does) and assign it to cmd.Env so only the child inherits
the vars. Modify BuildClaudeCmd to gather the env map for the chosen profile
(selected) and merge it with os.Environ(), apply
ShouldSkipPermissionsWithConfig(&selected, cfg) to set args, then set cmd.Env =
mergedEnvSlice before returning; remove the call to
SetEnvironmentVarsWithConfig(&selected) to avoid side effects on the current
process.
In `@internal/session/session.go`:
- Around line 141-153: GetRunningByProject and RunningCount read session.Status
while only holding Manager.mu, causing a data race because the monitor goroutine
writes session.Status under the session's own mutex; to fix, acquire the
session-level lock (s.mu) around reads of s.Status in GetRunningByProject and
RunningCount (e.g., lock s.mu, read s.Status into a local var, unlock s.mu) so
you synchronize with the monitor goroutine that writes Status, or alternatively
convert Status to an atomic type if you prefer lock-free access.
- Around line 106-117: The background monitor goroutine started in StartSession
that polls isProcessAlive leaks because there's no cancellation; change
StartSession to create a cancellable context (or done channel) stored on the
session (e.g., s.cancel or s.ctx) and have the goroutine select on a
ticker/context.Done() alongside the isProcessAlive polling so it can exit when
canceled; ensure KillSession and KillByProject call s.cancel() when terminating
a session so the monitor stops; also fix concurrent access to s.Status by
protecting reads in GetRunningByProject and RunningCount with s.mu (or convert
status to an atomic/locked accessor) to match the writes guarded by s.mu.
In `@internal/session/terminal_darwin.go`:
- Around line 61-81: The AppleScript strings in openTerminalApp and openITerm
interpolate raw values and break when those values contain quotes; fix by
escaping embedded quotes before formatting: in openTerminalApp, sanitize
scriptPath by replacing each single-quote (') with the shell-escaped sequence
'\'' so the resulting bash '...'-wrapped path remains valid; in openITerm,
sanitize sessionID by escaping backslashes and double-quotes (replace \ with \\
and " with \") so the AppleScript double-quoted string "codes: %s" is not
broken. Apply these transforms to the values used in fmt.Sprintf in
openTerminalApp and openITerm (or use a helper escape function) before building
the appleScript and calling exec.Command.
In `@internal/tui/model.go`:
- Around line 243-253: The closure passed as the tea.Cmd mutates m.cfg
(m.cfg.Default = ...) and calls config.SaveConfig(...) which runs in a separate
goroutine causing a data race with renderHeader; instead have the Cmd only
return a profileSwitchedMsg (carry the selected profile name or entire cfg if
needed) and perform the mutation and SaveConfig inside the Update handler that
processes profileSwitchedMsg; update the branch in the viewProfiles case to
return a Cmd that does not touch m.cfg, and modify the Update switch handling
for profileSwitchedMsg to set m.cfg.Default and call config.SaveConfig.
- Around line 440-447: The Enter handling calls m.sessionMgr.FocusSession()
without specifying which session to focus, so the selected session
(running[m.sessionCursor]) is not being targeted; change the call to pass the
selected session's ID (e.g., call
m.sessionMgr.FocusSession(running[m.sessionCursor].ID)) and update the
SessionMgr method signature and implementation (or add a new FocusSessionByID(id
string) that uses the session ID) so the OS-specific focus logic activates the
correct terminal window/session rather than just the terminal app; ensure the
call site uses sessionCursor and running[] ID like in KillSession.
In `@README.md`:
- Line 115: Update the documented Claude Code MCP config path: replace the
incorrect path "~/.claude/claude_desktop_config.json" with the correct
user-scope path "~/.claude.json" and mention the project-scope path ".mcp.json"
located at the project root; also add the usage hint for the management command
"claude mcp add --scope [user|project]" so readers know how to add config in
either scope.
🟡 Minor comments (13)
internal/tui/settings.go-27-73 (1)
27-73:⚠️ Potential issue | 🟡 MinorCustom terminal values are silently lost when cycling options.
config.TerminalOptions()returns["terminal", "iterm", "warp"]. If a user has a custom terminal command in their config (e.g.,"alacritty"),newSettingsModelcorrectly picks it up (line 34-36), but theoptionsslice doesn't include it. When the user cycles the setting in the TUI, the custom value will be replaced by one of the three built-in options with no way to restore it.Consider appending the current value to
optionsif it's not already present.internal/session/terminal_unix.go-22-29 (1)
22-29:⚠️ Potential issue | 🟡 MinorMisleading comment: no SIGKILL fallback is implemented.
The doc comment says "sends SIGTERM to the process, then SIGKILL if needed" but the implementation only sends SIGTERM.
♻️ Fix the comment to match behavior
-// killProcess sends SIGTERM to the process, then SIGKILL if needed. +// killProcess sends SIGTERM to the process.internal/mcp/tools.go-65-70 (1)
65-70:⚠️ Potential issue | 🟡 Minor
removeProjectHandlerdoesn't validate empty name, unlikeaddProjectHandler.
addProjectHandler(line 46) checks for empty inputs, butremoveProjectHandleraccepts an emptyNameand silently succeeds. Add a validation check for consistency.♻️ Suggested fix
func removeProjectHandler(ctx context.Context, req *mcpsdk.CallToolRequest, input removeProjectInput) (*mcpsdk.CallToolResult, removeProjectOutput, error) { + if input.Name == "" { + return nil, removeProjectOutput{}, fmt.Errorf("name is required") + } if err := config.RemoveProject(input.Name); err != nil {internal/tui/settings.go-103-103 (1)
103-103:⚠️ Potential issue | 🟡 Minor
%-20spadding won't align correctly on ANSI-styled strings.
labelis already rendered through lipgloss (line 89), so it contains ANSI escape sequences.fmt.Sprintf("%-20s", label)pads based on byte/rune count including escape codes, producing misaligned columns. The same issue applies to line 109.Consider using
lipgloss.Width()to compute visible width and pad manually, or pad the raw text before styling.♻️ Suggested approach
- b.WriteString(fmt.Sprintf("%s%-20s %s\n", cursor, label, valueStr)) + padded := item.label + strings.Repeat(" ", max(0, 20-len(item.label))) + b.WriteString(fmt.Sprintf("%s%s %s\n", cursor, labelStyle.Render(padded), valueStr))internal/session/terminal_linux.go-42-49 (1)
42-49:⚠️ Potential issue | 🟡 MinorCustom terminal fallback hardcodes
-eflag, which doesn't work for all emulators.When a user-specified
terminalis configured, the code always invokes it with-e bash scriptPath(line 44). This works for some emulators (e.g., xterm, alacritty) but fails for others (e.g., gnome-terminal needs--, kitty doesn't use-e). Meanwhile, the auto-detection path at lines 52-64 correctly uses per-emulator argument functions.Consider looking up the custom terminal in
terminalEmulatorsfirst and falling back to-eonly if not found:♻️ Suggested approach
if terminal != "" { + // Check if it's a known emulator with specific args + for _, te := range terminalEmulators { + if te.name == terminal { + cmd := exec.Command(te.name, te.args(scriptPath)...) + if err := cmd.Start(); err == nil { + go cmd.Wait() + started = true + } + break + } + } + // Fallback: try generic -e flag + if !started { - cmd := exec.Command(terminal, "-e", "bash", scriptPath) - if err := cmd.Start(); err == nil { - go cmd.Wait() - started = true + cmd := exec.Command(terminal, "-e", "bash", scriptPath) + if err := cmd.Start(); err == nil { + go cmd.Wait() + started = true + } } }internal/tui/projects.go-158-164 (1)
158-164:⚠️ Potential issue | 🟡 MinorKeybinding hint "k: kill" is ambiguous — it means different things per panel.
When not focused (left panel),
kkills all sessions for the project (KillByProjectinmodel.goLine 205). When focused (right panel),kkills only the selected session (KillSessioninmodel.goLine 428). The hints both say "k: kill" without distinguishing this.Consider disambiguating, e.g.
k: kill allvsk: kill session.internal/session/terminal_darwin.go-88-97 (1)
88-97:⚠️ Potential issue | 🟡 Minor
openCustomfallback assumes the terminal supports-eflag.Line 96 passes
-e,"bash",scriptPathto the custom terminal command. Many terminal emulators (e.g., Alacritty, kitty, WezTerm) use different flags to execute a command. This fallback will silently fail for most custom terminals.Consider documenting this limitation or supporting a more flexible execution pattern (e.g.,
$TERMINAL -e bash /path/to/script).internal/output/output.go-20-31 (1)
20-31:⚠️ Potential issue | 🟡 MinorNil
textFnwill panic whenJSONModeis false.The current call site (
output.Print(infos, nil)incommands.go) is guarded byif output.JSONMode, so it's safe today. However, the public API allows any caller to passnilfortextFn, leading to a nil-function panic in non-JSON mode.Proposed defensive guard
func Print(data interface{}, textFn func()) { if JSONMode { out, err := json.MarshalIndent(Result{Success: true, Data: data}, "", " ") if err != nil { PrintError(err) return } fmt.Println(string(out)) return } - textFn() + if textFn != nil { + textFn() + } }internal/commands/cobra.go-292-293 (1)
292-293:⚠️ Potential issue | 🟡 MinorStale comment:
// CompletionCmd generates shell completion scriptsprecedesTerminalCmd.This comment was left over from the old code layout. It should be removed as it's misleading — the actual
CompletionCmddefinition is at Line 332.Proposed fix
-// CompletionCmd generates shell completion scripts // TerminalCmd represents the terminal command var TerminalCmd = &cobra.Command{internal/tui/model.go-195-201 (1)
195-201:⚠️ Potential issue | 🟡 MinorError from
config.RemoveProjectis silently ignored.If the delete fails (e.g. disk I/O error), the UI will still receive
projectDeletedMsgand refresh the list as if it succeeded. Consider propagating the error.Proposed fix
+type projectDeletedMsg struct { + name string + err error +} case msg.String() == "d" && m.state == viewProjects: if item, ok := m.projectList.SelectedItem().(projectItem); ok { return m, func() tea.Msg { - config.RemoveProject(item.info.Name) - return projectDeletedMsg{name: item.info.Name} + err := config.RemoveProject(item.info.Name) + return projectDeletedMsg{name: item.info.Name, err: err} } }internal/session/session.go-71-80 (1)
71-80:⚠️ Potential issue | 🟡 MinorSession IDs contain
#, which can be problematic in file paths and shell contexts.
nextSessionIDproduces IDs like"myapp#1". This#character ends up in PID file names (line 73) and could cause issues with certain shell expansions, Windows file systems, or downstream tooling that parses these IDs.Consider using a safer delimiter such as
_or-.Proposed fix
func (m *Manager) nextSessionID(projectName string) string { m.counter[projectName]++ - return fmt.Sprintf("%s#%d", projectName, m.counter[projectName]) + return fmt.Sprintf("%s-%d", projectName, m.counter[projectName]) }README.md-334-348 (1)
334-348:⚠️ Potential issue | 🟡 MinorAdd a language identifier to the fenced code block.
The project structure code block is missing a language specifier. This triggers a markdownlint warning (MD040) and some renderers won't apply syntax highlighting.
Proposed fix
-``` +```text codes/ ├── cmd/codes/ # Main entry pointinternal/tui/profileform.go-20-24 (1)
20-24:⚠️ Potential issue | 🟡 MinorRemove unused
profileTestResultMsgtype.
profileTestResultMsgis defined but never constructed or used anywhere in the codebase. The async API test completion instead returnsprofileAddedMsg(line 135) with the test results in itstestedandstatusfields. Delete lines 20–24.
🧹 Nitpick comments (22)
internal/tui/styles.go (1)
5-74: Clean, centralized style definitions.The hardcoded light foreground colors (e.g.,
#E5E7EBon Line 44) assume a dark terminal background. On light-themed terminals,detailValueStyletext will be nearly invisible. Consider usinglipgloss.AdaptiveColorfor key foreground values to handle both light and dark terminals.Example using AdaptiveColor
detailValueStyle = lipgloss.NewStyle(). - Foreground(lipgloss.Color("#E5E7EB")) + Foreground(lipgloss.AdaptiveColor{Light: "#374151", Dark: "#E5E7EB"})internal/tui/keys.go (1)
40-48:Helpbinding is defined but never included inShortHelporFullHelp.The
?/ help binding on Lines 40-43 won't be discoverable by users since it's absent from both help methods. If it's functional, add it toShortHelp; otherwise consider removing the dead definition.Include Help in ShortHelp
func (k keyMap) ShortHelp() []key.Binding { - return []key.Binding{k.Quit, k.Tab, k.Enter, k.Add, k.Delete} + return []key.Binding{k.Quit, k.Tab, k.Enter, k.Add, k.Delete, k.Help} }internal/tui/addform.go (2)
59-66: Both tab directions produce the same movement — coincidentally correct for 2 fields but fragile.
(m.focused + 1) % 2is used for both forward (tab/down) and backward (shift+tab/up) navigation. This works only because there are exactly two fields. If a third field is ever added, backward navigation will break.Make direction explicit for future-proofing
case "tab", "down": - m.focused = (m.focused + 1) % 2 + m.focused = (m.focused + 1) % 2 // forward m.focusInput() return m, nil case "shift+tab", "up": - m.focused = (m.focused + 1) % 2 + m.focused = (m.focused - 1 + 2) % 2 // backward m.focusInput() return m, nil
67-86: No path existence validation before submission.The form accepts any string as
pathwithout verifying it exists on disk. If the path is invalid, the error will surface later. Consider adding anos.Statcheck here to give the user immediate feedback, similar to how name/path emptiness is checked.go.mod (1)
41-41:golang.org/x/textv0.3.8 is outdated; consider updating to a recent version.This dependency is from October 2022 and the latest version is v0.34.0 (Feb 2026). While v0.3.8 itself contains the fix for CVE-2022-32149 (a DoS vulnerability in language parsing), updating to a recent version ensures you have all subsequent bug fixes and improvements. Run
go get -u golang.org/x/textor checkgovulncheckfor any other applicable vulnerabilities.cmd/codes/main.go (3)
53-58: TUI errors are silently swallowed.If
tui.Run()fails, the process exits with code 1 but prints nothing. Users won't know what went wrong.♻️ Suggested fix
if term.IsTerminal(int(os.Stdin.Fd())) { if err := tui.Run(); err != nil { + fmt.Fprintf(os.Stderr, "Error: %v\n", err) os.Exit(1) }
42-50:--jsonon the root command silently ignores subcommand intent.When
--jsonis passed, the rootRunhandler unconditionally callsRunProjectList()and returns. This meanscodes --jsonalways dumps the project list, regardless of any other flags or future expectations. This is fine as a starting point, but consider documenting this behavior or making it more explicit (e.g., acodes projects --jsonsubcommand).
70-73: Consider movingPersistentPreRunto subcommands if any define their own hooks.Cobra does not chain
PersistentPreRun— if a subcommand defines its own, it replaces the parent's hook entirely. Currently, none of your subcommands definePreRunorPersistentPreRun, so this works fine. However, if future subcommands need their own pre-execution logic, they would need to explicitly call the parent's hook to preserve theJSONModepropagation. To avoid this dependency, consider settingJSONModewithin individual subcommands that need it, or documenting that all subcommands must chain the parent hook if they define their ownPersistentPreRun.internal/mcp/server.go (1)
50-50: No graceful shutdown on signals.
context.Background()means the server ignores SIGTERM/SIGINT. For a stdio-based server this is often acceptable (EOF on stdin triggers shutdown), but worth noting if the server holds any state or needs cleanup.internal/session/terminal_linux.go (1)
72-84: PID file polling with no cleanup on timeout.If the 5-second timeout is reached, the function returns an error but leaves the script file on disk (only the
!startedpath at line 68 removes it). The script's owntrap cleanup EXITshould eventually handle this, but if the terminal process hangs before executing the trap, the temp file leaks.internal/session/terminal_unix.go (1)
48-52: Environment variable keys are interpolated into the script without sanitization.Line 51 writes
export %s='%s'wherekis inserted directly. While values are properly single-quote-escaped, a malicious or malformed key (e.g., containing=, spaces, or shell metacharacters) could produce a broken or exploitable export statement. Consider validating that keys match^[a-zA-Z_][a-zA-Z0-9_]*$.internal/mcp/tools.go (1)
45-53: No validation that the project path exists or is absolute.The
addProjectInput.Pathjsonschema tag says "Absolute path to project directory" but the handler doesn't verify this. A relative path or non-existent directory would be silently persisted. Consider at minimum checkingfilepath.IsAbs(input.Path).internal/output/output.go (1)
34-42:PrintErroralways callsos.Exit(1), preventing deferred cleanup and making the function untestable.This is fine for a simple CLI, but be aware that any deferred calls in the caller's stack (e.g., file handles, temp file cleanup) will be skipped. If you ever need testability or graceful shutdown, consider returning an error or a status code instead.
internal/tui/projects.go (2)
136-144: Asymmetric CLAUDE.md status display.When present:
"CLAUDE.md ✓". When absent: just"✗"with no label. This makes the negative case cryptic in context. Consider"CLAUDE.md ✗"or"no CLAUDE.md"for symmetry.Proposed fix
claudeStatus := statusErrorStyle.Render("✗") if info.HasClaudeMD { claudeStatus = statusOkStyle.Render("CLAUDE.md ✓") + } else { + claudeStatus = statusErrorStyle.Render("CLAUDE.md ✗") }
25-44:loadProjectssilently swallows errors.If
config.ListProjects()fails (e.g. corrupt config file), the user sees an empty project list with no feedback. Consider logging or surfacing the error.internal/tui/model.go (1)
73-106: Multiple errors silently ignored during initialization.
loadProfiles()error (Line 86),config.LoadConfig()error (Line 95) are both discarded. If the config file is corrupt or missing, the TUI launches with empty state and no feedback. At minimum, consider settingm.errso the error bar displays something.internal/session/terminal_darwin.go (1)
46-58: Busy-wait PID polling with no cleanup on timeout.On timeout (Line 58), the function returns an error but leaves the script file on disk. The script is only removed on launch error (Line 42), not on PID-file timeout. The launched terminal process may still be running with the script file needed.
internal/config/config.go (1)
544-580: Git helper functions spawn separate processes — consider performance with many projects.
GetProjectInfospawns 3 git processes per project (branch, status, recent branches). InloadProjects(TUI), this is called for every configured project at startup. With many projects this could noticeably delay TUI launch.Not blocking, but worth noting for future optimization (e.g., combining into a single git invocation or adding caching).
internal/session/session.go (2)
155-177: Nested lock ordering (Manager.mu → Session.mu) is correct butKillSessionsilently ignores an unknown session ID.Returning
nilfor an unknown ID (line 162) may hide bugs at call sites. Consider returning a sentinel error or at least documenting the behavior so callers don't mistake "session not found" for success.
238-247: Deleting from the map while iterating is safe in Go, but exited sessions are never cleaned automatically.
CleanExitedmust be called explicitly. If no caller invokes it, thesessionsmap grows indefinitely with stale exited entries. Consider callingCleanExitedperiodically (e.g., insideRefreshStatus) or document the expectation clearly.internal/tui/profileform.go (2)
74-141:Updateuses a value receiver but calls pointer-receiver methodfocusProfileInput— works but is inconsistent.
Updatehas a value receiver (func (m profileFormModel) Update(...)) whilefocusProfileInputhas a pointer receiver (func (m *profileFormModel) ...). This works becausemis addressable insideUpdate, but it's an unusual pattern in Bubble Tea models. More importantly, the Bubble Tea convention is to use value receivers forUpdateand return the modified model, which you do — so this is fine functionally. Just flag for awareness.
157-213: Repeated label-rendering pattern could be extracted but is acceptable at this scale.The focused/unfocused label pattern is repeated four times (Name, URL, Token, Skip). A small helper like
renderLabel(label string, focused bool)would reduce duplication but this is a minor nit for a 4-field form.
| // RunServe starts the MCP server mode. | ||
| func RunServe() { | ||
| if err := mcpserver.RunServer(); err != nil { | ||
| // EOF is expected when client disconnects | ||
| if err.Error() != "server is closing: EOF" { | ||
| ui.ShowError("MCP server error", err) | ||
| os.Exit(1) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Fragile error comparison using string matching.
err.Error() != "server is closing: EOF" will silently break if the MCP SDK changes its error message. Prefer checking with errors.Is, errors.As, or errors.Unwrap if the SDK exposes typed errors, or at minimum use strings.Contains.
Proposed fix (minimal)
func RunServe() {
if err := mcpserver.RunServer(); err != nil {
- if err.Error() != "server is closing: EOF" {
+ // EOF is expected when the client disconnects.
+ if !errors.Is(err, io.EOF) && !strings.Contains(err.Error(), "EOF") {
ui.ShowError("MCP server error", err)
os.Exit(1)
}
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // RunServe starts the MCP server mode. | |
| func RunServe() { | |
| if err := mcpserver.RunServer(); err != nil { | |
| // EOF is expected when client disconnects | |
| if err.Error() != "server is closing: EOF" { | |
| ui.ShowError("MCP server error", err) | |
| os.Exit(1) | |
| } | |
| } | |
| } | |
| // RunServe starts the MCP server mode. | |
| func RunServe() { | |
| if err := mcpserver.RunServer(); err != nil { | |
| // EOF is expected when the client disconnects. | |
| if !errors.Is(err, io.EOF) && !strings.Contains(err.Error(), "EOF") { | |
| ui.ShowError("MCP server error", err) | |
| os.Exit(1) | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
In `@internal/commands/commands.go` around lines 1487 - 1496, In RunServe, avoid
fragile string comparison on err.Error() when calling mcpserver.RunServer();
instead detect the expected EOF/closing condition by using errors.Is or
errors.As with the SDK's typed error (or if none exist, use
strings.Contains(err.Error(), "server is closing") or
strings.Contains(err.Error(), "EOF")) and only suppress exit for that condition;
update the error check in RunServe to use errors.Is/As against the SDK's
sentinel error (or fallback to strings.Contains) before calling
ui.ShowError/os.Exit so the logic remains robust to message changes.
| // BuildClaudeCmd creates an *exec.Cmd for launching Claude Code in the given directory. | ||
| // It loads the current config, sets environment variables, and applies skip-permissions if configured. | ||
| func BuildClaudeCmd(dir string) *exec.Cmd { | ||
| cfg, _ := LoadConfig() | ||
|
|
||
| var selected APIConfig | ||
| if cfg != nil { | ||
| for _, c := range cfg.Profiles { | ||
| if c.Name == cfg.Default { | ||
| selected = c | ||
| break | ||
| } | ||
| } | ||
| } | ||
|
|
||
| SetEnvironmentVarsWithConfig(&selected) | ||
|
|
||
| var args []string | ||
| if ShouldSkipPermissionsWithConfig(&selected, cfg) { | ||
| args = []string{"--dangerously-skip-permissions"} | ||
| } | ||
|
|
||
| cmd := exec.Command("claude", args...) | ||
| cmd.Dir = dir | ||
| return cmd | ||
| } |
There was a problem hiding this comment.
BuildClaudeCmd has a side effect: it mutates the current process environment.
Line 413 calls SetEnvironmentVarsWithConfig(&selected), which calls os.Setenv on the current process. This permanently modifies the host process's environment. In contrast, ClaudeCmdSpec (Line 427) returns the env map without side effects.
If the intent is for the subprocess to inherit these vars, consider setting them on cmd.Env instead of the process environment, or document this side effect clearly.
Proposed fix: set env on cmd instead of process
func BuildClaudeCmd(dir string) *exec.Cmd {
cfg, _ := LoadConfig()
var selected APIConfig
if cfg != nil {
for _, c := range cfg.Profiles {
if c.Name == cfg.Default {
selected = c
break
}
}
}
- SetEnvironmentVarsWithConfig(&selected)
+ envVars := GetEnvironmentVars(&selected)
var args []string
if ShouldSkipPermissionsWithConfig(&selected, cfg) {
args = []string{"--dangerously-skip-permissions"}
}
cmd := exec.Command("claude", args...)
cmd.Dir = dir
+ cmd.Env = os.Environ()
+ for k, v := range envVars {
+ cmd.Env = append(cmd.Env, fmt.Sprintf("%s=%s", k, v))
+ }
return cmd
}🤖 Prompt for AI Agents
In `@internal/config/config.go` around lines 398 - 423, BuildClaudeCmd currently
calls SetEnvironmentVarsWithConfig(&selected) which mutates the host process
environment; instead have BuildClaudeCmd construct the subprocess environment
(like ClaudeCmdSpec does) and assign it to cmd.Env so only the child inherits
the vars. Modify BuildClaudeCmd to gather the env map for the chosen profile
(selected) and merge it with os.Environ(), apply
ShouldSkipPermissionsWithConfig(&selected, cfg) to set args, then set cmd.Env =
mergedEnvSlice before returning; remove the call to
SetEnvironmentVarsWithConfig(&selected) to avoid side effects on the current
process.
| go func() { | ||
| for { | ||
| time.Sleep(2 * time.Second) | ||
| if !isProcessAlive(pid) { | ||
| s.mu.Lock() | ||
| s.Status = StatusExited | ||
| s.mu.Unlock() | ||
| os.Remove(pidFilePath(id)) | ||
| return | ||
| } | ||
| } | ||
| }() |
There was a problem hiding this comment.
Background monitor goroutine leaks if the process never exits.
The polling goroutine runs indefinitely with no cancellation mechanism. If the spawned process becomes a zombie or the Manager is discarded, this goroutine will never stop. Consider accepting a context.Context (or a done channel) and selecting on it alongside the ticker so callers can shut the monitor down.
Additionally, this goroutine writes s.Status under s.mu, but several reader methods (GetRunningByProject, RunningCount) read s.Status without acquiring s.mu — see the related comment below.
Proposed fix — add cancellation support
// Session represents a single Claude Code session.
type Session struct {
ID string
ProjectName string
ProjectPath string
Status Status
PID int
StartedAt time.Time
+ cancel context.CancelFunc // stops the monitor goroutine
mu sync.Mutex
}In StartSession, create a cancellable context and pass it to the goroutine:
+ ctx, cancel := context.WithCancel(context.Background())
s := &Session{
ID: id,
ProjectName: name,
ProjectPath: path,
Status: StatusRunning,
PID: pid,
StartedAt: time.Now(),
+ cancel: cancel,
}
m.sessions[id] = s
go func() {
+ ticker := time.NewTicker(2 * time.Second)
+ defer ticker.Stop()
for {
- time.Sleep(2 * time.Second)
- if !isProcessAlive(pid) {
- s.mu.Lock()
- s.Status = StatusExited
- s.mu.Unlock()
- os.Remove(pidFilePath(id))
- return
+ select {
+ case <-ctx.Done():
+ return
+ case <-ticker.C:
+ if !isProcessAlive(pid) {
+ s.mu.Lock()
+ s.Status = StatusExited
+ s.mu.Unlock()
+ os.Remove(pidFilePath(id))
+ return
+ }
}
}
}()Then call s.cancel() in KillSession / KillByProject before or after killing the process.
🤖 Prompt for AI Agents
In `@internal/session/session.go` around lines 106 - 117, The background monitor
goroutine started in StartSession that polls isProcessAlive leaks because
there's no cancellation; change StartSession to create a cancellable context (or
done channel) stored on the session (e.g., s.cancel or s.ctx) and have the
goroutine select on a ticker/context.Done() alongside the isProcessAlive polling
so it can exit when canceled; ensure KillSession and KillByProject call
s.cancel() when terminating a session so the monitor stops; also fix concurrent
access to s.Status by protecting reads in GetRunningByProject and RunningCount
with s.mu (or convert status to an atomic/locked accessor) to match the writes
guarded by s.mu.
| // GetRunningByProject returns running sessions for a given project. | ||
| func (m *Manager) GetRunningByProject(name string) []*Session { | ||
| m.mu.RLock() | ||
| defer m.mu.RUnlock() | ||
|
|
||
| var result []*Session | ||
| for _, s := range m.sessions { | ||
| if s.ProjectName == name && s.Status == StatusRunning { | ||
| result = append(result, s) | ||
| } | ||
| } | ||
| return result | ||
| } |
There was a problem hiding this comment.
Data race: s.Status read without s.mu while the monitor goroutine writes it under s.mu.
GetRunningByProject (line 148) and RunningCount (line 216) read s.Status while holding only m.mu.RLock. The background monitor goroutine mutates s.Status under s.mu. This is a textbook data race that go test -race will flag.
Lock s.mu around the Status read, or switch Status to an atomic.Int32.
Proposed fix (lock approach)
func (m *Manager) GetRunningByProject(name string) []*Session {
m.mu.RLock()
defer m.mu.RUnlock()
var result []*Session
for _, s := range m.sessions {
+ s.mu.Lock()
+ running := s.Status == StatusRunning
+ s.mu.Unlock()
- if s.ProjectName == name && s.Status == StatusRunning {
+ if s.ProjectName == name && running {
result = append(result, s)
}
}
return result
} func (m *Manager) RunningCount() int {
m.mu.RLock()
defer m.mu.RUnlock()
count := 0
for _, s := range m.sessions {
+ s.mu.Lock()
+ running := s.Status == StatusRunning
+ s.mu.Unlock()
- if s.Status == StatusRunning {
+ if running {
count++
}
}
return count
}Also applies to: 209-221
🤖 Prompt for AI Agents
In `@internal/session/session.go` around lines 141 - 153, GetRunningByProject and
RunningCount read session.Status while only holding Manager.mu, causing a data
race because the monitor goroutine writes session.Status under the session's own
mutex; to fix, acquire the session-level lock (s.mu) around reads of s.Status in
GetRunningByProject and RunningCount (e.g., lock s.mu, read s.Status into a
local var, unlock s.mu) so you synchronize with the monitor goroutine that
writes Status, or alternatively convert Status to an atomic type if you prefer
lock-free access.
| } else if m.state == viewProfiles { | ||
| if item, ok := m.profileList.SelectedItem().(profileItem); ok { | ||
| return m, func() tea.Msg { | ||
| if m.cfg != nil { | ||
| m.cfg.Default = item.cfg.Name | ||
| config.SaveConfig(m.cfg) | ||
| } | ||
| return profileSwitchedMsg{name: item.cfg.Name} | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Data race: m.cfg is mutated inside a tea.Cmd goroutine.
Bubble Tea executes tea.Cmd functions in separate goroutines. This closure writes to m.cfg.Default (Line 247) and calls SaveConfig(m.cfg) (Line 248) concurrently with the main Update/View loop, which reads m.cfg.Default in renderHeader (Line 559). This is a data race.
Move the config mutation into the message handler instead:
Proposed fix
} else if m.state == viewProfiles {
if item, ok := m.profileList.SelectedItem().(profileItem); ok {
+ name := item.cfg.Name
return m, func() tea.Msg {
- if m.cfg != nil {
- m.cfg.Default = item.cfg.Name
- config.SaveConfig(m.cfg)
+ cfg, err := config.LoadConfig()
+ if err == nil {
+ cfg.Default = name
+ config.SaveConfig(cfg)
}
- return profileSwitchedMsg{name: item.cfg.Name}
+ return profileSwitchedMsg{name: name}
}
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| } else if m.state == viewProfiles { | |
| if item, ok := m.profileList.SelectedItem().(profileItem); ok { | |
| return m, func() tea.Msg { | |
| if m.cfg != nil { | |
| m.cfg.Default = item.cfg.Name | |
| config.SaveConfig(m.cfg) | |
| } | |
| return profileSwitchedMsg{name: item.cfg.Name} | |
| } | |
| } | |
| } | |
| } else if m.state == viewProfiles { | |
| if item, ok := m.profileList.SelectedItem().(profileItem); ok { | |
| name := item.cfg.Name | |
| return m, func() tea.Msg { | |
| cfg, err := config.LoadConfig() | |
| if err == nil { | |
| cfg.Default = name | |
| config.SaveConfig(cfg) | |
| } | |
| return profileSwitchedMsg{name: name} | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
In `@internal/tui/model.go` around lines 243 - 253, The closure passed as the
tea.Cmd mutates m.cfg (m.cfg.Default = ...) and calls config.SaveConfig(...)
which runs in a separate goroutine causing a data race with renderHeader;
instead have the Cmd only return a profileSwitchedMsg (carry the selected
profile name or entire cfg if needed) and perform the mutation and SaveConfig
inside the Update handler that processes profileSwitchedMsg; update the branch
in the viewProfiles case to return a Cmd that does not touch m.cfg, and modify
the Update switch handling for profileSwitchedMsg to set m.cfg.Default and call
config.SaveConfig.
| case "enter": | ||
| if item, ok := m.projectList.SelectedItem().(projectItem); ok { | ||
| running := m.sessionMgr.GetRunningByProject(item.info.Name) | ||
| if m.sessionCursor < len(running) { | ||
| // Focus existing session terminal | ||
| m.sessionMgr.FocusSession() | ||
| m.focus = focusLeft | ||
| return m, nil |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find the FocusSession method signature and implementation
ast-grep --pattern 'func ($_ $_) FocusSession($$$) {
$$$
}'Repository: ourines/codes
Length of output: 222
🏁 Script executed:
#!/bin/bash
# Find focusTerminalWindow implementation
ast-grep --pattern 'func focusTerminalWindow($$$) {
$$$
}'Repository: ourines/codes
Length of output: 1178
🏁 Script executed:
#!/bin/bash
# Find GetRunningByProject implementation and signature
ast-grep --pattern 'func ($_ $_) GetRunningByProject($$$) {
$$$
}'Repository: ourines/codes
Length of output: 39
🏁 Script executed:
#!/bin/bash
# Search for GetRunningByProject using rg
rg "GetRunningByProject" -A 5 -B 2Repository: ourines/codes
Length of output: 3228
🏁 Script executed:
#!/bin/bash
# Search for all FocusSession-related methods
rg "FocusSession" -B 2 -A 8Repository: ourines/codes
Length of output: 1261
FocusSession() doesn't accept a session ID parameter — the selected session isn't being focused.
At line 445, when the user presses Enter to open the selected session, the code calls m.sessionMgr.FocusSession() without arguments. While the selected session is available via running[m.sessionCursor] and has an .ID field (as shown in the delete case with KillSession(running[m.sessionCursor].ID)), FocusSession() takes no parameters and only activates the terminal application via osascript. This means only the terminal app is brought to the foreground, not the specific session/window selected by the user, contradicting the "Enter: open" UX hint.
🤖 Prompt for AI Agents
In `@internal/tui/model.go` around lines 440 - 447, The Enter handling calls
m.sessionMgr.FocusSession() without specifying which session to focus, so the
selected session (running[m.sessionCursor]) is not being targeted; change the
call to pass the selected session's ID (e.g., call
m.sessionMgr.FocusSession(running[m.sessionCursor].ID)) and update the
SessionMgr method signature and implementation (or add a new FocusSessionByID(id
string) that uses the session ID) so the OS-specific focus logic activates the
correct terminal window/session rather than just the terminal app; ensure the
call site uses sessionCursor and running[] ID like in KillSession.
| ### `codes completion [shell]` | ||
|
|
||
| Generate shell completion scripts. While `codes init` automatically sets up completion, you can also generate scripts manually. | ||
| **Claude Code MCP config** (`~/.claude/claude_desktop_config.json`): |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Claude Code MCP server configuration file path
💡 Result:
Claude Code MCP server configs live in different files depending on the scope:
- Project scope (shared):
<project-root>/.mcp.json(meant to be committed) [1] - User scope (cross-project, private to you):
~/.claude.json[1] - Local scope (default; only for the current project, private to you): also in
~/.claude.json, stored under your project’s path [1] - Managed (org/admin-controlled):
managed-mcp.jsonin system directories [1]
Tip: claude mcp add --scope project|user|local ... controls which path is used [1].
Correct the Claude Code MCP config file path.
The documented path ~/.claude/claude_desktop_config.json is incorrect for Claude Code. Claude Code MCP configuration uses:
- User scope:
~/.claude.json - Project scope:
<project-root>/.mcp.json
Use claude mcp add --scope [user|project] to manage configuration across scopes. Update the documentation to reflect the correct path(s).
🤖 Prompt for AI Agents
In `@README.md` at line 115, Update the documented Claude Code MCP config path:
replace the incorrect path "~/.claude/claude_desktop_config.json" with the
correct user-scope path "~/.claude.json" and mention the project-scope path
".mcp.json" located at the project root; also add the usage hint for the
management command "claude mcp add --scope [user|project]" so readers know how
to add config in either scope.
…dition - Sanitize session IDs to alphanumeric/underscore/hyphen/dot only - Escape AppleScript strings to prevent injection via project names - Validate shell env var names against POSIX pattern before export - Fix TUI profile switch closure to not mutate shared config pointer
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@internal/session/session.go`:
- Around line 248-257: The loop in Manager.CleanExited reads s.Status without
holding the session's mutex (s.mu), causing a data race; fix it by, while
holding m.mu and iterating m.sessions, acquiring s.mu before reading s.Status
and releasing s.mu afterwards, and only call delete(m.sessions, id) after
confirming Status==StatusExited under s.mu; reference Manager.CleanExited,
m.sessions, s.Status and s.mu so you can locate the loop and add the per-session
locking around the status check and deletion.
In `@internal/tui/model.go`:
- Around line 209-225: The current code replaces m.sessionMgr by calling
session.NewManager(next), leaking previously tracked sessions and their monitor
goroutines; instead add an in-place setter on the Manager (e.g., func (m
*Manager) SetTerminal(terminal string)) that acquires the Manager mutex, updates
its terminal field, and returns; then replace the session.NewManager(next) call
in the key handler with m.sessionMgr.SetTerminal(next) so the existing Manager
continues owning and cleaning its sessions; alternatively, if you must replace
the Manager, call an explicit cleanup/close method on the old session.Manager
(e.g., m.sessionMgr.CloseAll() or m.sessionMgr.Shutdown()) before assigning a
new session.NewManager(next).
- Around line 381-398: When applying a "terminal" setting in Model.applySetting,
update the in-memory session manager as well as the persisted config so new
sessions use the new terminal; after setting cfg.Terminal = value, call the
session manager update (e.g., m.sessionMgr.SetTerminal(value) or equivalent)
before/after config.SaveConfig(cfg). Ensure the Manager interface/implementation
exposes SetTerminal so m.sessionMgr can be updated (alternatively, perform this
sync in the settingChangedMsg handler if you prefer centralizing updates).
🧹 Nitpick comments (3)
internal/session/session.go (1)
209-217:ListSessionsreturns mutable*Sessionpointers to callers — shared mutable state escapes the lock.
ListSessions,GetSessionsByProject, andGetRunningByProjectall return[]*Sessionpointers. Callers (including the TUI'sView()method, which runs concurrently withUpdate()in Bubble Tea) can then reads.Status,s.PID, etc. without any synchronization, creating additional race windows beyond the already-flaggeds.muissue.Consider returning value copies (e.g., a
SessionInfosnapshot struct) instead of raw pointers to prevent races from leaking outside the manager.internal/tui/model.go (2)
258-265: Session error display is cleared on success but never re-cleared on subsequent ticks.When
sessionStartedMsghas a non-nil error,m.erris set. But on the next successful session start, it's cleared. This is fine. However, note thatm.stateis unconditionally set toviewProjectsat line 264 — if the user navigated away (unlikely mid-Cmd, but possible with a slow terminal launch), this would snap them back.This is a minor UX concern rather than a bug.
271-297: Synchronous I/O inUpdatehandlers forprojectAddedMsgandprofileAddedMsg.Lines 272, 284–290, and 295 perform file I/O (
AddProject,LoadConfig,SaveConfig) directly in the synchronousUpdatepath. While these are fast local file operations and unlikely to block noticeably, the pattern is inconsistent with other handlers (e.g., project delete at line 197) that offload I/O totea.Cmdclosures.Not a blocking concern, but worth aligning for consistency if you refactor later.
| // CleanExited removes exited sessions from tracking. | ||
| func (m *Manager) CleanExited() { | ||
| m.mu.Lock() | ||
| defer m.mu.Unlock() | ||
|
|
||
| for id, s := range m.sessions { | ||
| if s.Status == StatusExited { | ||
| delete(m.sessions, id) | ||
| } | ||
| } |
There was a problem hiding this comment.
Data race in CleanExited: s.Status read without s.mu.
Same class of bug as GetRunningByProject and RunningCount — s.Status is read at line 254 without acquiring s.mu, while the monitor goroutine writes it under s.mu.
Proposed fix
func (m *Manager) CleanExited() {
m.mu.Lock()
defer m.mu.Unlock()
for id, s := range m.sessions {
- if s.Status == StatusExited {
+ s.mu.Lock()
+ exited := s.Status == StatusExited
+ s.mu.Unlock()
+ if exited {
delete(m.sessions, id)
}
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // CleanExited removes exited sessions from tracking. | |
| func (m *Manager) CleanExited() { | |
| m.mu.Lock() | |
| defer m.mu.Unlock() | |
| for id, s := range m.sessions { | |
| if s.Status == StatusExited { | |
| delete(m.sessions, id) | |
| } | |
| } | |
| // CleanExited removes exited sessions from tracking. | |
| func (m *Manager) CleanExited() { | |
| m.mu.Lock() | |
| defer m.mu.Unlock() | |
| for id, s := range m.sessions { | |
| s.mu.Lock() | |
| exited := s.Status == StatusExited | |
| s.mu.Unlock() | |
| if exited { | |
| delete(m.sessions, id) | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
In `@internal/session/session.go` around lines 248 - 257, The loop in
Manager.CleanExited reads s.Status without holding the session's mutex (s.mu),
causing a data race; fix it by, while holding m.mu and iterating m.sessions,
acquiring s.mu before reading s.Status and releasing s.mu afterwards, and only
call delete(m.sessions, id) after confirming Status==StatusExited under s.mu;
reference Manager.CleanExited, m.sessions, s.Status and s.mu so you can locate
the loop and add the per-session locking around the status check and deletion.
| case msg.String() == "t" && m.state == viewProjects: | ||
| // Cycle terminal: terminal → iterm → warp → terminal | ||
| options := config.TerminalOptions() | ||
| current := config.GetTerminal() | ||
| if current == "" { | ||
| current = "terminal" | ||
| } | ||
| next := options[0] | ||
| for i, opt := range options { | ||
| if opt == current && i+1 < len(options) { | ||
| next = options[i+1] | ||
| break | ||
| } | ||
| } | ||
| config.SetTerminal(next) | ||
| m.sessionMgr = session.NewManager(next) | ||
| return m, nil |
There was a problem hiding this comment.
Replacing sessionMgr orphans tracked sessions and leaks monitor goroutines.
When the user presses t to cycle the terminal emulator, line 224 creates a brand-new session.Manager, discarding the old one. Any sessions previously tracked (and their background monitor goroutines) become unreachable — running sessions are lost from the UI and the goroutines spin indefinitely.
Instead of replacing the manager, consider adding a method to update the terminal preference in-place (e.g., m.sessionMgr.SetTerminal(next)), or at minimum, kill/clean all sessions on the old manager before replacing it.
Sketch: update terminal in-place
Add to Manager:
func (m *Manager) SetTerminal(terminal string) {
m.mu.Lock()
defer m.mu.Unlock()
m.terminal = terminal
}Then in model.go:
config.SetTerminal(next)
- m.sessionMgr = session.NewManager(next)
+ m.sessionMgr.SetTerminal(next)
return m, nil🤖 Prompt for AI Agents
In `@internal/tui/model.go` around lines 209 - 225, The current code replaces
m.sessionMgr by calling session.NewManager(next), leaking previously tracked
sessions and their monitor goroutines; instead add an in-place setter on the
Manager (e.g., func (m *Manager) SetTerminal(terminal string)) that acquires the
Manager mutex, updates its terminal field, and returns; then replace the
session.NewManager(next) call in the key handler with
m.sessionMgr.SetTerminal(next) so the existing Manager continues owning and
cleaning its sessions; alternatively, if you must replace the Manager, call an
explicit cleanup/close method on the old session.Manager (e.g.,
m.sessionMgr.CloseAll() or m.sessionMgr.Shutdown()) before assigning a new
session.NewManager(next).
| func (m Model) applySetting(key, value string) tea.Cmd { | ||
| return func() tea.Msg { | ||
| cfg, err := config.LoadConfig() | ||
| if err != nil { | ||
| return nil | ||
| } | ||
| switch key { | ||
| case "terminal": | ||
| cfg.Terminal = value | ||
| case "defaultBehavior": | ||
| cfg.DefaultBehavior = value | ||
| case "skipPermissions": | ||
| cfg.SkipPermissions = value == "on" | ||
| } | ||
| config.SaveConfig(cfg) | ||
| return settingChangedMsg{} | ||
| } | ||
| } |
There was a problem hiding this comment.
Settings terminal change doesn't update the session manager.
When the terminal is changed via the Settings tab (applySetting with key "terminal"), only the config file on disk is updated. The in-memory m.sessionMgr still holds the old terminal preference, so new sessions will launch in the wrong terminal.
Compare with the t key handler (line 223–224) which does update the manager. These two paths should be consistent.
Proposed fix
One approach: handle it in the settingChangedMsg handler so the session manager stays in sync after any setting change:
case settingChangedMsg:
m.cfg, _ = config.LoadConfig()
+ if m.cfg != nil {
+ term := m.cfg.Terminal
+ if term == "" {
+ term = "terminal"
+ }
+ m.sessionMgr.SetTerminal(term)
+ }
return m, nilThis assumes SetTerminal is added to Manager as suggested in the other comment.
🤖 Prompt for AI Agents
In `@internal/tui/model.go` around lines 381 - 398, When applying a "terminal"
setting in Model.applySetting, update the in-memory session manager as well as
the persisted config so new sessions use the new terminal; after setting
cfg.Terminal = value, call the session manager update (e.g.,
m.sessionMgr.SetTerminal(value) or equivalent) before/after
config.SaveConfig(cfg). Ensure the Manager interface/implementation exposes
SetTerminal so m.sessionMgr can be updated (alternatively, perform this sync in
the settingChangedMsg handler if you prefer centralizing updates).
1 participant
Summary
list_projects,add_project,remove_project,list_profiles,switch_profile,get_project_info)--jsonflag for scripting"configs"field auto-migrates to"profiles")Key Changes
internal/config/config.goProfilesfield +UnmarshalJSONbackward compatinternal/tui/*.go(8 files)internal/mcp/server.go,tools.gointernal/session/*.go(5 files)internal/output/output.gointernal/commands/*.goTest plan
go build ./...compiles cleanconfig.jsonwith old"configs"field loads correctly (auto-migrates to"profiles")./codeslaunches TUI with Projects / Profiles / Settings tabsaadds newcodes servestarts MCP server,list_profiles/switch_profilework correctlycodes test/codes selectuse updated "Profiles" terminologyCloses #21, closes #19, closes #16
Summary by CodeRabbit
New Features
Documentation
Chores