Implement open prompt in system text editor via Ctrl+X#818
Merged
doringeman merged 9 commits intodocker:mainfrom Apr 2, 2026
Merged
Implement open prompt in system text editor via Ctrl+X#818doringeman merged 9 commits intodocker:mainfrom
doringeman merged 9 commits intodocker:mainfrom
Conversation
sathiraumesh
changed the title
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 1 security issue, 3 other issues, and left some high level feedback:
Security issues:
- Detected non-static command inside Command. Audit the input to 'exec.Command'. If unverified user data can reach this call site, this is a code injection vulnerability. A malicious actor can inject a malicious script to execute arbitrary code. (link)
General comments:
- The
runEditorfunction usesstrings.Fieldsto split$EDITOR, which will break for editor commands that include spaces or quoted paths; consider using a shell (sh -con Unix) or a more robust parser so values likecode --waitor paths with spaces work reliably across platforms. - In
openInEditorfor both Unix and Windows, the error path callsSetRawMode(fd)without restoring from the existingtermios/State, which may leave the terminal in a different configuration than whenopenInEditorwas entered; it would be safer to restore the original terminal state rather than relying on a fresh raw mode configuration.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `runEditor` function uses `strings.Fields` to split `$EDITOR`, which will break for editor commands that include spaces or quoted paths; consider using a shell (`sh -c` on Unix) or a more robust parser so values like `code --wait` or paths with spaces work reliably across platforms.
- In `openInEditor` for both Unix and Windows, the error path calls `SetRawMode(fd)` without restoring from the existing `termios`/`State`, which may leave the terminal in a different configuration than when `openInEditor` was entered; it would be safer to restore the original terminal state rather than relying on a fresh raw mode configuration.
## Individual Comments
### Comment 1
<location path="cmd/cli/readline/readline_unix.go" line_range="28-29" />
<code_context>
+ return content, err
+ }
+
+ edited, err := runEditor(content, "vi")
+ if err != nil {
+ SetRawMode(fd)
+ return content, err
</code_context>
<issue_to_address>
**issue (bug_risk):** The SetRawMode error after editor failure is ignored, which can leave the terminal in an inconsistent state.
Here, `SetRawMode(fd)` is called when `runEditor` fails, but its error is ignored. Please capture and handle that error (e.g., return it, possibly combined with the editor error, or at least log it) so terminal restoration failures are not silently swallowed.
</issue_to_address>
### Comment 2
<location path="cmd/cli/readline/readline_windows.go" line_range="15-16" />
<code_context>
+ return content, err
+ }
+
+ edited, err := runEditor(content, "notepad")
+ if err != nil {
+ SetRawMode(fd)
+ return content, err
</code_context>
<issue_to_address>
**issue (bug_risk):** On Windows, the error from SetRawMode after editor failure is also ignored.
This mirrors the Unix path and also ignores the `SetRawMode(fd)` error on editor failure. Please apply the same error handling here so terminal state restoration is consistent and we don’t risk leaving the console in an invalid mode.
</issue_to_address>
### Comment 3
<location path="cmd/cli/readline/readline.go" line_range="212-215" />
<code_context>
buf.ClearScreen()
case CharCtrlW:
buf.DeleteWord()
+ case CharCtrlX:
+ fd := os.Stdin.Fd()
+ edited, err := openInEditor(fd, i.Terminal.termios, buf.String())
+ if err == nil {
+ buf.Replace([]rune(edited))
+ }
</code_context>
<issue_to_address>
**suggestion (bug_risk):** The error from openInEditor is silently ignored, which may hide useful feedback.
If `openInEditor` fails (e.g., editor not found, temp file issues, terminal restore failure), the user gets no indication and the buffer remains unchanged. Consider at least logging the error to stderr or showing a brief message so users know why Ctrl+X appeared to do nothing.
Suggested implementation:
```golang
case CharCtrlX:
fd := os.Stdin.Fd()
edited, err := openInEditor(fd, i.Terminal.termios, buf.String())
if err != nil {
fmt.Fprintln(os.Stderr, "error launching editor:", err)
break
}
buf.Replace([]rune(edited))
case CharCtrlZ:
```
To support the new stderr logging, ensure that `fmt` is imported in `cmd/cli/readline/readline.go` (and not removed by gofmt/goimports). For example, add `fmt` to the existing import block if it is not already present.
</issue_to_address>
### Comment 4
<location path="cmd/cli/readline/editor.go" line_range="30" />
<code_context>
cmd := exec.Command(parts[0], args...)
</code_context>
<issue_to_address>
**security (go.lang.security.audit.dangerous-exec-command):** Detected non-static command inside Command. Audit the input to 'exec.Command'. If unverified user data can reach this call site, this is a code injection vulnerability. A malicious actor can inject a malicious script to execute arbitrary code.
*Source: opengrep*
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces a feature allowing users to edit their input prompt in an external text editor by pressing Ctrl + x. The implementation handles temporary file creation, editor invocation based on environment variables, and the necessary terminal mode toggling for both Unix and Windows systems. Feedback was provided regarding the need to trim trailing newlines from the editor output to avoid breaking terminal UI rendering and to prevent unintended newlines in the final prompt.
Contributor
There was a problem hiding this comment.
Hey - I've found 2 security issues, 2 other issues, and left some high level feedback:
Security issues:
- Detected non-static command inside Command. Audit the input to 'exec.Command'. If unverified user data can reach this call site, this is a code injection vulnerability. A malicious actor can inject a malicious script to execute arbitrary code. (link)
- Detected non-static command inside Command. Audit the input to 'exec.Command'. If unverified user data can reach this call site, this is a code injection vulnerability. A malicious actor can inject a malicious script to execute arbitrary code. (link)
General comments:
- In
buildEditorCmdon Windows,strings.Fields(editor)is used without checking for an empty result, so a whitespace-only$EDITORwill cause an index panic; consider validatingpartslength and falling back to the default editor. - In
Readline()the error fromopenInEditoris silently ignored; consider surfacing the failure to the user (e.g., viastderr) so they understand why the editor shortcut didn’t have any effect.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `buildEditorCmd` on Windows, `strings.Fields(editor)` is used without checking for an empty result, so a whitespace-only `$EDITOR` will cause an index panic; consider validating `parts` length and falling back to the default editor.
- In `Readline()` the error from `openInEditor` is silently ignored; consider surfacing the failure to the user (e.g., via `stderr`) so they understand why the editor shortcut didn’t have any effect.
## Individual Comments
### Comment 1
<location path="cmd/cli/readline/editor.go" line_range="43-44" />
<code_context>
+ }
+
+ var cmd *exec.Cmd
+ if runtime.GOOS == "windows" {
+ parts := strings.Fields(editor)
+ args := append(parts[1:], filePath)
+ cmd = exec.Command(parts[0], args...)
</code_context>
<issue_to_address>
**issue (bug_risk):** EDITOR parsing on Windows breaks when the editor path or arguments contain spaces or quotes.
`strings.Fields(editor)` splits on spaces and strips quotes, so a value like `"C:\Program Files\Notepad++\notepad++.exe" -multiInst` produces an invalid `parts[0]` on Windows. Either delegate parsing to `cmd.exe` (e.g. `cmd /C` with the full `editor` string), or implement proper quoted-argument parsing instead of `strings.Fields`. At the very least, avoid splitting on whitespace and use the full `editor` as the command, appending `filePath` as a separate arg.
</issue_to_address>
### Comment 2
<location path="cmd/cli/readline/readline.go" line_range="212-215" />
<code_context>
buf.ClearScreen()
case CharCtrlW:
buf.DeleteWord()
+ case CharCtrlX:
+ fd := os.Stdin.Fd()
+ edited, err := openInEditor(fd, i.Terminal.termios, buf.String())
+ if err == nil {
+ buf.Replace([]rune(edited))
+ }
</code_context>
<issue_to_address>
**suggestion:** Errors from opening the editor are silently ignored in the readline loop.
If `openInEditor` fails (e.g., invalid `$EDITOR` or non‑zero exit), the buffer stays unchanged and the user gets no indication, so Ctrl+X looks like a no‑op. Consider surfacing the error (e.g., brief stderr message) while keeping the original buffer intact.
Suggested implementation:
```golang
case CharCtrlX:
fd := os.Stdin.Fd()
edited, err := openInEditor(fd, i.Terminal.termios, buf.String())
if err != nil {
fmt.Fprintf(os.Stderr, "failed to open editor for Ctrl+X: %v\n", err)
break
}
buf.Replace([]rune(edited))
```
If `fmt` is not already imported in `cmd/cli/readline/readline.go`, add it to the import block:
<<<<<<< SEARCH
import (
=======
import (
"fmt"
>>>>>>> REPLACE
Place `"fmt"` alongside the other standard library imports, following the existing import style.
</issue_to_address>
### Comment 3
<location path="cmd/cli/readline/editor.go" line_range="46" />
<code_context>
cmd = exec.Command(parts[0], args...)
</code_context>
<issue_to_address>
**security (go.lang.security.audit.dangerous-exec-command):** Detected non-static command inside Command. Audit the input to 'exec.Command'. If unverified user data can reach this call site, this is a code injection vulnerability. A malicious actor can inject a malicious script to execute arbitrary code.
*Source: opengrep*
</issue_to_address>
### Comment 4
<location path="cmd/cli/readline/editor.go" line_range="48" />
<code_context>
cmd = exec.Command("sh", "-c", editor+" \"$1\"", "--", filePath)
</code_context>
<issue_to_address>
**security (go.lang.security.audit.dangerous-exec-command):** Detected non-static command inside Command. Audit the input to 'exec.Command'. If unverified user data can reach this call site, this is a code injection vulnerability. A malicious actor can inject a malicious script to execute arbitrary code.
*Source: opengrep*
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Contributor
There was a problem hiding this comment.
Hey - I've found 1 security issue, 2 other issues, and left some high level feedback:
Security issues:
- Detected non-static command inside Command. Audit the input to 'exec.Command'. If unverified user data can reach this call site, this is a code injection vulnerability. A malicious actor can inject a malicious script to execute arbitrary code. (link)
General comments:
- The
openInEditorimplementations inreadline_unix.goandreadline_windows.goare nearly identical aside from the concrete type cast; consider factoring this into a single helper that works with an interface or a small wrapper to avoid duplication and keep platform differences localized. - In
openInEditor, the error handling aroundSetRawModecan mask the editor error and leaves the new terminal state unused; consider (a) restoring raw mode in adeferthat preserves the original editor error when possible, and (b) making sure any new terminal state returned bySetRawModeis wired back intoi.Terminal.termiosif required by the rest of the code. - In
editor.go, the fallback shell is hardcoded to/bin/bash, which may not exist on minimal or non-GNU Unix systems; using/bin/shas the default would be more portable while still supporting the same usage here.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `openInEditor` implementations in `readline_unix.go` and `readline_windows.go` are nearly identical aside from the concrete type cast; consider factoring this into a single helper that works with an interface or a small wrapper to avoid duplication and keep platform differences localized.
- In `openInEditor`, the error handling around `SetRawMode` can mask the editor error and leaves the new terminal state unused; consider (a) restoring raw mode in a `defer` that preserves the original editor error when possible, and (b) making sure any new terminal state returned by `SetRawMode` is wired back into `i.Terminal.termios` if required by the rest of the code.
- In `editor.go`, the fallback shell is hardcoded to `/bin/bash`, which may not exist on minimal or non-GNU Unix systems; using `/bin/sh` as the default would be more portable while still supporting the same usage here.
## Individual Comments
### Comment 1
<location path="cmd/cli/readline/editor.go" line_range="58-62" />
<code_context>
+func buildEditorCmd(filePath string) *exec.Cmd {
+ args, shell := resolveEditor()
+
+ if shell {
+ // The editor string is the last element — append the file path to it
+ // so the shell interprets the full command.
+ args[len(args)-1] = fmt.Sprintf("%s %s", args[len(args)-1], filePath)
+ } else {
+ args = append(args, filePath)
+ }
</code_context>
<issue_to_address>
**🚨 issue (security):** File path is concatenated into a shell command without quoting, which will break on paths with spaces and can be unsafe.
When `shell == true`, the file path is appended via `fmt.Sprintf("%s %s", ...)` without any quoting/escaping, so any spaces or shell‑special characters in `filePath` can break the command or be interpreted by the shell. Please either quote/escape `filePath` correctly for the target shell (e.g., single quotes on POSIX, `"` on `cmd.exe`) or avoid constructing a single shell string and instead pass the editor and file path as separate arguments so the shell doesn’t reinterpret them.
</issue_to_address>
### Comment 2
<location path="cmd/cli/readline/readline_unix.go" line_range="30" />
<code_context>
+
+ edited, err := runEditor(content)
+
+ if _, restoreErr := SetRawMode(fd); restoreErr != nil {
+ return content, restoreErr
+ }
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Error from restoring raw mode overwrites any prior editor error, potentially hiding the root cause.
If `runEditor` fails and `SetRawMode` also fails, this will return only `restoreErr` and drop the original editor error. Consider either preserving the original `err` (and logging the restore failure), combining both errors, or only returning `restoreErr` when there was no prior error, so failures in launching the editor aren’t masked by terminal restore issues.
Suggested implementation:
```golang
if _, restoreErr := SetRawMode(fd); restoreErr != nil {
if err != nil {
return content, fmt.Errorf("editor error: %w (also failed to restore terminal: %v)", err, restoreErr)
}
return content, restoreErr
}
if err != nil {
return content, err
}
```
1. Ensure that `fmt` is imported in `cmd/cli/readline/readline_unix.go` (or a shared file for this build tag). If it is not already imported, add:
`import "fmt"`.
2. If your project has a preferred error-wrapping or logging utility, you may want to replace `fmt.Errorf` with that, e.g. `errors.Join(err, restoreErr)` (Go 1.20+) or a project-specific helper, to better align with your existing error-handling conventions.
</issue_to_address>
### Comment 3
<location path="cmd/cli/readline/editor.go" line_range="66" />
<code_context>
cmd := exec.Command(args[0], args[1:]...)
</code_context>
<issue_to_address>
**security (go.lang.security.audit.dangerous-exec-command):** Detected non-static command inside Command. Audit the input to 'exec.Command'. If unverified user data can reach this call site, this is a code injection vulnerability. A malicious actor can inject a malicious script to execute arbitrary code.
*Source: opengrep*
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
doringeman
reviewed
Apr 1, 2026
doringeman
reviewed
Apr 1, 2026
Contributor
|
@sourcery-ai review |
Contributor
There was a problem hiding this comment.
Hey - I've found 1 security issue, and left some high level feedback:
Security issues:
- Detected non-static command inside Command. Audit the input to 'exec.Command'. If unverified user data can reach this call site, this is a code injection vulnerability. A malicious actor can inject a malicious script to execute arbitrary code. (link)
General comments:
- The
openInEditorimplementation is duplicated betweenreadline_unix.goandreadline_windows.go; consider moving the shared logic into a common helper and keeping only the platform-specific type assertions where necessary. - The editor resolution/quoting logic in
editor.golooks fragile on Windows: paths likeC:\Program Files\...will trigger theshellbranch due to backslashes and then be wrapped in single quotes for acmd /Cinvocation, which cmd.exe does not interpret as quoting, so editors with spaces in their path or arguments may fail; you may want to separate Windows and Unix quoting strategies more explicitly. - In
buildEditorCmd, treating anyEDITORcontaining quotes or backslashes as a shell command conflates path syntax with intentional shell expressions; it might be safer to parse simplecommand arg1 arg2forms without invoking a shell and only fall back toSHELL -cwhen an explicit shell metacharacter is present.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `openInEditor` implementation is duplicated between `readline_unix.go` and `readline_windows.go`; consider moving the shared logic into a common helper and keeping only the platform-specific type assertions where necessary.
- The editor resolution/quoting logic in `editor.go` looks fragile on Windows: paths like `C:\Program Files\...` will trigger the `shell` branch due to backslashes and then be wrapped in single quotes for a `cmd /C` invocation, which cmd.exe does not interpret as quoting, so editors with spaces in their path or arguments may fail; you may want to separate Windows and Unix quoting strategies more explicitly.
- In `buildEditorCmd`, treating any `EDITOR` containing quotes or backslashes as a shell command conflates path syntax with intentional shell expressions; it might be safer to parse simple `command arg1 arg2` forms without invoking a shell and only fall back to `SHELL -c` when an explicit shell metacharacter is present.
## Individual Comments
### Comment 1
<location path="cmd/cli/readline/editor.go" line_range="65" />
<code_context>
cmd := exec.Command(args[0], args[1:]...)
</code_context>
<issue_to_address>
**security (go.lang.security.audit.dangerous-exec-command):** Detected non-static command inside Command. Audit the input to 'exec.Command'. If unverified user data can reach this call site, this is a code injection vulnerability. A malicious actor can inject a malicious script to execute arbitrary code.
*Source: opengrep*
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Contributor
|
You can run |
Contributor
Author
|
@doringeman, anything more needed to be done for merging ? |
doringeman
approved these changes
Apr 2, 2026
Contributor
|
LGTM @sathiraumesh, thanks! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
docker model run)$EDITORenv var with fallback tovi(Unix) /notepad(Windows)Fixes #468