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: 21 additions & 5 deletions .github/workflows/go-ci-windows.yml
Original file line number Diff line number Diff line change
@@ -1,13 +1,11 @@
# This workflow will build a golang project

name: Windows CI

on:
push:
branches: [ "windows-tests" ]
branches: [ "main" ]

pull_request:
branches: [ "windows-tests" ]
branches: [ "main" ]

# Allows manual triggering of the workflow
workflow_dispatch:
Expand All @@ -24,10 +22,28 @@ jobs:
with:
go-version: '1.23'

# cache simple-responder to save the build time
- name: Restore Simple Responder
id: restore-simple-responder
uses: actions/cache/restore@v4
with:
path: ./build
key: ${{ runner.os }}-simple-responder-${{ hashFiles('misc/simple-responder/simple-responder.go') }}

# necessary for testing proxy/Process swapping
- name: Create simple-responder
if: steps.restore-simple-responder.outputs.cache-hit != 'true'
shell: bash
run: make simple-responder
run: make simple-responder-windows

- name: Save Simple Responder
# nothing new to save ... skip this step
if: steps.restore-simple-responder.outputs.cache-hit != 'true'
id: save-simple-responder
uses: actions/cache/save@v4
with:
path: ./build
key: ${{ runner.os }}-simple-responder-${{ hashFiles('misc/simple-responder/simple-responder.go') }}

- name: Test all
shell: bash
Expand Down
19 changes: 17 additions & 2 deletions .github/workflows/go-ci.yml
Original file line number Diff line number Diff line change
@@ -1,5 +1,3 @@
# This workflow will build a golang project

name: Linux CI

on:
Expand All @@ -24,9 +22,26 @@ jobs:
with:
go-version: '1.23'

# cache simple-responder to save the build time
- name: Restore Simple Responder
id: restore-simple-responder
uses: actions/cache/restore@v4
with:
path: ./build
key: ${{ runner.os }}-simple-responder-${{ hashFiles('misc/simple-responder/simple-responder.go') }}

# necessary for testing proxy/Process swapping
- name: Create simple-responder
run: make simple-responder

- name: Save Simple Responder
# nothing new to save ... skip this step
if: steps.restore-simple-responder.outputs.cache-hit != 'true'
id: save-simple-responder
uses: actions/cache/save@v4
with:
path: ./build
key: ${{ runner.os }}-simple-responder-${{ hashFiles('misc/simple-responder/simple-responder.go') }}

- name: Test all
run: make test-all
4 changes: 4 additions & 0 deletions Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,10 @@ simple-responder:
GOOS=darwin GOARCH=arm64 go build -o $(BUILD_DIR)/simple-responder_darwin_arm64 misc/simple-responder/simple-responder.go
GOOS=linux GOARCH=amd64 go build -o $(BUILD_DIR)/simple-responder_linux_amd64 misc/simple-responder/simple-responder.go

simple-responder-windows:
@echo "Building simple responder for windows"
GOOS=windows GOARCH=amd64 go build -o $(BUILD_DIR)/simple-responder.exe misc/simple-responder/simple-responder.go

# Ensure build directory exists
$(BUILD_DIR):
mkdir -p $(BUILD_DIR)
Expand Down
2 changes: 1 addition & 1 deletion go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -5,14 +5,14 @@ go 1.23.0
require (
github.com/fsnotify/fsnotify v1.9.0
github.com/gin-gonic/gin v1.10.0
github.com/google/shlex v0.0.0-20191202100458-e7afc7fbc510
github.com/stretchr/testify v1.9.0
github.com/tidwall/gjson v1.18.0
github.com/tidwall/sjson v1.2.5
gopkg.in/yaml.v3 v3.0.1
)

