From 1ab97bff4302e95bd0c20d492ca9f91dd32dfcb8 Mon Sep 17 00:00:00 2001 From: m3ta-chiron Date: Fri, 2 Oct 2026 12:34:57 +0200 Subject: [PATCH] feat(client): sync --force to escape the locally-modified skip MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Managed items with local modifications were kept (skip + warn) with no escape hatch other than deleting the deployed file by hand — after an upstream change the deployed content silently stayed old, which reads exactly like a stale cache. --force turns the two skip cases into update (repository version wins) and remove (upstream deleted + modified). Unmanaged items and never- managed path occupancies stay untouched even with --force; the default behavior is unchanged. --- README.md | 7 ++- internal/cli/sync.go | 9 +++- internal/client/planner.go | 17 +++++-- internal/client/planner_test.go | 56 ++++++++++++++++++++-- internal/client/sync.go | 6 ++- internal/e2e/sync_force_test.go | 83 +++++++++++++++++++++++++++++++++ 6 files changed, 164 insertions(+), 14 deletions(-) create mode 100644 internal/e2e/sync_force_test.go diff --git a/README.md b/README.md index d12dfbb..e9a7bcd 100644 --- a/README.md +++ b/README.md @@ -196,6 +196,11 @@ repository, the local manifest and the on-disk content: - upstream deleted & unmodified → **remove** - upstream deleted & modified → **skip + warn** — your version stays +With `--force` the two skip cases turn into **update** and **remove** — +the repository version wins over local modifications. Unmanaged items +(leave + warn) are never touched, even with `--force`; neither are paths +occupied by items agent-lib never deployed. + Sync is idempotent (a second run is a byte-level no-op) and fails cleanly: an unreachable repository never leaves a half-deployed machine. The default branch is resolved from the remote's advertised `HEAD`, so a sync without @@ -275,7 +280,7 @@ flag that drives the non-zero exit code — suitable for headless drift checks. | `error: … include entry "x" not found upstream` | Typo, or the item was renamed/removed upstream. Fix the lockfile include list, or re-add with a corrected `--include`. | | `error: … collides with own skills "x"` | Deployed-name collision. Add `--rename =` (add) or fix the rename map (update). | | `validate` reports tree/lockfile divergence | Someone edited `external/` by hand. Re-run `vendor update ` to restore the pinned state. | -| Skill didn't update, `skipped (local modifications kept): 1` | Working as designed — you edited the deployed item. Restore the company version by deleting the local copy and re-syncing. | +| Skill didn't update, `skipped (local modifications kept): 1` | Working as designed — you edited the deployed item. Run `agent-lib sync --force` to overwrite local modifications with the repository version, or delete the local copy and re-sync. | | `status` exits 1 | Local modifications block repository updates (or occupy names the repo needs). See `status` output for the item list. | | Sync fails with clone/fetch error | Work repository unreachable, or token expired. Previous local state remains intact; fix config/network and re-run. | | No toast on Windows | Toasts need an interactive user session — the scheduled task must run as the logged-in user, not SYSTEM. | diff --git a/internal/cli/sync.go b/internal/cli/sync.go index f7bd823..a856941 100644 --- a/internal/cli/sync.go +++ b/internal/cli/sync.go @@ -10,6 +10,7 @@ import ( func newSyncCmd() *cobra.Command { var configPath, repoURL, ref string + var force bool cmd := &cobra.Command{ Use: "sync", Short: "Pull the work repository and deploy its content to OpenCode", @@ -17,10 +18,13 @@ func newSyncCmd() *cobra.Command { "into the OpenCode runtime paths. The repository comes from --repo, " + "or from the config chain: --config, " + client.ConfigEnv + " env, the " + "user-level config, or the admin-protected platform default. " + - "Idempotent: re-running changes nothing when content and disk match.", + "Idempotent: re-running changes nothing when content and disk match. " + + "Locally modified managed items are kept by default (skip + warn); " + + "--force overwrites them with the repository version and removes " + + "them after upstream deletion. Unmanaged items are never touched.", Args: cobra.NoArgs, RunE: func(cmd *cobra.Command, args []string) error { - opts := client.SyncOptions{ConfigPath: configPath, RepoURL: repoURL, Ref: ref} + opts := client.SyncOptions{ConfigPath: configPath, RepoURL: repoURL, Ref: ref, Force: force} if _, err := client.Sync(opts, cmd.OutOrStdout()); err != nil { return fmt.Errorf("sync: %w", err) } @@ -30,5 +34,6 @@ func newSyncCmd() *cobra.Command { cmd.Flags().StringVar(&configPath, "config", "", "client config path (default: "+client.ConfigEnv+" env, user config, or platform default)") cmd.Flags().StringVar(&repoURL, "repo", "", "work repository URL or path — overrides any config, works without one") cmd.Flags().StringVar(&ref, "ref", "", "branch or tag to pull (requires --repo)") + cmd.Flags().BoolVar(&force, "force", false, "overwrite locally modified managed items with the repository version; unmanaged items are never touched") return cmd } diff --git a/internal/client/planner.go b/internal/client/planner.go index 2062165..4c499fe 100644 --- a/internal/client/planner.go +++ b/internal/client/planner.go @@ -47,8 +47,11 @@ type PlanEntry struct { // Plan is a pure function from (inventory, manifest, on-disk facts) to an // action plan. pathFor injects the target path mapping so the planner never // touches the filesystem or the environment. The plan is sorted by -// (type, name) for deterministic execution and reporting. -func Plan(items []PlannedItem, manifest *Manifest, disk map[string]DiskFact, pathFor func(typ, name string) string) []PlanEntry { +// (type, name) for deterministic execution and reporting. With force set, +// locally modified managed items are overwritten with the upstream version +// (and removed after upstream deletion) instead of being skipped; items +// agent-lib never managed are never touched, even with force. +func Plan(items []PlannedItem, manifest *Manifest, disk map[string]DiskFact, pathFor func(typ, name string) string, force bool) []PlanEntry { invByKey := map[string]PlannedItem{} for _, it := range items { invByKey[manifestKey(it.Type, it.Name)] = it @@ -93,13 +96,15 @@ func Plan(items []PlannedItem, manifest *Manifest, disk map[string]DiskFact, pat emit(ActDeploy, it, "") } case inInv && it.Hash == rec.Hash: - decideAgainstDisk(it, rec, fact, emit, "unchanged") + decideAgainstDisk(it, rec, fact, emit, "unchanged", force) case inInv: - decideAgainstDisk(it, rec, fact, emit, "changed") + decideAgainstDisk(it, rec, fact, emit, "changed", force) case rec != nil && !fact.Exists: emit(ActRemove, PlannedItem{Type: rec.Type, Name: rec.Name, Origin: rec.Origin}, "already gone from disk; dropping manifest record") case rec != nil && fact.Hash == rec.Hash: emit(ActRemove, PlannedItem{Type: rec.Type, Name: rec.Name, Origin: rec.Origin}, "upstream deleted") + case rec != nil && force: + emit(ActRemove, PlannedItem{Type: rec.Type, Name: rec.Name, Origin: rec.Origin}, "upstream deleted; forced removal of the locally modified copy") case rec != nil: emit(ActSkip, PlannedItem{Type: rec.Type, Name: rec.Name, Origin: rec.Origin}, "upstream deleted but the local copy was modified; keeping it") @@ -111,7 +116,7 @@ func Plan(items []PlannedItem, manifest *Manifest, disk map[string]DiskFact, pat return plan } -func decideAgainstDisk(it PlannedItem, rec *ManifestItem, fact DiskFact, emit func(Action, PlannedItem, string), upstreamState string) { +func decideAgainstDisk(it PlannedItem, rec *ManifestItem, fact DiskFact, emit func(Action, PlannedItem, string), upstreamState string, force bool) { switch { case !fact.Exists: emit(ActUpdate, it, "managed item missing on disk; restoring") @@ -121,6 +126,8 @@ func decideAgainstDisk(it PlannedItem, rec *ManifestItem, fact DiskFact, emit fu } else { emit(ActUpdate, it, "") } + case force: + emit(ActUpdate, it, "forced: locally modified copy overwritten") default: emit(ActSkip, it, "locally modified; keeping the local version (upstream "+upstreamState+")") } diff --git a/internal/client/planner_test.go b/internal/client/planner_test.go index 3e6335e..a7fc519 100644 --- a/internal/client/planner_test.go +++ b/internal/client/planner_test.go @@ -98,7 +98,7 @@ func TestPlanMatrix(t *testing.T) { diskFacts[key] = DiskFact{Exists: true, Hash: hashMod} } - plan := Plan(items, manifest, diskFacts, testPathFor) + plan := Plan(items, manifest, diskFacts, testPathFor, false) if tc.disk == diskNone && tc.up == upAbsent { if len(plan) != 0 { t.Fatalf("a key present nowhere must produce no entry, got %+v", plan) @@ -123,6 +123,54 @@ func TestPlanMatrix(t *testing.T) { } } +// TestPlanForceMatrix asserts how --force changes the skip cases: locally +// modified managed items become updates (or removes after upstream +// deletion), while never-managed occupancies stay skipped even with force. +func TestPlanForceMatrix(t *testing.T) { + cases := []struct { + name string + up string // "changed", "unchanged", "gone" + want Action + }{ + {"modified/changed", "changed", ActUpdate}, + {"modified/unchanged", "unchanged", ActUpdate}, + {"modified/gone", "gone", ActRemove}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + key := manifestKey("skills", "alpha") + var items []PlannedItem + manifest := NewManifest() + manifest.Items[key] = &ManifestItem{Name: "alpha", Type: "skills", Hash: hashV1} + if tc.up != "gone" { + hash := hashV2 + if tc.up == "unchanged" { + hash = hashV1 + } + items = append(items, PlannedItem{Type: "skills", Name: "alpha", RelPath: "skills/alpha", Hash: hash}) + } + disk := map[string]DiskFact{key: {Exists: true, Hash: hashMod}} + + plan := Plan(items, manifest, disk, testPathFor, true) + if len(plan) != 1 { + t.Fatalf("want exactly 1 entry, got %d: %+v", len(plan), plan) + } + if plan[0].Action != tc.want { + t.Errorf("action = %s, want %s (reason: %s)", plan[0].Action, tc.want, plan[0].Reason) + } + }) + } + + t.Run("never-managed occupancy stays skipped", func(t *testing.T) { + items := []PlannedItem{{Type: "skills", Name: "alpha", RelPath: "skills/alpha", Hash: hashV1}} + disk := map[string]DiskFact{manifestKey("skills", "alpha"): {Exists: true, Hash: hashMod}} + plan := Plan(items, NewManifest(), disk, testPathFor, true) + if len(plan) != 1 || plan[0].Action != ActSkip { + t.Fatalf("force must not deploy over a never-managed item: %+v", plan) + } + }) +} + // TestPlanMixedFixture combines several states in one plan and asserts the // full deterministic output. func TestPlanMixedFixture(t *testing.T) { @@ -151,7 +199,7 @@ func TestPlanMixedFixture(t *testing.T) { "agents/helper": {Exists: true, Hash: hashV1}, // unmodified × unchanged } - plan := Plan(items, manifest, disk, testPathFor) + plan := Plan(items, manifest, disk, testPathFor, false) got := map[string]Action{} for _, e := range plan { got[e.Type+"/"+e.Name] = e.Action @@ -195,8 +243,8 @@ func TestPlanPureGuarantee(t *testing.T) { manifest := manifestWith(map[string]*ManifestItem{"skills/b": {Name: "b", Type: "skills", Hash: hashV1}}) disk := map[string]DiskFact{"skills/b": {Exists: true, Hash: hashV1}} - p1 := Plan(items, manifest, disk, testPathFor) - p2 := Plan(items, manifest, disk, testPathFor) + p1 := Plan(items, manifest, disk, testPathFor, false) + p2 := Plan(items, manifest, disk, testPathFor, false) if len(p1) != len(p2) { t.Fatal("same inputs must yield same plan length") } diff --git a/internal/client/sync.go b/internal/client/sync.go index 265e647..891a0e9 100644 --- a/internal/client/sync.go +++ b/internal/client/sync.go @@ -12,11 +12,13 @@ import ( // SyncOptions parameterize a client sync run. RepoURL (with optional Ref) // is the inline --repo override: it works without any config file and beats -// a repo_url discovered in the config chain. +// a repo_url discovered in the config chain. Force switches the planner +// from skip+warn to overwrite for locally modified managed items. type SyncOptions struct { ConfigPath string RepoURL string Ref string + Force bool } // prepared is everything sync and status share: the pulled repository, the @@ -80,7 +82,7 @@ func prepare(opts SyncOptions) (*prepared, error) { path, _ := oc.DeployPath(typ, name) return path } - plan := Plan(items, manifest, diskFacts(oc, manifest, items), pathFor) + plan := Plan(items, manifest, diskFacts(oc, manifest, items), pathFor, opts.Force) for _, e := range plan { if !safePath(e.DstPath) { return nil, fmt.Errorf("refusing unsafe deploy path %q", e.DstPath) diff --git a/internal/e2e/sync_force_test.go b/internal/e2e/sync_force_test.go new file mode 100644 index 0000000..d05edca --- /dev/null +++ b/internal/e2e/sync_force_test.go @@ -0,0 +1,83 @@ +package e2e + +import ( + "os" + "path/filepath" + "testing" +) + +// TestSyncForceOverwritesLocallyModified pins the --force escape hatch: +// locally modified managed items are overwritten with the repository +// version, upstream-deleted modified items are removed, and unmanaged +// local items are never touched. +func TestSyncForceOverwritesLocallyModified(t *testing.T) { + h := newHarness(t) + env := newSyncEnv(t, h) + buildWorkRepo(t, h, env.workRepo) + env.writeConfig(t, env.workRepo, "") + env.run(t) + + // local drift on a managed item + upstream change to the same item + deployed := filepath.Join(env.home, ".config/opencode/commands/deploy.md") + os.WriteFile(deployed, []byte("---\nname: Deploy\n---\nlocal drift\n"), 0o644) + upstream := filepath.Join(env.workRepo, "commands/deploy.md") + os.WriteFile(upstream, []byte("---\nname: Deploy\ndescription: updated\n---\nnew body\n"), 0o644) + + // an unmanaged local item that must survive --force untouched + unmanaged := filepath.Join(env.home, ".config/opencode/commands/mine.md") + os.MkdirAll(filepath.Dir(unmanaged), 0o755) + os.WriteFile(unmanaged, []byte("---\nname: Mine\n---\nprecious\n"), 0o644) + + h.git(t, env.workRepo, "add", "-A") + h.git(t, env.workRepo, "commit", "-m", "update deploy command") + + out, err := env.run(t) + if err != nil { + t.Fatalf("sync failed: %v\n%s", err, out) + } + if !contains(out, "skipped (local modifications kept): 1") { + t.Errorf("default sync must keep the local modification:\n%s", out) + } + + out, err = env.run(t, "--force") + if err != nil { + t.Fatalf("sync --force failed: %v\n%s", err, out) + } + if !contains(out, "updated: 1") { + t.Errorf("force must update the locally modified item:\n%s", out) + } + if got := readFile(t, deployed); !contains(got, "new body") { + t.Error("force must overwrite the local drift with the repository version") + } + if got := readFile(t, unmanaged); !contains(got, "precious") { + t.Error("force must never touch unmanaged local items") + } + + // upstream deletion of a locally modified item: force removes it + os.WriteFile(deployed, []byte("---\nname: Deploy\n---\nmore drift\n"), 0o644) + os.Remove(upstream) + h.git(t, env.workRepo, "add", "-A") + h.git(t, env.workRepo, "commit", "-m", "delete deploy command") + + out, err = env.run(t) + if err != nil { + t.Fatalf("sync failed: %v\n%s", err, out) + } + if !contains(out, "skipped (local modifications kept): 1") { + t.Errorf("default sync must keep the modified item after upstream deletion:\n%s", out) + } + if _, err := os.Stat(deployed); err != nil { + t.Fatal("item must still exist before force") + } + + out, err = env.run(t, "--force") + if err != nil { + t.Fatalf("sync --force failed: %v\n%s", err, out) + } + if !contains(out, "removed: 1") { + t.Errorf("force must remove the locally modified item after upstream deletion:\n%s", out) + } + if _, err := os.Stat(deployed); !os.IsNotExist(err) { + t.Error("forced removal must delete the deployed file") + } +}