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.
Develop poc acp #8196
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?
Uh oh!
There was an error while loading. Please reload this page.
Develop poc acp #8196
Changes from all commits
7c088ccc61f3d0903b32d7320192f3f2183File filter
Filter by extension
Conversations
Uh oh!
There was an error while loading. Please reload this page.
Jump to
Uh oh!
There was an error while loading. Please reload this page.
There are no files selected for viewing
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.
Excluding python-sidecar from SCRIPTS_SRC means these new .py/.md files won't be covered by the repo's formatting/license checks (make fmt / make lint-license). If this code is intended to live in-repo (even as a PoC), consider keeping it in the lint set and adding the standard license headers instead of opting out.
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 request body is decoded without any size limit, so a large POST can cause excessive memory usage. Consider wrapping r.Body with http.MaxBytesReader (and optionally validating req.Prompt length / non-empty) before decoding.
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.
This introduces a new public HTTP endpoint (/api/ai/chat) with streaming behavior and external sidecar dependency, but there are no corresponding tests. Consider adding at least a basic handler test (similar to existing http_handler_test.go patterns) that exercises request validation, sidecar dial failure handling, and streaming output on a mocked websocket/ACP connection.
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 websocket sidecar endpoint is hard-coded to ws://localhost:9000, which makes the feature unusable in non-local deployments and complicates ops (sidecar on different host/port, TLS, etc.). Please make the sidecar URL configurable (e.g., via QueryOptions / env var) and validate it at startup.
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.
is this the connection to the sidecar? please add comments like 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.
architecturally what does having MCP servers here mean?
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.
Using a fixed 2s wait for the end-of-turn marker risks truncating responses for longer model/tool runs. Please either wait indefinitely until the marker/ctx cancellation, or make the timeout configurable and large enough for real traces.
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.
This adapter uses recursion on io.EOF (calling Read() again). If the peer sends a sequence of empty frames/messages, this can recurse deeply and risk stack growth. Prefer a loop that resets w.r and continues reading until non-EOF data or a real error.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.