From dfaa69f683966b3fdba1e54aa6475bbcbec74697 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E6=A7=90=E6=9E=9D?= Date: Mon, 13 Jul 2026 17:58:24 +0800 Subject: [PATCH] refactor(runtime): replace TargetGOOS with shell descriptor --- internal/agent/agent.go | 26 +++--- internal/agent/agent_test.go | 34 +++++++- internal/agent/claude_code.go | 4 +- internal/agent/claude_code_test.go | 4 +- internal/agent/cli.go | 6 +- internal/agent/codex.go | 4 +- internal/agent/codex_test.go | 4 +- internal/agent/node_install_test.go | 4 +- internal/agent/prompt_delivery_test.go | 5 +- internal/agent/qodercli.go | 4 +- internal/agent/qodercli_test.go | 4 +- internal/agent/qwen_code.go | 2 +- internal/agent/qwen_code_test.go | 5 +- internal/evaluator/evaluator_test.go | 4 +- internal/judge/helpers_test.go | 5 +- internal/judge/interpreter.go | 35 ++++---- internal/judge/interpreter_test.go | 70 +++++++++------- internal/judge/script.go | 5 +- internal/judge/script_test.go | 4 +- internal/platform/platform.go | 108 +++++++++++++++++++++---- internal/platform/platform_test.go | 84 +++++++++++++++++-- internal/platform/shell_other.go | 19 +++-- internal/platform/shell_windows.go | 37 ++------- internal/runtime/docker.go | 8 +- internal/runtime/docker_test.go | 13 +++ internal/runtime/none.go | 8 +- internal/runtime/none_test.go | 8 ++ internal/runtime/opensandbox.go | 9 ++- internal/runtime/opensandbox_test.go | 13 +++ internal/runtime/runtime.go | 33 +++++--- 30 files changed, 399 insertions(+), 170 deletions(-) diff --git a/internal/agent/agent.go b/internal/agent/agent.go index b5cf8d01..ae7fb819 100644 --- a/internal/agent/agent.go +++ b/internal/agent/agent.go @@ -24,23 +24,19 @@ import ( // ErrAgentNotFound is returned when the agent executable is not found. var ErrAgentNotFound = errors.New("agent not found in PATH") -// ErrAgentRequiresBash is returned when an agent CLI is invoked on a Windows -// host where Git Bash (or another bash discoverable by platform.DiscoverBash) -// is not available. Agent commands are POSIX-quoted and assume a bash -// interpreter; running them through the cmd.exe fallback would let metachars -// (`& | " %VAR%`) in the instruction reach the shell unprotected. See -// docs/guide/windows.md for the documented limitation. -var ErrAgentRequiresBash = errors.New("agent CLI execution on Windows requires bash; install Git for Windows or set SKILL_UP_BASH") - -// requireBashOnWindowsHost rejects agent execution when the runtime's target -// is Windows but the host shell would be cmd.exe. We only enforce this for -// runtimes whose target matches the host (NoneRuntime today); sandboxed -// runtimes target a non-Windows guest and never go through platform.Host(). -func requireBashOnWindowsHost(rt Runtime) error { - if rt.TargetGOOS() != platform.GOOSWindows { +// ErrAgentRequiresBash is returned when an agent CLI targets Windows without +// a bash-compatible target shell. Agent commands use POSIX shell syntax. +var ErrAgentRequiresBash = errors.New("agent CLI execution on Windows requires a bash target shell") + +func requireBashTargetShell(rt Runtime) error { + shell := rt.Shell() + if err := shell.Validate(); err != nil { + return fmt.Errorf("invalid runtime shell: %w", err) + } + if shell.GOOS != platform.GOOSWindows { return nil } - if platform.Host().IsBash { + if shell.IsBash() { return nil } return ErrAgentRequiresBash diff --git a/internal/agent/agent_test.go b/internal/agent/agent_test.go index 8e2a34c5..05bd5f5f 100644 --- a/internal/agent/agent_test.go +++ b/internal/agent/agent_test.go @@ -316,6 +316,7 @@ type probeMergeTestRuntime struct { probeExit int probeErr error merged map[string]string + shell platform.Shell } func (r *probeMergeTestRuntime) Create(context.Context) error { return nil } @@ -341,7 +342,38 @@ func (r *probeMergeTestRuntime) MergeEnv(env map[string]string) { } maps.Copy(r.merged, env) } -func (r *probeMergeTestRuntime) TargetGOOS() string { return platform.GOOSLinux } + +func (r *probeMergeTestRuntime) Shell() platform.Shell { + if r.shell.GOOS != "" { + return r.shell + } + return platform.Shell{GOOS: platform.GOOSLinux, Family: platform.ShellPOSIX} +} + +func TestRequireBashTargetShell(t *testing.T) { + t.Parallel() + tests := []struct { + name string + shell platform.Shell + wantErr bool + }{ + {name: "linux posix", shell: platform.Shell{GOOS: platform.GOOSLinux, Family: platform.ShellPOSIX}}, + {name: "windows bash", shell: platform.Shell{GOOS: platform.GOOSWindows, Family: platform.ShellPOSIX, BashPath: `C:\\Git\\bin\\bash.exe`}}, + {name: "windows cmd", shell: platform.Shell{GOOS: platform.GOOSWindows, Family: platform.ShellCmd}, wantErr: true}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + err := requireBashTargetShell(&probeMergeTestRuntime{shell: tt.shell}) + if tt.wantErr && !errors.Is(err, ErrAgentRequiresBash) { + t.Fatalf("error = %v, want ErrAgentRequiresBash", err) + } + if !tt.wantErr && err != nil { + t.Fatalf("unexpected error: %v", err) + } + }) + } +} func TestProbeAndMergePATH_HappyPath(t *testing.T) { t.Parallel() diff --git a/internal/agent/claude_code.go b/internal/agent/claude_code.go index ef3082cb..00cbd745 100644 --- a/internal/agent/claude_code.go +++ b/internal/agent/claude_code.go @@ -108,7 +108,7 @@ func (a *ClaudeCodeAgent) CheckCredentials(ctx context.Context) error { // Run executes the claude-code agent with the given messages via stream-json. // It streams messages to stdin and parses stream-json output to build the transcript. func (a *ClaudeCodeAgent) Run(ctx context.Context, rt Runtime, opts ExecOptions, messages []transcript.Message) (*SessionResult, error) { - if err := requireBashOnWindowsHost(rt); err != nil { + if err := requireBashTargetShell(rt); err != nil { return nil, fmt.Errorf("%s: %w", a.Name(), err) } start := time.Now() @@ -459,7 +459,7 @@ func (a *ClaudeCodeAgent) RunTurn(ctx context.Context, rt Runtime, opts ExecOpti return a.Run(ctx, rt, opts, []transcript.Message{message}) } - if err := requireBashOnWindowsHost(rt); err != nil { + if err := requireBashTargetShell(rt); err != nil { return nil, fmt.Errorf("%s: %w", a.Name(), err) } start := time.Now() diff --git a/internal/agent/claude_code_test.go b/internal/agent/claude_code_test.go index 40093033..348e008f 100644 --- a/internal/agent/claude_code_test.go +++ b/internal/agent/claude_code_test.go @@ -922,4 +922,6 @@ func (r *claudeCodeTestRuntime) MergeEnv(env map[string]string) { maps.Copy(r.mergedEnv, env) } -func (r *claudeCodeTestRuntime) TargetGOOS() string { return platform.GOOSLinux } +func (r *claudeCodeTestRuntime) Shell() platform.Shell { + return platform.Shell{GOOS: platform.GOOSLinux, Family: platform.ShellPOSIX} +} diff --git a/internal/agent/cli.go b/internal/agent/cli.go index 7c49f850..e2b965c0 100644 --- a/internal/agent/cli.go +++ b/internal/agent/cli.go @@ -124,7 +124,7 @@ func (a *CLIAgent) InstallSkill(ctx context.Context, rt Runtime, skillCfg runtim // Run executes the agent with the given messages and returns the session result. func (a *CLIAgent) Run(ctx context.Context, rt Runtime, opts ExecOptions, messages []transcript.Message) (*SessionResult, error) { - if err := requireBashOnWindowsHost(rt); err != nil { + if err := requireBashTargetShell(rt); err != nil { return nil, fmt.Errorf("%s: %w", a.Name(), err) } start := time.Now() @@ -185,14 +185,14 @@ func checkCommandForOS(checkCmd, goos string) string { // Check verifies the agent executable is available. func (a *CLIAgent) Check(ctx context.Context, rt Runtime) error { - if err := requireBashOnWindowsHost(rt); err != nil { + if err := requireBashTargetShell(rt); err != nil { return fmt.Errorf("%s: %w", a.Name(), err) } checkCmd := a.Cfg.CheckCmd if checkCmd == "" { return fmt.Errorf("CheckCmd not configured for agent %s", a.Name()) } - checkCmd = checkCommandForOS(checkCmd, rt.TargetGOOS()) + checkCmd = checkCommandForOS(checkCmd, rt.Shell().GOOS) result, err := rt.Exec(ctx, checkCmd, a.mergeExecOptionsEnv(ctx, ExecOptions{}, nil, nil)) if err != nil { diff --git a/internal/agent/codex.go b/internal/agent/codex.go index 7f764988..0556a273 100644 --- a/internal/agent/codex.go +++ b/internal/agent/codex.go @@ -242,7 +242,7 @@ func (a *CodexAgent) RunTurn(ctx context.Context, rt Runtime, opts ExecOptions, return a.Run(ctx, rt, opts, []transcript.Message{message}) } - if err := requireBashOnWindowsHost(rt); err != nil { + if err := requireBashTargetShell(rt); err != nil { return nil, fmt.Errorf("%s: %w", a.Name(), err) } start := time.Now() @@ -298,7 +298,7 @@ func (a *CodexAgent) RunTurn(ctx context.Context, rt Runtime, opts ExecOptions, // //nolint:dupl func (a *CodexAgent) Run(ctx context.Context, rt Runtime, opts ExecOptions, messages []transcript.Message) (*SessionResult, error) { - if err := requireBashOnWindowsHost(rt); err != nil { + if err := requireBashTargetShell(rt); err != nil { return nil, fmt.Errorf("%s: %w", a.Name(), err) } start := time.Now() diff --git a/internal/agent/codex_test.go b/internal/agent/codex_test.go index 85a7cc62..2504c718 100644 --- a/internal/agent/codex_test.go +++ b/internal/agent/codex_test.go @@ -1140,4 +1140,6 @@ func (r *codexTestRuntime) MergeEnv(env map[string]string) { maps.Copy(r.mergedEnv, env) } -func (r *codexTestRuntime) TargetGOOS() string { return platform.GOOSLinux } +func (r *codexTestRuntime) Shell() platform.Shell { + return platform.Shell{GOOS: platform.GOOSLinux, Family: platform.ShellPOSIX} +} diff --git a/internal/agent/node_install_test.go b/internal/agent/node_install_test.go index 7912d038..74f64d2a 100644 --- a/internal/agent/node_install_test.go +++ b/internal/agent/node_install_test.go @@ -140,4 +140,6 @@ func (r *nodeBootstrapTestRuntime) Exec(_ context.Context, command string, _ run func (r *nodeBootstrapTestRuntime) Workspace() string { return "/workspace" } func (r *nodeBootstrapTestRuntime) RequiresProcessSandbox() bool { return false } func (r *nodeBootstrapTestRuntime) MergeEnv(map[string]string) {} -func (r *nodeBootstrapTestRuntime) TargetGOOS() string { return platform.GOOSLinux } +func (r *nodeBootstrapTestRuntime) Shell() platform.Shell { + return platform.Shell{GOOS: platform.GOOSLinux, Family: platform.ShellPOSIX} +} diff --git a/internal/agent/prompt_delivery_test.go b/internal/agent/prompt_delivery_test.go index 346ac52d..5088d048 100644 --- a/internal/agent/prompt_delivery_test.go +++ b/internal/agent/prompt_delivery_test.go @@ -7,6 +7,7 @@ import ( "strings" "testing" + "github.com/alibaba/skill-up/internal/platform" "github.com/alibaba/skill-up/internal/runtime" ) @@ -142,4 +143,6 @@ func (r *promptDeliveryTestRuntime) Exec(context.Context, string, runtime.ExecOp func (r *promptDeliveryTestRuntime) MergeEnv(map[string]string) {} func (r *promptDeliveryTestRuntime) Workspace() string { return r.workspace } func (r *promptDeliveryTestRuntime) RequiresProcessSandbox() bool { return false } -func (r *promptDeliveryTestRuntime) TargetGOOS() string { return "linux" } +func (r *promptDeliveryTestRuntime) Shell() platform.Shell { + return platform.Shell{GOOS: platform.GOOSLinux, Family: platform.ShellPOSIX} +} diff --git a/internal/agent/qodercli.go b/internal/agent/qodercli.go index 25243336..e4508fa4 100644 --- a/internal/agent/qodercli.go +++ b/internal/agent/qodercli.go @@ -70,7 +70,7 @@ func (a *QoderCLIAgent) CheckCredentials(ctx context.Context) error { // //nolint:dupl func (a *QoderCLIAgent) Run(ctx context.Context, rt Runtime, opts ExecOptions, messages []transcript.Message) (*SessionResult, error) { - if err := requireBashOnWindowsHost(rt); err != nil { + if err := requireBashTargetShell(rt); err != nil { return nil, fmt.Errorf("%s: %w", a.Name(), err) } start := time.Now() @@ -237,7 +237,7 @@ func (a *QoderCLIAgent) RunTurn(ctx context.Context, rt Runtime, opts ExecOption return a.Run(ctx, rt, opts, []transcript.Message{message}) } - if err := requireBashOnWindowsHost(rt); err != nil { + if err := requireBashTargetShell(rt); err != nil { return nil, fmt.Errorf("%s: %w", a.Name(), err) } start := time.Now() diff --git a/internal/agent/qodercli_test.go b/internal/agent/qodercli_test.go index 731c1638..fdf362a8 100644 --- a/internal/agent/qodercli_test.go +++ b/internal/agent/qodercli_test.go @@ -546,4 +546,6 @@ func (r *qoderTestRuntime) MergeEnv(env map[string]string) { maps.Copy(r.mergedEnv, env) } -func (r *qoderTestRuntime) TargetGOOS() string { return platform.GOOSLinux } +func (r *qoderTestRuntime) Shell() platform.Shell { + return platform.Shell{GOOS: platform.GOOSLinux, Family: platform.ShellPOSIX} +} diff --git a/internal/agent/qwen_code.go b/internal/agent/qwen_code.go index 991b39a5..75be9881 100644 --- a/internal/agent/qwen_code.go +++ b/internal/agent/qwen_code.go @@ -125,7 +125,7 @@ func (a *QwenCodeAgent) CheckCredentials(ctx context.Context) error { // Run executes qwen non-interactively (instruction piped to `qwen --yolo`) and // builds a transcript from the session file (falling back to stdout). func (a *QwenCodeAgent) Run(ctx context.Context, rt Runtime, opts ExecOptions, messages []transcript.Message) (*SessionResult, error) { - if err := requireBashOnWindowsHost(rt); err != nil { + if err := requireBashTargetShell(rt); err != nil { return nil, fmt.Errorf("%s: %w", a.Name(), err) } start := time.Now() diff --git a/internal/agent/qwen_code_test.go b/internal/agent/qwen_code_test.go index cc5c8439..81f9a533 100644 --- a/internal/agent/qwen_code_test.go +++ b/internal/agent/qwen_code_test.go @@ -317,4 +317,7 @@ func (r *qwenTestRuntime) MergeEnv(env map[string]string) { } maps.Copy(r.mergedEnv, env) } -func (r *qwenTestRuntime) TargetGOOS() string { return platform.GOOSLinux } + +func (r *qwenTestRuntime) Shell() platform.Shell { + return platform.Shell{GOOS: platform.GOOSLinux, Family: platform.ShellPOSIX} +} diff --git a/internal/evaluator/evaluator_test.go b/internal/evaluator/evaluator_test.go index 0f3c777a..1c7e762f 100644 --- a/internal/evaluator/evaluator_test.go +++ b/internal/evaluator/evaluator_test.go @@ -100,7 +100,9 @@ func (m *mockRuntime) Workspace() string { return m.workspace } func (m *mockRuntime) RequiresProcessSandbox() bool { return true } func (m *mockRuntime) MergeEnv(_ map[string]string) {} -func (m *mockRuntime) TargetGOOS() string { return platform.GOOSLinux } +func (m *mockRuntime) Shell() platform.Shell { + return platform.Shell{GOOS: platform.GOOSLinux, Family: platform.ShellPOSIX} +} func (m *mockRuntime) Start(_ context.Context) error { return nil } func (m *mockRuntime) Stop(_ context.Context) error { return nil } func (m *mockRuntime) UploadFile(_ context.Context, _, _ string) error { return nil } diff --git a/internal/judge/helpers_test.go b/internal/judge/helpers_test.go index 53108f63..8f25c65b 100644 --- a/internal/judge/helpers_test.go +++ b/internal/judge/helpers_test.go @@ -9,6 +9,7 @@ import ( "time" "github.com/alibaba/skill-up/internal/agent" + "github.com/alibaba/skill-up/internal/platform" "github.com/alibaba/skill-up/internal/runtime" "github.com/alibaba/skill-up/pkg/transcript" ) @@ -122,7 +123,9 @@ func (m *mockJudgeTestRuntime) Workspace() string { return "/tmp/te func (m *mockJudgeTestRuntime) RequiresProcessSandbox() bool { return true } func (m *mockJudgeTestRuntime) MergeEnv(_ map[string]string) {} -func (m *mockJudgeTestRuntime) TargetGOOS() string { return "linux" } +func (m *mockJudgeTestRuntime) Shell() platform.Shell { + return platform.Shell{GOOS: platform.GOOSLinux, Family: platform.ShellPOSIX} +} func (m *mockJudgeTestRuntime) Start(_ context.Context) error { return nil } func (m *mockJudgeTestRuntime) Stop(_ context.Context) error { return nil } func (m *mockJudgeTestRuntime) UploadFile(_ context.Context, _, _ string) error { return nil } diff --git a/internal/judge/interpreter.go b/internal/judge/interpreter.go index f5af1c2e..0693419d 100644 --- a/internal/judge/interpreter.go +++ b/internal/judge/interpreter.go @@ -2,6 +2,7 @@ package judge import ( "bufio" + "errors" "fmt" "os" "path" @@ -38,13 +39,16 @@ type scriptPlan struct { func identityEnvPath(p string) string { return p } // planScript determines how to execute scriptPath in a runtime whose commands -// run on targetGOOS. +// run with the target shell. // // POSIX targets keep the original behavior: the script is uploaded verbatim // and run via its own shebang. Windows targets dispatch to an interpreter // based on the file extension (or shebang when the extension is absent). -func planScript(scriptPath, targetGOOS string) (scriptPlan, error) { - if targetGOOS != platform.GOOSWindows { +func planScript(scriptPath string, shell platform.Shell) (scriptPlan, error) { + if err := shell.Validate(); err != nil { + return scriptPlan{}, fmt.Errorf("invalid runtime shell: %w", err) + } + if shell.GOOS != platform.GOOSWindows { return scriptPlan{ uploadName: "script", command: func(remoteScript string) string { @@ -57,10 +61,15 @@ func planScript(scriptPath, targetGOOS string) (scriptPlan, error) { envPath: identityEnvPath, }, nil } - return planWindowsScript(scriptPath, platform.Host()) + return planWindowsScript(scriptPath, shell) } -func planWindowsScript(scriptPath string, shell platform.HostShell) (scriptPlan, error) { +func planWindowsScript(scriptPath string, shell platform.Shell) (scriptPlan, error) { + quote, err := shell.Quoter() + if err != nil { + return scriptPlan{}, fmt.Errorf("select runtime shell quoter: %w", err) + } + // Read and classify the shebang once: both the .ps1 (pwsh-vs-powershell // selection, option forwarding) and .sh (bash strict-mode flags) plans // consume the result. shebangInterp is "" when no recognized shebang @@ -72,12 +81,8 @@ func planWindowsScript(scriptPath string, shell platform.HostShell) (scriptPlan, ext = extensionForShebangInterpreter(shebangInterp) } - // Every command we emit (script run + cleanup) is quoted with the host - // shell's own quoter so the same shell semantics applies end to end. - // shell.Quote already accounts for bash-vs-cmd selection -- there is no - // second discovery here, ruling out the chance of the two decisions - // disagreeing. - quote := shell.Quote + // Every command we emit (script run + cleanup) uses the target shell's + // quoter so the same shell semantics applies end to end. // .ps1 / .cmd / .bat all execute through cmd.exe, so cleanup through cmd // keeps quoting / strip semantics inside one shell. The `/d /s /c` flags // match the cmd fallback in platform.Host so the strip rule behaves the @@ -135,16 +140,14 @@ func planWindowsScript(scriptPath string, shell platform.HostShell) (scriptPlan, envPath: identityEnvPath, }, nil case ".sh", ".bash": - if !shell.IsBash { - return scriptPlan{}, fmt.Errorf( - "script judge: .sh script requires bash on Windows; install Git Bash or set %s", - platform.BashEnvOverride) + if !shell.IsBash() { + return scriptPlan{}, errors.New("script judge: .sh script requires a bash target shell on Windows") } // Forward shebang-encoded options (`#!/bin/bash -eu`, // `#!/usr/bin/env -S bash -eu`, ...) so strict-mode flags that // POSIX honors via shebang aren't silently dropped when we invoke // bash explicitly on Windows. - bashArgs := []string{quote(shell.Bash)} + bashArgs := []string{quote(shell.BashPath)} // Forward only when the shebang actually names a POSIX shell, so a // `.sh` file with an unrelated shebang (e.g. `#!/usr/bin/env pwsh` // renamed to .sh) does not feed PowerShell flags to bash. diff --git a/internal/judge/interpreter_test.go b/internal/judge/interpreter_test.go index 5d4f8433..4728cd99 100644 --- a/internal/judge/interpreter_test.go +++ b/internal/judge/interpreter_test.go @@ -3,15 +3,21 @@ package judge import ( "os" "path/filepath" - goruntime "runtime" "strings" "testing" "github.com/alibaba/skill-up/internal/platform" ) +var ( + testLinuxShell = platform.Shell{GOOS: platform.GOOSLinux, Family: platform.ShellPOSIX} + testDarwinShell = platform.Shell{GOOS: platform.GOOSDarwin, Family: platform.ShellPOSIX} + testWindowsCmdShell = platform.Shell{GOOS: platform.GOOSWindows, Family: platform.ShellCmd} + testWindowsBashShell = platform.Shell{GOOS: platform.GOOSWindows, Family: platform.ShellPOSIX, BashPath: `C:\Program Files\Git\bin\bash.exe`} +) + func TestPlanScript_POSIXTarget(t *testing.T) { - plan, err := planScript("/skill/evals/check.sh", "linux") + plan, err := planScript("/skill/evals/check.sh", testLinuxShell) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -34,16 +40,20 @@ func TestPlanScript_POSIXTarget(t *testing.T) { // $ / backtick doubled so bash decodes them back to the original byte) and // plain QuoteWindows otherwise. func TestHostShellQuote(t *testing.T) { - shell := platform.Host() - got := shell.Quote(`C:\tmp\$VAR\script.cmd`) - if goruntime.GOOS != "windows" { + target := platform.Host().Target + quote, err := target.Quoter() + if err != nil { + t.Fatalf("host target Quoter: %v", err) + } + got := quote(`C:\tmp\$VAR\script.cmd`) + if target.GOOS != platform.GOOSWindows { want := `'C:\tmp\$VAR\script.cmd'` if got != want { t.Fatalf("posix shell.Quote = %q, want %q", got, want) } return } - if shell.IsBash { + if target.IsBash() { want := `"C:\\tmp\\\$VAR\\script.cmd"` if got != want { t.Fatalf("windows+bash shell.Quote = %q, want %q", got, want) @@ -126,7 +136,7 @@ func TestParseShebang(t *testing.T) { // POSIX targets preserve the original behavior: the file extension is ignored // and the script runs via its own shebang. func TestPlanScript_POSIXTarget_IgnoresExtension(t *testing.T) { - plan, err := planScript("/skill/evals/check.ps1", "darwin") + plan, err := planScript("/skill/evals/check.ps1", testDarwinShell) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -148,7 +158,7 @@ func TestPlanWindowsScript(t *testing.T) { } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - plan, err := planWindowsScript(tt.scriptPath, platform.Host()) + plan, err := planWindowsScript(tt.scriptPath, testWindowsCmdShell) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -168,7 +178,7 @@ func TestPlanWindowsScript_UnknownInterpreter(t *testing.T) { if err := os.WriteFile(scriptPath, []byte("echo hi\n"), 0o600); err != nil { t.Fatalf("write: %v", err) } - _, err := planWindowsScript(scriptPath, platform.Host()) + _, err := planWindowsScript(scriptPath, testWindowsCmdShell) if err == nil || !strings.Contains(err.Error(), "cannot determine interpreter") { t.Fatalf("expected cannot-determine-interpreter error, got: %v", err) } @@ -182,7 +192,7 @@ func TestPlanWindowsScript_PS1ShebangPwsh(t *testing.T) { if err := os.WriteFile(scriptPath, []byte("#!/usr/bin/env pwsh\nWrite-Host hi\n"), 0o600); err != nil { t.Fatalf("write: %v", err) } - plan, err := planWindowsScript(scriptPath, platform.Host()) + plan, err := planWindowsScript(scriptPath, testWindowsCmdShell) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -203,7 +213,7 @@ func TestPlanWindowsScript_PS1ShebangForwardsOpts(t *testing.T) { if err := os.WriteFile(scriptPath, []byte("#!/usr/bin/env -S pwsh -NoLogo\nWrite-Host hi\n"), 0o600); err != nil { t.Fatalf("write: %v", err) } - plan, err := planWindowsScript(scriptPath, platform.Host()) + plan, err := planWindowsScript(scriptPath, testWindowsCmdShell) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -227,7 +237,7 @@ func TestPlanWindowsScript_PS1NoShebangUsesLegacyDefault(t *testing.T) { if err := os.WriteFile(scriptPath, []byte("Write-Host hi\n"), 0o600); err != nil { t.Fatalf("write: %v", err) } - plan, err := planWindowsScript(scriptPath, platform.Host()) + plan, err := planWindowsScript(scriptPath, testWindowsCmdShell) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -237,21 +247,22 @@ func TestPlanWindowsScript_PS1NoShebangUsesLegacyDefault(t *testing.T) { } } -// TestPlanWindowsScript_ShellScript covers the .sh branch, whose outcome -// depends on whether bash is discoverable on the host running the test. +// TestPlanWindowsScript_ShellScript proves the planner consumes the injected +// target shell instead of re-discovering the shell on the test host. func TestPlanWindowsScript_ShellScript(t *testing.T) { - plan, err := planWindowsScript(`C:\skill\check.sh`, platform.Host()) - if _, ok := platform.DiscoverBash(); ok { - if err != nil { - t.Fatalf("bash is available but planning failed: %v", err) - } - if plan.uploadName != "script.sh" { - t.Fatalf("uploadName = %q, want \"script.sh\"", plan.uploadName) - } - return + plan, err := planScript(`C:\skill\check.sh`, testWindowsBashShell) + if err != nil { + t.Fatalf("planScript: %v", err) + } + if plan.uploadName != "script.sh" { + t.Fatalf("uploadName = %q, want \"script.sh\"", plan.uploadName) } - if err == nil || !strings.Contains(err.Error(), "requires bash on Windows") { - t.Fatalf("expected bash-required error, got: %v", err) +} + +func TestPlanWindowsScript_ShellScriptRejectsCmdTarget(t *testing.T) { + _, err := planScript(`C:\skill\check.sh`, testWindowsCmdShell) + if err == nil || !strings.Contains(err.Error(), "requires a bash target shell") { + t.Fatalf("error = %v, want bash target shell requirement", err) } } @@ -296,7 +307,7 @@ func TestShebangExtension(t *testing.T) { } func TestCleanupCommand_POSIX(t *testing.T) { - plan, err := planScript("/skill/check.sh", "linux") + plan, err := planScript("/skill/check.sh", testLinuxShell) if err != nil { t.Fatalf("planScript: %v", err) } @@ -306,7 +317,7 @@ func TestCleanupCommand_POSIX(t *testing.T) { } func TestCleanupCommand_Windows(t *testing.T) { - plan, err := planWindowsScript(`C:\skill\check.ps1`, platform.Host()) + plan, err := planWindowsScript(`C:\skill\check.ps1`, testWindowsCmdShell) if err != nil { t.Fatalf("planWindowsScript: %v", err) } @@ -319,10 +330,7 @@ func TestCleanupCommand_Windows(t *testing.T) { // .sh on Windows cleans up via bash `rm -rf` rather than dispatching to cmd, // avoiding the bash -> cmd hop that the .ps1/.cmd/.bat paths necessarily take. func TestCleanupCommand_Windows_ShellScriptUsesBashRm(t *testing.T) { - if _, ok := platform.DiscoverBash(); !ok { - t.Skip("requires bash to plan a .sh Windows script") - } - plan, err := planWindowsScript(`C:\skill\check.sh`, platform.Host()) + plan, err := planWindowsScript(`C:\skill\check.sh`, testWindowsBashShell) if err != nil { t.Fatalf("planWindowsScript: %v", err) } diff --git a/internal/judge/script.go b/internal/judge/script.go index 441737b4..16f556cb 100644 --- a/internal/judge/script.go +++ b/internal/judge/script.go @@ -74,8 +74,9 @@ func (j *ScriptJudge) runtime(ctx context.Context) (evalruntime.Runtime, func(), } func (j *ScriptJudge) evaluateInRuntime(ctx context.Context, rt evalruntime.Runtime, in Input, timeout time.Duration) (*Result, error) { - targetGOOS := rt.TargetGOOS() - plan, err := planScript(j.ScriptPath, targetGOOS) + shell := rt.Shell() + targetGOOS := shell.GOOS + plan, err := planScript(j.ScriptPath, shell) if err != nil { return nil, fmt.Errorf("script execution failed: %w", err) } diff --git a/internal/judge/script_test.go b/internal/judge/script_test.go index 3af25501..3ddae782 100644 --- a/internal/judge/script_test.go +++ b/internal/judge/script_test.go @@ -262,7 +262,9 @@ func (r *scriptJudgeRuntime) Workspace() string { return r.workspace func (r *scriptJudgeRuntime) RequiresProcessSandbox() bool { return true } func (r *scriptJudgeRuntime) MergeEnv(_ map[string]string) {} -func (r *scriptJudgeRuntime) TargetGOOS() string { return "linux" } +func (r *scriptJudgeRuntime) Shell() platform.Shell { + return platform.Shell{GOOS: platform.GOOSLinux, Family: platform.ShellPOSIX} +} func (r *scriptJudgeRuntime) UploadFile(_ context.Context, _, targetPath string) error { r.uploads = append(r.uploads, targetPath) diff --git a/internal/platform/platform.go b/internal/platform/platform.go index 944502e0..1b22575d 100644 --- a/internal/platform/platform.go +++ b/internal/platform/platform.go @@ -5,7 +5,12 @@ package platform import ( "context" + "errors" + "fmt" "os/exec" + "strings" + + "github.com/alibaba/skill-up/internal/shellquote" ) // BashEnvOverride is the environment variable a user may set to point at a @@ -14,7 +19,7 @@ import ( const BashEnvOverride = "SKILL_UP_BASH" // GOOS-value constants that callers across packages compare against -// runtime.GOOS / Runtime.TargetGOOS(). Centralizing them avoids string +// runtime.GOOS / Runtime.Shell(). Centralizing them avoids string // literals duplicated across the OS-dispatch sites. const ( GOOSWindows = "windows" @@ -22,27 +27,102 @@ const ( GOOSDarwin = "darwin" ) +// ShellFamily identifies the command language understood by a runtime's +// outer command interpreter. It deliberately describes syntax rather than a +// concrete executable: remote runtimes may choose bash or sh dynamically. +type ShellFamily string + +const ( + // ShellPOSIX is the sh-compatible command language used by POSIX shells. + ShellPOSIX ShellFamily = "posix" + // ShellCmd is the Windows cmd.exe command language. + ShellCmd ShellFamily = "cmd" +) + +// Shell describes the target command interpreter used by a runtime's Exec +// method. GOOS is the target environment, not necessarily the skill-up host. +type Shell struct { + GOOS string + Family ShellFamily + // BashPath is set when a Windows POSIX target explicitly invokes bash. + // POSIX targets may leave it empty when the runtime owns shell selection. + BashPath string +} + +// Validate rejects incomplete or contradictory target-shell descriptors. +func (s Shell) Validate() error { + if s.GOOS == "" { + return errors.New("shell GOOS must not be empty") + } + switch s.Family { + case ShellPOSIX: + if s.GOOS == GOOSWindows && s.BashPath == "" { + return errors.New("windows POSIX shell requires BashPath") + } + case ShellCmd: + if s.GOOS != GOOSWindows { + return fmt.Errorf("cmd shell requires windows GOOS, got %q", s.GOOS) + } + if s.BashPath != "" { + return errors.New("cmd shell must not set BashPath") + } + case "": + return errors.New("shell family must not be empty") + default: + return fmt.Errorf("unsupported shell family %q", s.Family) + } + return nil +} + +// Quoter returns the argument quoting function matching the target shell. +func (s Shell) Quoter() (func(string) string, error) { + if err := s.Validate(); err != nil { + return nil, err + } + if s.Family == ShellCmd { + return shellquote.QuoteWindows, nil + } + if s.GOOS == GOOSWindows { + return quoteForBashDoubleQuote, nil + } + return shellquote.QuotePOSIX, nil +} + +// IsBash reports whether the descriptor identifies an explicit bash target. +func (s Shell) IsBash() bool { + return s.Family == ShellPOSIX && s.BashPath != "" +} + // HostShell describes the shell that NoneRuntime.Exec will use on the -// current host. Callers that need to quote arguments for the same shell or -// inject extra environment variables it depends on should reach for Quote -// and Env rather than re-deriving them. +// current host. Callers that need to quote arguments for the same shell use +// Target.Quoter; Env carries any required environment variables. type HostShell struct { + // Target describes the command language interpreted by Cmd. + Target Shell // Cmd builds an exec.Cmd configured to run `command` through the host // shell. The caller still sets Dir, Env (which must be merged with // HostShell.Env), Stdout, and Stderr. Cmd func(ctx context.Context, command string) *exec.Cmd - // Quote returns the argument-quoting suitable for the host shell. - Quote func(s string) string // Env lists extra environment variables the shell needs to behave // predictably (for example MSYS_NO_PATHCONV for Git Bash on Windows). // Callers append it to their own env list; nil means "no extras". Env []string - // IsBash reports whether the chosen shell is bash. POSIX hosts are - // always true; Windows is true only when DiscoverBash succeeded. - IsBash bool - // Bash is the discovered bash interpreter path, populated when IsBash - // is true. Callers building "bash invokes bash" pipelines (the .sh - // script-judge plan on Windows) read it from here so the choice of - // bash binary matches what Cmd will launch. - Bash string +} + +// quoteForBashDoubleQuote returns s wrapped in double quotes with every +// character that bash treats as active inside double quotes escaped with a +// backslash. It is used for a Windows target executed through Git Bash. +func quoteForBashDoubleQuote(s string) string { + var b strings.Builder + b.Grow(len(s) + 2) + b.WriteByte('"') + for i := range len(s) { + c := s[i] + if c == '\\' || c == '"' || c == '$' || c == '`' { + b.WriteByte('\\') + } + b.WriteByte(c) + } + b.WriteByte('"') + return b.String() } diff --git a/internal/platform/platform_test.go b/internal/platform/platform_test.go index b793c1df..defa770a 100644 --- a/internal/platform/platform_test.go +++ b/internal/platform/platform_test.go @@ -2,17 +2,18 @@ package platform import ( "context" + "strings" "testing" ) func TestHost(t *testing.T) { shell := Host() + if err := shell.Target.Validate(); err != nil { + t.Fatalf("Host returned an invalid target shell: %v", err) + } if shell.Cmd == nil { t.Fatal("Host returned a HostShell with nil Cmd") } - if shell.Quote == nil { - t.Fatal("Host returned a HostShell with nil Quote") - } cmd := shell.Cmd(context.Background(), "echo hi") if cmd == nil { t.Fatal("HostShell.Cmd returned nil") @@ -22,7 +23,80 @@ func TestHost(t *testing.T) { } // On POSIX hosts bash should be discoverable (PATH has it); IsBash // being false would point at a misconfigured runner. - if !shell.IsBash { - t.Logf("note: HostShell.IsBash is false (no bash discovered); the cmd fallback is exercised") + if !shell.Target.IsBash() { + t.Logf("note: HostShell.Target.IsBash is false (no bash discovered); the fallback is exercised") + } +} + +func TestShellValidate(t *testing.T) { + tests := []struct { + name string + shell Shell + wantErr string + }{ + {name: "linux posix", shell: Shell{GOOS: GOOSLinux, Family: ShellPOSIX}}, + {name: "windows bash", shell: Shell{GOOS: GOOSWindows, Family: ShellPOSIX, BashPath: `C:\Program Files\Git\bin\bash.exe`}}, + {name: "windows cmd", shell: Shell{GOOS: GOOSWindows, Family: ShellCmd}}, + {name: "empty goos", shell: Shell{Family: ShellPOSIX}, wantErr: "GOOS must not be empty"}, + {name: "empty family", shell: Shell{GOOS: GOOSLinux}, wantErr: "family must not be empty"}, + {name: "unknown family", shell: Shell{GOOS: GOOSLinux, Family: ShellFamily("fish")}, wantErr: `unsupported shell family "fish"`}, + {name: "cmd on linux", shell: Shell{GOOS: GOOSLinux, Family: ShellCmd}, wantErr: "cmd shell requires windows"}, + {name: "windows posix without bash", shell: Shell{GOOS: GOOSWindows, Family: ShellPOSIX}, wantErr: "requires BashPath"}, + {name: "cmd with bash path", shell: Shell{GOOS: GOOSWindows, Family: ShellCmd, BashPath: `C:\bash.exe`}, wantErr: "cmd shell must not set BashPath"}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := tt.shell.Validate() + if tt.wantErr == "" { + if err != nil { + t.Fatalf("Validate() error = %v", err) + } + return + } + if err == nil || !strings.Contains(err.Error(), tt.wantErr) { + t.Fatalf("Validate() error = %v, want substring %q", err, tt.wantErr) + } + }) + } +} + +func TestShellQuoter(t *testing.T) { + tests := []struct { + name string + shell Shell + input string + want string + }{ + { + name: "posix", + shell: Shell{GOOS: GOOSLinux, Family: ShellPOSIX}, + input: "a'b", + want: `'a'\''b'`, + }, + { + name: "windows bash", + shell: Shell{GOOS: GOOSWindows, Family: ShellPOSIX, BashPath: `C:\Program Files\Git\bin\bash.exe`}, + input: `C:\tmp\$VAR\script.sh`, + want: `"C:\\tmp\\\$VAR\\script.sh"`, + }, + { + name: "windows cmd", + shell: Shell{GOOS: GOOSWindows, Family: ShellCmd}, + input: `C:\tmp\$VAR\script.cmd`, + want: `"C:\tmp\$VAR\script.cmd"`, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + quote, err := tt.shell.Quoter() + if err != nil { + t.Fatalf("Quoter() error = %v", err) + } + if got := quote(tt.input); got != tt.want { + t.Fatalf("quote(%q) = %q, want %q", tt.input, got, tt.want) + } + }) } } diff --git a/internal/platform/shell_other.go b/internal/platform/shell_other.go index bb7bd6fe..d42741b5 100644 --- a/internal/platform/shell_other.go +++ b/internal/platform/shell_other.go @@ -5,18 +5,15 @@ package platform import ( "context" "os/exec" + "runtime" "sync" - - "github.com/alibaba/skill-up/internal/shellquote" ) // Host returns the descriptor of the shell NoneRuntime.Exec will use on the -// current host: how to launch a command, how to quote an argument for that -// shell, and any extra environment variables the shell needs to behave -// predictably. +// current host: its target command language, how to launch a command, and any +// extra environment variables the shell needs to behave predictably. // -// Centralizing all three lets callers (the script-judge planner especially) -// pick a quoter that matches the shell actually launched, without +// Centralizing these details lets callers select quoting from Target without // re-deriving the shell choice independently. // // On POSIX the shell is bash when discoverable, sh otherwise. POSIX @@ -36,11 +33,13 @@ func buildHostShell() HostShell { shell = bash } return HostShell{ + Target: Shell{ + GOOS: runtime.GOOS, + Family: ShellPOSIX, + BashPath: bash, + }, Cmd: func(ctx context.Context, command string) *exec.Cmd { return exec.CommandContext(ctx, shell, "-c", command) }, - Quote: shellquote.QuotePOSIX, - IsBash: hasBash, - Bash: bash, } } diff --git a/internal/platform/shell_windows.go b/internal/platform/shell_windows.go index 6c0b5cf3..da7b4412 100644 --- a/internal/platform/shell_windows.go +++ b/internal/platform/shell_windows.go @@ -5,11 +5,8 @@ package platform import ( "context" "os/exec" - "strings" "sync" "syscall" - - "github.com/alibaba/skill-up/internal/shellquote" ) // Host returns the descriptor of the shell NoneRuntime.Exec will use on the @@ -44,48 +41,26 @@ func buildHostShell() HostShell { bash, hasBash := DiscoverBash() if hasBash { return HostShell{ + Target: Shell{ + GOOS: GOOSWindows, + Family: ShellPOSIX, + BashPath: bash, + }, Cmd: func(ctx context.Context, command string) *exec.Cmd { return exec.CommandContext(ctx, bash, "-c", command) }, - Quote: quoteForBashDoubleQuote, Env: []string{ "MSYS_NO_PATHCONV=1", "MSYS2_ARG_CONV_EXCL=*", }, - IsBash: true, - Bash: bash, } } return HostShell{ + Target: Shell{GOOS: GOOSWindows, Family: ShellCmd}, Cmd: func(ctx context.Context, command string) *exec.Cmd { cmd := exec.CommandContext(ctx, "cmd") cmd.SysProcAttr = &syscall.SysProcAttr{CmdLine: `cmd /d /s /c "` + command + `"`} return cmd }, - Quote: shellquote.QuoteWindows, - IsBash: false, - } -} - -// quoteForBashDoubleQuote returns s wrapped in double quotes with every -// character that bash treats as active inside double quotes escaped with a -// backslash. The four actives are \, ", $, `. After bash decodes the -// resulting string each of those bytes is delivered intact to the program -// bash spawns (cmd / powershell / a second bash), so a path like -// `C:\tmp\$foo\script.ps1` survives the bash -c hop without losing the -// backslash before `$`. cmd.exe never sees this encoding because we only -// pick it when bash is the chosen shell. -func quoteForBashDoubleQuote(s string) string { - var b strings.Builder - b.Grow(len(s) + 2) - b.WriteByte('"') - for i := range len(s) { - c := s[i] - if c == '\\' || c == '"' || c == '$' || c == '`' { - b.WriteByte('\\') - } - b.WriteByte(c) } - b.WriteByte('"') - return b.String() } diff --git a/internal/runtime/docker.go b/internal/runtime/docker.go index ecf26b29..affb5d33 100644 --- a/internal/runtime/docker.go +++ b/internal/runtime/docker.go @@ -247,10 +247,10 @@ func (r *DockerRuntime) Close() error { return nil } -// TargetGOOS reports the GOOS of the container's guest OS. skill-up's -// docker runtime currently provisions a Linux image, so commands executed -// via `docker exec` run on a Linux guest regardless of the host platform. -func (r *DockerRuntime) TargetGOOS() string { return platform.GOOSLinux } +// Shell reports the POSIX command language used by DockerRuntime.Exec. +func (r *DockerRuntime) Shell() platform.Shell { + return platform.Shell{GOOS: platform.GOOSLinux, Family: platform.ShellPOSIX} +} // Start starts the container if it is not already running. func (r *DockerRuntime) Start(ctx context.Context) error { diff --git a/internal/runtime/docker_test.go b/internal/runtime/docker_test.go index dca4c376..0e8b7bbb 100644 --- a/internal/runtime/docker_test.go +++ b/internal/runtime/docker_test.go @@ -9,6 +9,8 @@ import ( "sync" "sync/atomic" "testing" + + "github.com/alibaba/skill-up/internal/platform" ) // fakeDockerCall is one captured invocation of the docker CLI. @@ -85,6 +87,17 @@ func newDockerRuntimeForTest(t *testing.T, cfg Config, fd *fakeDocker) *DockerRu return r } +func TestDockerRuntimeShellIsLinuxPOSIX(t *testing.T) { + rt, err := NewDockerRuntime(Config{Image: "alpine:3.20"}) + if err != nil { + t.Fatalf("NewDockerRuntime: %v", err) + } + want := platform.Shell{GOOS: platform.GOOSLinux, Family: platform.ShellPOSIX} + if got := rt.Shell(); got != want { + t.Fatalf("Shell() = %+v, want %+v", got, want) + } +} + // createScript returns the canonical scripted-call sequence that Create // performs (docker create → docker start → docker exec mkdir -p workspace). // Test scripts prepend this before the steps they actually want to exercise. diff --git a/internal/runtime/none.go b/internal/runtime/none.go index ecb46b20..5605f734 100644 --- a/internal/runtime/none.go +++ b/internal/runtime/none.go @@ -8,7 +8,6 @@ import ( "os" "os/exec" "path/filepath" - goruntime "runtime" "time" "go.opentelemetry.io/otel/attribute" @@ -382,8 +381,5 @@ func (r *NoneRuntime) RequiresProcessSandbox() bool { return true } -// TargetGOOS reports the host OS, since NoneRuntime executes commands directly -// on the host. -func (r *NoneRuntime) TargetGOOS() string { - return goruntime.GOOS -} +// Shell reports the host shell selected by NoneRuntime.Exec. +func (r *NoneRuntime) Shell() platform.Shell { return platform.Host().Target } diff --git a/internal/runtime/none_test.go b/internal/runtime/none_test.go index 2b2a1aa4..74d092b0 100644 --- a/internal/runtime/none_test.go +++ b/internal/runtime/none_test.go @@ -18,6 +18,7 @@ import ( "go.opentelemetry.io/otel/sdk/trace/tracetest" "github.com/alibaba/skill-up/internal/logging" + "github.com/alibaba/skill-up/internal/platform" ) // osWindows is the goruntime.GOOS value for Windows, used by tests that skip @@ -27,6 +28,13 @@ const osWindows = "windows" var logCaptureMu sync.Mutex +func TestNoneRuntimeShellMatchesHostTarget(t *testing.T) { + rt := &NoneRuntime{} + if got, want := rt.Shell(), platform.Host().Target; got != want { + t.Fatalf("Shell() = %+v, want host target %+v", got, want) + } +} + func TestNoneRuntime_CreateAndClose(t *testing.T) { t.Parallel() diff --git a/internal/runtime/opensandbox.go b/internal/runtime/opensandbox.go index 39f4b1d2..3a320fec 100644 --- a/internal/runtime/opensandbox.go +++ b/internal/runtime/opensandbox.go @@ -19,6 +19,7 @@ import ( "github.com/alibaba/skill-up/internal/logging" "github.com/alibaba/skill-up/internal/observability" + "github.com/alibaba/skill-up/internal/platform" "github.com/alibaba/skill-up/internal/shellquote" ) @@ -557,10 +558,10 @@ func (r *OpenSandboxRuntime) MergeEnv(env map[string]string) { mergeIntoEnvBaseline(&r.cfg.Env, env) } -// TargetGOOS reports "linux": OpenSandbox always executes commands inside a -// Linux sandbox regardless of the host OS. -func (r *OpenSandboxRuntime) TargetGOOS() string { - return "linux" +// Shell reports the POSIX command language used by the current Linux +// OpenSandbox profile. A future Windows profile must return its target shell. +func (r *OpenSandboxRuntime) Shell() platform.Shell { + return platform.Shell{GOOS: platform.GOOSLinux, Family: platform.ShellPOSIX} } func (r *OpenSandboxRuntime) connectionConfig() opensandbox.ConnectionConfig { diff --git a/internal/runtime/opensandbox_test.go b/internal/runtime/opensandbox_test.go index 86f22405..90ab4789 100644 --- a/internal/runtime/opensandbox_test.go +++ b/internal/runtime/opensandbox_test.go @@ -16,6 +16,8 @@ import ( "time" opensandbox "github.com/alibaba/OpenSandbox/sdks/sandbox/go" + + "github.com/alibaba/skill-up/internal/platform" ) const testWorkspaceMount = "/work" @@ -110,6 +112,17 @@ func TestOpenSandboxCreateUsesSDKOptions(t *testing.T) { } } +func TestOpenSandboxRuntimeShellIsLinuxPOSIX(t *testing.T) { + rt, err := NewOpenSandboxRuntime(Config{}) + if err != nil { + t.Fatalf("NewOpenSandboxRuntime: %v", err) + } + want := platform.Shell{GOOS: platform.GOOSLinux, Family: platform.ShellPOSIX} + if got := rt.Shell(); got != want { + t.Fatalf("Shell() = %+v, want %+v", got, want) + } +} + func TestOpenSandboxCreatePassesNetworkPolicy(t *testing.T) { origCreate := createOpenSandbox t.Cleanup(func() { createOpenSandbox = origCreate }) diff --git a/internal/runtime/runtime.go b/internal/runtime/runtime.go index 4c031e23..c5681a51 100644 --- a/internal/runtime/runtime.go +++ b/internal/runtime/runtime.go @@ -3,11 +3,14 @@ package runtime import ( "context" "errors" + "fmt" "io" "maps" "os" "strings" "time" + + "github.com/alibaba/skill-up/internal/platform" ) const ( @@ -143,15 +146,10 @@ type Runtime interface { Workspace() string // RequiresProcessSandbox reports whether agents should enable their own process sandbox. RequiresProcessSandbox() bool - // TargetGOOS reports the GOOS value of the environment where Exec - // runs commands. NoneRuntime executes on the host so it returns - // runtime.GOOS; OpenSandboxRuntime always returns "linux" because - // it executes inside a Linux sandbox. Implementations must return a - // non-empty value -- callers (most importantly the script-judge - // planner) use it to choose between POSIX and Windows command shapes, - // and silently defaulting to "linux" would mask configuration - // mistakes in any future Windows-targeting runtime. - TargetGOOS() string + // Shell describes the target command interpreter used by Exec. The target + // may differ from the skill-up host (for example, a Linux container on a + // Windows host), so callers must not re-derive it from platform.Host. + Shell() platform.Shell } // FileReadSeeker combines io.ReadSeeker for file access. @@ -216,14 +214,25 @@ type SkillConfig struct { // NewRuntime creates a Runtime based on the config type. func NewRuntime(cfg Config) (Runtime, error) { + var ( + rt Runtime + err error + ) switch cfg.Type { case "none": - return &NoneRuntime{cfg: cfg}, nil + rt = &NoneRuntime{cfg: cfg} case "opensandbox": - return NewOpenSandboxRuntime(cfg) + rt, err = NewOpenSandboxRuntime(cfg) case "docker": - return NewDockerRuntime(cfg) + rt, err = NewDockerRuntime(cfg) default: return nil, errors.New("unknown runtime type: " + cfg.Type) } + if err != nil { + return nil, err + } + if err := rt.Shell().Validate(); err != nil { + return nil, fmt.Errorf("invalid %s runtime shell: %w", cfg.Type, err) + } + return rt, nil }