Add proof-bound WSL container signal transport - #120
Conversation
CherylSnowVeil
left a comment
There was a problem hiding this comment.
Review — initial pass on ea2e7ff
The implementation is clean and faithfully follows the WaitContainer pattern from #117: input validation runs before the endpoint proof, the container ID is constrained to exact 64-char lowercase hex (so the /containers/<id>/kill path is injection-safe), the signal query parameter is always sent (never relying on the endpoint's implicit SIGKILL default), only HTTP 204 is accepted, and the request reuses the fixed-socket/root-peer/device-inode-bound transport with the standard 30s/1MiB bounds. Non-Linux callers fail closed, and the tests correctly assert the request shape, boundary signals (1, 2, 64), pre-proof rejection of invalid inputs, and the two statSocket calls that execute performs around the operation.
Findings
🟡 [important] docs/wsl.md and docs/roadmap-implementation-requirements.md not updated for the new primitive
The prior primitive PRs maintained both files: #117 (wait) updated docs/wsl.md's "Docker Desktop integration proof" enumeration and both the WSL2 table row and the status paragraph in docs/roadmap-implementation-requirements.md. This PR updates only docs/wsl-process-contract.md. Consequences:
docs/wsl.md(~line 240) still states these primitives "do not ... implement container creation/start, terminal behavior, resize, signals, or end-to-end exit-code propagation" — now inaccurate in that a proof-bound signal operation does exist, and the paragraph that enumerates attach and wait omits it entirely.docs/roadmap-implementation-requirements.mdline 82 and ~line 609 enumerate "constrained attach transport and exact context-bound container-wait operation" but not the container-signal operation.
A reader of the boundary-tracking docs would conclude no signal-sending primitive has shipped. Suggest mirroring the #117 edits: add the signal operation (exact ID + explicit 1..64 signal, always signal=, 204-only) to both enumerations, and re-scope the "signals remain" claims to host-signal interception/forwarding policy, which genuinely remains future work.
🟢 [nit] fmtSignal and a small coverage asymmetry in signal_test.go
fmtSignal is a one-line alias for strconv.Itoa; calling strconv.Itoa directly (or fmt.Sprintf("signal %d", ...)) for the subtest names/expected query would remove the indirection. Also, the sibling wait_test.go covers a 64-char non-hex container ID (strings.Repeat("g", 64)); signal_test.go covers short/uppercase IDs but not that case. Marginal since validateContainerID is shared and covered elsewhere — optional.
Summary
Code is correct, well-tested, and consistent with the package's established proof-binding contract; CI is green (govulncheck, CodeQL, Windows/Linux builds, race tests). The only substantive gap is the stale documentation in wsl.md/roadmap-implementation-requirements.md, which conventionally tracks each shipped primitive. No blocking issues found.
|
Addressed Cheryl's review in signed commit
Focused validation: |
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
The implementation is narrowly scoped, consistently uses the existing proof-bound transport, validates inputs before proof, and has appropriate tests and documentation.
Review effort: Balanced
Findings: None
What changed in this PR
Adds a proof-bound Docker Engine signal primitive for the future native WSL frontend.
Changes:
- Validates exact container IDs and Linux signals
1..64. - Sends explicit signal requests through the existing trusted socket transport.
- Adds tests and updates WSL roadmap/process documentation.
| File | Description |
|---|---|
internal/wsldocker/signal.go |
Implements validation and signal request construction. |
internal/wsldocker/signal_test.go |
Tests request binding and invalid inputs. |
internal/wsldocker/signal_linux.go |
Exposes the Linux implementation. |
internal/wsldocker/signal_other.go |
Fails closed on non-Linux platforms. |
docs/wsl.md |
Documents the primitive and remaining integration work. |
docs/wsl-process-contract.md |
Defines signal behavior and policy boundaries. |
docs/roadmap-implementation-requirements.md |
Updates WSL implementation status. |
docs/roadmap-decisions.md |
Records the completed signal foundation. |
💡 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-review — pass on e1bc01e
Validation of previous findings
🟡 docs/wsl.md + docs/roadmap-implementation-requirements.md stale — fixed. wsl.md now describes the proof-bound signal operation (exact ID, explicit 1..64 signal, always signal=, 204-only) and correctly re-scopes the "signals remain" claim to host-signal interception/forwarding policy, which genuinely is future work. roadmap-implementation-requirements.md is updated in both places (WSL2 table row and the status paragraph), and roadmap-decisions.md is updated as well.
🟢 fmtSignal + missing non-hex case — fixed. strconv.Itoa is used directly and the 64-char non-hex rejection case (strings.Repeat("g", 64)) is present.
Fresh review of the current revision
Correctness verified against the shared machinery: the exact 64-char lowercase-hex ID keeps the /containers/<id>/kill path injection-safe; the 1..64 bound matches the Linux signal domain and guarantees the signal query is always explicit (never the endpoint's implicit SIGKILL); accepting only HTTP 204 surfaces 404/409/500 as bounded APIErrors; execute repeats the full Docker Desktop proof with device/inode stability and a root peer; the operation uses the standard 30s/1MiB bounds; and the non-Linux stub fails closed. Tests assert the request shape, boundary signals (1, 2, 64), pre-proof rejection ordering via a panicking check, and the two statSocket calls.
Findings
🟢 [nit] Package doc comment omits the new transport
internal/wsldocker/wsldocker.go:3-4 describes the package as providing "proof-bound bounded control requests plus separately constrained container-attach and wait transports." #117 extended that enumeration for wait; this PR adds a third sibling transport (SignalContainer) without updating it, so the package-level capability summary is now incomplete. Relatedly, docs/architecture.md (~line 451) describes the detector and attach transport but already omits wait and the raw-stream decoder — a pre-existing gap, not introduced here, but worth noting if you sweep for consistency.
Summary
Both prior findings are resolved and the docs now accurately describe what shipped versus what remains gated. No correctness, security, or contract issues found; the single nit above is optional. CI is green across Linux/Windows builds, race tests, CodeQL, govulncheck, and zizmor.
|
Addressed the re-review nit and resolved the conflicts with current
Validation after the merge: |
Summary
1..64domain before proving Docker Desktop identitysignalquery parameter, so the operation never falls back to the endpoint's defaultSIGKILLThe primitive preserves the exact Linux signal number without choosing signal-handling policy. It does not install handlers, create process groups, enable WSL execution, or infer a replacement signal.
Validation
gofmt -l .go vet ./...go test -race ./...internal/wsldockertest-binary compilecb.exe versionsmoke checkgit diff --checkRefs #2