require (
github.com/billziss-gh/golib v0.2.0 // indirect
github.com/bytedance/sonic v1.11.6 // indirect
github.com/bytedance/sonic/loader v0.1.1 // indirect
github.com/cloudwego/base64x v0.1.4 // indirect
Expand Down
2 changes: 2 additions & 0 deletions go.sum
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
github.com/billziss-gh/golib v0.2.0 h1:NyvcAQdfvM8xokKkKotiligKjKXzuQD4PPykg1nKc/8=
github.com/billziss-gh/golib v0.2.0/go.mod h1:mZpUYANXZkDKSnyYbX9gfnyxwe0ddRhUtfXcsD5r8dw=
github.com/bytedance/sonic v1.11.6 h1:oUp34TzMlL+OY1OUWxHqsdkgC/Zfc85zGqw9siXjrc0=
github.com/bytedance/sonic v1.11.6/go.mod h1:LysEHSvpvDySVdC2f87zGWf6CIKJcAvqab1ZaiQtds4=
github.com/bytedance/sonic/loader v0.1.1 h1:c+e5Pt1k/cy5wMveRDyk2X4B9hF4g7an8N3zCYjJFNM=
Expand Down
11 changes: 7 additions & 4 deletions proxy/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,11 +4,12 @@ import (
"fmt"
"io"
"os"
"runtime"
"sort"
"strconv"
"strings"

"github.com/google/shlex"
"github.com/billziss-gh/golib/shlex"
"gopkg.in/yaml.v3"
)

Expand Down Expand Up @@ -233,9 +234,11 @@ func SanitizeCommand(cmdStr string) ([]string, error) {
cmdStr = strings.ReplaceAll(cmdStr, "\\\n", " ")

// Split the command into arguments
args, err := shlex.Split(cmdStr)
if err != nil {
return nil, err
var args []string
if runtime.GOOS == "windows" {
args = shlex.Windows.Split(cmdStr)
} else {
args = shlex.Posix.Split(cmdStr)
}

// Ensure the command is not empty
Expand Down
36 changes: 36 additions & 0 deletions proxy/config_posix_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
//go:build !windows

package proxy

import (
"testing"

"github.com/stretchr/testify/assert"
)

func TestConfig_SanitizeCommand(t *testing.T) {
// Test a command with spaces and newlines
args, err := SanitizeCommand(`python model1.py \
-a "double quotes" \
--arg2 'single quotes'
-s
--arg3 123 \
--arg4 '"string in string"'
-c "'single quoted'"
`)
assert.NoError(t, err)
assert.Equal(t, []string{
"python", "model1.py",
"-a", "double quotes",
"--arg2", "single quotes",
"-s",
"--arg3", "123",
"--arg4", `"string in string"`,
"-c", `'single quoted'`,
}, args)

// Test an empty command
args, err = SanitizeCommand("")
assert.Error(t, err)
assert.Nil(t, args)
}
28 changes: 0 additions & 28 deletions proxy/config_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -258,34 +258,6 @@ func TestConfig_FindConfig(t *testing.T) {
assert.Equal(t, ModelConfig{}, modelConfig)
}

func TestConfig_SanitizeCommand(t *testing.T) {

// Test a command with spaces and newlines
args, err := SanitizeCommand(`python model1.py \
-a "double quotes" \
--arg2 'single quotes'
-s
--arg3 123 \
--arg4 '"string in string"'
-c "'single quoted'"
`)
assert.NoError(t, err)
assert.Equal(t, []string{
"python", "model1.py",
"-a", "double quotes",
"--arg2", "single quotes",
"-s",
"--arg3", "123",
"--arg4", `"string in string"`,
"-c", `'single quoted'`,
}, args)

// Test an empty command
args, err = SanitizeCommand("")
assert.Error(t, err)
assert.Nil(t, args)
}

func TestConfig_AutomaticPortAssignments(t *testing.T) {

t.Run("Default Port Ranges", func(t *testing.T) {
Expand Down
35 changes: 35 additions & 0 deletions proxy/config_windows_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
//go:build windows

package proxy

import (
"testing"

"github.com/stretchr/testify/assert"
)

func TestConfig_SanitizeCommand(t *testing.T) {
// does not support single quoted strings like in config_posix_test.go
args, err := SanitizeCommand(`python model1.py \
-a "double quotes" \
-s
--arg3 123 \
--arg4 '"string in string"'
-c "'single quoted'"
`)
assert.NoError(t, err)
assert.Equal(t, []string{
"python", "model1.py",
"-a", "double quotes",
"-s",
"--arg3", "123",
"--arg4", "'string in string'", // this is a little weird but the lexer says so...?
"-c", `'single quoted'`,
}, args)

// Test an empty command
args, err = SanitizeCommand("")
assert.Error(t, err)
assert.Nil(t, args)

}
Comment on lines +1 to +35
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Verification agent

🧩 Analysis chain

Consider adding Windows-specific path tests.

Since this PR aims to improve Windows path compatibility, consider adding tests specifically for Windows path handling with backslashes, which often cause issues due to their use as escape characters.

func TestConfig_SanitizeCommand(t *testing.T) {
	// existing tests...
	
+	// Test Windows path handling
+	pathArgs, pathErr := SanitizeCommand(`python "C:\Program Files\script.py" --input "D:\Data Files\input.txt"`)
+	assert.NoError(t, pathErr)
+	assert.Equal(t, []string{
+		"python", "C:\\Program Files\\script.py",
+		"--input", "D:\\Data Files\\input.txt",
+	}, pathArgs)
}

Let's verify how the current Windows command parsing handles backslashes in paths:


🏁 Script executed:

#!/bin/bash
# Check how shlex library is imported and used
grep -r "import \"github.com/mattn/go-shellwords\"" --include="*.go" .
grep -r "shlex" --include="*.go" .

Length of output: 269


Add Windows-specific backslash path tests in proxy/config_windows_test.go

To ensure SanitizeCommand correctly handles Windows paths (which include backslashes), add a test case in TestConfig_SanitizeCommand that exercises typical C:\… and D:\… paths.

• File: proxy/config_windows_test.go
Location: inside the existing func TestConfig_SanitizeCommand(t *testing.T) { … } block

 func TestConfig_SanitizeCommand(t *testing.T) {
     // existing tests…

+    // Windows path handling with backslashes
+    pathArgs, pathErr := SanitizeCommand(`python "C:\Program Files\script.py" --input "D:\Data Files\input.txt"`)
+    assert.NoError(t, pathErr)
+    assert.Equal(t, []string{
+        "python", `C:\Program Files\script.py`,
+        "--input", `D:\Data Files\input.txt`,
+    }, pathArgs)
 }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
//go:build windows
package proxy
import (
"testing"
"github.com/stretchr/testify/assert"
)
func TestConfig_SanitizeCommand(t *testing.T) {
// does not support single quoted strings like in config_posix_test.go
args, err := SanitizeCommand(`python model1.py \
-a "double quotes" \
-s
--arg3 123 \
--arg4 '"string in string"'
-c "'single quoted'"
`)
assert.NoError(t, err)
assert.Equal(t, []string{
"python", "model1.py",
"-a", "double quotes",
"-s",
"--arg3", "123",
"--arg4", "'string in string'", // this is a little weird but the lexer says so...?
"-c", `'single quoted'`,
}, args)
// Test an empty command
args, err = SanitizeCommand("")
assert.Error(t, err)
assert.Nil(t, args)
}
func TestConfig_SanitizeCommand(t *testing.T) {
// does not support single quoted strings like in config_posix_test.go
args, err := SanitizeCommand(`python model1.py \
-a "double quotes" \
-s
--arg3 123 \
--arg4 '"string in string"'
-c "'single quoted'"
`)
assert.NoError(t, err)
assert.Equal(t, []string{
"python", "model1.py",
"-a", "double quotes",
"-s",
"--arg3", "123",
"--arg4", "'string in string'", // this is a little weird but the lexer says so...?
"-c", `'single quoted'`,
}, args)
// Test an empty command
args, err = SanitizeCommand("")
assert.Error(t, err)
assert.Nil(t, args)
// Windows path handling with backslashes
pathArgs, pathErr := SanitizeCommand(`python "C:\Program Files\script.py" --input "D:\Data Files\input.txt"`)
assert.NoError(t, pathErr)
assert.Equal(t, []string{
"python", `C:\Program Files\script.py`,
"--input", `D:\Data Files\input.txt`,
}, pathArgs)
}
🤖 Prompt for AI Agents
In proxy/config_windows_test.go within the TestConfig_SanitizeCommand function
(lines 1-35), add a new test case that includes Windows-style paths with
backslashes, such as "C:\\path\\to\\file" and "D:\\another\\path", to verify
that SanitizeCommand correctly parses these paths without misinterpreting
backslashes as escape characters. This will improve test coverage for
Windows-specific path handling and ensure the function behaves as expected on
Windows systems.

7 changes: 6 additions & 1 deletion proxy/helpers_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,12 @@ func TestMain(m *testing.M) {
func getSimpleResponderPath() string {
goos := runtime.GOOS
goarch := runtime.GOARCH
return filepath.Join("..", "build", fmt.Sprintf("simple-responder_%s_%s", goos, goarch))

if goos == "windows" {
return filepath.Join("..", "build", "simple-responder.exe")
} else {
return filepath.Join("..", "build", fmt.Sprintf("simple-responder_%s_%s", goos, goarch))
}
}

func getTestPort() int {
Expand Down
8 changes: 7 additions & 1 deletion proxy/process_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import (
"net/http"
"net/http/httptest"
"os"
"runtime"
"sync"
"testing"
"time"
Expand Down Expand Up @@ -432,7 +433,12 @@ func TestProcess_ForceStopWithKill(t *testing.T) {

// unexpected EOF because the kill happened, the "1" is sent before the kill
// then the unexpected EOF is sent after the kill
assert.Equal(t, "1unexpected EOF\n", w.Body.String())
if runtime.GOOS == "windows" {
assert.Contains(t, w.Body.String(), "wsarecv: An existing connection was forcibly closed by the remote host")
} else {
assert.Contains(t, w.Body.String(), "unexpected EOF")
}

close(waitChan)
}()

Expand Down