app-server: add Unix socket transport#18255
Conversation
euroelessar
force-pushed
the
ruslan/uds-stdio-to-uds
branch
from
7d60a42 to
32adbbe
Compare
euroelessar
force-pushed
the
ruslan/app-server-unix-socket
branch
2 times, most recently
from
986fdd5 to
ecd2554
Compare
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ecd2554fb8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
viyatb-oai
left a comment
viyatb-oai
left a comment
There was a problem hiding this comment.
Reviewing in the context of SSH remote control over Unix sockets.
euroelessar
force-pushed
the
ruslan/app-server-unix-socket
branch
2 times, most recently
from
bdb0567 to
6469e20
Compare
- add unix:// app-server transport backed by the shared codex-uds crate - reuse the websocket connection loop for axum and tungstenite-backed streams - add codex app-server proxy to bridge stdio clients to the control socket - tolerate Windows UDS backends that report a missing rendezvous path as connection refused before binding - cargo test -p codex-app-server control_socket_acceptor_forwards_websocket_text_messages_and_pings - cargo test -p codex-app-server - just fmt - just fix -p codex-app-server - git -c core.fsmonitor=false diff --check
euroelessar
force-pushed
the
ruslan/app-server-unix-socket
branch
from
5aec8a7 to
8c9337e
Compare
| Stdio, | ||
| WebSocket { bind_address: SocketAddr }, | ||
| UnixSocket { | ||
| socket_path: Option<AbsolutePathBuf>, |
There was a problem hiding this comment.
I would say this should be defined at construction so that the user of AppServerTransport never needs a reference to $CODEX_HOME.
| socket_path: Option<AbsolutePathBuf>, | |
| socket_path: AbsolutePathBuf, |
| app_server_from_args(["codex", "app-server", "--listen", "unix://"].as_ref()); | ||
| assert_eq!( | ||
| app_server.listen, | ||
| codex_app_server::AppServerTransport::UnixSocket { socket_path: None } |
There was a problem hiding this comment.
The only time you are constructing with None is in a test, anyway?
There was a problem hiding this comment.
we're also constructing it while parsing --listen flag, at which point we don't have the config yet, so can't materialize the path
| } | ||
| } | ||
|
|
||
| #[cfg(test)] |
There was a problem hiding this comment.
Prefer unix_socket_tests.rs with the path= thing. I need to codemod the rest of the repo so it looks like codex-core.
bolinfest
left a comment
bolinfest
left a comment
There was a problem hiding this comment.
Just some small things: nice job with the tests!
3 participants
Summary
Tests