-
Notifications
You must be signed in to change notification settings - Fork 80
feat: accept initial prompt #112
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
✅ Preview binaries are ready! To test with modules: |
// Send initial prompt when agent becomes stable for the first time | ||
if !s.initialPromptSent && convertStatus(currentStatus) == AgentStatusStable { | ||
if err := s.conversation.SendMessage(FormatMessage(s.agentType, s.initialPrompt)...); err != nil { | ||
s.logger.Error("Failed to send initial prompt", "error", err) | ||
} else { | ||
s.initialPromptSent = true | ||
currentStatus = st.ConversationStatusChanging | ||
s.logger.Info("Initial prompt sent successfully") | ||
} | ||
} |
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.
There's an inherent race condition here: we will report "stable" status for a short time period before "changing". Is it possible to prevent this?
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.
I think line 322 would prevent that:
currentStatus = st.ConversationStatusChanging
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.
Ah I see now, I assumed that the status updates were done in a separate goroutine but it's actually done in UpdateStatusAndEmitChanges()
below in line 326.
I think it's worth adding a test for this behaviour so that we can validate that we don't send an extraneous status update.
I also think this logic might be better encapsulated inside the Conversation
.
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.
Can we add tests for this new functionality?
// Send initial prompt when agent becomes stable for the first time | ||
if !s.initialPromptSent && convertStatus(currentStatus) == AgentStatusStable { | ||
if err := s.conversation.SendMessage(FormatMessage(s.agentType, s.initialPrompt)...); err != nil { | ||
s.logger.Error("Failed to send initial prompt", "error", err) | ||
} else { | ||
s.initialPromptSent = true | ||
currentStatus = st.ConversationStatusChanging | ||
s.logger.Info("Initial prompt sent successfully") | ||
} | ||
} |
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.
Ah I see now, I assumed that the status updates were done in a separate goroutine but it's actually done in UpdateStatusAndEmitChanges()
below in line 326.
I think it's worth adding a test for this behaviour so that we can validate that we don't send an extraneous status update.
I also think this logic might be better encapsulated inside the Conversation
.
✅ Preview binaries are ready! To test with modules: |
Done
+1, I'm confused on how to test this. |
Approving so we can get this released for the copilot-cli module release |
With the current implementation, I think you'd need to test the |
Created a separate issue to track this #114 |
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.
Approving to unblock.
Closes #111
Usage example
agentapi server -I="Hello World" --term-width 76 --term-height 1190 -- copilot