feat(runtime): support Windows OpenSandbox guests - #148
Conversation
|
Extended CI follow-up: https://github.com/alibaba/skill-up/actions/runs/29551950851
|
|
@copilot resolve the merge conflicts in this pull request |
|
|
Resolved in commit |
roark47
left a comment
roark47
left a comment
There was a problem hiding this comment.
Thanks for the comprehensive implementation. I checked the current head locally: make verify and the full make test race suite both pass.
I don't think this is ready to merge yet:
- The real OpenSandbox Windows guest lifecycle has not completed successfully. The documented Extended CI run skipped it because the selected runner lacked usable KVM. Since Windows guest support is the core behavior of this PR, please obtain at least one successful end-to-end run on a trusted KVM/TUN-capable runner.
- Please address the self-hosted runner scoping and mutable Windows image findings below.
- Please harden the cmd.exe path fallback quoting described below.
- Resource values are currently validated only for non-blank text. Please either validate the accepted CPU/memory/disk syntax (and document that quotas are enforced server-side), or explicitly justify delegating all validation to OpenSandbox.
- Before merging, the PR also needs to leave Draft state, be updated with main, obtain review approval, and clear the currently pending CLA check caused by the Copilot conflict-resolution commits.
The overall architecture—guest-specific path handling, traversal checks, SDK create path, and cross-component Windows adaptations—looks solid; the request for changes is about closing the security and validation gaps before merging.
| e2e-opensandbox-windows: | ||
| name: E2E (OpenSandbox Windows guest) | ||
| if: github.event_name == 'workflow_dispatch' && inputs.windows_guest | ||
| runs-on: self-hosted |
There was a problem hiding this comment.
[P1] Please scope this to a dedicated trusted KVM-capable runner instead of the generic self-hosted pool. This job checks out and executes the selected ref, installs packages, starts Docker with host networking, and accesses /dev/kvm and /dev/net/tun; scheduling it on any self-hosted runner unnecessarily broadens the blast radius. Labels such as [self-hosted, linux, x64, docker, trusted, kvm] would also prevent the core test from silently landing on an incapable runner.
| { | ||
| echo "OPENSANDBOX_API_KEY=$api_key" | ||
| echo "OPENSANDBOX_BASE_URL=http://127.0.0.1:8081" | ||
| echo "OPENSANDBOX_WINDOWS_IMAGE=dockurr/windows:latest" |
There was a problem hiding this comment.
[P1] Please pin the Windows image by digest rather than using dockurr/windows:latest. This image runs on a privileged, KVM/TUN-capable self-hosted runner with host networking, so a mutable tag creates a meaningful supply-chain and reproducibility risk.
| } | ||
|
|
||
| func quoteCmdPath(value string) string { | ||
| return `"` + strings.ReplaceAll(value, "%", "%%") + `"` |
There was a problem hiding this comment.
[P1] This is not sufficient cmd.exe quoting for a configuration-controlled path. Doubling % does not handle embedded ", &, |, <, >, ^, or delayed-expansion !; an input such as a crafted workspace_mount can terminate the quoted argument and alter the fallback command when CreateDirectory fails. Please use the shared cmd quoter if it covers path operands, or reject/escape cmd metacharacters here, and add adversarial tests.
| return errs | ||
| } | ||
|
|
||
| func validateOpenSandboxResources(env Environment) []string { |
There was a problem hiding this comment.
[P2] cpu, memory, and disk are only checked for whitespace-only values, so arbitrary strings and unbounded requests pass validation and fail later—or consume excessive resources if accepted by the server. Please validate the SDK/server-supported syntax and document/enforce where quotas live, or explicitly document that these opaque values are intentionally delegated to a quota-enforcing OpenSandbox server.
Summary
platformand optionalresourcesconfiguration with Windows guest defaults and user overridesCompatibility
Omitting
environment.platformandenvironment.resourcespreserves the existing Linux behavior. Built-in Agent CLI bootstrap inside Windows guests remains out of scope (tracked by #48); preinstalled CLIs and Custom Engines are supported.Validation
make verifygo test -race ./internal/config ./internal/agent ./internal/judge ./internal/evaluatorgo test -race ./internal/runtime -skip TestNoneRuntime_ExecTerminatesGracefullyThenEscalatesgo test -tags e2e ./e2e -run "TestCustomEngine_OpenSandboxWindowsGuest|TestPipeline_CustomEngine_LocalTransport" -count=1windows_guestworkflow input and requires KVM/TUN/Dockermake testalso exercised the full suite, but the existing 300 ms SIGTERM timing testTestNoneRuntime_ExecTerminatesGracefullyThenEscalatesintermittently missed its trap under the race run. A subsequent runtime race run excluding only that known flaky passed. The local full quick E2E run additionally hit a broken machine-local Codex install (spawn .../codex ENOENT); the targeted Custom Engine E2E passed.Closes #45