Add bounded WSL container inspection - #121
Conversation
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Missing boolean fields are accepted as false instead of failing closed.
Review effort: Balanced
Findings: 1
What changed in this PR
Adds proof-bound Docker container inspection for WSL, producing immutable snapshots from exact container IDs.
Changes:
- Adds bounded inspection and snapshot decoding.
- Adds Linux implementation, non-Linux rejection, and tests.
| File | Description |
|---|---|
internal/wsldocker/inspect.go |
Implements snapshot decoding and validation. |
internal/wsldocker/inspect_linux.go |
Connects inspection to the proven socket. |
internal/wsldocker/inspect_other.go |
Rejects unsupported platforms. |
internal/wsldocker/inspect_test.go |
Tests requests, bounds, identity, and immutability. |
💡 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.
Initial review of 4cfe0e2 ("Add bounded WSL container inspection"). CI is green across Linux/Windows/ARM64, vet, CodeQL, zizmor, and govulncheck.
Findings
[important] internal/wsldocker/inspect.go:56-63 — absent Running/Tty/OpenStdin decode to fabricated false instead of being rejected
Config and State are decoded as *struct so their presence is enforced, but the three lifecycle booleans inside them are plain bool fields: a response like "State":{} or "Config":{"Labels":{...}} is accepted and produces a snapshot claiming not-running / non-TTY / stdin-closed even though the engine never supplied those values. That is inconsistent with the PR's stated contract ("enforce … required configuration/state fields" — only the objects' presence is enforced, not the fields), and with the same-package precedent in decodeContainerWaitResponse, which uses StatusCode *int64 precisely to reject a missing scalar (wait.go:57-59).
Consequence: the snapshot is documented as the input to "later ownership and lifecycle checks". A fabricated Running()==false is a conservative misread, but a fabricated TTY()==false is not — later attach wiring would assume multiplexed application/vnd.docker.raw-stream framing on a container that is actually TTY and misdecode the stream, and fabricated OpenStdin()==false could suppress a stdin attach the container expects. A real Docker daemon always emits these fields, so this is only reachable on a non-conforming response — but this package's entire design is fail-closed-on-anomaly (double socket re-stat, double peer-UID check, returned-ID rebinding), so the gap is worth closing. Fix: decode the three fields as *bool, reject nil, and add omitted/null cases to the unsafe-shape test table. (Both automated reviewers flagged this independently; I agree it is a real defect given the codebase's own convention.)
[nit] Docs do not record the new primitive
docs/wsl.md's wsldocker narrative, the WSL2 row in docs/roadmap-implementation-requirements.md, and the wsldocker package doc each enumerate the shipped primitives ("bounded control requests plus separately constrained container-attach and wait transports"). Prior primitive PRs updated these when landing (e.g., #117 updated docs/wsl.md and the roadmap row for WaitContainer). This PR adds a new exported primitive (InspectContainer/ContainerSnapshot) with no doc touch-up. A one-line mention keeps the status docs accurate.
What looks good
- Exact-identity binding is tight: 64-hex lowercase ID validation before the request,
validateContainerIDon the returnedId, then an exact-match check — so name collisions or prefix matches can never alias a different container. Validating before comparing also means the%qin the mismatch error only ever prints canonical hex. - Reuses the full proof-bound
executepath (identity probe, pre/post socket dev/ino checks, root-peer enforcement) rather than a shortcut, and rejects invalid inputs before reaching the proof (test verifiescheckpanics if reached). - Bounded twice:
performDockerRequestcaps the read atmaxContainerInspectOutput+1, and decode re-checks the bound — same defense-in-depth pattern asdecodeContainerWaitResponse. - Immutability contract is real: labels are cloned on construction and again on every
Labels()call; the test proves mutation isolation. Zero-value snapshot returns a non-nil empty map, no panics. - Platform split is consistent with
wait/attach(inspect_other.gofails closed outside Linux), tests are platform-agnostic so Windows CI exercises the decoder, andoperationTimeoutis correctly applied (unlike wait's intentional unbounded long-poll).
Summary
A clean, well-scoped primitive that follows this package's established conventions almost exactly. The one substantive issue is the missing presence checks on the three lifecycle booleans — a small fix (*bool + nil rejection + test cases) that brings the decoder in line with both the PR description and the wait.go precedent.

Summary
Roadmap: #2
Validation
gofmtgo vet ./...go test -race ./...git diff --checkCommit
4cfe0e2is hardware-signed.