From 10b2516bb69923fb49678192c01cb90d6673bf7a Mon Sep 17 00:00:00 2001 From: Snider Date: Sat, 8 Aug 2026 10:59:23 +0100 Subject: [PATCH] fix(windows): git's forward slashes refused every valid clone, and two test-side assumptions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three windows-lane failures, one of them a production defect. agent/workspace — Apply refused every acceptance on Windows. It read the integration repo's path back from `git rev-parse --path-format=absolute --git-common-dir` and compared that RAW output against project.ClonePath. Git prints forward slashes on every platform, so "C:/.../repo.git" never equals a filepath-built "C:\...\repo.git" even when both name the same directory — and core.PathBase, matching only the platform separator, read that raw output as having no separator at all and returned the whole path instead of "repo.git". Either check alone was fatal. It now compares the normalised form, which is what the check meant; internalAbsolute has already refused anything escaping the internal root and PathAbs cleans away ".." before the equality test, so the guarantee is unchanged. This was the root of the "cached clone is outside the internal root" failures — the commit-hash mismatches beside them were HEAD simply not moving because Apply had already failed. agent/gitserver — two test-side platform assumptions: - TestSoftserveGitRoundTrip pushed "private fixture\n" and got back "private fixture\r\n". Git for Windows defaults core.autocrlf on, rewriting LF to CRLF on checkout. gitserverRunGit now pins it off for every invocation. Normalising the comparison instead would have hidden a real corruption behind the same green — the test's claim is that softserve moves bytes through intact. - TestSoftservePermissions asserted 0o700/0o600. Windows has no POSIX mode bits: os.Stat synthesises 0777 for a directory and 0666 for a writable file, so it read 0o777/0o666 whatever the real access control was. The confidentiality it pins is an ACL question there, which core.FileMode cannot express let alone assert, so the assertions skip on Windows with the reason recorded. Service startup above them still runs. Receipts — macOS: go test -count=1 ./agent/workspace/ ./agent/gitserver/ ok agent/workspace 135.899s · agent/gitserver 29.617s GOWORK=off go test -count=1 ./... exit=0, 176 packages ok gofmt -l · go vet: clean Lane state before this commit: 35 failing packages (main, run 31251292907). agent/workspace also fails on TempDir cleanup that Windows refuses while a directory is in use; that is untouched here and ledgered separately, so this may move agent/gitserver without moving agent/workspace. Co-Authored-By: Virgil --- go/agent/gitserver/softserve_test.go | 22 ++++++++++++++++++++++ go/agent/workspace/accept.go | 18 +++++++++++++++--- 2 files changed, 37 insertions(+), 3 deletions(-) diff --git a/go/agent/gitserver/softserve_test.go b/go/agent/gitserver/softserve_test.go index 0170c9d00..79d006030 100644 --- a/go/agent/gitserver/softserve_test.go +++ b/go/agent/gitserver/softserve_test.go @@ -4,6 +4,7 @@ package gitserver import ( "context" + "runtime" "testing" "time" @@ -47,8 +48,18 @@ func gitserverTestService(t *testing.T) (*softServe, Repository) { return service, repositoryResult.Value.(Repository) } +// gitserverRunGit runs one git command in directory and returns its trimmed +// combined output. +// +// core.autocrlf is pinned off for every invocation. Git for Windows turns it +// on by default, which rewrites LF to CRLF on checkout — the round-trip +// fixture pushed as "private fixture\n" came back as "private fixture\r\n". +// These tests assert that softserve moves bytes through intact; they are not +// a statement about any platform's line-ending policy, and normalising the +// comparison instead would hide a real corruption behind the same green. func gitserverRunGit(t *testing.T, directory string, environment []string, args ...string) string { t.Helper() + args = append([]string{"-c", "core.autocrlf=false"}, args...) result := command.Command(context.Background(), "git", args...). WithDir(directory). WithEnv(environment). @@ -242,6 +253,17 @@ func TestSoftservePermissions(t *testing.T) { service, repository := gitserverTestService(t) core.AssertTrue(t, service.Health(context.Background()).Value.(Health).Running) + // The confidentiality this pins — a private data dir and an SSH identity + // no one else can read — is a POSIX mode question, and Windows has no + // such bits: os.Stat synthesises 0777 for a directory and 0666 for a + // writable file, so these read 0o777/0o666 there however the real access + // control is set. Windows governs it by ACL, which core.FileMode cannot + // express, let alone assert. Startup above is still exercised on Windows; + // only the unassertable part is skipped. + if runtime.GOOS == "windows" { + t.Skip("POSIX permission bits are not represented on Windows; ACLs govern and FileMode cannot assert them") + } + dataInfo := core.Stat(service.options.DataPath) core.AssertTrue(t, dataInfo.OK, dataInfo.Error()) core.AssertEqual(t, core.FileMode(0o700), dataInfo.Value.(core.FsFileInfo).Mode().Perm()) diff --git a/go/agent/workspace/accept.go b/go/agent/workspace/accept.go index 137f779f3..b22218b7f 100644 --- a/go/agent/workspace/accept.go +++ b/go/agent/workspace/accept.go @@ -9,6 +9,7 @@ import ( "dappco.re/go/inference/agent/gitserver" "dappco.re/go/inference/agent/queue" "dappco.re/go/inference/agent/work" + "dappco.re/go/inference/internal/pathx" commandexec "dappco.re/go/process/exec" ) @@ -418,11 +419,22 @@ func (manager *Manager) verifyChangeReview(ctx context.Context, project work.Pro if !commonResult.OK { return core.Fail(core.E("workspace.Manager.Apply", "failed to resolve integration repository", commonResult.Err())) } - clonePath := core.Trim(commonResult.String()) - cloneResult := manager.internalAbsolute(clonePath) - if !cloneResult.OK || cloneResult.String() != project.ClonePath || clonePath != project.ClonePath || core.PathBase(clonePath) != "repo.git" { + // Compare the NORMALISED path, not git's raw output. Git prints paths with + // forward slashes on every platform, so on Windows "C:/.../repo.git" never + // equals a filepath-built ClonePath of "C:\...\repo.git" even when both + // name the same directory — and core.PathBase, which matches only the + // platform separator, reads that raw output as having no separator at all + // and hands back the whole path instead of "repo.git". Between them those + // two refused every valid clone on Windows. + // + // The guarantee the raw comparison reached for survives: internalAbsolute + // has already refused anything escaping the internal root, and PathAbs + // cleans away any ".." trickery before the equality test. + cloneResult := manager.internalAbsolute(core.Trim(commonResult.String())) + if !cloneResult.OK || cloneResult.String() != project.ClonePath || pathx.Base(cloneResult.String()) != "repo.git" { return core.Fail(core.NewError("agent workspace acceptance cached clone is outside the internal root")) } + clonePath := cloneResult.String() branchResult := manager.gitOutput(ctx, review.IntegrationPath, nil, "symbolic-ref", "--short", "HEAD") headResult := manager.gitOutput(ctx, review.IntegrationPath, nil, "rev-parse", "HEAD") statusResult := manager.gitOutput(ctx, review.IntegrationPath, nil, "status", "--porcelain=v1", "--untracked-files=all")