diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index b4c36f1..d075d90 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -33,6 +33,28 @@ jobs: - name: go test run: go test -race ./... + windows-test: + name: Windows Owner Suite + runs-on: windows-2025 + permissions: + contents: read + steps: + - uses: actions/checkout@v7 + + - name: Set up Go + uses: actions/setup-go@v7 + with: + go-version-file: go.mod + + - name: Configure git for private modules + run: git config --global url."https://${{ secrets.RELEASES_TOKEN }}@github.com/".insteadOf "https://github.com/" + + - name: go vet + run: go vet ./... + + - name: go test + run: go test -race -p=1 ./... + validate-contracts: runs-on: ubuntu-latest permissions: diff --git a/CHANGELOG.md b/CHANGELOG.md index ff6c063..fd5bcd3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - README verified-status banner per workflow#714 (multi-repo QoL sweep). - CONTRIBUTING.md, examples/minimal/config.yaml, and GitHub issue/PR templates. +### Fixed +- Normalize native Windows separators for policy glob matching, including deny rules used by the executor. +- Validate Linux container mount destinations independently of host path conventions, preserving critical-directory denials. +- Correct platform-specific owner-suite fixtures and assertions while retaining the full Windows and Linux test suites. + ## [v0.9.3] - 2026-05-19 Initial CHANGELOG entry tracking the current release. See git tags for prior versions. diff --git a/executor/policy_path_integration_test.go b/executor/policy_path_integration_test.go new file mode 100644 index 0000000..97f12d3 --- /dev/null +++ b/executor/policy_path_integration_test.go @@ -0,0 +1,114 @@ +package executor + +import ( + "context" + "os" + "path/filepath" + "runtime" + "strings" + "testing" + + "github.com/GoCodeAlone/workflow-plugin-agent/policy" + "github.com/GoCodeAlone/workflow-plugin-agent/provider" + "github.com/GoCodeAlone/workflow-plugin-agent/tools" +) + +func TestExecute_PolicyBridgeNativePathDeny(t *testing.T) { + home := t.TempDir() + if runtime.GOOS == "windows" { + t.Setenv("USERPROFILE", home) + } else { + t.Setenv("HOME", home) + } + if err := os.Mkdir(filepath.Join(home, ".ssh"), 0o700); err != nil { + t.Fatal(err) + } + deniedPath := filepath.Join(home, ".ssh", "id_rsa") + allowedPath := filepath.Join(home, "notes.txt") + if err := os.WriteFile(deniedPath, []byte("denied fixture"), 0o600); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(allowedPath, []byte("allowed sentinel"), 0o600); err != nil { + t.Fatal(err) + } + + // The provider supplies tool calls, and the tool records real file reads. + // Policy, its bridge, the executor, and registry run without substitutes. + var invokedPaths []string + reg := tools.NewRegistry() + reg.Register(&simpleTool{ + name: "file_read", + def: provider.ToolDef{ + Name: "file_read", + Description: "read a fixture file", + }, + fn: func(_ context.Context, args map[string]any) (any, error) { + path := args["path"].(string) + invokedPaths = append(invokedPaths, path) + data, err := os.ReadFile(path) + return string(data), err + }, + }) + te := policy.NewTrustEngine("custom", []policy.TrustRule{ + {Pattern: "file_read", Action: policy.Allow}, + {Pattern: "path:**", Action: policy.Allow}, + {Pattern: "path:~/.ssh/*", Action: policy.Deny}, + }, nil) + chatCalls := 0 + p := &callCountProvider{ + onChat: func() (*provider.Response, error) { + chatCalls++ + if chatCalls == 1 { + return &provider.Response{ToolCalls: []provider.ToolCall{ + {ID: "native-denied", Name: "file_read", Arguments: map[string]any{"path": deniedPath}}, + {ID: "native-allowed", Name: "file_read", Arguments: map[string]any{"path": allowedPath}}, + }}, nil + } + return &provider.Response{Content: "done"}, nil + }, + } + var events []Event + result, err := Execute(context.Background(), Config{ + Provider: p, + ToolRegistry: reg, + TrustEngine: NewPolicyTrustBridge(te), + MaxIterations: 2, + OnEvent: func(e Event) { + events = append(events, e) + }, + }, "read the allowed fixture", "check native path trust", "path-regression") + if err != nil { + t.Fatalf("Execute: %v", err) + } + if result.Status != "completed" || chatCalls != 2 { + t.Errorf("executor completion: status %q, provider calls %d; want completed after 2 calls", result.Status, chatCalls) + } + if len(invokedPaths) != 1 || invokedPaths[0] != allowedPath { + t.Errorf("file_read invoked for %q, want only allowed path %q; denied native path %q must never execute", invokedPaths, allowedPath, deniedPath) + } + + denyEvents, allowEvents := 0, 0 + toolResults := make(map[string]Event) + for _, e := range events { + switch e.Type { + case EventTrustDeny: + if e.ToolName != "file_read" { + t.Errorf("trust deny tool = %q, want file_read", e.ToolName) + } + denyEvents++ + case EventTrustAllow: + allowEvents++ + case EventToolCallResult: + toolResults[e.ToolCallID] = e + } + } + if denyEvents != 1 || allowEvents != 1 { + t.Errorf("trust events: deny %d, allow %d; want one deny and one allowed control", denyEvents, allowEvents) + } + if e, ok := toolResults["native-denied"]; !ok || !e.ToolError || !strings.Contains(e.ToolResult, "denied by trust policy") { + t.Errorf("denied tool result = %+v (present %v), want trust-policy error", e, ok) + } + if e, ok := toolResults["native-allowed"]; !ok || e.ToolError || e.ToolResult != `"allowed sentinel"` { + t.Errorf("allowed tool result = %+v (present %v), want actual fixture contents", e, ok) + } +} diff --git a/genkit/pty_provider_test.go b/genkit/pty_provider_test.go index 2abf894..8f0bab6 100644 --- a/genkit/pty_provider_test.go +++ b/genkit/pty_provider_test.go @@ -6,20 +6,21 @@ import ( "os" "os/exec" "path/filepath" + "runtime" "testing" "time" "github.com/GoCodeAlone/workflow-plugin-agent/provider" ) -// mockCLIAdapter is a test adapter that drives a shell script mock CLI. +// mockCLIAdapter is a test adapter that drives a compiled mock CLI. type mockCLIAdapter struct { name string binary string workDir string } -func (m *mockCLIAdapter) Name() string { return m.name } +func (m *mockCLIAdapter) Name() string { return m.name } func (m *mockCLIAdapter) Binary() string { return m.binary } func (m *mockCLIAdapter) NonInteractiveArgs(msg string) []string { @@ -82,6 +83,9 @@ func main() { dir := t.TempDir() srcFile := filepath.Join(dir, "mock_cli.go") binFile := filepath.Join(dir, "mock_cli") + if runtime.GOOS == "windows" { + binFile += ".exe" + } if err := os.WriteFile(srcFile, []byte(src), 0644); err != nil { t.Fatalf("write mock CLI src: %v", err) @@ -122,8 +126,8 @@ func TestPTYProviderChat(t *testing.T) { if resp == nil { t.Fatal("Chat() returned nil response") } - if resp.Content == "" { - t.Error("Chat() returned empty content") + if want := "RESPONSE: hello\n"; resp.Content != want { + t.Errorf("Chat() content = %q, want %q", resp.Content, want) } } diff --git a/orchestrator/container_manager.go b/orchestrator/container_manager.go index d399e77..f309c51 100644 --- a/orchestrator/container_manager.go +++ b/orchestrator/container_manager.go @@ -7,6 +7,7 @@ import ( "fmt" "io" "log" + "path" "path/filepath" "strconv" "strings" @@ -483,16 +484,16 @@ func parseMemoryString(s string) (int64, error) { return n, nil } -// validateMountPaths checks that a bind mount source and target are safe. -// Source must be an absolute path. Target must not be a critical system directory. +// validateMountPaths requires a native absolute host source and an absolute Linux +// container target. The cleaned target must not equal a listed critical directory. func validateMountPaths(source, target string) error { if !filepath.IsAbs(source) { return fmt.Errorf("mount source must be an absolute path: %q", source) } - if !filepath.IsAbs(target) { + if !path.IsAbs(target) { return fmt.Errorf("mount target must be an absolute path: %q", target) } - cleanTarget := filepath.Clean(target) + cleanTarget := path.Clean(target) for _, critical := range []string{"/", "/etc", "/bin", "/usr", "/lib", "/lib64", "/sbin", "/sys", "/proc", "/dev"} { if cleanTarget == critical { return fmt.Errorf("mount target %q is a critical system path", cleanTarget) diff --git a/orchestrator/container_manager_test.go b/orchestrator/container_manager_test.go index 905456c..5aef3e2 100644 --- a/orchestrator/container_manager_test.go +++ b/orchestrator/container_manager_test.go @@ -369,6 +369,8 @@ func TestContainerManager_EnsureContainerPullDoesNotStarveStatus(t *testing.T) { } func TestContainerManagerDockerLifecycleOptions(t *testing.T) { + workspaceDir := t.TempDir() + cacheDir := t.TempDir() client := &recordingDockerClient{ imageInspectErr: errors.New("image not found"), containerCreateResponse: client.ContainerCreateResult{ID: "container-1"}, @@ -387,13 +389,13 @@ func TestContainerManagerDockerLifecycleOptions(t *testing.T) { available: true, } - id, err := cm.EnsureContainer(t.Context(), "project-1", "/tmp/workspace", executor.SandboxConfig{ + id, err := cm.EnsureContainer(t.Context(), "project-1", workspaceDir, executor.SandboxConfig{ Image: "alpine:3.22", Memory: "512m", CPU: 1.5, Network: "bridge", Mounts: []executor.SandboxMount{ - {Src: "/tmp/cache", Dst: "/cache", ReadOnly: true}, + {Src: cacheDir, Dst: "/cache", ReadOnly: true}, }, }) if err != nil { @@ -420,8 +422,8 @@ func TestContainerManagerDockerLifecycleOptions(t *testing.T) { t.Errorf("host config resources = %#v", hostConfig.Resources) } if len(hostConfig.Mounts) != 2 || - hostConfig.Mounts[0].Source != "/tmp/workspace" || - hostConfig.Mounts[1].Source != "/tmp/cache" || + hostConfig.Mounts[0].Source != workspaceDir || + hostConfig.Mounts[1].Source != cacheDir || hostConfig.Mounts[1].Target != "/cache" || !hostConfig.Mounts[1].ReadOnly { t.Errorf("host config mounts = %#v", hostConfig.Mounts) @@ -474,6 +476,39 @@ func TestContainerManagerDockerLifecycleOptions(t *testing.T) { } } +func TestValidateMountPathsNativeHostAndLinuxTarget(t *testing.T) { + source := t.TempDir() + cases := []struct { + name, source, target, wantError string + }{ + {"native host and Linux container", source, "/cache", ""}, + {"normalized noncritical target", source, "/cache/./nested/..", ""}, + {"relative source", "relative-cache", "/cache", "mount source must be an absolute path"}, + {"relative target", source, "cache", "mount target must be an absolute path"}, + {"empty target", source, "", "mount target must be an absolute path"}, + {"Windows host path is not a Linux target", source, `C:\cache`, "mount target must be an absolute path"}, + {"cleaned critical target", source, "/cache/../etc", "critical system path"}, + {"repeated slash critical target", source, "//etc", "critical system path"}, + } + for _, target := range []string{"/", "/etc", "/bin", "/usr", "/lib", "/lib64", "/sbin", "/sys", "/proc", "/dev"} { + cases = append(cases, struct{ name, source, target, wantError string }{ + "critical " + target, source, target, "critical system path", + }) + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + err := validateMountPaths(tc.source, tc.target) + if tc.wantError == "" { + if err != nil { + t.Fatalf("valid native source/Linux target rejected: %v", err) + } + } else if err == nil || !strings.Contains(err.Error(), tc.wantError) { + t.Errorf("validateMountPaths(%q, %q) = %v, want %q", tc.source, tc.target, err, tc.wantError) + } + }) + } +} + func TestContainerManagerDockerErrors(t *testing.T) { t.Run("image pull", func(t *testing.T) { client := &recordingDockerClient{ diff --git a/orchestrator/provider_registry_test.go b/orchestrator/provider_registry_test.go index 235e864..02f5ad1 100644 --- a/orchestrator/provider_registry_test.go +++ b/orchestrator/provider_registry_test.go @@ -277,8 +277,9 @@ func TestProviderRegistryTestConnection(t *testing.T) { if msg != "connection successful" { t.Errorf("expected 'connection successful', got %q", msg) } - if latency <= 0 { - t.Error("expected positive latency") + // An immediate mock call can complete within one Windows clock tick. + if latency < 0 { + t.Errorf("expected nonnegative latency, got %v", latency) } } diff --git a/orchestrator/tools/tools_test.go b/orchestrator/tools/tools_test.go index 3b30283..e0b4325 100644 --- a/orchestrator/tools/tools_test.go +++ b/orchestrator/tools/tools_test.go @@ -377,17 +377,35 @@ func TestShellExecTool_Execute(t *testing.T) { }) t.Run("command runs in workspace dir", func(t *testing.T) { - result, err := tool.Execute(ctx, map[string]any{"command": "pwd"}) + const marker = "workspace-marker\n" + if err := os.WriteFile(filepath.Join(ws, "cwd-marker.txt"), []byte(marker), 0600); err != nil { + t.Fatal(err) + } + // A relative read proves the real shell's working directory without + // interpreting Git Bash's /c/... output as a native Windows path. + result, err := tool.Execute(ctx, map[string]any{"command": "cat cwd-marker.txt"}) if err != nil { t.Fatalf("unexpected error: %v", err) } m := result.(map[string]any) - stdout := m["stdout"].(string) - // resolve symlinks for comparison (temp dirs may be symlinked) - resolvedWs, _ := filepath.EvalSymlinks(ws) - resolvedStdout, _ := filepath.EvalSymlinks(stdout[:len(stdout)-1]) // strip trailing newline - if resolvedStdout != resolvedWs { - t.Errorf("expected working dir %q, got %q", resolvedWs, resolvedStdout) + if m["stdout"] != marker || m["exit_code"] != 0 || m["stderr"] != "" { + t.Errorf("workspace relative read = %#v, want marker and successful exit", m) + } + }) + + t.Run("context overrides workspace dir", func(t *testing.T) { + overrideDir := t.TempDir() + const marker = "context-workspace-marker\n" + if err := os.WriteFile(filepath.Join(overrideDir, "cwd-marker.txt"), []byte(marker), 0600); err != nil { + t.Fatal(err) + } + result, err := tool.Execute(WithWorkspacePath(ctx, overrideDir), map[string]any{"command": "cat cwd-marker.txt"}) + if err != nil { + t.Fatal(err) + } + m := result.(map[string]any) + if m["stdout"] != marker || m["exit_code"] != 0 || m["stderr"] != "" { + t.Errorf("context workspace relative read = %#v, want override marker and successful exit", m) } }) } diff --git a/orchestrator/vault_config.go b/orchestrator/vault_config.go index 7e80e8e..62ab6a6 100644 --- a/orchestrator/vault_config.go +++ b/orchestrator/vault_config.go @@ -106,8 +106,9 @@ func vaultConfigDir() string { // deriveKey derives a 32-byte AES key from the config directory path and a // machine-local keyfile. The keyfile is created on first use and stored at -// dir/.vault-key with 0600 permissions. This ensures the token can only be -// decrypted on the same machine in the same data directory. +// dir/.vault-key with mode 0600 on Unix; Windows inherits directory ACLs. +// The keyfile and directory must be protected by the caller. Key material and +// the directory path determine the encryption key. func deriveKey(dir string) ([]byte, error) { keyPath := filepath.Join(dir, ".vault-key") keyMaterial, err := os.ReadFile(keyPath) diff --git a/orchestrator/vault_config_test.go b/orchestrator/vault_config_test.go index 7c903cf..f27447b 100644 --- a/orchestrator/vault_config_test.go +++ b/orchestrator/vault_config_test.go @@ -4,6 +4,7 @@ import ( "encoding/json" "os" "path/filepath" + "runtime" "strings" "testing" ) @@ -116,8 +117,17 @@ func TestSaveVaultConfig_KeyFileCreated(t *testing.T) { if err != nil { t.Fatalf("key file not created: %v", err) } - if info.Mode().Perm() != 0600 { - t.Errorf("key file permissions: got %o, want 0600", info.Mode().Perm()) + if !info.Mode().IsRegular() { + t.Fatalf("key file must be regular, got %v", info.Mode()) + } + wantMode := os.FileMode(0600) + if runtime.GOOS == "windows" { + // Go reports writable Windows files as 0666; ACLs are inherited + // from the directory and cannot be checked through POSIX mode bits. + wantMode = 0666 + } + if info.Mode().Perm() != wantMode { + t.Errorf("key file reported permissions: got %o, want %o", info.Mode().Perm(), wantMode) } } diff --git a/policy/path.go b/policy/path.go index 147d81a..749460b 100644 --- a/policy/path.go +++ b/policy/path.go @@ -63,6 +63,8 @@ func expandPatterns(patterns []string) []string { // matchGlob matches a path against a pattern using doublestar for ** support. func matchGlob(pattern, path string) bool { + pattern = filepath.ToSlash(pattern) + path = filepath.ToSlash(path) matched, err := doublestar.Match(pattern, path) if err != nil { // Malformed pattern — fall back to exact match. diff --git a/policy/path_native_test.go b/policy/path_native_test.go new file mode 100644 index 0000000..d98ddc5 --- /dev/null +++ b/policy/path_native_test.go @@ -0,0 +1,66 @@ +package policy + +import ( + "os" + "path/filepath" + "runtime" + "testing" +) + +func TestGlobMatcherNativeSeparators(t *testing.T) { + root := filepath.Join(t.TempDir(), "workspace") + slashRoot := filepath.ToSlash(root) + cases := []struct { + name string + allow []string + deny []string + path string + want Action + }{ + {"native path slash allow", []string{slashRoot + "/**"}, nil, filepath.Join(root, "src", "main.go"), Allow}, + {"slash path native allow", []string{filepath.Join(root, "**")}, nil, slashRoot + "/src/main.go", Allow}, + {"mixed separators", []string{slashRoot + "/**"}, nil, root + "/src/" + filepath.FromSlash("pkg/main.go"), Allow}, + {"native exact match", []string{filepath.Join(root, "main.go")}, nil, slashRoot + "/main.go", Allow}, + {"slash deny wins", []string{slashRoot + "/**"}, []string{"**/.git/**"}, filepath.Join(root, ".git", "config"), Deny}, + {"native deny wins", []string{slashRoot + "/**"}, []string{filepath.Join(root, ".git", "**")}, slashRoot + "/.git/config", Deny}, + {"secret suffix deny", []string{slashRoot + "/**"}, []string{"**/*.secret"}, filepath.Join(root, "secrets", "prod.secret"), Deny}, + {"single star stops at directory", []string{filepath.Join(root, "*")}, nil, filepath.Join(root, "src", "main.go"), Ask}, + {"cleaned path enters deny", []string{slashRoot + "/**"}, []string{slashRoot + "/.git/**"}, root + "/src/../.git/./config", Deny}, + {"malformed native exact fallback", []string{filepath.Join(root, "[")}, nil, slashRoot + "/[", Allow}, + {"malformed native deny fallback", []string{slashRoot + "/**"}, []string{filepath.Join(root, "[")}, slashRoot + "/[", Deny}, + {"malformed fallback does not allow another path", []string{filepath.Join(root, "[")}, nil, slashRoot + "/other", Ask}, + {"malformed fallback does not deny another path", []string{slashRoot + "/**"}, []string{filepath.Join(root, "[")}, slashRoot + "/other", Allow}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + if got := NewGlobMatcher(tc.allow, tc.deny).Check(tc.path); got != tc.want { + t.Errorf("Check(%q) = %v, want %v (allow %q, deny %q)", tc.path, got, tc.want, tc.allow, tc.deny) + } + }) + } +} + +func TestEvaluatePathNativeTildeDenyOverridesAllow(t *testing.T) { + home := t.TempDir() + if runtime.GOOS == "windows" { + t.Setenv("USERPROFILE", home) + } else { + t.Setenv("HOME", home) + } + resolvedHome, err := os.UserHomeDir() + if err != nil || resolvedHome != home { + t.Fatalf("home fixture: got %q, %v; want %q", resolvedHome, err, home) + } + te := NewTrustEngine("custom", []TrustRule{ + {Pattern: "path:**", Action: Allow}, + {Pattern: "path:~/.ssh/*", Action: Deny}, + }, nil) + denied := filepath.Join(home, ".ssh", "id_rsa") + if got := te.EvaluatePath(denied); got != Deny { + t.Errorf("native home path %q = %v, want Deny despite broad allow", denied, got) + } + allowed := filepath.Join(home, "notes.txt") + if got := te.EvaluatePath(allowed); got != Allow { + t.Errorf("allowed control %q = %v, want Allow", allowed, got) + } +} diff --git a/policy/path_unix_test.go b/policy/path_unix_test.go new file mode 100644 index 0000000..ef284f8 --- /dev/null +++ b/policy/path_unix_test.go @@ -0,0 +1,28 @@ +//go:build !windows + +package policy + +import "testing" + +func TestGlobMatcherUnixEscaping(t *testing.T) { + cases := []struct { + name string + allow []string + deny []string + path string + want Action + }{ + {"escaped wildcard deny", []string{"/workspace/**"}, []string{`/workspace/\*.secret`}, "/workspace/*.secret", Deny}, + {"escaped wildcard stays literal", []string{"/workspace/**"}, []string{`/workspace/\*.secret`}, "/workspace/prod.secret", Allow}, + {"literal backslash deny", []string{"/workspace/**"}, []string{`/workspace/name\\file`}, `/workspace/name\file`, Deny}, + {"backslash is not separator", []string{`/workspace/name/file`}, nil, `/workspace/name\file`, Ask}, + {"case sensitive deny preserved", []string{"/workspace/**"}, []string{"/workspace/Secret"}, "/workspace/secret", Allow}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + if got := NewGlobMatcher(tc.allow, tc.deny).Check(tc.path); got != tc.want { + t.Errorf("Check(%q) = %v, want %v (allow %q, deny %q)", tc.path, got, tc.want, tc.allow, tc.deny) + } + }) + } +} diff --git a/policy/path_windows_test.go b/policy/path_windows_test.go new file mode 100644 index 0000000..d69140d --- /dev/null +++ b/policy/path_windows_test.go @@ -0,0 +1,30 @@ +package policy + +import "testing" + +func TestGlobMatcherWindowsDriveAndUNC(t *testing.T) { + cases := []struct { + name string + allow []string + deny []string + path string + want Action + }{ + {"drive native path", []string{"C:/workspace/**"}, nil, `C:\workspace\src\main.go`, Allow}, + {"drive native pattern", []string{`C:\workspace\**`}, nil, "C:/workspace/src/main.go", Allow}, + {"drive mixed separators deny", []string{`C:\workspace\**`}, []string{"**/.git/**"}, `C:\workspace/src\.git/HEAD`, Deny}, + {"different drive", []string{`C:\workspace\**`}, nil, `D:\workspace\main.go`, Ask}, + {"case sensitive deny preserved", []string{"C:/workspace/**"}, []string{"C:/workspace/Secret"}, `C:\workspace\secret`, Allow}, + {"UNC native path", []string{"//server/share/workspace/**"}, nil, `\\server\share\workspace\main.go`, Allow}, + {"UNC native pattern", []string{`\\server\share\workspace\**`}, nil, "//server/share/workspace/src/main.go", Allow}, + {"UNC deny wins", []string{"//server/share/workspace/**"}, []string{`\\server\share\workspace\.git\**`}, "//server/share/workspace/.git/config", Deny}, + {"different UNC share", []string{`\\server\share\workspace\**`}, nil, `\\server\other\workspace\main.go`, Ask}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + if got := NewGlobMatcher(tc.allow, tc.deny).Check(tc.path); got != tc.want { + t.Errorf("Check(%q) = %v, want %v (allow %q, deny %q)", tc.path, got, tc.want, tc.allow, tc.deny) + } + }) + } +} diff --git a/provider/huggingface_test.go b/provider/huggingface_test.go index 75caf13..dc9f663 100644 --- a/provider/huggingface_test.go +++ b/provider/huggingface_test.go @@ -7,7 +7,9 @@ import ( "net/http/httptest" "os" "path/filepath" + "runtime" "strings" + "sync/atomic" "testing" ) @@ -193,7 +195,12 @@ func TestDownloadHuggingFaceFile_ErrorStatus(t *testing.T) { // TestDownloadHuggingFaceFile_DefaultOutputDir verifies fallback to ~/.cache/workflow/models/. func TestDownloadHuggingFaceFile_DefaultOutputDir(t *testing.T) { content := []byte("small model") + var requests atomic.Int64 srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + requests.Add(1) + if r.Method != http.MethodGet || r.URL.Path != "/org/model/resolve/main/tiny.bin" { + t.Errorf("download request = %s %s, want GET /org/model/resolve/main/tiny.bin", r.Method, r.URL.Path) + } w.Header().Set("Content-Length", fmt.Sprintf("%d", len(content))) _, _ = w.Write(content) })) @@ -203,21 +210,31 @@ func TestDownloadHuggingFaceFile_DefaultOutputDir(t *testing.T) { SetHuggingFaceBaseURL(srv.URL) defer SetHuggingFaceBaseURL(origURL) - // Override HOME so we don't pollute real user cache + // Isolate the native home directory so the real user cache is untouched. tmpHome := t.TempDir() - t.Setenv("HOME", tmpHome) + homeEnv := "HOME" + if runtime.GOOS == "windows" { + homeEnv = "USERPROFILE" + } + t.Setenv(homeEnv, tmpHome) path, err := DownloadHuggingFaceFile(context.Background(), "org/model", "tiny.bin", "", nil) if err != nil { t.Fatalf("DownloadHuggingFaceFile default dir: %v", err) } - // Path must be under ~/.cache/workflow/models/ - expectedPrefix := filepath.Join(tmpHome, ".cache", "workflow", "models") - if !strings.HasPrefix(path, expectedPrefix) { - t.Errorf("path %q should be under %q", path, expectedPrefix) + expected := filepath.Join(tmpHome, ".cache", "workflow", "models", "org--model", "tiny.bin") + if path != expected { + t.Errorf("path: want %q, got %q", expected, path) + } + data, err := os.ReadFile(path) + if err != nil { + t.Fatalf("read downloaded file: %v", err) + } + if string(data) != string(content) { + t.Errorf("content: want %q, got %q", content, data) + } + if got := requests.Load(); got != 1 { + t.Errorf("download requests: want 1, got %d", got) } - - // Cleanup - _ = os.RemoveAll(expectedPrefix) } diff --git a/provider/llama_cpp_download_test.go b/provider/llama_cpp_download_test.go index 93a3310..b42ac0d 100644 --- a/provider/llama_cpp_download_test.go +++ b/provider/llama_cpp_download_test.go @@ -10,10 +10,21 @@ import ( "os" "path/filepath" "runtime" + "strings" + "sync/atomic" "testing" ) -// buildTestZip creates an in-memory zip containing a llama-server binary. +const testLlamaServerPayload = "#!/bin/sh\necho llama-server" + +func testLlamaServerBinaryName() string { + if runtime.GOOS == "windows" { + return "llama-server.exe" + } + return "llama-server" +} + +// buildTestZip creates an in-memory zip containing a named binary payload. func buildTestZip(t *testing.T, binName string) []byte { t.Helper() var buf bytes.Buffer @@ -22,7 +33,7 @@ func buildTestZip(t *testing.T, binName string) []byte { if err != nil { t.Fatalf("zip.Create: %v", err) } - if _, err := f.Write([]byte("#!/bin/sh\necho llama-server")); err != nil { + if _, err := f.Write([]byte(testLlamaServerPayload)); err != nil { t.Fatalf("zip.Write: %v", err) } if err := w.Close(); err != nil { @@ -110,9 +121,12 @@ func TestParseLlamaCppRelease_NoMatchingAsset(t *testing.T) { // TestEnsureLlamaServer_DownloadsAndCaches verifies the full download + cache flow. func TestEnsureLlamaServer_DownloadsAndCaches(t *testing.T) { - zipData := buildTestZip(t, "llama-server") + binName := testLlamaServerBinaryName() + zipData := buildTestZip(t, binName) + var requests atomic.Int64 srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + requests.Add(1) w.Header().Set("Content-Type", "application/zip") _, _ = w.Write(zipData) })) @@ -131,8 +145,19 @@ func TestEnsureLlamaServer_DownloadsAndCaches(t *testing.T) { if err != nil { t.Fatalf("EnsureLlamaServer: %v", err) } - if _, err := os.Stat(path); err != nil { - t.Errorf("binary not found at %s: %v", path, err) + wantPath := filepath.Join(llamaCppCacheDir, llamaCppPinnedVersion, binName) + if path != wantPath { + t.Errorf("binary path: want %q, got %q", wantPath, path) + } + data, err := os.ReadFile(path) + if err != nil { + t.Fatalf("read downloaded binary: %v", err) + } + if string(data) != testLlamaServerPayload { + t.Errorf("binary content: want %q, got %q", testLlamaServerPayload, data) + } + if got := requests.Load(); got != 1 { + t.Errorf("download requests: want 1, got %d", got) } // Second call should use cache (no new HTTP request). @@ -143,25 +168,33 @@ func TestEnsureLlamaServer_DownloadsAndCaches(t *testing.T) { if path != path2 { t.Errorf("cached path mismatch: %q vs %q", path, path2) } + if got := requests.Load(); got != 1 { + t.Errorf("download requests after cached call: want 1, got %d", got) + } } // TestEnsureLlamaServer_GitHubAPIFlow verifies that the GitHub API is called and parsed correctly. func TestEnsureLlamaServer_GitHubAPIFlow(t *testing.T) { - zipData := buildTestZip(t, "llama-server") + binName := testLlamaServerBinaryName() + zipData := buildTestZip(t, binName) - // Determine the asset suffix for the current platform so we can craft a matching release. + // Advertise the ZIP format served by the fixture for the current platform. suffix := assetNameForPlatform("b9999", runtime.GOOS, runtime.GOARCH) + suffix = strings.Replace(suffix, ".tar.gz", ".zip", 1) assetName := "llama-b9999" + suffix - var zipSrvURL string + var downloadRequests atomic.Int64 zipSrv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + downloadRequests.Add(1) w.Header().Set("Content-Type", "application/zip") _, _ = w.Write(zipData) })) defer zipSrv.Close() - zipSrvURL = zipSrv.URL + "/download.zip" + zipSrvURL := zipSrv.URL + "/download.zip" + var apiRequests atomic.Int64 apiSrv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + apiRequests.Add(1) w.Header().Set("Content-Type", "application/json") release := ghRelease{ TagName: "b9999", @@ -189,20 +222,43 @@ func TestEnsureLlamaServer_GitHubAPIFlow(t *testing.T) { if err != nil { t.Fatalf("EnsureLlamaServer via API: %v", err) } - if _, err := os.Stat(path); err != nil { - t.Errorf("binary not at %s: %v", path, err) + wantPath := filepath.Join(llamaCppCacheDir, llamaCppPinnedVersion, binName) + if path != wantPath { + t.Errorf("binary path: want %q, got %q", wantPath, path) + } + data, err := os.ReadFile(path) + if err != nil { + t.Fatalf("read downloaded binary: %v", err) + } + if string(data) != testLlamaServerPayload { + t.Errorf("binary content: want %q, got %q", testLlamaServerPayload, data) + } + if got := apiRequests.Load(); got != 1 { + t.Errorf("GitHub API requests: want 1, got %d", got) + } + if got := downloadRequests.Load(); got != 1 { + t.Errorf("download requests: want 1, got %d", got) } } func TestExtractLlamaServerFromZip_Found(t *testing.T) { - zipData := buildTestZip(t, "llama-server") - dest := filepath.Join(t.TempDir(), "llama-server") + binName := testLlamaServerBinaryName() + for _, entry := range []string{binName, "bin/" + binName} { + t.Run(entry, func(t *testing.T) { + zipData := buildTestZip(t, entry) + dest := filepath.Join(t.TempDir(), binName) - if err := extractLlamaServerFromZip(zipData, dest, "b1234"); err != nil { - t.Fatalf("extractLlamaServerFromZip: %v", err) - } - if _, err := os.Stat(dest); err != nil { - t.Errorf("dest not created: %v", err) + if err := extractLlamaServerFromZip(zipData, dest, "b1234"); err != nil { + t.Fatalf("extractLlamaServerFromZip: %v", err) + } + data, err := os.ReadFile(dest) + if err != nil { + t.Fatalf("read extracted binary: %v", err) + } + if string(data) != testLlamaServerPayload { + t.Errorf("binary content: want %q, got %q", testLlamaServerPayload, data) + } + }) } } @@ -213,7 +269,7 @@ func TestExtractLlamaServerFromZip_NotFound(t *testing.T) { _, _ = f.Write([]byte("data")) _ = w.Close() - dest := filepath.Join(t.TempDir(), "llama-server") + dest := filepath.Join(t.TempDir(), testLlamaServerBinaryName()) err := extractLlamaServerFromZip(buf.Bytes(), dest, "b1234") if err == nil { t.Error("expected error when binary not in zip") diff --git a/sdk/session.go b/sdk/session.go index cff3d0d..7e50958 100644 --- a/sdk/session.go +++ b/sdk/session.go @@ -29,7 +29,8 @@ func LoadSession(path string) SessionState { } // SaveSession writes the session state to path, creating or overwriting the file. -// The file is written with mode 0600 (owner read/write only). +// New files are created with mode 0600 (owner read/write on Unix). On Windows, +// this requests a writable file; access is governed by inherited directory ACLs. func SaveSession(path string, s SessionState) error { data, err := json.Marshal(s) if err != nil { diff --git a/sdk/session_test.go b/sdk/session_test.go index beff6c6..64a26e3 100644 --- a/sdk/session_test.go +++ b/sdk/session_test.go @@ -3,6 +3,7 @@ package sdk_test import ( "os" "path/filepath" + "runtime" "testing" "github.com/GoCodeAlone/workflow-plugin-agent/sdk" @@ -45,13 +46,21 @@ func TestSessionRoundTrip(t *testing.T) { t.Errorf("Custom[difficulty]: want %q, got %v", "hard", v) } - // File must be owner-only. + // Unix creation permissions are owner-only. Windows stat synthesizes + // readable/writable mode bits; those bits do not describe the file's ACL. info, err := os.Stat(path) if err != nil { t.Fatalf("stat: %v", err) } - if perm := info.Mode().Perm(); perm != 0o600 { - t.Errorf("file permissions: want 0600, got %04o", perm) + if !info.Mode().IsRegular() { + t.Errorf("session file must be regular, got mode %v", info.Mode()) + } + wantPerm := os.FileMode(0o600) + if runtime.GOOS == "windows" { + wantPerm = 0o666 + } + if perm := info.Mode().Perm(); perm != wantPerm { + t.Errorf("file permissions: want %04o, got %04o", wantPerm, perm) } }