Add proof-bound WSL container TTY resize - #119
Conversation
CherylSnowVeil
left a comment
There was a problem hiding this comment.
Reviewed the current head (e162d16). This is an initial review — no prior review rounds from this account.
Overall
The change is a small, well-scoped addition that faithfully follows the merged WaitContainer/OpenAttach proof-bound pattern. resizeContainer rejects a nil context, non-canonical container IDs, and zero dimensions before the identity proof is attempted, then delegates to execute, which repeats the full Docker Desktop socket/peer proof and binds the request to the proven device/inode with a root peer before and after the call. The request shape matches the Engine API contract (POST /containers/{id}/resize with h/w query params, only HTTP 200 accepted), uint16 matches the terminal winsize row/column representation, and the 30-second operationTimeout is the correct bound for a non-long-poll operation. The non-Linux stub fails closed, build tags are consistent across the file trio and tests, and the package doc plus docs/wsl.md, docs/wsl-process-contract.md, and docs/roadmap-implementation-requirements.md were updated accurately. I could not run gofmt/go vet/go test in this environment, but git diff --check is clean and CI is running.
No blocking or important findings. Two optional nits:
- 🟢
[nit]docs/roadmap-decisions.md(§3 "Remaining WSL2", ~line 349) still says "multiplexed output, terminal, resize, signal and exit-code semantics remain". That line was already stale for the multiplexed decoder (#115) and wait (#117) since those PRs did not touch this file, and this PR now makes it stale for resize as well. Since this PR deliberately refreshed the other capability-tracking docs, consider updating that bullet for consistency — e.g. noting the decoder, wait and resize primitives are implemented, leaving terminal event collection, signal and exit-code propagation as remaining. - 🟢
[nit]internal/wsldocker/resize_test.gocovers short and uppercase IDs but omits a 64-char non-hex ID case (e.g.strings.Repeat("g", 64)) thatwait_test.goincludes. The sharedvalidateContainerIDis already proven wired-in by the existing cases, so this adds little real coverage — skip if you prefer.
Non-blocking observations, for the record:
- Requiring positive
h/w(rather than passing 0 through to the daemon's "unchanged" semantics) is a deliberate, documented constraint and the right fail-closed choice for a primitive whose callers must supply proven TTY dimensions. - Each resize call re-runs the full
/infoidentity proof plus socket stats. For terminal drag-resize event bursts this is heavier than strictly necessary, but it is the package's explicit, documented trust-boundary model and consistent with every sibling operation — flagging only so future lifecycle wiring is aware of the per-event cost.
Overall this is merge-ready from my review once CI completes.
|
Addressed both optional review nits in signed commit
Focused validation: |
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
The implementation is bounded, fail-closed, consistent with existing proof-bound operations, and adequately tested and documented.
Review effort: Balanced
Findings: None
What changed in this PR
Adds a proof-bound Docker Engine TTY resize primitive for future native WSL frontend integration.
Changes:
- Validates exact container IDs and positive 16-bit dimensions.
- Sends resize requests through the existing verified socket transport.
- Updates WSL capability and roadmap documentation.
| File | Description |
|---|---|
internal/wsldocker/wsldocker.go |
Updates package capabilities. |
internal/wsldocker/resize.go |
Implements validated resize requests. |
internal/wsldocker/resize_test.go |
Tests request binding and validation. |
internal/wsldocker/resize_linux.go |
Exposes the Linux implementation. |
internal/wsldocker/resize_other.go |
Fails closed outside Linux. |
docs/wsl.md |
Documents resize and decoder status. |
docs/wsl-process-contract.md |
Defines the resize contract. |
docs/roadmap-implementation-requirements.md |
Updates implementation progress. |
docs/roadmap-decisions.md |
Records the completed primitive. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
CherylSnowVeil
left a comment
There was a problem hiding this comment.
Re-reviewed the current head (6138bab). Previous round reviewed e162d16.
Validation of previous findings
Both optional nits from my prior review are fixed in 6138bab (verified against the current diff, not just the author's reply):
- 🟢
docs/roadmap-decisions.md— the §3 "Remaining WSL2" bullet now records the strict raw-stream decoder, exact wait, and TTY-resize operations as implemented, and correctly scopes what remains to terminal event collection, signal, and end-to-end exit-code propagation. Accurate against the merged code (multiplex.go,wait.go,resize.go). - 🟢
internal/wsldocker/resize_test.go— now includes the 64-char non-hex rejection case (strings.Repeat("g", 64)), matching thewait_test.gocoverage shape.
Fresh review of the current PR
I re-reviewed the complete current diff (9 files, +162/-17) from scratch, not just the delta since e162d16.
resize.govalidates nil context, exact full lowercase 64-hex ID, and positiveheight/widthbefore any proof is attempted, checks dependency completeness, then delegates toexecute— the same ordering and fail-closed posture aswaitContainer/openAttach. The request is the correct Engine contract (POST /containers/{id}/resizewithh/wquery parameters, only HTTP 200 accepted;validateRequestpermits a bodyless POST).resize_linux.goappliesoperationTimeout(30s) andmaxOperationOutputwithexpectedPeerUID0 — the right bounds for a non-long-poll operation and identical toExecute's binding.resize_other.gofails closed off Linux, consistent withwait_other.go. Build-tag layout mirrors the wait file trio.- Tests verify exact request binding (path, method,
h=24&w=80encoding, single success status), the two socket stats bracketing the operation, rejection of all invalid inputs before the proof runs (checkpanics if reached), and incomplete dependencies. docs/wsl.md,docs/wsl-process-contract.md, anddocs/roadmap-implementation-requirements.mddescribe the primitive accurately — exact-ID-only, positive uint16 dimensions, 200-only acceptance, no terminal observation or event subscription, nothing wired into tool execution.- All CI checks pass (format/vet/test on Linux, Windows and ARM64 test/build, release bundle + reproduction, CodeQL, govulncheck, zizmor).
git diff --checkis clean. Go is still not installed in this environment, so I again could not rungofmt/go vet/go testlocally — CI coverage stands in.
No new actionable findings; nothing was missed on the fresh pass. The change remains a correctly scoped, unexposed primitive consistent with the package's documented trust-boundary model. Merge-ready from my review.
Summary
POST /containers/{id}/resizeoperation for the native WSL frontend foundationThis remains an unexposed primitive: it neither enables the WSL frontend nor guesses terminal state. Later lifecycle wiring must obtain dimensions from a proven TTY and invoke resize for each accepted event.
Validation
gofmt -l .go vet ./...go test -race ./...internal/wsldockertest-binary compilecb.exe versionsmoke checkgit diff --checkRefs #2