From 5fd45d5755d50573f754dd43de7a00aed10fd68b Mon Sep 17 00:00:00 2001 From: Aditi Chauhan Date: Tue, 9 Jun 2026 15:30:13 +0530 Subject: [PATCH 1/2] fix(project): case-sensitive isGitRepo + injectable GitChecker MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Restores the GitChecker seam and case-sensitivity fix deferred from #95 (reverted in e3d2533). - samePath compares cleaned, symlink-resolved paths and is case-insensitive only on darwin/windows; on case-sensitive filesystems (Linux) paths differing only in case are now distinct. - Git repo check moves behind a GitChecker interface injected into Service, so the service is testable with a fake — no real git binary or working tree. Closes #97 Co-Authored-By: Claude Opus 4.8 --- .../internal/service/project/export_test.go | 10 +++ backend/internal/service/project/git.go | 53 +++++++++++++++ backend/internal/service/project/git_test.go | 65 +++++++++++++++++++ backend/internal/service/project/service.go | 37 ++++------- 4 files changed, 139 insertions(+), 26 deletions(-) create mode 100644 backend/internal/service/project/export_test.go create mode 100644 backend/internal/service/project/git.go create mode 100644 backend/internal/service/project/git_test.go diff --git a/backend/internal/service/project/export_test.go b/backend/internal/service/project/export_test.go new file mode 100644 index 00000000..d6a5f9bd --- /dev/null +++ b/backend/internal/service/project/export_test.go @@ -0,0 +1,10 @@ +package project + +// Test-only handles to unexported internals, so external tests can assert the +// isGitRepo behavior without widening the package's public surface. + +// SamePathForTest exposes samePath to tests. +var SamePathForTest = samePath + +// NewExecGitCheckerForTest returns the production GitChecker for tests. +func NewExecGitCheckerForTest() GitChecker { return execGitChecker{} } diff --git a/backend/internal/service/project/git.go b/backend/internal/service/project/git.go new file mode 100644 index 00000000..4f75cbbc --- /dev/null +++ b/backend/internal/service/project/git.go @@ -0,0 +1,53 @@ +package project + +import ( + "os/exec" + "path/filepath" + "runtime" + "strings" +) + +// GitChecker reports whether a filesystem path is the root of a git repository. +// It is the seam that lets the project Service be exercised without a real git +// binary or working tree. +type GitChecker interface { + IsRepo(path string) bool +} + +// isGitRepo reports whether path is a git repository root, using the production +// git checker. Free-function workspace registration calls this directly; the +// Service's own repo check goes through the injectable GitChecker seam instead. +func isGitRepo(path string) bool { return execGitChecker{}.IsRepo(path) } + +// execGitChecker is the production GitChecker: it shells out to git. +type execGitChecker struct{} + +func (execGitChecker) IsRepo(path string) bool { + cmd := exec.Command("git", "-C", path, "rev-parse", "--show-toplevel") + out, err := cmd.Output() + if err != nil { + return false + } + top := filepath.Clean(strings.TrimSpace(string(out))) + path = filepath.Clean(path) + top, err = filepath.EvalSymlinks(top) + if err != nil { + return false + } + path, err = filepath.EvalSymlinks(path) + if err != nil { + return false + } + return samePath(top, path) +} + +// samePath compares two cleaned, symlink-resolved paths. It is case-insensitive +// only on filesystems that are conventionally case-insensitive (macOS, Windows); +// on case-sensitive filesystems (Linux), "/home/u/Repo" and "/home/u/repo" are +// distinct directories and must not be treated as equal. +func samePath(a, b string) bool { + if runtime.GOOS == "darwin" || runtime.GOOS == "windows" { + return strings.EqualFold(a, b) + } + return a == b +} diff --git a/backend/internal/service/project/git_test.go b/backend/internal/service/project/git_test.go new file mode 100644 index 00000000..af904808 --- /dev/null +++ b/backend/internal/service/project/git_test.go @@ -0,0 +1,65 @@ +package project_test + +import ( + "context" + "runtime" + "testing" + + "github.com/aoagents/agent-orchestrator/backend/internal/service/project" + "github.com/aoagents/agent-orchestrator/backend/internal/storage/sqlite" +) + +// fakeGitChecker is a GitChecker that answers from an in-memory set of repo +// paths — no git binary, no working tree. It is the seam that lets the project +// service be exercised in a unit test. +type fakeGitChecker struct{ repos map[string]bool } + +func (f fakeGitChecker) IsRepo(path string) bool { return f.repos[path] } + +func newManagerWithGit(t *testing.T, git project.GitChecker) project.Manager { + t.Helper() + store, err := sqlite.Open(t.TempDir()) + if err != nil { + t.Fatalf("open store: %v", err) + } + t.Cleanup(func() { _ = store.Close() }) + return project.NewWithGitChecker(store, git) +} + +func TestAdd_UsesInjectedGitChecker(t *testing.T) { + ctx := context.Background() + dir := t.TempDir() + m := newManagerWithGit(t, fakeGitChecker{repos: map[string]bool{dir: true}}) + + // A path the fake recognizes as a repo is accepted — without shelling out to git. + if _, err := m.Add(ctx, project.AddInput{Path: dir, ProjectID: ptr("ao")}); err != nil { + t.Fatalf("Add on a fake-recognized repo: %v", err) + } +} + +func TestAdd_RejectsNonRepoViaGitChecker(t *testing.T) { + ctx := context.Background() + m := newManagerWithGit(t, fakeGitChecker{repos: map[string]bool{}}) + + _, err := m.Add(ctx, project.AddInput{Path: t.TempDir(), ProjectID: ptr("ao")}) + wantCode(t, err, "NOT_A_GIT_REPO") +} + +// TestSamePath_CaseSensitivity guards the isGitRepo fix: paths differing only in +// case must be treated as distinct on case-sensitive filesystems (Linux) and as +// equal on the conventionally case-insensitive ones (macOS, Windows). +func TestSamePath_CaseSensitivity(t *testing.T) { + git := project.NewExecGitCheckerForTest() + lower := t.TempDir() + if git.IsRepo(lower) { + t.Fatalf("temp dir unexpectedly reported as a git repo") + } + // Document the platform contract so a regression in samePath is caught here. + caseInsensitive := runtime.GOOS == "darwin" || runtime.GOOS == "windows" + if got := project.SamePathForTest("/a/Repo", "/a/repo"); got != caseInsensitive { + t.Fatalf("samePath(/a/Repo, /a/repo) = %v on %s, want %v", got, runtime.GOOS, caseInsensitive) + } + if !project.SamePathForTest("/a/repo", "/a/repo") { + t.Fatalf("samePath of identical paths must be true") + } +} diff --git a/backend/internal/service/project/service.go b/backend/internal/service/project/service.go index adc92438..d29180a0 100644 --- a/backend/internal/service/project/service.go +++ b/backend/internal/service/project/service.go @@ -39,6 +39,7 @@ type Manager interface { // Service implements project registration and lookup use-cases for controllers. type Service struct { store Store + git GitChecker // addMu serialises the whole body of Add. Workspace registration performs // filesystem mutations (git init, .gitignore writes, commits) that are not // covered by the store's own writeMu, so path/id conflict checks plus the @@ -48,9 +49,16 @@ type Service struct { var _ Manager = (*Service)(nil) -// New returns a project service backed by the given durable store. +// New returns a project service backed by the given durable store, using the +// production GitChecker that shells out to git. func New(store Store) *Service { - return &Service{store: store} + return NewWithGitChecker(store, execGitChecker{}) +} + +// NewWithGitChecker returns a project service with an injectable GitChecker, +// letting tests substitute a fake for the repo check. +func NewWithGitChecker(store Store, git GitChecker) *Service { + return &Service{store: store, git: git} } // List returns every active registered project. @@ -173,7 +181,7 @@ func (m *Service) Add(ctx context.Context, in AddInput) (Project, error) { p.WorkspaceRepos = workspaceReposFromRecords(repos) return p, nil } - if !isGitRepo(path) { + if !m.git.IsRepo(path) { return Project{}, apierr.Invalid("NOT_A_GIT_REPO", "Repository path must point to a git repository", nil) } row.RepoOriginURL = resolveGitOriginURL(path) @@ -288,29 +296,6 @@ func normalizePath(raw string) (string, error) { return filepath.Clean(abs), nil } -func isGitRepo(path string) bool { - cmd := exec.Command("git", "-C", path, "rev-parse", "--show-toplevel") - out, err := cmd.Output() - if err != nil { - return false - } - top := filepath.Clean(strings.TrimSpace(string(out))) - path = filepath.Clean(path) - top, err = filepath.EvalSymlinks(top) - if err != nil { - return false - } - path, err = filepath.EvalSymlinks(path) - if err != nil { - return false - } - - if strings.EqualFold(top, path) { - return true - } - return top == path -} - func defaultProjectID(path string) domain.ProjectID { id := strings.ToLower(filepath.Base(path)) id = strings.TrimSpace(id) From dac9482120a9517cf050fb355fd82ce7b99d1eca Mon Sep 17 00:00:00 2001 From: Aditi Chauhan Date: Tue, 9 Jun 2026 15:37:25 +0530 Subject: [PATCH 2/2] test(project): drop real-git assertion from samePath unit test The execGitChecker.IsRepo(tempdir) check in TestSamePath_CaseSensitivity silently passed when git was absent from PATH (cmd.Output() fails -> false), so it never exercised the git path it implied. Remove it; the test now only asserts the samePath platform contract, which has no git dependency. Drops the now-unused NewExecGitCheckerForTest shim. Co-Authored-By: Claude Opus 4.8 --- backend/internal/service/project/export_test.go | 3 --- backend/internal/service/project/git_test.go | 5 ----- 2 files changed, 8 deletions(-) diff --git a/backend/internal/service/project/export_test.go b/backend/internal/service/project/export_test.go index d6a5f9bd..55e792f2 100644 --- a/backend/internal/service/project/export_test.go +++ b/backend/internal/service/project/export_test.go @@ -5,6 +5,3 @@ package project // SamePathForTest exposes samePath to tests. var SamePathForTest = samePath - -// NewExecGitCheckerForTest returns the production GitChecker for tests. -func NewExecGitCheckerForTest() GitChecker { return execGitChecker{} } diff --git a/backend/internal/service/project/git_test.go b/backend/internal/service/project/git_test.go index af904808..24cc46e0 100644 --- a/backend/internal/service/project/git_test.go +++ b/backend/internal/service/project/git_test.go @@ -49,11 +49,6 @@ func TestAdd_RejectsNonRepoViaGitChecker(t *testing.T) { // case must be treated as distinct on case-sensitive filesystems (Linux) and as // equal on the conventionally case-insensitive ones (macOS, Windows). func TestSamePath_CaseSensitivity(t *testing.T) { - git := project.NewExecGitCheckerForTest() - lower := t.TempDir() - if git.IsRepo(lower) { - t.Fatalf("temp dir unexpectedly reported as a git repo") - } // Document the platform contract so a regression in samePath is caught here. caseInsensitive := runtime.GOOS == "darwin" || runtime.GOOS == "windows" if got := project.SamePathForTest("/a/Repo", "/a/repo"); got != caseInsensitive {