-
Notifications
You must be signed in to change notification settings - Fork 82
Add /set system command to interactive mode #575
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
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 enhances the interactive command-line interface by introducing a new Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. 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
|
1284e99 to
039966a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey - I've found 1 issue, and left some high level feedback:
- The
/set systemhandling usesHasPrefixon the raw line, so input like/set systematicswill be interpreted as the command; consider matching on an exact token (e.g.,/set systemfollowed by whitespace or EOL) to avoid false positives. - When
/set systemis used without any trailing text the system prompt is silently cleared but still printsSet system message.; consider distinguishing between setting and clearing the prompt in both behavior and user feedback.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `/set system` handling uses `HasPrefix` on the raw line, so input like `/set systematics` will be interpreted as the command; consider matching on an exact token (e.g., `/set system` followed by whitespace or EOL) to avoid false positives.
- When `/set system` is used without any trailing text the system prompt is silently cleared but still prints `Set system message.`; consider distinguishing between setting and clearing the prompt in both behavior and user feedback.
## Individual Comments
### Comment 1
<location> `cmd/cli/commands/run.go:223-225` </location>
<code_context>
}
continue
+ case strings.HasPrefix(line, "/set system"):
+ // Extract the system prompt text after "/set system "
+ systemPrompt = strings.TrimPrefix(line, "/set system")
+ systemPrompt = strings.TrimSpace(systemPrompt)
+ fmt.Fprintln(os.Stderr, "Set system message.")
+ continue
</code_context>
<issue_to_address>
**issue (bug_risk):** Align the prefix trimming with the comment to avoid subtle command parsing issues.
The comment mentions extracting text after `"/set system "` (with a trailing space), but `TrimPrefix` uses `"/set system"` (no space). This allows `/set systemfoo` to be parsed as `foo`, which is likely unintended. To tighten parsing and match the comment, use `strings.TrimPrefix(line, "/set system ")`; `TrimSpace` will still clean up any surrounding whitespace.
</issue_to_address>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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces a /set system command to the interactive mode, allowing users to specify a system message. The implementation is straightforward, but I've identified a minor bug in the command parsing logic that could cause incorrect behavior for commands with a similar prefix. I've provided a specific suggestion to make the command matching more precise. The rest of the changes for incorporating the system prompt into the chat history are well-implemented.
The /set system command allows users to set or update the system message during interactive sessions. The system prompt is now included in the message history sent to the chat endpoint, enabling customized behavior for the AI assistant. Signed-off-by: Eric Curtin <[email protected]>
039966a to
38776dd
Compare
doringeman
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
$ docker model run llama3.2
> /set system my name's Dorin
Set system message.
> what's my name?
Your name is Dorin.
The /set system command allows users to set or update the system message during interactive sessions. The system prompt is now included in the message history sent to the chat endpoint, enabling customized behavior for the AI assistant.