Conversation
Owner
Author
|
@codex review |
There was a problem hiding this comment.
Pull request overview
Adds a Docker Engine “PTY adapter” for controlled-session workloads, enabling the host to attach to a container’s TTY stream, forward input bytes, resize the PTY via the Engine API, and observe exit status independently. This fits into the controlled-session stack by implementing the workload-side PTY boundary while leaving controller/protocol orchestration for later slices.
Changes:
- Implement Docker Engine attach + resize over local unix/npipe endpoints and a hijacked PTY stream writer.
- Add
DockerWorkloadPTYV1to create an inert workload container, attach before start, start + resize, signal stop, and wait for exit observation. - Add unit + integration coverage for ordering, exact byte preservation, resize, stop behavior, and exit observation.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/dockerdeploy/docker_pty_attach.go | Implements Docker Engine HTTP attach/resize over a hijacked local connection and a context-aware write path. |
| internal/dockerdeploy/docker_pty_attach_windows.go | Windows npipe dialing for local Docker endpoints. |
| internal/dockerdeploy/docker_pty_attach_unix.go | Unix socket dialing for local Docker endpoints. |
| internal/dockerdeploy/controlled_session_workload_pty.go | Adds the workload PTY lifecycle wrapper (prepare/attach, start+resize, input, resize, stop, wait). |
| internal/dockerdeploy/controlled_session_workload_pty_test.go | Unit tests for ordering, rollback, cancellation, and exact writes. |
| internal/dockerdeploy/controlled_session_workload_pty_integration_test.go | Docker integration evidence for PTY I/O, resize, Ctrl-C, stop behavior, and exact exit. |
| go.mod | Promotes github.com/Microsoft/go-winio to a direct dependency for Windows dialing. |
| docs/CONTROLLED_SESSION_DESIGN.md | Updates design doc to reflect that the Docker workload PTY adapter slice is now implemented. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+25
to
+36
| func resizeDockerContainerPTYV1( | ||
| ctx context.Context, | ||
| docker CommandSpec, | ||
| container string, | ||
| columns uint32, | ||
| rows uint32, | ||
| timeout time.Duration, | ||
| ) error { | ||
| endpoint, source, err := effectiveDockerEndpointV1(ctx, docker, timeout) | ||
| if err != nil { | ||
| return err | ||
| } |
Comment on lines
+80
to
+89
| func attachDockerContainerPTYV1( | ||
| ctx context.Context, | ||
| docker CommandSpec, | ||
| container string, | ||
| timeout time.Duration, | ||
| ) (dockerPTYAttachmentV1, error) { | ||
| endpoint, source, err := effectiveDockerEndpointV1(ctx, docker, timeout) | ||
| if err != nil { | ||
| return nil, err | ||
| } |
Comment on lines
+151
to
+161
| workload.stateMu.Lock() | ||
| workload.started = true | ||
| workload.stateMu.Unlock() | ||
| go workload.observeExit() | ||
|
|
||
| columns, _ := strconv.ParseUint(workload.plan.InitialColumns, 10, 16) | ||
| rows, _ := strconv.ParseUint(workload.plan.InitialRows, 10, 16) | ||
| if err := workload.resizeLocked(ctx, uint32(columns), uint32(rows)); err != nil { | ||
| return fmt.Errorf("apply initial controlled-session workload PTY dimensions: %w", err) | ||
| } | ||
| return nil |
Comment on lines
+32
to
+36
| The Docker workload PTY adapter is implemented: it creates the frozen | ||
| workload plan inert, establishes the Engine attachment before start, applies | ||
| the initial and later dimensions through the Engine API, preserves exact | ||
| input and output bytes, independently observes the container exit code, and | ||
| exposes graceful and forced stop operations. Controller launch, protocol/PTY |
|
Codex Review: Didn't find any major issues. Swish! 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". |
This was referenced Aug 8, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Stack created with Sapling. Best reviewed with ReviewStack.