New run prompt #230
New run prompt #230
Conversation
Reviewer's GuideThis PR replaces the basic scanner-based chat prompt with a fully featured readline-like interface by integrating a new 'readline' package, adding enhanced interactive and fallback modes in run.go, adjusting dependencies to support the new package, and updating documentation to reflect the changes. Sequence diagram for enhanced interactive chat input with readlinesequenceDiagram
actor User
participant CLI
participant Readline
participant DesktopClient
User->>CLI: Start interactive chat
CLI->>Readline: Initialize readline prompt
alt Readline initialization fails
Readline-->>CLI: Error
CLI->>CLI: Fallback to basic input
else Readline initialized
Readline-->>CLI: Ready for input
loop For each user input
User->>Readline: Enter message (supports advanced editing)
Readline->>CLI: Return input line
CLI->>DesktopClient: chatWithMarkdown(userInput)
DesktopClient-->>CLI: Response
CLI->>User: Display response
end
end
Class diagram for new readline package typesclassDiagram
class Prompt {
+string Prompt
+string AltPrompt
+string Placeholder
+string AltPlaceholder
+bool UseAlt
+prompt() string
+placeholder() string
}
class Buffer {
+int DisplayPos
+int Pos
+arraylist.List~rune~ Buf
+arraylist.List~bool~ LineHasSpace
+Prompt* Prompt
+int LineWidth
+int Width
+int Height
+Add(r rune)
+Remove()
+Delete()
+MoveLeft()
+MoveRight()
+MoveLeftWord()
+MoveRightWord()
+MoveToStart()
+MoveToEnd()
+ClearScreen()
+Replace(r []rune)
+String() string
+IsEmpty() bool
}
class History {
+arraylist.List~string~ Buf
+bool Autosave
+int Pos
+int Limit
+string Filename
+bool Enabled
+Add(s string)
+Prev() string
+Next() string
+Size() int
+Save() error
+Clear()
+Compact()
}
class Terminal {
+chan rune outchan
+bool rawmode
+any termios
+Read() (rune, error)
}
class Instance {
+Prompt* Prompt
+Terminal* Terminal
+History* History
+bool Pasting
+Readline() (string, error)
+HistoryEnable()
+HistoryDisable()
}
Prompt <.. Buffer
Prompt <.. Instance
Terminal <.. Instance
History <.. Instance
Class diagram for updated run.go interactive mode functionsclassDiagram
class RunCommand {
+generateInteractiveWithReadline(cmd, desktopClient, backend, model, apiKey)
+generateInteractiveBasic(cmd, desktopClient, backend, model, apiKey)
+readMultilineInput(cmd, scanner)
}
class Readline {
+New(Prompt) (*Instance, error)
+Instance.Readline() (string, error)
}
RunCommand --> Readline: uses
RunCommand --> DesktopClient: uses
RunCommand --> chatWithMarkdown: calls
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Summary of ChangesHello @ericcurtin, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly upgrades the interactive prompt experience for the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
ericcurtin
force-pushed
the
new-run-prompt
branch
from
5f27066 to
e04fd77
Compare
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- Several loops (e.g. in Buffer.DisplaySize and History.Save) use “for … range Size()” where Size() returns an int; update these to ordinary for-loops (for i := 0; i < Size(); i++) so they actually iterate over elements.
- The new readline package is very large and mixes buffer management, terminal control, and history in one module—consider breaking it into smaller subpackages or files to improve readability and maintainability.
- generateInteractiveWithReadline and generateInteractiveBasic share similar chat and multiline-input handling logic; extract common functionality to reduce duplication and make the flow more DRY.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Several loops (e.g. in Buffer.DisplaySize and History.Save) use “for … range Size()” where Size() returns an int; update these to ordinary for-loops (for i := 0; i < Size(); i++) so they actually iterate over elements.
- The new readline package is very large and mixes buffer management, terminal control, and history in one module—consider breaking it into smaller subpackages or files to improve readability and maintainability.
- generateInteractiveWithReadline and generateInteractiveBasic share similar chat and multiline-input handling logic; extract common functionality to reduce duplication and make the flow more DRY.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
There was a problem hiding this comment.
Pull Request Overview
This PR introduces a fully-featured readline-like implementation to replace the basic input handling in the CLI. The implementation adds advanced keyboard usage support, history management, multi-line input handling, and platform-specific terminal control.
- Implements cross-platform terminal control with raw mode support for enhanced keyboard input
- Adds readline functionality with history, cursor movement, and line editing features
- Integrates the new readline implementation into the interactive chat mode
Reviewed Changes
Copilot reviewed 17 out of 19 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| go.mod | Updates dependencies and adds new indirect dependencies |
| cmd/cli/readline/*.go | New readline package implementing terminal control, input buffer management, history, and cross-platform support |
| cmd/cli/go.mod | Adds new direct dependencies for readline functionality |
| cmd/cli/commands/run.go | Replaces basic input with enhanced readline-based interactive mode |
| cmd/cli/docs/reference/*.md | Removes outdated "Interactive chat mode started" message from documentation |
| cmd/cli/README.md | Updates example to remove the outdated startup message |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a sophisticated readline-like implementation for the interactive run prompt, significantly improving the user experience with features like command history, advanced keyboard shortcuts, and better multi-line input handling. The implementation is extensive, adding a new readline package with cross-platform support for terminal operations.
My review focuses on several areas:
- Critical bugs: I found a potential panic in command parsing and a significant security concern with a downgraded cryptography library.
- High-severity issues: I've pointed out a bug in history saving that could lead to data loss, and risks associated with using an alpha dependency and another downgraded dependency.
- Medium-severity issues: I've noted areas for improvement in code robustness, efficiency, and design clarity within the new
readlinepackage.
Overall, this is a great feature addition. Addressing the critical and high-severity issues is strongly recommended before merging.
ericcurtin
force-pushed
the
new-run-prompt
branch
2 times, most recently
from
e5bc540 to
772df83
Compare
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 16 out of 17 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
cmd/cli/readline/buffer.go:1
- The magic number 8 for tab size should be defined as a named constant to improve maintainability and make it configurable if needed.
package readline
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Much more fully featured readline-like implementation which advanced keyboard usage. Signed-off-by: Eric Curtin <[email protected]>
ericcurtin
force-pushed
the
new-run-prompt
branch
from
772df83 to
9bfb0ed
Compare
|
Seems we may be heading toward a good moment to revive - docker/model-cli#144 ? |
|
Genuinely I'm happy to continue here, we don't need 65k lines of code for a simple feature like this. Have you tested both to see what the UX is like? |
IIRC most of that change was Edit: yep, on our side there is ~1k lines of code in that PR |
Did you test this implementation vs that one? The difference is drastic. |
| fmt.Fprintln(os.Stderr, "") | ||
| fmt.Fprintln(os.Stderr, " Ctrl + l Clear the screen") | ||
| fmt.Fprintln(os.Stderr, " Ctrl + c Stop the model from responding") | ||
| fmt.Fprintln(os.Stderr, " Ctrl + d Exit (/bye)") |
There was a problem hiding this comment.
| fmt.Fprintln(os.Stderr, " Ctrl + d Exit (/bye)") | |
| fmt.Fprintln(os.Stderr, " Ctrl + d Delete the character under the cursor. If the input line is empty, exit the chat (/bye)") |
There was a problem hiding this comment.
I just tried and it's actually the same as /bye
There was a problem hiding this comment.
This makes sense! Ctrl-d is exit prompt in all shells, I guess you can't type /bye mid-prompt which this is highlighting.
Not yet, I wasn't assuming they do the same thing. More that if we are aiming for a richer CLI interface we could benefit from using sth like bubbletea. |
The bubbletea PR is badly broken, tried bubbletea and huh... Not impressed |
It's still important to stay lean binary size and dependancies are not 100% free, still require maintenance, large dependancies increase the chance of CVE's appearing. |
|
TBH I don't think binary size should differ that much (plus it's golang, so the lean binary ship has long sailed 🙈). But if we don't believe bubble tea fits our bill, that's fair enough for me. |
ilopezluna
left a comment
ilopezluna
left a comment
There was a problem hiding this comment.
MHO, the UX in this PR is much better than what we currently have, so I vote to merge it. Once users benefit from the improved UX, we can focus on optimization.
In general, unless we find a blocker, I’m in favor of merging anything that improves the UX. There’s always time to optimize later.
| fmt.Fprintln(os.Stderr, "") | ||
| fmt.Fprintln(os.Stderr, " Ctrl + l Clear the screen") | ||
| fmt.Fprintln(os.Stderr, " Ctrl + c Stop the model from responding") | ||
| fmt.Fprintln(os.Stderr, " Ctrl + d Exit (/bye)") |
There was a problem hiding this comment.
I just tried and it's actually the same as /bye
|
I think we should merge though because of better UX. I do appreciate the issues we are finding though, I think we should fill up our issue tracker with these. |
Much more fully featured readline-like implementation which advanced keyboard usage.
Summary by Sourcery
Provide an enhanced interactive "run" prompt by introducing a custom readline-like implementation with advanced keyboard shortcuts, multi-line input markers, history and command handling, while falling back to a basic input mode when not running in a terminal
New Features:
Enhancements:
Build:
Documentation: