Conversation
|
@codex review |
![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: c2fe752396
ℹ️ 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".
| if err := backend.run(controlledSessionCommandSpecV1(plan.Create), RunOptions{Context: ctx}); err != nil { | ||
| return nil, fmt.Errorf("create controlled-session controller container %q: %w", plan.Container, err) |
There was a problem hiding this comment.
When docker create succeeds in the daemon but the client loses the response or its context expires, this path returns without attempting the planned cleanup. That leaves an orphaned inert controller container and its reserved name, preventing a retry for the same live run; handle create errors with the same bounded force-removal used for the explicitly recognized ambiguous docker start failure.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
Adds a Docker-side adapter for the controlled-session controller container, completing the Docker boundary needed to launch controllers through the lease-private session channel and manage their lifecycle consistently with the existing workload PTY adapter.
Changes:
- Introduces
DockerControllerV1(prepare/create, start-once, wait w/ independent observation, graceful/forced stop, cleanup) plus unit + Docker integration coverage. - Adds Linux-only validation that the controller’s private session channel mount points at an existing Unix socket before container creation.
- Freezes (clones) caller-owned controlled-session container plan slices in Docker adapters to prevent post-prepare mutation from affecting executed commands.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| internal/dockerdeploy/testdata/session_channel_helper/main.go | Extends the integration helper to optionally wait for SIGTERM and exit with a testable code. |
| internal/dockerdeploy/controlled_session_workload_pty.go | Clones the workload container plan on prepare to prevent caller mutation. |
| internal/dockerdeploy/controlled_session_workload_pty_test.go | Adds coverage ensuring caller-owned plan slices are frozen for workload execution. |
| internal/dockerdeploy/controlled_session_plan.go | Adds a helper to clone/freeze ControlledSessionContainerPlanV1 slice fields. |
| internal/dockerdeploy/controlled_session_controller.go | Adds the Docker controlled-session controller adapter implementation (DockerControllerV1). |
| internal/dockerdeploy/controlled_session_controller_test.go | Adds unit tests for controller lifecycle ordering, rollback, observation loss, cleanup retries, and plan freezing. |
| internal/dockerdeploy/controlled_session_controller_integration_test.go | Adds Docker integration evidence for controller launch and stop semantics through the private channel. |
| internal/dockerdeploy/controlled_session_controller_channel_unsupported.go | Provides a non-Linux stub returning a clear unsupported error. |
| internal/dockerdeploy/controlled_session_controller_channel_linux.go | Validates the controller’s private channel mount shape and that the socket exists before create. |
| internal/dockerdeploy/controlled_session_controller_channel_linux_test.go | Adds Linux-only unit tests for the channel readiness validation logic. |
| docs/CONTROLLED_SESSION_DESIGN.md | Updates the design document to reflect the new Docker controller adapter implementation. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| exposes graceful and forced stop operations. The Docker controller adapter | ||
| is also implemented: it requires the exact lease-private channel socket | ||
| before container creation, creates the frozen controller plan inert, starts | ||
| it at most once, independently observes its exit, exposes graceful and forced | ||
| stop operations, and removes only the exact planned container. Protocol/PTY |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fc987951af
ℹ️ 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".
| return fmt.Errorf("refuse to remove container because its controlled-session ownership labels do not match the immutable plan") | ||
| } | ||
| } | ||
| return rollbackControlledSessionControllerContainerV1(backend, plan) |
There was a problem hiding this comment.
When two preparations for the same live-run plan overlap before the first controller claims the channel, the second docker create encounters the first container, finds identical ownership labels, and executes the force-removal command even if that controller is already running. Because an ambiguously successful docker create can only have produced a container in the created state, inspect and require that state before removing it; matching labels alone do not distinguish the current attempt's inert container from an active same-lease collision.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e06556fdd0
ℹ️ 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".
| if ctx == nil { | ||
| ctx = context.Background() | ||
| } | ||
| if err := controller.backend.run(controlledSessionCommandSpecV1(controller.plan.Start), RunOptions{Context: ctx}); err != nil { |
There was a problem hiding this comment.
When overlapping preparations remove and recreate this live-run name, an earlier adapter still calls docker start with plan.Container, so it can start and subsequently observe, signal, or remove the later adapter's container instead of failing because its own container disappeared. Docker resolves the CONTAINER operand when each command executes (Docker start reference); capture the ID produced by the successful create and use that immutable ID for all later lifecycle operations.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 17adc24411
ℹ️ 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".
| } | ||
| removeCtx, removeCancel := context.WithTimeout(context.Background(), defaultDockerPreflightTimeout) | ||
| defer removeCancel() | ||
| remove := CommandSpec{Name: plan.Cleanup.Name, Args: []string{"container", "rm", plan.Container}} |
There was a problem hiding this comment.
The current revision pins normal lifecycle operations to IDs, but if the planned name is rebound between the preceding inspection and this rm—for example, overlapping reconciliation removes the inspected container and a retry creates a replacement—this command resolves plan.Container again and can delete a created replacement whose labels and state were never inspected. The Docker CLI reference defines the command as docker container rm [OPTIONS] CONTAINER [CONTAINER...], accepting a name or ID; include .Id in the inspection output and remove that exact ID.
Useful? React with 👍 / 👎.
Stack created with Sapling. Best reviewed with ReviewStack.