diff --git a/go/internal/fast/build.go b/go/internal/fast/build.go index 9d2f73c..7fdbf6b 100644 --- a/go/internal/fast/build.go +++ b/go/internal/fast/build.go @@ -25,6 +25,7 @@ import ( "github.com/Agent-Field/SWE-AF/go/internal/config" "github.com/Agent-Field/SWE-AF/go/internal/harnessx" "github.com/Agent-Field/SWE-AF/go/internal/schemas" + "github.com/Agent-Field/SWE-AF/go/internal/workspace" ) // --------------------------------------------------------------------------- @@ -169,7 +170,7 @@ func Build(ctx context.Context, deps *Deps, input map[string]any) (any, error) { repoPath := in.RepoPath // Auto-derive repo_path from repo_url when not specified. if effectiveRepoURL != "" && repoPath == "" { - repoPath = "/workspaces/" + repoNameFromURL(effectiveRepoURL) + repoPath = filepath.Join(workspace.Root(), repoNameFromURL(effectiveRepoURL)) } if repoPath == "" { return nil, errors.New("Either repo_path or repo_url must be provided") diff --git a/go/internal/orch/build.go b/go/internal/orch/build.go index 8e29178..2ee2710 100644 --- a/go/internal/orch/build.go +++ b/go/internal/orch/build.go @@ -19,6 +19,7 @@ import ( "github.com/Agent-Field/SWE-AF/go/internal/envelope" "github.com/Agent-Field/SWE-AF/go/internal/hitl" "github.com/Agent-Field/SWE-AF/go/internal/schemas" + "github.com/Agent-Field/SWE-AF/go/internal/workspace" ) // Handlers is the name→handler registration surface consumed by node wiring. @@ -73,7 +74,7 @@ func Build(ctx context.Context, deps *Deps, input map[string]any) (any, error) { // Auto-derive repo_path from repo_url, build-scoped. if cfg.RepoURL != "" && repoPath == "" { repoName := deriveRepoName(cfg.RepoURL) - repoPath = fmt.Sprintf("/workspaces/%s-%s", repoName, buildID) + repoPath = filepath.Join(workspace.Root(), fmt.Sprintf("%s-%s", repoName, buildID)) } // Multi-repo: derive repo_path from the primary repo. @@ -83,7 +84,7 @@ func Build(ctx context.Context, deps *Deps, input map[string]any) (any, error) { primary = &cfg.Repos[0] } repoName := deriveRepoName(primary.RepoURL) - repoPath = fmt.Sprintf("/workspaces/%s-%s", repoName, buildID) + repoPath = filepath.Join(workspace.Root(), fmt.Sprintf("%s-%s", repoName, buildID)) } if repoPath == "" { @@ -491,7 +492,11 @@ func prepareSingleRepo(ctx context.Context, deps *Deps, cfg *config.BuildConfig, switch { case cfg.RepoURL != "" && !pathExists(gitDir): deps.Note(ctx, fmt.Sprintf("Cloning %s → %s", cfg.RepoURL, repoPath), "build", "clone") - _ = os.MkdirAll(repoPath, 0o755) + // Create only the parent; git clone creates the leaf itself. Pre-creating + // the destination leaf makes git refuse it as "already exists and is not + // an empty directory" on Windows, where it cannot re-open the dir the node + // just made (issue #107). + _ = os.MkdirAll(filepath.Dir(repoPath), 0o755) r := runGit(ctx, "", "clone", cfg.RepoURL, repoPath) if r.ExitCode != 0 { errMsg := strings.TrimSpace(r.Stderr) @@ -524,7 +529,8 @@ func prepareSingleRepo(ctx context.Context, deps *Deps, cfg *config.BuildConfig, deps.Note(ctx, fmt.Sprintf("Reset to origin/%s failed — re-cloning", defaultBranch), "build", "clone", "reclone") _ = os.RemoveAll(repoPath) - _ = os.MkdirAll(repoPath, 0o755) + // Parent-only: git clone re-creates the leaf (issue #107). + _ = os.MkdirAll(filepath.Dir(repoPath), 0o755) clone := runGit(ctx, "", "clone", cfg.RepoURL, repoPath) if clone.ExitCode != 0 { return fmt.Errorf("git re-clone failed: %s", strings.TrimSpace(clone.Stderr)) diff --git a/go/internal/orch/build_test.go b/go/internal/orch/build_test.go index bdb2b04..e6f70b2 100644 --- a/go/internal/orch/build_test.go +++ b/go/internal/orch/build_test.go @@ -4,11 +4,14 @@ import ( "context" "errors" "fmt" + "path/filepath" "strings" "sync" "testing" "github.com/Agent-Field/agentfield/sdk/go/agent" + + "github.com/Agent-Field/SWE-AF/go/internal/workspace" ) // buildHandler routes mock reasoner responses by target suffix. Overridable @@ -203,8 +206,8 @@ func TestBuildIsolationConcurrent(t *testing.T) { func TestBuildScopedPathIncludesBuildID(t *testing.T) { repoURL := "https://github.com/example/my-repo.git" name := deriveRepoName(repoURL) - a := fmt.Sprintf("/workspaces/%s-%s", name, newBuildID()) - b := fmt.Sprintf("/workspaces/%s-%s", name, newBuildID()) + a := filepath.Join(workspace.Root(), fmt.Sprintf("%s-%s", name, newBuildID())) + b := filepath.Join(workspace.Root(), fmt.Sprintf("%s-%s", name, newBuildID())) if a == b { t.Fatal("two builds for the same repo must produce different workspace paths") } diff --git a/go/internal/orch/resolve.go b/go/internal/orch/resolve.go index b68a111..571c6bf 100644 --- a/go/internal/orch/resolve.go +++ b/go/internal/orch/resolve.go @@ -6,10 +6,12 @@ import ( "errors" "fmt" "os" + "path/filepath" "strings" "time" "github.com/Agent-Field/SWE-AF/go/internal/config" + "github.com/Agent-Field/SWE-AF/go/internal/workspace" ) // resolveInput mirrors the Python resolve() signature (param names + defaults). @@ -58,13 +60,16 @@ func ResolveHandler(ctx context.Context, deps *Deps, input map[string]any) (any, buildID := newBuildID() repoName := deriveRepoName(in.RepoURL) - repoPath := fmt.Sprintf("/workspaces/%s-resolve-%s", repoName, buildID) + repoPath := filepath.Join(workspace.Root(), fmt.Sprintf("%s-resolve-%s", repoName, buildID)) deps.Note(ctx, fmt.Sprintf("Resolve starting (build_id=%s) — PR #%d", buildID, in.PRNumber), "resolve", "start") // ---- 1. Clone ---------------------------------------------------------- - _ = os.MkdirAll(repoPath, 0o755) + // Create only the parent; git clone creates the leaf. Pre-creating the leaf + // makes git refuse it as "already exists and is not an empty directory" on + // Windows, where it cannot re-open the node-created dir (issue #107). + _ = os.MkdirAll(filepath.Dir(repoPath), 0o755) clone := runGit(ctx, "", "clone", in.RepoURL, repoPath) if clone.ExitCode != 0 { errMsg := strings.TrimSpace(clone.Stderr) diff --git a/go/internal/workspace/workspace.go b/go/internal/workspace/workspace.go new file mode 100644 index 0000000..43aff5e --- /dev/null +++ b/go/internal/workspace/workspace.go @@ -0,0 +1,59 @@ +// Package workspace resolves the base directory into which SWE-AF clones build +// repositories when the caller supplies only a repo_url (no explicit +// repo_path). It exists so that default never resolves to a drive-relative +// path on Windows. +// +// A hardcoded "/workspaces" base is drive-relative on Windows: it carries no +// drive letter, so the Windows Go node's spawn context resolves it +// unpredictably. os.MkdirAll appears to succeed, but the following `git clone` +// fails with "destination path ... already exists and is not an empty +// directory" — git's is_empty_dir reports "not empty" when it cannot open the +// directory it was handed from that context. Rooting the default under an +// absolute drive-letter base (%LOCALAPPDATA%) removes the ambiguity while +// keeping byte-identical "/workspaces" behavior on every other platform +// (Docker parity). +// +// Ref: https://github.com/Agent-Field/SWE-AF/issues/107 +package workspace + +import ( + "os" + "path/filepath" + "runtime" +) + +// tempDir is indirected so tests can drive the LOCALAPPDATA-empty Windows +// fallback deterministically. Production always uses os.TempDir. +var tempDir = os.TempDir + +// Root returns the base directory under which build repositories are cloned. +// See rootFor for the resolution rules. +func Root() string { + return rootFor(runtime.GOOS, os.Getenv) +} + +// rootFor is the testable core of Root. Resolution order: +// +// 1. SWE_WORKSPACE_ROOT, when non-empty, on every platform — the explicit +// operator override. +// 2. On Windows: %LOCALAPPDATA%\agentfield\workspaces. When LOCALAPPDATA is +// empty, fall back to \agentfield\workspaces so the base is +// still an absolute drive-letter path (never drive-relative "/workspaces"). +// 3. Everywhere else: exactly "/workspaces" — byte-identical to the historical +// default (Docker parity). +// +// goos and getenv are injected so tests can exercise every branch without +// touching the real runtime environment. +func rootFor(goos string, getenv func(string) string) string { + if root := getenv("SWE_WORKSPACE_ROOT"); root != "" { + return root + } + if goos == "windows" { + base := getenv("LOCALAPPDATA") + if base == "" { + base = tempDir() + } + return filepath.Join(base, "agentfield", "workspaces") + } + return "/workspaces" +} diff --git a/go/internal/workspace/workspace_test.go b/go/internal/workspace/workspace_test.go new file mode 100644 index 0000000..cb2592e --- /dev/null +++ b/go/internal/workspace/workspace_test.go @@ -0,0 +1,72 @@ +package workspace + +import ( + "path/filepath" + "testing" +) + +// fakeEnv builds a getenv closure from a fixed map so each table case drives an +// isolated environment without mutating the real process env. +func fakeEnv(vars map[string]string) func(string) string { + return func(k string) string { return vars[k] } +} + +// TestRootFor covers the full resolution matrix from issue #107: the override +// wins everywhere, Windows never yields a drive-relative "/workspaces", and +// non-Windows stays byte-identical to the historical default. +func TestRootFor(t *testing.T) { + const fakeTemp = `C:\FakeTemp` + orig := tempDir + tempDir = func() string { return fakeTemp } + t.Cleanup(func() { tempDir = orig }) + + cases := []struct { + name string + goos string + env map[string]string + want string + }{ + { + name: "windows override wins over LOCALAPPDATA", + goos: "windows", + env: map[string]string{ + "SWE_WORKSPACE_ROOT": `E:\builds`, + "LOCALAPPDATA": `C:\Users\me\AppData\Local`, + }, + want: `E:\builds`, + }, + { + name: "windows LOCALAPPDATA default", + goos: "windows", + env: map[string]string{"LOCALAPPDATA": `C:\Users\me\AppData\Local`}, + want: filepath.Join(`C:\Users\me\AppData\Local`, "agentfield", "workspaces"), + }, + { + name: "windows neither falls back to tempdir", + goos: "windows", + env: map[string]string{}, + want: filepath.Join(fakeTemp, "agentfield", "workspaces"), + }, + { + name: "linux default is exactly /workspaces", + goos: "linux", + env: map[string]string{}, + want: "/workspaces", + }, + { + name: "linux override wins", + goos: "linux", + env: map[string]string{"SWE_WORKSPACE_ROOT": "/mnt/scratch/ws"}, + want: "/mnt/scratch/ws", + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + got := rootFor(tc.goos, fakeEnv(tc.env)) + if got != tc.want { + t.Fatalf("rootFor(%q, ...) = %q, want %q", tc.goos, got, tc.want) + } + }) + } +} diff --git a/swe_af/app.py b/swe_af/app.py index ad42b5a..7f6069d 100644 --- a/swe_af/app.py +++ b/swe_af/app.py @@ -47,6 +47,7 @@ def __init__(self, message: str, *, result=None, error_details=None) -> None: _default_planning_model, _default_runtime, _derive_repo_name as _repo_name_from_url, + _workspace_root, ) NODE_ID = os.getenv("NODE_ID", "swe-planner") @@ -141,7 +142,10 @@ async def _clone_single(spec: WorkspaceRepo) -> tuple[str, str]: # type: ignore git_dir = os.path.join(dest, ".git") if spec.repo_url and not os.path.exists(git_dir): - os.makedirs(dest, exist_ok=True) + # Parent-only (workspace_root already exists): git clone creates the + # leaf. Pre-creating it makes git refuse it as "already exists and is + # not an empty directory" on Windows (issue #107). + os.makedirs(os.path.dirname(dest) or ".", exist_ok=True) cmd = ["git", "clone", spec.repo_url, dest] if spec.branch: cmd += ["--branch", spec.branch] @@ -516,7 +520,9 @@ async def build( This is the single entry point. Pass a goal, get working code. If ``repo_url`` is provided and ``repo_path`` is empty, the repo is cloned - into ``/workspaces/`` automatically (useful in Docker). + into a build-scoped directory under the workspace root automatically + (``/workspaces`` in Docker, ``%LOCALAPPDATA%\\agentfield\\workspaces`` on + Windows, or ``$SWE_WORKSPACE_ROOT`` when set — see ``_workspace_root``). """ cfg = BuildConfig(**config) if config else BuildConfig() @@ -535,13 +541,13 @@ async def build( # concurrent builds from sharing git state, artifacts, or worktrees. if cfg.repo_url and not repo_path: repo_name = _repo_name_from_url(cfg.repo_url) - repo_path = f"/workspaces/{repo_name}-{build_id}" + repo_path = os.path.join(_workspace_root(), f"{repo_name}-{build_id}") # Multi-repo: derive repo_path from primary repo; _clone_repos handles cloning later if not repo_path and len(cfg.repos) > 1: primary = next((r for r in cfg.repos if r.role == "primary"), cfg.repos[0]) repo_name = _repo_name_from_url(primary.repo_url) - repo_path = f"/workspaces/{repo_name}-{build_id}" + repo_path = os.path.join(_workspace_root(), f"{repo_name}-{build_id}") if not repo_path: raise ValueError("Either repo_path or repo_url must be provided") @@ -560,7 +566,10 @@ async def build( git_dir = os.path.join(repo_path, ".git") if cfg.repo_url and not os.path.exists(git_dir): app.note(f"Cloning {cfg.repo_url} → {repo_path}", tags=["build", "clone"]) - os.makedirs(repo_path, exist_ok=True) + # Create only the parent; git clone creates the leaf itself. + # Pre-creating the leaf makes git refuse it as "already exists and is + # not an empty directory" on Windows (issue #107). + os.makedirs(os.path.dirname(repo_path) or ".", exist_ok=True) clone_result = subprocess.run( ["git", "clone", cfg.repo_url, repo_path], capture_output=True, @@ -614,7 +623,8 @@ async def build( ) import shutil shutil.rmtree(repo_path, ignore_errors=True) - os.makedirs(repo_path, exist_ok=True) + # Parent-only: git clone re-creates the leaf (issue #107). + os.makedirs(os.path.dirname(repo_path) or ".", exist_ok=True) clone_result = subprocess.run( ["git", "clone", cfg.repo_url, repo_path], capture_output=True, text=True, @@ -1748,7 +1758,7 @@ async def resolve( build_id = uuid.uuid4().hex[:8] repo_name = _repo_name_from_url(repo_url) - repo_path = f"/workspaces/{repo_name}-resolve-{build_id}" + repo_path = os.path.join(_workspace_root(), f"{repo_name}-resolve-{build_id}") app.note( f"Resolve starting (build_id={build_id}) — PR #{pr_number}", @@ -1756,7 +1766,10 @@ async def resolve( ) # ---- 1. Clone ----------------------------------------------------------- - os.makedirs(repo_path, exist_ok=True) + # Parent-only: git clone creates the leaf itself; pre-creating it makes git + # refuse it as "already exists and is not an empty directory" on Windows + # (issue #107). + os.makedirs(os.path.dirname(repo_path) or ".", exist_ok=True) clone = subprocess.run( ["git", "clone", repo_url, repo_path], capture_output=True, text=True, diff --git a/swe_af/execution/schemas.py b/swe_af/execution/schemas.py index 4dd1c36..d8ea04b 100644 --- a/swe_af/execution/schemas.py +++ b/swe_af/execution/schemas.py @@ -5,6 +5,7 @@ import logging import os import re +import tempfile from enum import Enum from typing import Any, Literal @@ -73,6 +74,34 @@ def _derive_repo_name(url: str) -> str: return name +def _workspace_root() -> str: + """Base directory into which builds clone repositories by default. + + Resolution order: + 1. ``SWE_WORKSPACE_ROOT`` env var, when set, on every platform. + 2. On Windows (``os.name == "nt"``): + ``%LOCALAPPDATA%\\agentfield\\workspaces`` — an absolute drive-letter + path. Falls back to ``\\agentfield\\workspaces`` when + LOCALAPPDATA is unset. + 3. Everywhere else: exactly ``/workspaces`` (Docker parity). + + A hardcoded ``/workspaces`` base is *drive-relative* on Windows (no drive + letter), which the node's spawn context resolves unpredictably: makedirs + appears to succeed but ``git clone`` then fails with "destination path ... + already exists and is not an empty directory". Rooting the default under an + absolute base avoids that. + + Ref: https://github.com/Agent-Field/SWE-AF/issues/107 + """ + root = os.environ.get("SWE_WORKSPACE_ROOT") + if root: + return root + if os.name == "nt": + base = os.environ.get("LOCALAPPDATA") or tempfile.gettempdir() + return os.path.join(base, "agentfield", "workspaces") + return "/workspaces" + + # --------------------------------------------------------------------------- # Multi-repo models # --------------------------------------------------------------------------- diff --git a/swe_af/fast/app.py b/swe_af/fast/app.py index 8b31e2f..c91adc9 100644 --- a/swe_af/fast/app.py +++ b/swe_af/fast/app.py @@ -18,6 +18,7 @@ from agentfield import Agent from swe_af.execution.envelope import unwrap_call_result as _unwrap +from swe_af.execution.schemas import _workspace_root from swe_af.fast import fast_router from swe_af.fast.schemas import FastBuildConfig, FastBuildResult, fast_resolve_models @@ -90,7 +91,9 @@ async def build( # Auto-derive repo_path from repo_url when not specified if effective_repo_url and not repo_path: - repo_path = f"/workspaces/{_repo_name_from_url(effective_repo_url)}" + repo_path = os.path.join( + _workspace_root(), _repo_name_from_url(effective_repo_url) + ) if not repo_path: raise ValueError("Either repo_path or repo_url must be provided") diff --git a/tests/test_build_isolation.py b/tests/test_build_isolation.py index f7e8659..97c5fda 100644 --- a/tests/test_build_isolation.py +++ b/tests/test_build_isolation.py @@ -9,6 +9,7 @@ from __future__ import annotations +import os import re from pathlib import Path @@ -26,7 +27,8 @@ def test_auto_derived_repo_path_includes_build_id(self) -> None: """When repo_url is provided without repo_path, the derived path must include the build_id to isolate concurrent builds.""" assert re.search( - r'repo_path\s*=\s*f"/workspaces/\{repo_name\}-\{build_id\}"', + r'repo_path\s*=\s*os\.path\.join\(\s*_workspace_root\(\),\s*' + r'f"\{repo_name\}-\{build_id\}"\s*\)', APP_SOURCE, ), ( "Auto-derived repo_path must include build_id suffix to isolate " @@ -52,7 +54,8 @@ def test_multi_repo_path_includes_build_id(self) -> None: assert multi_repo_start > 0, "Multi-repo section not found" multi_repo_section = APP_SOURCE[multi_repo_start:multi_repo_start + 500] assert re.search( - r'repo_path\s*=\s*f"/workspaces/\{repo_name\}-\{build_id\}"', + r'repo_path\s*=\s*os\.path\.join\(\s*_workspace_root\(\),\s*' + r'f"\{repo_name\}-\{build_id\}"\s*\)', multi_repo_section, ), ( "Multi-repo derived repo_path must include build_id suffix (see issue #43)" @@ -103,3 +106,63 @@ def test_artifacts_dir_isolated_by_build_scoped_path(self) -> None: assert artifacts_a != artifacts_b, ( "Artifacts dirs must differ between concurrent builds" ) + + +class TestWorkspaceRoot: + """Issue #107: the default workspace root must never resolve to the + drive-relative ``/workspaces`` on Windows.""" + + def test_override_wins_on_every_platform(self, monkeypatch) -> None: + """SWE_WORKSPACE_ROOT, when set, is the base regardless of os.name.""" + from swe_af.execution.schemas import _workspace_root + + monkeypatch.setenv("SWE_WORKSPACE_ROOT", "/mnt/scratch/ws") + + monkeypatch.setattr(os, "name", "nt") + assert _workspace_root() == "/mnt/scratch/ws" + + monkeypatch.setattr(os, "name", "posix") + assert _workspace_root() == "/mnt/scratch/ws" + + def test_windows_default_uses_localappdata(self, monkeypatch) -> None: + """On Windows with no override, the base is under %LOCALAPPDATA% — + absolute, never the drive-relative '/workspaces'.""" + from swe_af.execution.schemas import _workspace_root + + monkeypatch.delenv("SWE_WORKSPACE_ROOT", raising=False) + monkeypatch.setattr(os, "name", "nt") + monkeypatch.setenv("LOCALAPPDATA", r"C:\Users\me\AppData\Local") + + got = _workspace_root() + assert got == os.path.join( + r"C:\Users\me\AppData\Local", "agentfield", "workspaces" + ) + assert got != "/workspaces" + + def test_windows_falls_back_to_tempdir_without_localappdata( + self, monkeypatch + ) -> None: + """On Windows with neither override nor LOCALAPPDATA, fall back to the + temp dir — still an absolute base, never '/workspaces'.""" + import tempfile + + from swe_af.execution.schemas import _workspace_root + + monkeypatch.delenv("SWE_WORKSPACE_ROOT", raising=False) + monkeypatch.delenv("LOCALAPPDATA", raising=False) + monkeypatch.setattr(os, "name", "nt") + + got = _workspace_root() + assert got == os.path.join( + tempfile.gettempdir(), "agentfield", "workspaces" + ) + assert got != "/workspaces" + + def test_non_windows_default_is_exactly_workspaces(self, monkeypatch) -> None: + """Non-Windows with no override derives exactly '/workspaces' — no + behavior change (Docker parity).""" + from swe_af.execution.schemas import _workspace_root + + monkeypatch.delenv("SWE_WORKSPACE_ROOT", raising=False) + monkeypatch.setattr(os, "name", "posix") + assert _workspace_root() == "/workspaces"