Conversation
|
Codex Review: Didn't find any major issues. Delightful! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
![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: 20a0ab9584
ℹ️ 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".
| @@ -1,2 +1,2 @@ | |||
| kind: Added | |||
| body: Freeze, realize, recover, and verify lease-private controlled-session networks and granted workload endpoints, with protocol-v2 session-local coordinates, fixed participant addresses, exact peer firewall grants, and ordinary public or local access preserved only when explicitly granted. | |||
| body: Freeze, realize, recover, and verify lease-private controlled-session networks and granted workload endpoints, with protocol-v1 session-local coordinates, fixed participant addresses, exact peer firewall grants, and ordinary public or local access preserved only when explicitly granted. | |||
There was a problem hiding this comment.
This fragment still describes only controlled-session networking, so the newly exposed Linux session client and the protocol's mandatory ready event/request gate will be absent from the release notes. Extend this entry or add a separate Changie fragment covering those user-facing changes.
AGENTS.md reference: AGENTS.md:L9-L10
Useful? React with 👍 / 👎.
| if event.Kind != controlledsession.EventReadyV1 { | ||
| fail("first post-opened event is not ready: %#v", event) |
There was a problem hiding this comment.
When the workload shell produces startup output quickly, the PTY pump—which starts before the workload—can already be writing an output frame when the supervisor queues ready; the event gate explicitly preserves that in-flight frame. This check treats such a valid frame as fatal, making the live supervisor and network integration tests timing-dependent. Wait until ready while retaining or processing any preceding output instead of requiring it to be the next frame.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
This PR normalizes the controlled-session wire protocol naming (treating the current on-wire format as v1, removing the unpublished “v2” naming), and introduces a payload-free ready lifecycle event to explicitly mark when the workload is started, networking (if applicable) is verified, and controller requests (other than terminal acknowledgement) are accepted. It also adds a Linux-only controller-side client API that consumes opened, gates ordinary requests on ready, and handles startup-failure acknowledgement and unsupported platforms cleanly.
Changes:
- Rename protocol types/helpers from
*V2to*V1and set the on-wire protocol version byte to1. - Add
EventReadyV1(payload-free) and reject non-ack controller requests beforereadyin the supervisor. - Add
SessionClientV1+DialSessionClientV1(Linux) with readiness gating, plus a non-Linux stub returning a clear error.
Reviewed changes
Copilot reviewed 23 out of 23 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| internal/dockerdeploy/testdata/session_channel_helper/main.go | Update helper to use v1 read/write helpers and to wait for ready before issuing requests in proofs. |
| internal/dockerdeploy/controlled_session_supervisor.go | Emit ready after activation, and reject non-ack requests before ready. |
| internal/dockerdeploy/controlled_session_supervisor_test.go | Extend event-order assertions and add coverage for pre-ready rejection and ready-delivery failure handling. |
| internal/dockerdeploy/controlled_session_session_io_integration_test.go | Update integration test framing helpers from *V2 to *V1. |
| internal/dockerdeploy/controlled_session_plan.go | Switch endpoint/opened types and validation from *V2 to *V1. |
| internal/dockerdeploy/controlled_session_plan_test.go | Update expected endpoint host constants and opened endpoint types to v1. |
| internal/dockerdeploy/controlled_session_network_integration_test.go | Update endpoint host constants to v1. |
| internal/dockerdeploy/controlled_session_channel.go | Update private channel config to use OpenedV1. |
| internal/dockerdeploy/controlled_session_channel_integration_test.go | Update private channel integration config to OpenedV1/EndpointV1. |
| internal/controlledsession/session_io.go | Treat ready as a lifecycle event for bridge gating. |
| internal/controlledsession/session_io_test.go | Update opened payload type in lifecycle-event validation test. |
| internal/controlledsession/protocol.go | Collapse “v2 naming” into v1, add ready event + wire kind, enforce payload-free readiness, and switch frame version byte to 1. |
| internal/controlledsession/protocol_test.go | Rename/adjust protocol tests for v1 naming, add ready payload-free validation coverage, update version assertions. |
| internal/controlledsession/client.go | Add SessionClientV1 with opened consumption, readiness/termination state tracking, and request gating. |
| internal/controlledsession/client_linux.go | Add Linux DialSessionClientV1 that validates socket path and claims the Unix socket. |
| internal/controlledsession/client_unsupported.go | Add non-Linux stub returning a clear “requires Linux” error. |
| internal/controlledsession/client_test.go | Add unit tests for opened consumption, ready gating, startup-failure acknowledgement behavior, and protocol invariants. |
| internal/controlledsession/client_linux_test.go | Add Linux tests verifying client can claim the private channel and rejects invalid paths. |
| internal/controlledsession/channel.go | Update channel framing and opened payload to v1 types and WriteEventV1/ReadRequestV1. |
| internal/controlledsession/channel_linux_test.go | Update channel tests to v1 read/write helpers and version byte expectations. |
| internal/controlledsession/authorization_test.go | Update endpoint test fixtures to EndpointV1 and v1 host constant. |
| docs/CONTROLLED_SESSION_DESIGN.md | Update protocol version narrative to v1 and document the new ready event semantics. |
| .changes/unreleased/+controlled-session-network-planning.yaml | Update release note wording from protocol-v2 to protocol-v1 coordinates. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Replace the unpublished protocol-v2 naming and compatibility path with the current protocol as v1, and add a payload-free ready event after workload startup and network verification. Add an embedded Linux controller client API that consumes opened, gates ordinary requests on ready, permits terminal acknowledgement after startup failure, and reports unsupported platforms cleanly. Update supervisor integration, tests, design documentation, and the release note.
Replace the unpublished protocol-v2 naming and compatibility path with the current protocol as v1, and add a payload-free ready event after workload startup and network verification.
Add an embedded Linux controller client API that consumes opened, gates ordinary requests on ready, permits terminal acknowledgement after startup failure, and reports unsupported platforms cleanly. Update supervisor integration, tests, design documentation, and the release note.
Stack created with Sapling. Best reviewed with ReviewStack.