Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 11 additions & 15 deletions internal/agent/agent.go
Original file line number Diff line number Diff line change
Expand Up @@ -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")
Comment thread
zpzjzj marked this conversation as resolved.

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
Expand Down
34 changes: 33 additions & 1 deletion internal/agent/agent_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 }
Expand All @@ -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()
Expand Down
4 changes: 2 additions & 2 deletions internal/agent/claude_code.go
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand Down Expand Up @@ -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()
Expand Down
4 changes: 3 additions & 1 deletion internal/agent/claude_code_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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}
}
6 changes: 3 additions & 3 deletions internal/agent/cli.go
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand Down Expand Up @@ -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 {
Expand Down
4 changes: 2 additions & 2 deletions internal/agent/codex.go
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand Down Expand Up @@ -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()
Expand Down
4 changes: 3 additions & 1 deletion internal/agent/codex_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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}
}
4 changes: 3 additions & 1 deletion internal/agent/node_install_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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}
}
5 changes: 4 additions & 1 deletion internal/agent/prompt_delivery_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import (
"strings"
"testing"

"github.com/alibaba/skill-up/internal/platform"
"github.com/alibaba/skill-up/internal/runtime"
)

Expand Down Expand Up @@ -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}
}
4 changes: 2 additions & 2 deletions internal/agent/qodercli.go
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand Down Expand Up @@ -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()
Expand Down
4 changes: 3 additions & 1 deletion internal/agent/qodercli_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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}
}
2 changes: 1 addition & 1 deletion internal/agent/qwen_code.go
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand Down
5 changes: 4 additions & 1 deletion internal/agent/qwen_code_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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}
}
4 changes: 3 additions & 1 deletion internal/evaluator/evaluator_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 }
Expand Down
5 changes: 4 additions & 1 deletion internal/judge/helpers_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)
Expand Down Expand Up @@ -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 }
Expand Down
35 changes: 19 additions & 16 deletions internal/judge/interpreter.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package judge

import (
"bufio"
"errors"
"fmt"
"os"
"path"
Expand Down Expand Up @@ -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 {
Expand All @@ -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
Expand All @@ -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
Expand Down Expand Up @@ -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")
}
Comment thread
zpzjzj marked this conversation as resolved.
// 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.
Expand Down
Loading
Loading