Conversation
![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: 9e4711f247
ℹ️ 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".
| // Use a client that doesn't enforce timeout on body reads | ||
| client := &http.Client{Timeout: timeout} | ||
| resp, err := client.Do(req) //nolint:bodyclose // body ownership is transferred to localHTTPStream |
There was a problem hiding this comment.
OpenHTTPStream builds an http.Client with Timeout set, but in Go that timeout applies to the full request lifecycle including response body reads. For long-lived SSE/streamable HTTP sessions, this forces healthy streams to terminate once the timeout (default 30s here) is reached, which breaks streaming transports on the local backend.
Useful? React with 👍 / 👎.
| go func() { | ||
| done <- p.cmd.Wait() | ||
| }() |
There was a problem hiding this comment.
Wait blocks on cmd.Wait() before consuming stdout/stderr, so a child that writes enough output to fill either pipe can block on write and never exit. In that case Wait hangs until context cancellation, so callers that rely on Wait to collect output (without concurrently reading pipes themselves) can deadlock on verbose commands.
Useful? React with 👍 / 👎.
| // Try boxsh first if available, then fall through to local | ||
| for _, name := range []string{"boxsh", "local"} { | ||
| factory, exists := r.factories[name] |
There was a problem hiding this comment.
The auto-selection path only checks boxsh and local explicitly, so any additional backend registered through Register is ignored when policy.Backend is empty. This can cause CreateSession to fail with “no registered backend can satisfy this policy” even when a compatible registered backend exists.
Useful? React with 👍 / 👎.
✨ feat: add boxsh sandbox foundation
1 participant
Stack
Summary
Testing