From e01f3419ea3f3d4ceafad6df1f17f9acf4c71b79 Mon Sep 17 00:00:00 2001 From: Lukasz Raczylo Date: Mon, 1 Jun 2026 13:11:07 +0100 Subject: [PATCH] fix(apply): propagate read errors, auto-open repo, dedupe adapter base (#6) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three robustness fixes for the apply pipeline: 1. Silent read errors on state/target files treated 'file absent' as 'file empty' and fast-forward-overwrote user edits. Add common.ReadExistingFile that returns (nil,nil) only on os.IsNotExist; other I/O errors propagate. Use it from apply.Run and from every adapter's Render (claudecode, crush, kilo, opencode, goose, zed). 2. apply.Run required callers to pre-construct *gitx.Repo; passing nil silently skipped the commit. Run now auto-opens a repo from Bundle.Root, gracefully no-ops when the root is not a git repo, and uses a new gitx.HasStagedChanges() check to skip the no-op 'nothing to commit' that triggered the original silent-skip. 3. Each of 7 adapter packages re-implemented the same 25-line Option/WithHome/New constructor. Hoist into adapter.Base + adapter.BaseOption + NewBase. Each adapter embeds *Base and reduces to a 9-line constructor. Public New(WithHome(h)) API preserved; a.home becomes a.Home. Tests: - apply: 5 new (propagates-read-error, auto-opens-repo, no-repo-skips-commit, skips-no-op-commit, commit-message-counts) - gitx: 2 new (HasStagedChanges empty/after-add) - adapter/common: 3 new (ReadExistingFile missing/content/perm) - adapter: 4 new (NewBase default/WithHome/order/typed) - existing adapter tests unchanged — verify a.Home still honoured. --- internal/adapter/base.go | 29 ++++ internal/adapter/base_test.go | 46 ++++++ internal/adapter/common/io.go | 13 ++ internal/adapter/common/io_existing_test.go | 64 ++++++++ internal/adapters/cagent/cagent.go | 23 ++- internal/adapters/claudecode/claudecode.go | 34 ++--- internal/adapters/crush/crush.go | 29 ++-- internal/adapters/goose/goose.go | 33 ++--- internal/adapters/kilo/kilo.go | 29 ++-- internal/adapters/opencode/opencode.go | 29 ++-- internal/adapters/zed/zed.go | 29 ++-- internal/apply/apply.go | 47 ++++-- internal/apply/apply_test.go | 154 ++++++++++++++++++++ internal/gitx/repo.go | 11 ++ internal/gitx/repo_test.go | 30 ++++ 15 files changed, 482 insertions(+), 118 deletions(-) create mode 100644 internal/adapter/base.go create mode 100644 internal/adapter/base_test.go create mode 100644 internal/adapter/common/io_existing_test.go diff --git a/internal/adapter/base.go b/internal/adapter/base.go new file mode 100644 index 0000000..e977de3 --- /dev/null +++ b/internal/adapter/base.go @@ -0,0 +1,29 @@ +package adapter + +import "github.com/lukaszraczylo/harness-sync/internal/adapter/common" + +// Base holds the cross-adapter defaults (currently just the home directory +// used to compute target paths). Embed it in each adapter struct to inherit +// the WithHome/New constructor pattern without re-implementing it. +type Base struct { + Home string +} + +// BaseOption configures a Base. Returned by WithHome and consumed by NewBase. +type BaseOption func(*Base) + +// WithHome overrides the home directory used for target paths. Defaults to +// common.DefaultHome() (current $HOME) when no option is supplied. +func WithHome(h string) BaseOption { + return func(b *Base) { b.Home = h } +} + +// NewBase returns a Base populated from opts, with Home defaulting to the +// current user's home directory. +func NewBase(opts ...BaseOption) *Base { + b := &Base{Home: common.DefaultHome()} + for _, o := range opts { + o(b) + } + return b +} diff --git a/internal/adapter/base_test.go b/internal/adapter/base_test.go new file mode 100644 index 0000000..a76e136 --- /dev/null +++ b/internal/adapter/base_test.go @@ -0,0 +1,46 @@ +package adapter_test + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/lukaszraczylo/harness-sync/internal/adapter" +) + +func TestNewBaseDefaultsHomeFromEnv(t *testing.T) { + b := adapter.NewBase() + // DefaultHome returns either $HOME or "". We only assert that the + // helper does not panic and that Home is a string (Go zero value is ""). + // Environment-dependence: skip if HOME is unset (CI minimal envs). + if b.Home == "" { + t.Skip("HOME unset in test env; cannot verify default") + } + assert.NotEmpty(t, b.Home) +} + +func TestNewBaseWithHomeOverrides(t *testing.T) { + b := adapter.NewBase(adapter.WithHome("/custom/home/path")) + assert.Equal(t, "/custom/home/path", b.Home) +} + +func TestNewBaseOptionsApplyInOrder(t *testing.T) { + // Last write wins — covers the basic Apply loop in NewBase. + b := adapter.NewBase( + adapter.WithHome("/first"), + adapter.WithHome("/second"), + ) + assert.Equal(t, "/second", b.Home) +} + +func TestBaseOptionTypedCorrectly(t *testing.T) { + // adapter.BaseOption is the public type; WithHome must return it. + opt := adapter.WithHome("/x") + require.NotNil(t, opt) + + // Apply it manually to confirm signature. + var b adapter.Base + opt(&b) + assert.Equal(t, "/x", b.Home) +} diff --git a/internal/adapter/common/io.go b/internal/adapter/common/io.go index 78d09fe..1bd6a92 100644 --- a/internal/adapter/common/io.go +++ b/internal/adapter/common/io.go @@ -29,6 +29,19 @@ func ReadIfExists(path string) (string, error) { return ReadIfExistsFS(fsx.OS(), path) } +// ReadExistingFile returns the file body or (nil, nil) when missing. +// Other errors (permission denied, I/O failure) are returned verbatim so +// read-modify-write callers can distinguish a fresh first-run state from a +// broken read. Treating every error as "file is empty" silently overwrites +// the user's on-disk edits during apply. +func ReadExistingFile(path string) ([]byte, error) { + b, err := os.ReadFile(path) //nolint:gosec // caller-controlled path + if err != nil && os.IsNotExist(err) { + return nil, nil + } + return b, err +} + // DirExistsFS reports whether path is an existing directory on fs. func DirExistsFS(fs fsx.FS, path string) bool { info, err := fs.Stat(path) diff --git a/internal/adapter/common/io_existing_test.go b/internal/adapter/common/io_existing_test.go new file mode 100644 index 0000000..7bac990 --- /dev/null +++ b/internal/adapter/common/io_existing_test.go @@ -0,0 +1,64 @@ +package common_test + +import ( + "errors" + "os" + "path/filepath" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/lukaszraczylo/harness-sync/internal/adapter/common" +) + +// ReadExistingFile: missing file returns (nil, nil). Permission errors and +// other I/O failures must NOT be silently swallowed — those would let +// read-modify-write code clobber the user's on-disk state. + +func TestReadExistingFileMissing(t *testing.T) { + got, err := common.ReadExistingFile(filepath.Join(t.TempDir(), "absent")) + require.NoError(t, err) + assert.Nil(t, got) +} + +func TestReadExistingFileContent(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "ok.txt") + require.NoError(t, os.WriteFile(path, []byte("body\n"), 0o600)) + + got, err := common.ReadExistingFile(path) + require.NoError(t, err) + assert.Equal(t, []byte("body\n"), got) +} + +func TestReadExistingFilePropagatesError(t *testing.T) { + // Create a directory the OS will refuse us read access to. + if os.Geteuid() == 0 { + t.Skip("running as root; cannot test permission denial") + } + dir := t.TempDir() + require.NoError(t, os.Mkdir(filepath.Join(dir, "locked"), 0o000)) + t.Cleanup(func() { _ = os.Chmod(filepath.Join(dir, "locked"), 0o755) }) + + _, err := common.ReadExistingFile(filepath.Join(dir, "locked", "file")) + require.Error(t, err) + assert.False(t, os.IsNotExist(err), "permission denied must not be reported as not-exist") + assert.True(t, errors.Is(err, os.ErrPermission) || isPermission(err), + "expected permission error, got %v", err) +} + +func isPermission(err error) bool { + // Some platforms wrap the error in *os.PathError. Check the string too + // for portability — it's a coarse signal but adequate here. + return err != nil && (contains(err.Error(), "permission") || contains(err.Error(), "access denied")) +} + +func contains(s, sub string) bool { + for i := 0; i+len(sub) <= len(s); i++ { + if s[i:i+len(sub)] == sub { + return true + } + } + return false +} diff --git a/internal/adapters/cagent/cagent.go b/internal/adapters/cagent/cagent.go index 7a22159..2f55d19 100644 --- a/internal/adapters/cagent/cagent.go +++ b/internal/adapters/cagent/cagent.go @@ -19,21 +19,16 @@ import ( const name = "cagent" // Adapter implements adapter.Adapter for the cagent harness. -type Adapter struct{ home string } +type Adapter struct { + *adapter.Base +} -// Option configures an Adapter. -type Option func(*Adapter) - -// WithHome overrides the home directory (defaults to os.UserHomeDir). -func WithHome(h string) Option { return func(a *Adapter) { a.home = h } } +// WithHome overrides the home directory used to resolve target paths. +func WithHome(h string) adapter.BaseOption { return func(b *adapter.Base) { b.Home = h } } // New returns a new Adapter with the given options applied. -func New(opts ...Option) *Adapter { - a := &Adapter{home: common.DefaultHome()} - for _, o := range opts { - o(a) - } - return a +func New(opts ...adapter.BaseOption) *Adapter { + return &Adapter{Base: adapter.NewBase(opts...)} } // Name returns the harness identifier. @@ -53,14 +48,14 @@ func (a *Adapter) Capabilities() adapter.HarnessCapabilities { // Detect returns true when ~/.config/cagent/ exists. func (a *Adapter) Detect() bool { - _, err := os.Stat(filepath.Join(a.home, ".config", "cagent")) + _, err := os.Stat(filepath.Join(a.Home, ".config", "cagent")) return err == nil } // Render produces a starter default.yaml for cagent. func (a *Adapter) Render(b *canonical.Bundle) (*adapter.FileSet, error) { fs := adapter.NewFileSet() - cfgPath := filepath.Join(a.home, ".config", "cagent", "default.yaml") + cfgPath := filepath.Join(a.Home, ".config", "cagent", "default.yaml") instructions := b.Instructions.Global if override, ok := b.Instructions.PerHarness[name]; ok && override != "" { diff --git a/internal/adapters/claudecode/claudecode.go b/internal/adapters/claudecode/claudecode.go index b019223..c91e534 100644 --- a/internal/adapters/claudecode/claudecode.go +++ b/internal/adapters/claudecode/claudecode.go @@ -2,6 +2,7 @@ package claudecode import ( + "fmt" "os" "path/filepath" @@ -14,22 +15,15 @@ const name = "claude-code" // Adapter implements adapter.Adapter for the claude-code harness. type Adapter struct { - home string + *adapter.Base } -// Option configures an Adapter. -type Option func(*Adapter) - -// WithHome overrides the home directory (defaults to os.UserHomeDir). -func WithHome(h string) Option { return func(a *Adapter) { a.home = h } } +// WithHome overrides the home directory used to resolve target paths. +func WithHome(h string) adapter.BaseOption { return func(b *adapter.Base) { b.Home = h } } // New returns a new Adapter with the given options applied. -func New(opts ...Option) *Adapter { - a := &Adapter{home: common.DefaultHome()} - for _, o := range opts { - o(a) - } - return a +func New(opts ...adapter.BaseOption) *Adapter { + return &Adapter{Base: adapter.NewBase(opts...)} } // Name returns the harness identifier. @@ -50,7 +44,7 @@ func (a *Adapter) Capabilities() adapter.HarnessCapabilities { // Detect returns true when ~/.claude exists. func (a *Adapter) Detect() bool { - _, err := os.Stat(filepath.Join(a.home, ".claude")) + _, err := os.Stat(filepath.Join(a.Home, ".claude")) return err == nil } @@ -59,7 +53,7 @@ func (a *Adapter) Detect() bool { // only manages skills/agents symlinks, CLAUDE.md, and MCP servers. func (a *Adapter) Render(b *canonical.Bundle) (*adapter.FileSet, error) { fs := adapter.NewFileSet() - base := filepath.Join(a.home, ".claude") + base := filepath.Join(a.Home, ".claude") fs.Add(adapter.File{ Dest: filepath.Join(base, "skills"), @@ -92,7 +86,10 @@ func (a *Adapter) Render(b *canonical.Bundle) (*adapter.FileSet, error) { overlay := map[string]any{"mcpServers": mcpMap} dedicatedPath := filepath.Join(base, "mcp_servers.json") - dedicatedExisting, _ := os.ReadFile(dedicatedPath) + dedicatedExisting, err := common.ReadExistingFile(dedicatedPath) + if err != nil { + return nil, fmt.Errorf("read %s: %w", dedicatedPath, err) + } dedicatedMerged, err := common.MergeJSONKeys(dedicatedExisting, overlay) if err != nil { return nil, err @@ -103,9 +100,12 @@ func (a *Adapter) Render(b *canonical.Bundle) (*adapter.FileSet, error) { Content: dedicatedMerged, }) - livePath := filepath.Join(a.home, ".claude.json") + livePath := filepath.Join(a.Home, ".claude.json") if _, err := os.Stat(livePath); err == nil { - liveExisting, _ := os.ReadFile(livePath) + liveExisting, err := common.ReadExistingFile(livePath) + if err != nil { + return nil, fmt.Errorf("read %s: %w", livePath, err) + } liveMerged, err := common.MergeJSONKeys(liveExisting, overlay) if err != nil { return nil, err diff --git a/internal/adapters/crush/crush.go b/internal/adapters/crush/crush.go index 5b17382..be7826d 100644 --- a/internal/adapters/crush/crush.go +++ b/internal/adapters/crush/crush.go @@ -2,6 +2,7 @@ package crush import ( + "fmt" "os" "path/filepath" @@ -13,21 +14,16 @@ import ( const name = "crush" // Adapter implements adapter.Adapter for the crush harness. -type Adapter struct{ home string } +type Adapter struct { + *adapter.Base +} -// Option configures an Adapter. -type Option func(*Adapter) - -// WithHome overrides the home directory (defaults to os.UserHomeDir). -func WithHome(h string) Option { return func(a *Adapter) { a.home = h } } +// WithHome overrides the home directory used to resolve target paths. +func WithHome(h string) adapter.BaseOption { return func(b *adapter.Base) { b.Home = h } } // New returns a new Adapter with the given options applied. -func New(opts ...Option) *Adapter { - a := &Adapter{home: common.DefaultHome()} - for _, o := range opts { - o(a) - } - return a +func New(opts ...adapter.BaseOption) *Adapter { + return &Adapter{Base: adapter.NewBase(opts...)} } // Name returns the harness identifier. @@ -47,7 +43,7 @@ func (a *Adapter) Capabilities() adapter.HarnessCapabilities { // Detect returns true when ~/.config/crush exists. func (a *Adapter) Detect() bool { - _, err := os.Stat(filepath.Join(a.home, ".config", "crush")) + _, err := os.Stat(filepath.Join(a.Home, ".config", "crush")) return err == nil } @@ -56,7 +52,7 @@ func (a *Adapter) Detect() bool { // MCP key is "mcp" (not "mcpServers"); entries include a "type" field. func (a *Adapter) Render(b *canonical.Bundle) (*adapter.FileSet, error) { fs := adapter.NewFileSet() - base := filepath.Join(a.home, ".config", "crush") + base := filepath.Join(a.Home, ".config", "crush") fs.Add(adapter.File{ Dest: filepath.Join(base, "skills"), @@ -65,7 +61,10 @@ func (a *Adapter) Render(b *canonical.Bundle) (*adapter.FileSet, error) { }) cfgPath := filepath.Join(base, "crush.json") - existing, _ := os.ReadFile(cfgPath) + existing, err := common.ReadExistingFile(cfgPath) + if err != nil { + return nil, fmt.Errorf("read %s: %w", cfgPath, err) + } overlay := map[string]any{} if providers := common.ProvidersAsCrushMap(&b.Profile); len(providers) > 0 { overlay["providers"] = providers diff --git a/internal/adapters/goose/goose.go b/internal/adapters/goose/goose.go index ccff6a5..0fe5105 100644 --- a/internal/adapters/goose/goose.go +++ b/internal/adapters/goose/goose.go @@ -10,6 +10,7 @@ package goose import ( + "fmt" "os" "path/filepath" "strings" @@ -22,21 +23,16 @@ import ( const name = "goose" // Adapter implements adapter.Adapter for the Goose harness. -type Adapter struct{ home string } +type Adapter struct { + *adapter.Base +} -// Option configures an Adapter. -type Option func(*Adapter) - -// WithHome overrides the home directory (defaults to os.UserHomeDir). -func WithHome(h string) Option { return func(a *Adapter) { a.home = h } } +// WithHome overrides the home directory used to resolve target paths. +func WithHome(h string) adapter.BaseOption { return func(b *adapter.Base) { b.Home = h } } // New returns a new Adapter with the given options applied. -func New(opts ...Option) *Adapter { - a := &Adapter{home: common.DefaultHome()} - for _, o := range opts { - o(a) - } - return a +func New(opts ...adapter.BaseOption) *Adapter { + return &Adapter{Base: adapter.NewBase(opts...)} } // Name returns the harness identifier. @@ -56,7 +52,7 @@ func (a *Adapter) Capabilities() adapter.HarnessCapabilities { // Detect returns true when ~/.config/goose/ exists. func (a *Adapter) Detect() bool { - _, err := os.Stat(filepath.Join(a.home, ".config", "goose")) + _, err := os.Stat(filepath.Join(a.Home, ".config", "goose")) return err == nil } @@ -67,23 +63,26 @@ func (a *Adapter) Detect() bool { // also added to the FileSet so goose can load the provider on first run. func (a *Adapter) Render(b *canonical.Bundle) (*adapter.FileSet, error) { fs := adapter.NewFileSet() - cfgPath := filepath.Join(a.home, ".config", "goose", "config.yaml") + cfgPath := filepath.Join(a.Home, ".config", "goose", "config.yaml") // goose reads skills from ~/.agents/skills//SKILL.md (open Agent Skills spec) fs.Add(adapter.File{ - Dest: filepath.Join(a.home, ".agents", "skills"), + Dest: filepath.Join(a.Home, ".agents", "skills"), Kind: adapter.SymlinkDir, SymlinkTarget: filepath.Join(b.Root, "skills"), }) - existing, _ := os.ReadFile(cfgPath) + existing, err := common.ReadExistingFile(cfgPath) + if err != nil { + return nil, fmt.Errorf("read %s: %w", cfgPath, err) + } provider, model := splitProviderModel(b.Profile.Gateway.DefaultModel) if b.Profile.Gateway.URL != "" { // Emit the custom provider JSON file. body, providerName := common.GooseCustomProviderFile(&b.Profile) if body != nil { - cpDir := filepath.Join(a.home, ".config", "goose", "custom_providers") + cpDir := filepath.Join(a.Home, ".config", "goose", "custom_providers") fs.Add(adapter.File{ Dest: filepath.Join(cpDir, providerName+".json"), Kind: adapter.RenderedFile, diff --git a/internal/adapters/kilo/kilo.go b/internal/adapters/kilo/kilo.go index c06596a..b1770a3 100644 --- a/internal/adapters/kilo/kilo.go +++ b/internal/adapters/kilo/kilo.go @@ -3,6 +3,7 @@ package kilo import ( "encoding/json" + "fmt" "maps" "os" "path/filepath" @@ -16,22 +17,15 @@ const name = "kilo" // Adapter implements adapter.Adapter for the kilo harness. type Adapter struct { - home string + *adapter.Base } -// Option configures an Adapter. -type Option func(*Adapter) - -// WithHome overrides the home directory (defaults to os.UserHomeDir). -func WithHome(h string) Option { return func(a *Adapter) { a.home = h } } +// WithHome overrides the home directory used to resolve target paths. +func WithHome(h string) adapter.BaseOption { return func(b *adapter.Base) { b.Home = h } } // New returns a new Adapter with the given options applied. -func New(opts ...Option) *Adapter { - a := &Adapter{home: common.DefaultHome()} - for _, o := range opts { - o(a) - } - return a +func New(opts ...adapter.BaseOption) *Adapter { + return &Adapter{Base: adapter.NewBase(opts...)} } // Name returns the harness identifier. @@ -51,7 +45,7 @@ func (a *Adapter) Capabilities() adapter.HarnessCapabilities { // Detect returns true when ~/.config/kilo exists. func (a *Adapter) Detect() bool { - _, err := os.Stat(filepath.Join(a.home, ".config", "kilo")) + _, err := os.Stat(filepath.Join(a.Home, ".config", "kilo")) return err == nil } @@ -62,7 +56,7 @@ func (a *Adapter) Detect() bool { // No "providers" key — kilo has no such concept. func (a *Adapter) Render(b *canonical.Bundle) (*adapter.FileSet, error) { fs := adapter.NewFileSet() - base := filepath.Join(a.home, ".config", "kilo") + base := filepath.Join(a.Home, ".config", "kilo") fs.Add(adapter.File{ Dest: filepath.Join(base, "agent"), @@ -71,13 +65,16 @@ func (a *Adapter) Render(b *canonical.Bundle) (*adapter.FileSet, error) { }) // kilo reads skills from ~/.kilo/skills//SKILL.md fs.Add(adapter.File{ - Dest: filepath.Join(a.home, ".kilo", "skills"), + Dest: filepath.Join(a.Home, ".kilo", "skills"), Kind: adapter.SymlinkDir, SymlinkTarget: filepath.Join(b.Root, "skills"), }) cfgPath := filepath.Join(base, "kilo.json") - existing, _ := os.ReadFile(cfgPath) + existing, err := common.ReadExistingFile(cfgPath) + if err != nil { + return nil, fmt.Errorf("read %s: %w", cfgPath, err) + } modelStr := common.KiloModelString(&b.Profile) overlay := map[string]any{} if providers := common.ProvidersAsMap(&b.Profile); len(providers) > 0 { diff --git a/internal/adapters/opencode/opencode.go b/internal/adapters/opencode/opencode.go index f732f64..1d2758d 100644 --- a/internal/adapters/opencode/opencode.go +++ b/internal/adapters/opencode/opencode.go @@ -3,6 +3,7 @@ package opencode import ( "encoding/json" + "fmt" "os" "path/filepath" @@ -14,21 +15,16 @@ import ( const name = "opencode" // Adapter implements adapter.Adapter for the opencode harness. -type Adapter struct{ home string } +type Adapter struct { + *adapter.Base +} -// Option configures an Adapter. -type Option func(*Adapter) - -// WithHome overrides the home directory (defaults to os.UserHomeDir). -func WithHome(h string) Option { return func(a *Adapter) { a.home = h } } +// WithHome overrides the home directory used to resolve target paths. +func WithHome(h string) adapter.BaseOption { return func(b *adapter.Base) { b.Home = h } } // New returns a new Adapter with the given options applied. -func New(opts ...Option) *Adapter { - a := &Adapter{home: common.DefaultHome()} - for _, o := range opts { - o(a) - } - return a +func New(opts ...adapter.BaseOption) *Adapter { + return &Adapter{Base: adapter.NewBase(opts...)} } // Name returns the harness identifier. @@ -48,7 +44,7 @@ func (a *Adapter) Capabilities() adapter.HarnessCapabilities { // Detect returns true when ~/.config/opencode exists. func (a *Adapter) Detect() bool { - _, err := os.Stat(filepath.Join(a.home, ".config", "opencode")) + _, err := os.Stat(filepath.Join(a.Home, ".config", "opencode")) return err == nil } @@ -58,7 +54,7 @@ func (a *Adapter) Detect() bool { // "mcp" (map with type: local|remote entries). func (a *Adapter) Render(b *canonical.Bundle) (*adapter.FileSet, error) { fs := adapter.NewFileSet() - base := filepath.Join(a.home, ".config", "opencode") + base := filepath.Join(a.Home, ".config", "opencode") // opencode reads skills from ~/.config/opencode/skills//SKILL.md fs.Add(adapter.File{ @@ -68,7 +64,10 @@ func (a *Adapter) Render(b *canonical.Bundle) (*adapter.FileSet, error) { }) cfgPath := filepath.Join(base, "opencode.jsonc") - existing, _ := os.ReadFile(cfgPath) + existing, err := common.ReadExistingFile(cfgPath) + if err != nil { + return nil, fmt.Errorf("read %s: %w", cfgPath, err) + } // Build the provider map, then absorb any existing providers at the same // gateway URL so duplicates don't accumulate across apply runs. diff --git a/internal/adapters/zed/zed.go b/internal/adapters/zed/zed.go index e944175..f088a33 100644 --- a/internal/adapters/zed/zed.go +++ b/internal/adapters/zed/zed.go @@ -7,6 +7,7 @@ package zed import ( "encoding/json" + "fmt" "os" "path/filepath" @@ -18,21 +19,16 @@ import ( const name = "zed" // Adapter implements adapter.Adapter for the Zed harness. -type Adapter struct{ home string } +type Adapter struct { + *adapter.Base +} -// Option configures an Adapter. -type Option func(*Adapter) - -// WithHome overrides the home directory (defaults to os.UserHomeDir). -func WithHome(h string) Option { return func(a *Adapter) { a.home = h } } +// WithHome overrides the home directory used to resolve target paths. +func WithHome(h string) adapter.BaseOption { return func(b *adapter.Base) { b.Home = h } } // New returns a new Adapter with the given options applied. -func New(opts ...Option) *Adapter { - a := &Adapter{home: common.DefaultHome()} - for _, o := range opts { - o(a) - } - return a +func New(opts ...adapter.BaseOption) *Adapter { + return &Adapter{Base: adapter.NewBase(opts...)} } // Name returns the harness identifier. @@ -53,7 +49,7 @@ func (a *Adapter) Capabilities() adapter.HarnessCapabilities { // Detect returns true when ~/.config/zed/ exists. func (a *Adapter) Detect() bool { - _, err := os.Stat(filepath.Join(a.home, ".config", "zed")) + _, err := os.Stat(filepath.Join(a.Home, ".config", "zed")) return err == nil } @@ -68,8 +64,11 @@ func (a *Adapter) Detect() bool { // pre-existing parse errors from user-configured entries. func (a *Adapter) Render(b *canonical.Bundle) (*adapter.FileSet, error) { fs := adapter.NewFileSet() - cfgPath := filepath.Join(a.home, ".config", "zed", "settings.json") - existing, _ := os.ReadFile(cfgPath) + cfgPath := filepath.Join(a.Home, ".config", "zed", "settings.json") + existing, err := common.ReadExistingFile(cfgPath) + if err != nil { + return nil, fmt.Errorf("read %s: %w", cfgPath, err) + } // Set context_servers to nil so MergeJSONKeys removes the key entirely, // clearing any entries that trigger the serde ContextServerSettingsContent bug. diff --git a/internal/apply/apply.go b/internal/apply/apply.go index 24bb6d8..e0ce5fb 100644 --- a/internal/apply/apply.go +++ b/internal/apply/apply.go @@ -9,6 +9,7 @@ import ( "path/filepath" "github.com/lukaszraczylo/harness-sync/internal/adapter" + "github.com/lukaszraczylo/harness-sync/internal/adapter/common" "github.com/lukaszraczylo/harness-sync/internal/canonical" "github.com/lukaszraczylo/harness-sync/internal/gitx" "github.com/lukaszraczylo/harness-sync/internal/merge" @@ -42,6 +43,11 @@ type Action struct { // Run renders all adapters and writes their FileSets to disk. // Conflicts are recorded as .rej files; processing continues past conflicts. // +// When Options.Repo is nil the canonical repo is auto-opened from Bundle.Root. +// If that directory is not a git repository the commit step is silently +// skipped — useful for one-off renders (CI smoke tests, dry-runs) where +// initialising a repo would be more surprising than useful. +// // IMPORTANT: harness-sync intentionally does NOT resolve ${VAR} references in // canonical configs. Every supported harness performs its own env-var // expansion at MCP launch / config-load time, so resolving here would only @@ -66,13 +72,30 @@ func Run(opt Options) (*Report, error) { return rep, renderErr } } - if !opt.DryRun && rep.Written > 0 && opt.Repo != nil { - if err := opt.Repo.AddAll(); err != nil { - return rep, err - } - if err := opt.Repo.Commit(fmt.Sprintf("apply: %d files, %d conflicts", rep.Written, rep.Conflicts)); err != nil { - return rep, err - } + if opt.DryRun || rep.Written == 0 { + return rep, nil + } + repo := opt.Repo + if repo == nil { + repo = gitx.New(opt.Bundle.Root) + } + if !repo.IsRepo() { + return rep, nil + } + if err := repo.AddAll(); err != nil { + return rep, err + } + dirty, err := repo.HasStagedChanges() + if err != nil { + return rep, err + } + if !dirty { + // Nothing changed inside the canonical repo (writes were outside it, + // e.g. symlinks to temp dirs in tests). Skip the no-op commit. + return rep, nil + } + if err := repo.Commit(fmt.Sprintf("apply: %d files, %d conflicts", rep.Written, rep.Conflicts)); err != nil { + return rep, err } return rep, nil } @@ -93,8 +116,14 @@ func statePath(root, adapterName, dest string) string { func handleRendered(opt Options, adapterName string, f adapter.File, rep *Report) error { sp := statePath(opt.Bundle.Root, adapterName, f.Dest) - base, _ := os.ReadFile(sp) - current, _ := os.ReadFile(f.Dest) + base, readErr := common.ReadExistingFile(sp) + if readErr != nil { + return fmt.Errorf("read state %s: %w", sp, readErr) + } + current, readErr := common.ReadExistingFile(f.Dest) + if readErr != nil { + return fmt.Errorf("read target %s: %w", f.Dest, readErr) + } if string(current) == string(f.Content) { rep.Skipped++ diff --git a/internal/apply/apply_test.go b/internal/apply/apply_test.go index fc10ff9..fb116d2 100644 --- a/internal/apply/apply_test.go +++ b/internal/apply/apply_test.go @@ -584,6 +584,160 @@ func TestApplyUnknownFileKind(t *testing.T) { assert.Contains(t, err.Error(), "unknown file kind") } +// --------------------------------------------------------------------------- +// TestApplyPropagatesReadErrors — regression for the silent-read footgun. +// A target file in a directory we can no longer read must surface as an +// error from Run, not as a fast-forward overwrite. +// --------------------------------------------------------------------------- + +func TestApplyPropagatesReadErrors(t *testing.T) { + if os.Geteuid() == 0 { + t.Skip("running as root; cannot test permission denial") + } + root := t.TempDir() + targetDir := t.TempDir() + target := filepath.Join(targetDir, "out.txt") + require.NoError(t, os.WriteFile(target, []byte("user-content\n"), 0o600)) + require.NoError(t, initCanonical(root)) + + // Strip read perms on the target's parent so os.ReadFile fails. + require.NoError(t, os.Chmod(targetDir, 0o000)) + t.Cleanup(func() { _ = os.Chmod(targetDir, 0o755) }) + + ad := &stubAdapter{name: "stub", files: []adapter.File{ + {Dest: target, Kind: adapter.RenderedFile, Content: []byte("new-render\n")}, + }} + _, err := Run(Options{ + Bundle: &canonical.Bundle{Root: root}, + Adapters: []adapter.Adapter{ad}, + Repo: gitx.New(root), + }) + require.Error(t, err) + assert.Contains(t, err.Error(), "read target") + + // User's on-disk file must be untouched. + _ = os.Chmod(targetDir, 0o755) + body, rerr := os.ReadFile(target) + require.NoError(t, rerr) + assert.Equal(t, "user-content\n", string(body), + "target must not be overwritten when read fails") +} + +// --------------------------------------------------------------------------- +// TestApplyAutoOpensRepoFromBundleRoot — regression: callers no longer need +// to pre-construct a *gitx.Repo. Run opens one from Bundle.Root, skips +// commit gracefully when the root is not a git repo, and commits when it is. +// --------------------------------------------------------------------------- + +func TestApplyAutoOpensRepoFromBundleRoot(t *testing.T) { + root := t.TempDir() + target := filepath.Join(t.TempDir(), "out.txt") + require.NoError(t, initCanonical(root)) + + ad := &stubAdapter{name: "stub", files: []adapter.File{ + {Dest: target, Kind: adapter.RenderedFile, Content: []byte("auto\n")}, + }} + // Intentionally pass Repo: nil — the new behaviour should open one itself. + rep, err := Run(Options{ + Bundle: &canonical.Bundle{Root: root}, + Adapters: []adapter.Adapter{ad}, + }) + require.NoError(t, err) + assert.Equal(t, 1, rep.Written) + + // git log should have a new commit from apply, on top of the init one. + out, gitErr := runGit(root, "log", "--oneline") + require.NoError(t, gitErr) + assert.GreaterOrEqual(t, countLines(out), 2) +} + +// --------------------------------------------------------------------------- +// TestApplyNoRepoSkipsCommit — when Bundle.Root isn't a git repo, Run must +// still succeed (no error, no commit attempt). One-off renders / dry-runs. +// --------------------------------------------------------------------------- + +func TestApplyNoRepoSkipsCommit(t *testing.T) { + root := t.TempDir() // NOT initCanonical — no .git + target := filepath.Join(t.TempDir(), "out.txt") + + ad := &stubAdapter{name: "stub", files: []adapter.File{ + {Dest: target, Kind: adapter.RenderedFile, Content: []byte("x\n")}, + }} + rep, err := Run(Options{ + Bundle: &canonical.Bundle{Root: root}, + Adapters: []adapter.Adapter{ad}, + }) + require.NoError(t, err) + assert.Equal(t, 1, rep.Written) + body, rerr := os.ReadFile(target) + require.NoError(t, rerr) + assert.Equal(t, "x\n", string(body)) +} + +// --------------------------------------------------------------------------- +// TestApplySkipsNoOpCommit — symlinks at a path outside the canonical repo +// do not write a state snapshot inside the repo (handleSymlink has no +// state-tracking), so git has nothing to commit. apply.Run must detect +// "no staged changes" and skip the commit instead of failing with +// "nothing to commit" from git itself. +// --------------------------------------------------------------------------- + +func TestApplySkipsNoOpCommit(t *testing.T) { + root := t.TempDir() + require.NoError(t, initCanonical(root)) + targetDir := t.TempDir() + target := filepath.Join(targetDir, "outside-link") + real := filepath.Join(targetDir, "real.txt") + require.NoError(t, os.WriteFile(real, []byte("real\n"), 0o600)) + + ad := &stubAdapter{name: "stub", files: []adapter.File{ + {Dest: target, Kind: adapter.SymlinkFile, SymlinkTarget: real}, + }} + rep, err := Run(Options{ + Bundle: &canonical.Bundle{Root: root}, + Adapters: []adapter.Adapter{ad}, + // Repo: nil — auto-open from Bundle.Root (which IS a git repo). + }) + require.NoError(t, err) + assert.Equal(t, 1, rep.Written) + + // Symlink exists on disk. + got, lerr := os.Readlink(target) + require.NoError(t, lerr) + assert.Equal(t, real, got) + + // No extra commit — still just the init commit. A failed "nothing to + // commit" would surface here as an error from Run. + out, gitErr := runGit(root, "log", "--oneline") + require.NoError(t, gitErr) + assert.Equal(t, 1, countLines(out), + "expected only the init commit; symlink outside canonical root should not create one") +} + +// --------------------------------------------------------------------------- +// TestApplyCommitMessageHasCounts — commit message includes the file and +// conflict counts so the rollback story is auditable from git log alone. +// --------------------------------------------------------------------------- + +func TestApplyCommitMessageHasCounts(t *testing.T) { + root := t.TempDir() + target := filepath.Join(t.TempDir(), "msg.txt") + require.NoError(t, initCanonical(root)) + + ad := &stubAdapter{name: "stub", files: []adapter.File{ + {Dest: target, Kind: adapter.RenderedFile, Content: []byte("m\n")}, + }} + _, err := Run(Options{ + Bundle: &canonical.Bundle{Root: root}, + Adapters: []adapter.Adapter{ad}, + }) + require.NoError(t, err) + + out, gitErr := runGit(root, "log", "-1", "--pretty=%s") + require.NoError(t, gitErr) + assert.Contains(t, out, "apply: 1 files, 0 conflicts") +} + // --------------------------------------------------------------------------- // helpers // --------------------------------------------------------------------------- diff --git a/internal/gitx/repo.go b/internal/gitx/repo.go index 7b68450..2c4aed9 100644 --- a/internal/gitx/repo.go +++ b/internal/gitx/repo.go @@ -71,6 +71,17 @@ func (r *Repo) IsRepo() bool { return err == nil } +// HasStagedChanges reports whether the index currently differs from HEAD. +// Used by callers that want to skip a no-op `git commit` (commit exits 1 +// when there is nothing to commit, which is the wrong signal to propagate). +func (r *Repo) HasStagedChanges() (bool, error) { + b, err := r.run("status", "--porcelain") + if err != nil { + return false, err + } + return len(bytes.TrimSpace(b)) > 0, nil +} + // Revert undoes the last N commits with `git revert --no-edit HEAD~N..HEAD`. // n must be >= 1. func (r *Repo) Revert(n int) error { diff --git a/internal/gitx/repo_test.go b/internal/gitx/repo_test.go index 3947c54..c942c1f 100644 --- a/internal/gitx/repo_test.go +++ b/internal/gitx/repo_test.go @@ -41,3 +41,33 @@ func TestShowFileAtHead(t *testing.T) { _, err = r.ShowFileAtHead("nope.txt") assert.Error(t, err) } + +func TestHasStagedChangesEmptyAfterInit(t *testing.T) { + dir := t.TempDir() + r := New(dir) + require.NoError(t, r.Init()) + require.NoError(t, r.Configure("nobody", "nobody@example.com")) + // No commits yet, working tree clean. + dirty, err := r.HasStagedChanges() + require.NoError(t, err) + assert.False(t, dirty) +} + +func TestHasStagedChangesAfterAdd(t *testing.T) { + dir := t.TempDir() + r := New(dir) + require.NoError(t, r.Init()) + require.NoError(t, r.Configure("nobody", "nobody@example.com")) + require.NoError(t, os.WriteFile(filepath.Join(dir, "a.txt"), []byte("x"), 0o600)) + require.NoError(t, r.AddAll()) + + dirty, err := r.HasStagedChanges() + require.NoError(t, err) + assert.True(t, dirty) + + // After commit, index matches HEAD again → no staged changes. + require.NoError(t, r.Commit("c")) + dirty, err = r.HasStagedChanges() + require.NoError(t, err) + assert.False(t, dirty) +}