diff --git a/.gitignore b/.gitignore index 5bae77f..b468f68 100644 --- a/.gitignore +++ b/.gitignore @@ -19,3 +19,4 @@ coverage.html # OS .DS_Store Thumbs.db +.core/ diff --git a/CLAUDE.md b/CLAUDE.md index dd2f551..b113ba3 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -32,7 +32,7 @@ Two files: ## Test Conventions -- `_Good`, `_Bad` suffix pattern for success/failure cases. +- `_Good` / `_Bad` suffix pattern for success / failure cases. - Tests use real git repos created by `initTestRepo()` in temp directories. - Service helper tests (in `service_test.go`) construct `Service` structs directly without the framework. - Framework integration tests (in `service_extra_test.go`) use `core.New()` and test handler dispatch. diff --git a/git.go b/git.go index 00ab648..2865ede 100644 --- a/git.go +++ b/git.go @@ -5,7 +5,7 @@ import ( "bytes" "context" "fmt" - "io" + goio "io" "os" "os/exec" "path/filepath" @@ -13,6 +13,8 @@ import ( "strconv" "strings" "sync" + + coreerr "forge.lthn.ai/core/go-log" ) // RepoStatus represents the git status of a single repository. @@ -81,7 +83,7 @@ func getStatus(ctx context.Context, path, name string) RepoStatus { // Validate path to prevent directory traversal if !filepath.IsAbs(path) { - status.Error = fmt.Errorf("path must be absolute: %s", path) + status.Error = coreerr.E("git.getStatus", "path must be absolute: "+path, nil) return status } @@ -200,7 +202,7 @@ func gitInteractive(ctx context.Context, dir string, args ...string) error { // Capture stderr for error reporting while also showing it var stderr bytes.Buffer - cmd.Stderr = io.MultiWriter(os.Stderr, &stderr) + cmd.Stderr = goio.MultiWriter(os.Stderr, &stderr) if err := cmd.Run(); err != nil { return &GitError{ @@ -272,6 +274,9 @@ func gitCommand(ctx context.Context, dir string, args ...string) (string, error) return stdout.String(), nil } +// Compile-time interface checks. +var _ error = (*GitError)(nil) + // GitError wraps a git command error with stderr output and command context. type GitError struct { Args []string @@ -285,9 +290,12 @@ func (e *GitError) Error() string { stderr := strings.TrimSpace(e.Stderr) if stderr != "" { - return fmt.Errorf("git command %q failed: %s", cmd, stderr).Error() + return fmt.Sprintf("git command %q failed: %s", cmd, stderr) + } + if e.Err != nil { + return fmt.Sprintf("git command %q failed: %v", cmd, e.Err) } - return fmt.Errorf("git command %q failed: %w", cmd, e.Err).Error() + return fmt.Sprintf("git command %q failed", cmd) } // Unwrap returns the underlying error for error chain inspection. diff --git a/git_test.go b/git_test.go index 0800e78..5c7d972 100644 --- a/git_test.go +++ b/git_test.go @@ -372,6 +372,13 @@ func TestGetStatus_Bad_InvalidPath(t *testing.T) { assert.Equal(t, "bad-repo", status.Name) } +func TestGetStatus_Bad_RelativePath(t *testing.T) { + status := getStatus(context.Background(), "relative/path", "rel-repo") + assert.Error(t, status.Error) + assert.Contains(t, status.Error.Error(), "path must be absolute") + assert.Equal(t, "rel-repo", status.Name) +} + // --- Status (parallel multi-repo) tests --- func TestStatus_Good_MultipleRepos(t *testing.T) { diff --git a/go.mod b/go.mod index 28fb664..bb224f1 100644 --- a/go.mod +++ b/go.mod @@ -3,15 +3,14 @@ module forge.lthn.ai/core/go-git go 1.26.0 require ( - forge.lthn.ai/core/go v0.3.0 + forge.lthn.ai/core/go v0.3.1 + forge.lthn.ai/core/go-log v0.0.4 github.com/stretchr/testify v1.11.1 ) require ( github.com/davecgh/go-spew v1.1.2-0.20180830191138-d8f796af33cc // indirect - github.com/kr/pretty v0.3.1 // indirect + github.com/kr/text v0.2.0 // indirect github.com/pmezard/go-difflib v1.0.1-0.20181226105442-5d4384ee4fb2 // indirect - github.com/rogpeppe/go-internal v1.14.1 // indirect - gopkg.in/check.v1 v1.0.0-20201130134442-10cb98267c6c // indirect gopkg.in/yaml.v3 v3.0.1 // indirect ) diff --git a/go.sum b/go.sum index 28d7bdf..8277259 100644 --- a/go.sum +++ b/go.sum @@ -1,19 +1,16 @@ -forge.lthn.ai/core/go v0.3.0 h1:mOG97ApMprwx9Ked62FdWVwXTGSF6JO6m0DrVpoH2Q4= -forge.lthn.ai/core/go v0.3.0/go.mod h1:gE6c8h+PJ2287qNhVUJ5SOe1kopEwHEquvinstpuyJc= +forge.lthn.ai/core/go v0.3.1 h1:5FMTsUhLcxSr07F9q3uG0Goy4zq4eLivoqi8shSY4UM= +forge.lthn.ai/core/go v0.3.1/go.mod h1:gE6c8h+PJ2287qNhVUJ5SOe1kopEwHEquvinstpuyJc= +forge.lthn.ai/core/go-log v0.0.4 h1:KTuCEPgFmuM8KJfnyQ8vPOU1Jg654W74h8IJvfQMfv0= +forge.lthn.ai/core/go-log v0.0.4/go.mod h1:r14MXKOD3LF/sI8XUJQhRk/SZHBE7jAFVuCfgkXoZPw= github.com/creack/pty v1.1.9/go.mod h1:oKZEueFk5CKHvIhNR5MUki03XCEU+Q6VDXinZuGJ33E= github.com/davecgh/go-spew v1.1.2-0.20180830191138-d8f796af33cc h1:U9qPSI2PIWSS1VwoXQT9A3Wy9MM3WgvqSxFWenqJduM= github.com/davecgh/go-spew v1.1.2-0.20180830191138-d8f796af33cc/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38= -github.com/kr/pretty v0.2.1/go.mod h1:ipq/a2n7PKx3OHsz4KJII5eveXtPO4qwEXGdVfWzfnI= github.com/kr/pretty v0.3.1 h1:flRD4NNwYAUpkphVc1HcthR4KEIFJ65n8Mw5qdRn3LE= github.com/kr/pretty v0.3.1/go.mod h1:hoEshYVHaxMs3cyo3Yncou5ZscifuDolrwPKZanG3xk= -github.com/kr/pty v1.1.1/go.mod h1:pFQYn66WHrOpPYNljwOMqo10TkYh1fy3cYio2l3bCsQ= -github.com/kr/text v0.1.0/go.mod h1:4Jbv+DJW3UT/LiOwJeYQe1efqtUx/iVham/4vfdArNI= github.com/kr/text v0.2.0 h1:5Nx0Ya0ZqY2ygV366QzturHI13Jq95ApcVaJBhpS+AY= github.com/kr/text v0.2.0/go.mod h1:eLer722TekiGuMkidMxC/pM04lWEeraHUUmBw8l2grE= -github.com/pkg/diff v0.0.0-20210226163009-20ebb0f2a09e/go.mod h1:pJLUxLENpZxwdsKMEsNbx1VGcRFpLqf3715MtcvvzbA= github.com/pmezard/go-difflib v1.0.1-0.20181226105442-5d4384ee4fb2 h1:Jamvg5psRIccs7FGNTlIRMkT8wgtp5eCXdBlqhYGL6U= github.com/pmezard/go-difflib v1.0.1-0.20181226105442-5d4384ee4fb2/go.mod h1:iKH77koFhYxTK1pcRnkKkqfTogsbg7gZNVY4sRDYZ/4= -github.com/rogpeppe/go-internal v1.9.0/go.mod h1:WtVeX8xhTBvf0smdhujwtBcq4Qrzq/fJaraNFVN+nFs= github.com/rogpeppe/go-internal v1.14.1 h1:UQB4HGPB6osV0SQTLymcB4TgvyWu6ZyliaW0tI/otEQ= github.com/rogpeppe/go-internal v1.14.1/go.mod h1:MaRKkUm5W0goXpeCfT7UZI6fk/L7L7so1lCWt35ZSgc= github.com/stretchr/testify v1.11.1 h1:7s2iGBzp5EwR7/aIZr8ao5+dra3wiQyKjjFuvgVKu7U= diff --git a/service.go b/service.go index ad28d67..48c982d 100644 --- a/service.go +++ b/service.go @@ -2,7 +2,6 @@ package git import ( "context" - "fmt" "iter" "path/filepath" "slices" @@ -10,6 +9,7 @@ import ( "sync" "forge.lthn.ai/core/go/pkg/core" + coreerr "forge.lthn.ai/core/go-log" ) // Queries for git service @@ -51,6 +51,9 @@ type ServiceOptions struct { WorkDir string } +// Compile-time interface checks. +var _ core.Startable = (*Service)(nil) + // Service provides git operations as a Core service. type Service struct { *core.ServiceRuntime[ServiceOptions] @@ -137,14 +140,14 @@ func (s *Service) handleTask(c *core.Core, t core.Task) (any, bool, error) { func (s *Service) validatePath(path string) error { if !filepath.IsAbs(path) { - return fmt.Errorf("path must be absolute: %s", path) + return coreerr.E("git.validatePath", "path must be absolute: "+path, nil) } workDir := s.opts.WorkDir if workDir != "" { rel, err := filepath.Rel(workDir, path) if err != nil || strings.HasPrefix(rel, "..") { - return fmt.Errorf("path %s is outside of allowed WorkDir %s", path, workDir) + return coreerr.E("git.validatePath", "path "+path+" is outside of allowed WorkDir "+workDir, nil) } } return nil diff --git a/service_extra_test.go b/service_extra_test.go index 69ea1e2..8229a16 100644 --- a/service_extra_test.go +++ b/service_extra_test.go @@ -13,6 +13,104 @@ import ( "forge.lthn.ai/core/go/pkg/core" ) +// --- validatePath tests --- + +func TestService_ValidatePath_Bad_RelativePath(t *testing.T) { + svc := &Service{opts: ServiceOptions{WorkDir: "/home/repos"}} + err := svc.validatePath("relative/path") + assert.Error(t, err) + assert.Contains(t, err.Error(), "path must be absolute") +} + +func TestService_ValidatePath_Bad_OutsideWorkDir(t *testing.T) { + svc := &Service{opts: ServiceOptions{WorkDir: "/home/repos"}} + err := svc.validatePath("/etc/passwd") + assert.Error(t, err) + assert.Contains(t, err.Error(), "outside of allowed WorkDir") +} + +func TestService_ValidatePath_Good_InsideWorkDir(t *testing.T) { + svc := &Service{opts: ServiceOptions{WorkDir: "/home/repos"}} + err := svc.validatePath("/home/repos/my-project") + assert.NoError(t, err) +} + +func TestService_ValidatePath_Good_NoWorkDir(t *testing.T) { + svc := &Service{opts: ServiceOptions{}} + err := svc.validatePath("/any/absolute/path") + assert.NoError(t, err) +} + +// --- handleQuery path validation --- + +func TestService_HandleQuery_Bad_InvalidPath(t *testing.T) { + c, err := core.New() + require.NoError(t, err) + + svc := &Service{ + ServiceRuntime: core.NewServiceRuntime(c, ServiceOptions{WorkDir: "/home/repos"}), + opts: ServiceOptions{WorkDir: "/home/repos"}, + } + + _, handled, err := svc.handleQuery(c, QueryStatus{ + Paths: []string{"/outside/path"}, + Names: map[string]string{"/outside/path": "bad"}, + }) + assert.True(t, handled) + assert.Error(t, err) + assert.Contains(t, err.Error(), "outside of allowed WorkDir") +} + +// --- handleTask path validation --- + +func TestService_HandleTask_Bad_PushInvalidPath(t *testing.T) { + c, err := core.New() + require.NoError(t, err) + + svc := &Service{ + ServiceRuntime: core.NewServiceRuntime(c, ServiceOptions{WorkDir: "/home/repos"}), + opts: ServiceOptions{WorkDir: "/home/repos"}, + } + + _, handled, err := svc.handleTask(c, TaskPush{Path: "relative/path"}) + assert.True(t, handled) + assert.Error(t, err) + assert.Contains(t, err.Error(), "path must be absolute") +} + +func TestService_HandleTask_Bad_PullInvalidPath(t *testing.T) { + c, err := core.New() + require.NoError(t, err) + + svc := &Service{ + ServiceRuntime: core.NewServiceRuntime(c, ServiceOptions{WorkDir: "/home/repos"}), + opts: ServiceOptions{WorkDir: "/home/repos"}, + } + + _, handled, err := svc.handleTask(c, TaskPull{Path: "/etc/passwd"}) + assert.True(t, handled) + assert.Error(t, err) + assert.Contains(t, err.Error(), "outside of allowed WorkDir") +} + +func TestService_HandleTask_Bad_PushMultipleInvalidPath(t *testing.T) { + c, err := core.New() + require.NoError(t, err) + + svc := &Service{ + ServiceRuntime: core.NewServiceRuntime(c, ServiceOptions{WorkDir: "/home/repos"}), + opts: ServiceOptions{WorkDir: "/home/repos"}, + } + + _, handled, err := svc.handleTask(c, TaskPushMultiple{ + Paths: []string{"/home/repos/ok", "/etc/bad"}, + Names: map[string]string{}, + }) + assert.True(t, handled) + assert.Error(t, err) + assert.Contains(t, err.Error(), "outside of allowed WorkDir") +} + func TestNewService_Good(t *testing.T) { opts := ServiceOptions{WorkDir: t.TempDir()} factory := NewService(opts) @@ -200,6 +298,140 @@ func TestService_HandleTask_Good_UnknownTask(t *testing.T) { assert.Nil(t, result) } +// --- validatePath tests --- + +func TestService_ValidatePath_Bad_RelativePath(t *testing.T) { + c, err := core.New() + require.NoError(t, err) + + svc := &Service{ + ServiceRuntime: core.NewServiceRuntime(c, ServiceOptions{WorkDir: "/home/repos"}), + opts: ServiceOptions{WorkDir: "/home/repos"}, + } + + err = svc.validatePath("relative/path") + assert.Error(t, err) + assert.Contains(t, err.Error(), "path must be absolute") +} + +func TestService_ValidatePath_Bad_OutsideWorkDir(t *testing.T) { + c, err := core.New() + require.NoError(t, err) + + svc := &Service{ + ServiceRuntime: core.NewServiceRuntime(c, ServiceOptions{WorkDir: "/home/repos"}), + opts: ServiceOptions{WorkDir: "/home/repos"}, + } + + err = svc.validatePath("/etc/evil") + assert.Error(t, err) + assert.Contains(t, err.Error(), "outside of allowed WorkDir") +} + +func TestService_ValidatePath_Good_InsideWorkDir(t *testing.T) { + c, err := core.New() + require.NoError(t, err) + + svc := &Service{ + ServiceRuntime: core.NewServiceRuntime(c, ServiceOptions{WorkDir: "/home/repos"}), + opts: ServiceOptions{WorkDir: "/home/repos"}, + } + + err = svc.validatePath("/home/repos/my-project") + assert.NoError(t, err) +} + +func TestService_ValidatePath_Good_NoWorkDir(t *testing.T) { + c, err := core.New() + require.NoError(t, err) + + svc := &Service{ + ServiceRuntime: core.NewServiceRuntime(c, ServiceOptions{}), + opts: ServiceOptions{}, + } + + // Without WorkDir constraint, any absolute path is valid. + err = svc.validatePath("/any/absolute/path") + assert.NoError(t, err) +} + +// --- handleQuery validation tests --- + +func TestService_HandleQuery_Bad_InvalidPath(t *testing.T) { + c, err := core.New() + require.NoError(t, err) + + svc := &Service{ + ServiceRuntime: core.NewServiceRuntime(c, ServiceOptions{WorkDir: "/home/repos"}), + opts: ServiceOptions{WorkDir: "/home/repos"}, + } + + _, _, err = svc.handleQuery(c, QueryStatus{ + Paths: []string{"/etc/outside"}, + Names: map[string]string{"/etc/outside": "bad"}, + }) + assert.Error(t, err) + assert.Contains(t, err.Error(), "outside of allowed WorkDir") +} + +// --- handleTask validation tests --- + +func TestService_HandleTask_Bad_PushInvalidPath(t *testing.T) { + c, err := core.New() + require.NoError(t, err) + + svc := &Service{ + ServiceRuntime: core.NewServiceRuntime(c, ServiceOptions{WorkDir: "/home/repos"}), + opts: ServiceOptions{WorkDir: "/home/repos"}, + } + + _, handled, err := svc.handleTask(c, TaskPush{Path: "relative/path"}) + assert.True(t, handled) + assert.Error(t, err) + assert.Contains(t, err.Error(), "path must be absolute") +} + +func TestService_HandleTask_Bad_PullOutsideWorkDir(t *testing.T) { + c, err := core.New() + require.NoError(t, err) + + svc := &Service{ + ServiceRuntime: core.NewServiceRuntime(c, ServiceOptions{WorkDir: "/home/repos"}), + opts: ServiceOptions{WorkDir: "/home/repos"}, + } + + _, handled, err := svc.handleTask(c, TaskPull{Path: "/etc/evil"}) + assert.True(t, handled) + assert.Error(t, err) + assert.Contains(t, err.Error(), "outside of allowed WorkDir") +} + +func TestService_HandleTask_Bad_PushMultipleInvalidPath(t *testing.T) { + c, err := core.New() + require.NoError(t, err) + + svc := &Service{ + ServiceRuntime: core.NewServiceRuntime(c, ServiceOptions{WorkDir: "/home/repos"}), + opts: ServiceOptions{WorkDir: "/home/repos"}, + } + + _, handled, err := svc.handleTask(c, TaskPushMultiple{ + Paths: []string{"/home/repos/ok", "/etc/bad"}, + Names: map[string]string{}, + }) + assert.True(t, handled) + assert.Error(t, err) + assert.Contains(t, err.Error(), "outside of allowed WorkDir") +} + +// --- getStatus relative path test --- + +func TestGetStatus_Bad_RelativePath(t *testing.T) { + status := getStatus(context.Background(), "relative/path", "bad-repo") + assert.Error(t, status.Error) + assert.Contains(t, status.Error.Error(), "path must be absolute") +} + // --- Additional git operation tests --- func TestGetStatus_Good_AheadBehindNoUpstream(t *testing.T) {