-
Notifications
You must be signed in to change notification settings - Fork 79
feat: abstract CLI Agent Handler #86
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
base: main
Are you sure you want to change the base?
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.
I think this is too large a change to make in one swoop.
The way I see it, we have two things we need to do:
- Refactor the existing codebase such that the HTTP handler only has to worry about a single interface that exposes the methods common to all agent implementations, e.g.
type AgentHandler interface {
GetStatus(ctx context.Context) (*types.StatusResponse, error)
GetMessages(ctx context.Context) (*types.MessagesResponse, error)
CreateMessage(ctx context.Context, input *types.MessageRequest) (*types.MessageResponse, error)
SubscribeEvents(ctx context.Context, input *struct{}) (<-chan Event, error)
}
A lot of agent-specific 'quirks' are currently handled in lib/msgfmt
but this could be still used behind the scenes of the implementation.
- Once the above is completed, a new implementation of the above interface can use the SDK methods but expose them in the same way.
What you're doing right now is essentially both 1 and 2 at the same time, which is a lot at once.
# Conflicts: # lib/httpapi/server.go # lib/httpapi/server_test.go
@johnstcn Thanks for the feedback. PS: I haven't exposed the |
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.
The attach
command is broken with this change, can you take a look?
# Conflicts: # lib/cli/msgfmt/msgfmt.go
✅ Preview binaries are ready! To test with modules: |
✅ Preview binaries are ready! To test with modules: |
1 similar comment
✅ Preview binaries are ready! To test with modules: |
@johnstcn fixed this. |
✅ Preview binaries are ready! To test with modules: |
Description
This PR handles the Step 1 of the feedback mentioned here #86 (review).