mirror of
https://github.com/lukaszraczylo/harness-sync.git
synced 2026-10-13 00:48:30 +00:00
fix(apply): propagate read errors, auto-open repo, dedupe adapter base (#6)
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.
This commit is contained in:
+38
-9
@@ -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 <dest>.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++
|
||||
|
||||
@@ -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
|
||||
// ---------------------------------------------------------------------------
|
||||
|
||||
Reference in New Issue
Block a user