fix(tmux): validate exec split args and surface pane creation errors#164
Conversation
Greptile SummaryThis PR properly introduces Key changes:
Confidence Score: 4/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant Caller as NewManager / /prepare
participant IE as InitExecPane
participant GAP as GetAvailablePane
participant TCNP as TmuxCreateNewPane
participant BSW as buildSplitWindowArgs
participant tmux as tmux split-window
Caller->>IE: InitExecPane()
IE->>GAP: GetAvailablePane()
GAP-->>IE: TmuxPaneDetails{} (empty)
IE->>TCNP: TmuxCreateNewPane(target, splitArgs)
TCNP->>BSW: buildSplitWindowArgs(target, splitArgs)
Note over BSW: Validate no reserved flags<br/>(-t, -P, -F)<br/>Assemble final arg slice
BSW-->>TCNP: []string or error
TCNP->>tmux: exec split-window ...args
tmux-->>TCNP: paneID (stdout)
TCNP-->>IE: paneID, nil
IE->>GAP: GetAvailablePane() [re-query]
alt pane found in list
GAP-->>IE: TmuxPaneDetails{Id: paneID, ...}
else timing race
GAP-->>IE: TmuxPaneDetails{} (empty)
Note over IE: Fallback: use paneID<br/>returned by TmuxCreateNewPane
end
IE-->>Caller: nil (success)
Last reviewed commit: cb46e48 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| availablePane = m.GetAvailablePane() | ||
| } | ||
| m.ExecPane = &availablePane |
There was a problem hiding this comment.
Silent failure when newly created pane is not discoverable
TmuxCreateNewPane is called and succeeds (no error returned), but the immediately-following GetAvailablePane() call can still return an empty TmuxPaneDetails{} — for example, due to a brief timing window before the pane is listed by list-panes. When that happens, m.ExecPane is set to a pointer to an empty struct where Id == "", and the function returns nil — signalling success to the caller even though the exec pane is essentially unusable. Any subsequent code that accesses m.ExecPane.Id without checking will produce silent failures.
The returned pane ID from TmuxCreateNewPane is also discarded (_), making this second lookup the only way to recover the pane, yet it is never validated.
| availablePane = m.GetAvailablePane() | |
| } | |
| m.ExecPane = &availablePane | |
| availablePane = m.GetAvailablePane() | |
| if availablePane.Id == "" { | |
| return fmt.Errorf("exec pane was created but could not be found afterwards") | |
| } | |
| } | |
| m.ExecPane = &availablePane | |
| return nil |
| for _, arg := range splitArgs { | ||
| if _, reserved := reservedSplitWindowArgs[arg]; reserved { | ||
| return nil, fmt.Errorf("exec_split_args cannot include reserved tmux flag %q", arg) | ||
| } | ||
| args = append(args, arg) |
There was a problem hiding this comment.
Reserved-flag check rejects legitimate argument values
The loop iterates over every element of splitArgs — both option names and their associated values — and compares each against the reserved set. This means a configuration like exec_split_args: ["-c", "-t"] (where -c sets the working directory and "-t" is an unusual but syntactically valid path) would be incorrectly rejected, since "-t" matches the reserved key even though it is a value, not a flag.
A safer approach is to only reject elements that look like flags (i.e. start with -) that appear in the reserved set, skipping elements that are clearly values (not starting with -):
for _, arg := range splitArgs {
if strings.HasPrefix(arg, "-") {
if _, reserved := reservedSplitWindowArgs[arg]; reserved {
return nil, fmt.Errorf("exec_split_args cannot include reserved tmux flag %q", arg)
}
}
args = append(args, arg)
}This still catches -t, -P, -F when used as flags while allowing them to appear as literal values for other flags.
|
Addressed. Changes pushed to this PR:
|
|
@greptile-app |
internal/exec_pane.go
Outdated
| if availablePane.Id == "" { | ||
| return fmt.Errorf("exec pane was created but could not be found afterwards") | ||
| } |
There was a problem hiding this comment.
Unreachable error guard — dead code
The second if availablePane.Id == "" block (lines 39-41) can never be reached, so the error "exec pane was created but could not be found afterwards" is silently swallowed as dead code instead of providing the intended safety net.
Here is why it is unreachable:
TmuxCreateNewPaneeither returns(non-empty-string, nil)or("", non-nil error). It explicitly validatespaneId == ""before returning, so a nil-error return always carries a non-emptypaneID.- The preceding
if err != nil { return … }guard means we only reach line 37 whenpaneID != "". - Line 37 sets
availablePane = system.TmuxPaneDetails{Id: paneID}, guaranteeingavailablePane.Id != "". - Therefore the second
if availablePane.Id == ""is always false.
The fix is to collapse the two checks into one, or drop the redundant guard entirely:
paneID, err := system.TmuxCreateNewPane(m.PaneId, m.Config.Tmux.ExecSplitArgs)
if err != nil {
return fmt.Errorf("failed to create exec pane: %w", err)
}
availablePane = m.GetAvailablePane()
if availablePane.Id == "" {
availablePane = system.TmuxPaneDetails{Id: paneID}
}
// availablePane.Id is guaranteed non-empty here — no second check needed| var splitWindowFlagsWithValues = map[string]struct{}{ | ||
| "-c": {}, | ||
| "-e": {}, | ||
| "-l": {}, | ||
| "-p": {}, | ||
| } |
There was a problem hiding this comment.
splitWindowFlagsWithValues may be incomplete for newer tmux versions
The skipNext mechanism correctly prevents a value that looks like a flag (e.g., -t as the argument to -c) from being rejected. However, splitWindowFlagsWithValues only enumerates four flags (-c, -e, -l, -p). Newer tmux 3.x versions added -x width and -y height to split-window. If a user on a newer tmux passes ["-x", "-5"] (malformed, but syntactically legal to tmux) or any future value-taking flag not listed here, the value would not have skipNext set, and if that value starts with - and happens to match a reserved flag, it would be incorrectly rejected.
Consider either documenting this limitation explicitly in the function comment, or adding -x and -y to the set for completeness:
var splitWindowFlagsWithValues = map[string]struct{}{
"-c": {},
"-e": {},
"-l": {},
"-p": {},
"-x": {}, // tmux 3.x: width
"-y": {}, // tmux 3.x: height
}This is a low-severity edge case, but worth tracking as tmux evolves.
|
Addressed the new review comments.
|
|
Verified against tmux 3.6a on this machine: Changes:
All tests pass, lint clean. |
|
@greptile-app |
Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
2 participants
Summary
tmux.exec_split_argssupport for exec pane creationbuildSplitWindowArgssoDefaultConfig()remains the single source of truth for the default split args-t,-P,-F) inexec_split_argsInitExecPaneinstead of discarding them silentlyWhy
PR #161 is pointed in the right direction, but two concerns were worth fixing before merge:
DefaultConfig()exec_split_argscould consume internally appended arguments and break pane-ID retrieval without any visible errorThis PR rebuilds that change with those guardrails in place.
Testing
go test ./...