refactor(runtime): replace TargetGOOS with shell descriptor#145
Conversation
There was a problem hiding this comment.
Pull request overview
This PR refactors how skill-up models the target execution environment by replacing Runtime.TargetGOOS() with a validated Runtime.Shell() descriptor (platform.Shell). This consolidates target OS + shell semantics into a single channel so planners/judges/agents don’t accidentally consult the host shell when the runtime target differs (e.g., container/remote runtimes).
Changes:
- Replace
Runtime.TargetGOOS()withRuntime.Shell() platform.Shelland validate shell descriptors during runtime construction. - Route script-judge planning (interpreter/quoting/cleanup) through the runtime’s target shell descriptor instead of
platform.Host(). - Update agent Windows bash requirements and add deterministic tests across Linux POSIX, Windows cmd, and Windows Git Bash scenarios.
Reviewed changes
Copilot reviewed 30 out of 30 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/runtime/runtime.go | Replace TargetGOOS with Shell() on the Runtime interface and validate Shell() in NewRuntime. |
| internal/runtime/opensandbox.go | Implement Shell() for OpenSandbox as Linux/POSIX. |
| internal/runtime/opensandbox_test.go | Add test asserting OpenSandbox runtime shell is Linux/POSIX. |
| internal/runtime/none.go | Implement Shell() for NoneRuntime via platform.Host().Target. |
| internal/runtime/none_test.go | Add test asserting NoneRuntime shell matches host target shell. |
| internal/runtime/docker.go | Implement Shell() for DockerRuntime as Linux/POSIX. |
| internal/runtime/docker_test.go | Add test asserting Docker runtime shell is Linux/POSIX. |
| internal/platform/shell_windows.go | Refactor Windows host shell descriptor to expose Target Shell (GOOS/family/bash path). |
| internal/platform/shell_other.go | Refactor POSIX host shell descriptor to expose Target Shell and remove HostShell quoting fields. |
| internal/platform/platform.go | Introduce Shell, ShellFamily, validation, and target-based quoter selection. |
| internal/platform/platform_test.go | Add validation and deterministic tests for Shell.Validate() and Shell.Quoter(). |
| internal/judge/script.go | Switch script judge planning to use rt.Shell() (target descriptor) rather than TargetGOOS. |
| internal/judge/script_test.go | Update mock runtime to provide Shell() instead of TargetGOOS(). |
| internal/judge/interpreter.go | Refactor script planning to accept platform.Shell, validate it, and use target shell quoting. |
| internal/judge/interpreter_test.go | Make interpreter planning tests deterministic by injecting explicit target shells. |
| internal/judge/helpers_test.go | Update mock runtime to implement Shell(). |
| internal/evaluator/evaluator_test.go | Update evaluator mock runtime to implement Shell(). |
| internal/agent/qwen_code.go | Switch agent preflight from requireBashOnWindowsHost to requireBashTargetShell. |
| internal/agent/qwen_code_test.go | Update test runtime to implement Shell(). |
| internal/agent/qodercli.go | Switch agent preflight from requireBashOnWindowsHost to requireBashTargetShell. |
| internal/agent/qodercli_test.go | Update test runtime to implement Shell(). |
| internal/agent/prompt_delivery_test.go | Update test runtime to implement Shell(). |
| internal/agent/node_install_test.go | Update test runtime to implement Shell(). |
| internal/agent/codex.go | Switch agent preflight from requireBashOnWindowsHost to requireBashTargetShell. |
| internal/agent/codex_test.go | Update test runtime to implement Shell(). |
| internal/agent/cli.go | Update agent checks to use rt.Shell().GOOS and requireBashTargetShell. |
| internal/agent/claude_code.go | Switch agent preflight from requireBashOnWindowsHost to requireBashTargetShell. |
| internal/agent/claude_code_test.go | Update test runtime to implement Shell(). |
| internal/agent/agent.go | Replace host-derived bash requirement with target-shell validation (requireBashTargetShell). |
| internal/agent/agent_test.go | Add tests covering requireBashTargetShell across Linux POSIX / Windows cmd / Windows bash descriptors. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Codex Review: Didn't find any major issues. More of your lovely PRs please. 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". |
Summary
Runtime.TargetGOOS()with a validatedRuntime.Shell()target descriptorplatform.Host()Design note
Issue #44 proposed returning
platform.HostShellfrom runtimes. This PR keeps the same single-channel architecture but refines the boundary:Runtime.Shell()returns pure target data (GOOS, shell family, optionalBashPath), whileHostShellretains the host-onlyexec.Cmdlauncher and environment. Docker and OpenSandbox therefore describe their Linux/POSIX guest without carrying meaningless host process callbacks.OpenSandbox Windows guest provisioning remains out of scope and blocked on its SDK platform field. Once available, #45 can return the requested Windows target descriptor from
OpenSandboxRuntime.Shell().Verification
make verifymake testGOOS=windows go build ./...go test -tags e2e -run 'Test(Pipeline_ScriptJudge|Contract_Judge_Script_ExitCode0_Pass)$' ./e2eTargetGOOSreferences remainCloses #44.
Refs #45.