From 3b5cd58124d70c89ea432bab4b2cfcb1ee6cb62d Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 5 Sep 2026 02:29:14 +0000 Subject: [PATCH 1/7] refactor(cli): one callable for the apply pipeline (#231) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Pure extraction. `applyRun(cmd, home, dryRun, noGitBackup, agentsCSV)` becomes `runApplyPipeline(cmd, home, applyOpts)`, where `applyOpts` carries exactly `apply`'s three flags and its zero value is a plain real apply of every enabled agent with git backup on — the shape a caller that is not the `apply` command wants. `newApplyCmd` builds the options once and passes them down both the unlocked dry-run arm and the locked arm; the lock structure is untouched and no line of the pipeline body moves. No behaviour change: `apply`'s output, and the plugin path's, are byte-identical to the parent commit across five CLI fixture scenarios (stdout, stderr and exit status). No test is edited. Each field of the options struct is load-bearing — forcing `dryRun`, `noGitBackup` or `agentsCSV` inside the pipeline fails the existing dry-run, git-backup and `--agents` suites respectively. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01M4VNyoCuGXx7pYxNfLVbFG --- internal/cli/apply.go | 52 ++++++++++++++++++++++++++++++++++++------- 1 file changed, 44 insertions(+), 8 deletions(-) diff --git a/internal/cli/apply.go b/internal/cli/apply.go index 070d09e8..408f734d 100644 --- a/internal/cli/apply.go +++ b/internal/cli/apply.go @@ -34,15 +34,16 @@ func newApplyCmd() *cobra.Command { Args: cobra.NoArgs, RunE: func(cmd *cobra.Command, _ []string) error { home := paths.AgentsyncHome(paths.OSEnv{}) + o := applyOpts{dryRun: dryRun, noGitBackup: noGitBackup, agentsCSV: agentsCSV} // Dry-run is read-only — it touches neither destinations nor // state. Acquiring the global lock would needlessly block // concurrent `status` / `diff` / other dry-runs behind a long // real apply. if dryRun { - return applyRun(cmd, home, dryRun, noGitBackup, agentsCSV) + return runApplyPipeline(cmd, home, o) } return withGlobalLock(home, func() error { - return applyRun(cmd, home, dryRun, noGitBackup, agentsCSV) + return runApplyPipeline(cmd, home, o) }) }, } @@ -65,9 +66,44 @@ func noAgentsEnabledHint(sc adapter.Scope, projectRoot string) string { return "no agents enabled; run `agentsync agent add claude` (or opencode)" } -// applyRun is the lock-protected body of the apply command. It is split -// out from newApplyCmd so the lock acquisition lives in one obvious place. -func applyRun(cmd *cobra.Command, home string, dryRun, noGitBackup bool, agentsCSV string) error { +// applyOpts carries the per-run knobs of the apply pipeline. The ZERO VALUE is +// a plain real apply of every enabled agent with destination git backup on — +// which is exactly what a caller that is not the `apply` command wants. The +// three fields are exactly `apply`'s three flags and nothing else: a behaviour +// gate added here would recreate, one field at a time, the divergence #231 +// closed. Anything a caller wants to say differently is said at the call site, +// before or after the call. +type applyOpts struct { + // dryRun is `apply --dry-run`: compute and print the plan, write nothing, + // skip the git backup. + dryRun bool + // noGitBackup is `apply --no-git-backup`: skip the destination git + // baseline/checkpoint for this run only. + noGitBackup bool + // agentsCSV is the raw `--agents` value. selectAgents consults it only when + // the CALLING command reports cmd.Flags().Changed("agents") — the one + // non-persistent flag the pipeline reads (every other cmd read inside is a + // root persistent flag: --color, --scope, --project, --no-input). pflag + // answers false for a flag the command never registered, so a caller whose + // command does not define `--agents` gets every enabled agent whatever this + // holds (pinned by TestPluginUpgrade_RendersEveryEnabledAgent). The residual + // is a caller whose command defines a same-named `--agents` with a + // DIFFERENT meaning (`mcp add --agents`): it must pass an explicit + // agentsCSV rather than rely on the flag being absent. + agentsCSV string +} + +// runApplyPipeline is the apply pipeline — load-projected source → resolve +// secrets → plan → git baseline → write → record state → checkpoint → report — +// and the lock-protected body of the apply command. It is split out from +// newApplyCmd so the lock acquisition lives in one obvious place, and it is +// called by BOTH `apply` and the re-apply tail of `plugin upgrade` +// (reapplyAfterPluginChange) so the two cannot diverge: the second copy had +// already lost the pre-apply baseline and checkpoint (#118/#143), the +// removal-aware headline, the backup pruning and the translation report +// (#231). home is the agentsync home; the printer, scope, secrets backend and +// state path are all derived inside, so a caller cannot hand in a stale one. +func runApplyPipeline(cmd *cobra.Command, home string, o applyOpts) error { p, err := newPrinter(cmd) if err != nil { return err @@ -108,7 +144,7 @@ func applyRun(cmd *cobra.Command, home string, dryRun, noGitBackup bool, agentsC // status/diff use (#200 F10). Applied after the enabled set is built, so an // unknown or disabled name is rejected rather than silently rendering nothing. if len(agents) > 0 { - sel, aerr := selectAgents(cmd, agents, enabled, agentsCSV) + sel, aerr := selectAgents(cmd, agents, enabled, o.agentsCSV) if aerr != nil { return aerr } @@ -139,7 +175,7 @@ func applyRun(cmd *cobra.Command, home string, dryRun, noGitBackup bool, agentsC return err } - if dryRun { + if o.dryRun { plan, err := render.Plan(resolved, reg, agents, sc, projectRoot, s, userHome) if err != nil { return err @@ -228,7 +264,7 @@ func applyRun(cmd *cobra.Command, home string, dryRun, noGitBackup bool, agentsC // checkpoint below so a fresh dir is inited/prompted exactly once. Best-effort with // a loud warning — a baseline failure never aborts the apply (honors // --no-git-backup / mode=off / project scope / a declined prompt, all as nil). - gb := newGitBackupSession(cmd, p, reg, agents, sc, projectRoot, home, c.Config.DestinationGitBackup, noGitBackup) + gb := newGitBackupSession(cmd, p, reg, agents, sc, projectRoot, home, c.Config.DestinationGitBackup, o.noGitBackup) gb.baseline(baselinePaths(plan, s, userHome, sc, projectRoot)) collisions, written, unchanged, applyErr := render.Apply(plan, reg, s, home, userHome, sc, projectRoot) From 24f601fd1fbaa18ade41878d383bca92a59950b1 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 5 Sep 2026 02:36:01 +0000 Subject: [PATCH 2/7] fix(cli): plugin upgrade re-applies through the real apply pipeline (#231) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `plugin upgrade ` and `plugin upgrade --all` ended in a second, hand-maintained transcription of the apply pipeline, and the copy had fallen behind: it took no pre-apply git baseline and no post-apply checkpoint (#118/#143), so an upgrade overwrote ~/.claude, ~/.codex, … with nothing for `agentsync revert` to undo; it printed an unconditional "applied: N ops" for a run that wrote nothing; it never pruned collision backups; it had no zero-agents guard; and it printed no translation report. `reapplyAfterPluginChange` is now a six-line delegation to `runApplyPipeline(cmd, home, applyOpts{})` with one error wrap (`re-apply after plugin upgrade:`), so every apply-side invariant holds on the plugin path by construction. The plugin commands define none of `apply`'s three flags, so the zero options are the right ones: a real apply of every enabled agent, git backup governed by the configured mode. User-visible: the upgrade now announces its scope, takes the git baseline/checkpoint, reports removals and idempotent runs honestly, warns and exits 0 with no agents enabled, and prints the per-plugin translation report; under the default `prompt` git-backup mode an unattended run also prints apply's hint and baseline warning (CHANGELOG, user guide and the updating/rollback guides say so). Tests: the source guard is renamed to `TestApplyPipelineLoadsStateAfterSourceReload` and strengthened — half A holds the load-state-after-reload ordering against `runApplyPipeline`, half B requires `reapplyAfterPluginChange` to call `runApplyPipeline` and not `state.Load`/`render.Plan`/`render.Apply`/`state.Save` itself, so a hand-rolled re-apply fails by name. Two `plugin_verbs_test.go` assertions accept apply's honest `up to date:` headline and additionally require the translation report, which only the real pipeline emits. Two new behavioural pins: the upgrade's re-apply records a pre-apply baseline and a checkpoint (dropping the git-backup pass fails nothing without it), and it renders every enabled agent (`--agents` narrowing cannot leak in). Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01M4VNyoCuGXx7pYxNfLVbFG --- CHANGELOG.md | 26 ++++ docs/architecture.md | 19 ++- docs/components.md | 6 +- docs/user-guide.md | 22 ++-- internal/cli/apply.go | 4 +- ....go => apply_state_order_internal_test.go} | 62 ++++++++-- internal/cli/plugin.go | 9 +- internal/cli/plugin_poll.go | 97 ++++----------- internal/cli/plugin_reapply_pipeline_test.go | 117 ++++++++++++++++++ internal/cli/plugin_verbs_test.go | 25 +++- website/src/content/docs/guides/rollback.mdx | 10 +- website/src/content/docs/guides/updating.mdx | 10 +- website/src/content/docs/reference/cli.mdx | 3 +- 13 files changed, 297 insertions(+), 113 deletions(-) rename internal/cli/{reapply_state_order_internal_test.go => apply_state_order_internal_test.go} (50%) create mode 100644 internal/cli/plugin_reapply_pipeline_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index d5a84b0b..7167ca38 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -222,6 +222,16 @@ source layout, CLI surface, and state schema are stabilizing but may still chang The user guide, the daily-loop guide and the CLI reference said the same wrong thing and are corrected with it. +- **`plugin upgrade` now git-backs-up the destinations it overwrites** + ([#231](https://github.com/spxrogers/agentsync/issues/231)). Both upgrade + forms end in a re-apply that writes user-scope destination dirs, but that + re-apply was a second copy of the apply pipeline with no + `[destination_directory_git_backup]` pass at all — so an upgrade overwrote + `~/.claude`, `~/.codex`, … with **no pre-apply baseline and no checkpoint**, + and `agentsync revert` could not undo it. The re-apply is now `apply` itself, + so the baseline/checkpoint (and the mode/prompt/`--no-input` policy) apply + identically. + ### Changed - **`status --json`, `diff` and `reconcile` now list a shared file's merged keys @@ -235,6 +245,22 @@ source layout, CLI surface, and state schema are stabilizing but may still chang runs, or a `reconcile` transcript compared against a previous one, will be stable for the first time. `explain` already sorted and is unchanged. +- **`plugin upgrade` prints what `apply` prints** + ([#231](https://github.com/spxrogers/agentsync/issues/231)). Its re-apply now + goes through the one apply pipeline, so it announces the effective scope, + reports removals honestly (an upgrade that re-renders identical bytes says + `up to date: N ops, no changes` instead of claiming `applied: N ops`), warns + and exits 0 when no agents are enabled instead of printing `applied: 0 ops`, + prunes old collision backups, and prints the per-plugin translation report — + which is what tells you whether the new version still translates. Two strings + changed with it: the foreign-collision warning is now apply's wording, and the + five `… after upgrade:` error prefixes collapse into one + `re-apply after plugin upgrade:`. Under the default + `[destination_directory_git_backup] mode = "prompt"`, an unattended run + (cron, `--no-input`, no TTY) also prints apply's git-backup hint and its + `could not take a pre-apply baseline` warning on every run until the mode is + set to `on` or `off` — the same two lines an unattended `apply` prints. + - **Internal: `status`, `diff`, `reconcile` and `explain` now share one plan→drift walk** ([#229](https://github.com/spxrogers/agentsync/issues/229)). `explain` now decodes a key-merged destination once per rendered section diff --git a/docs/architecture.md b/docs/architecture.md index e0fd8901..f26a3d7d 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -623,9 +623,10 @@ Key stages: foreign-collision backups (`internal/render`, `internal/iox`). 8. **Record** new hashes in `targets.json` (`internal/state`) and print the translation report. -9. **Git-backup** (issue #118) — for a user-scope apply, checkpoint each destination - **directory** into its own **local-only** git repo (`internal/cli/gitbackup.go` - → `internal/git`). The unit is the directory, not the agent: every enabled +9. **Git-backup** (issue #118) — for a user-scope apply (including the apply at the + tail of `plugin upgrade`), checkpoint each destination **directory** into its + own **local-only** git repo (`internal/cli/gitbackup.go` → `internal/git`). + The unit is the directory, not the agent: every enabled adapter declares its version roots via the optional `adapter.VersionedDirs` extension (its config dir plus any shared cross-agent dir it writes — Codex and several breadth agents all target `~/.agents/skills`; OpenCode targets @@ -670,6 +671,18 @@ Key stages: vs `→ write` and preview foreign-collision backups, and prints the plan/report — all without writing a byte (and it skips the git-backup step 9 entirely). +**One implementation.** The pipeline above lives in a single callable, +`runApplyPipeline` (`internal/cli/apply.go`), and `apply` is not its only +caller: the re-apply tail of `plugin upgrade` and `plugin upgrade --all` +(`reapplyAfterPluginChange`) runs the same function with the default options — +a real apply of every enabled agent, git backup on. It used to be a second, +hand-maintained transcription of steps 1–7 and the state-recording half of +step 8, and the copy had already fallen behind: no translation report (the rest +of step 8), no pre-apply baseline or checkpoint (step 9), no removal-aware +headline, no backup pruning. Every apply-side invariant added from here holds +on the plugin path by construction +([#231](https://github.com/spxrogers/agentsync/issues/231)). + --- ## 5. The capture pipeline (Destination ▶ Source) diff --git a/docs/components.md b/docs/components.md index aa8f89db..bc5be8d8 100644 --- a/docs/components.md +++ b/docs/components.md @@ -45,7 +45,11 @@ Wires every cobra subcommand into the root tree and dispatches to handlers; this is the only package that depends on nearly all the others. - **Key:** `NewRoot() *cobra.Command`, `Execute() int` (returns the process exit code and owns the terminal `✗ ERROR` line), `Version`/`Commit`/`Date`; - `walkPlanItems` — the single plan→state→destination drift walk behind + `runApplyPipeline` — the single apply pipeline (load-projected → resolve + secrets → plan → git baseline → write → record state → checkpoint → report), + shared by `apply` and the re-apply tail of `plugin upgrade` so the two cannot + diverge (#231); its `applyOpts` zero value is a real, all-agents, git-backed + apply; `walkPlanItems` — the single plan→state→destination drift walk behind `status`, `diff`, `reconcile` and `explain` (`planwalk.go`); its `planItem` is deliberately unexported field-for-field so it can never become a `--json` surface, because a plan built from `secrets.SubstituteCanonical` carries diff --git a/docs/user-guide.md b/docs/user-guide.md index 6b4dcad8..5e7b2d00 100644 --- a/docs/user-guide.md +++ b/docs/user-guide.md @@ -271,10 +271,11 @@ instead lists each one as left unresolved, so it works as a non-interactive `apply` can keep each user-scope destination dir (`~/.claude`, `~/.codex`, …) in its **own local-only git repo**, recording a checkpoint commit after every apply -that changes managed files there. **Even the first apply is revertible:** before -that apply overwrites the dir, agentsync records a **pre-apply baseline** commit of -the prior content of the files it is about to manage, so the apply checkpoint's parent -is the genuine pre-apply state — there is no "the first apply can't be undone" gap. +that changes managed files there — including the apply that ends a `plugin +upgrade`. **Even the first apply is revertible:** before that apply overwrites the +dir, agentsync records a **pre-apply baseline** commit of the prior content of the +files it is about to manage, so the apply checkpoint's parent is the genuine +pre-apply state — there is no "the first apply can't be undone" gap. Pre-existing files agentsync did **not** write (an agent's credentials, conversation transcripts, your own scratch files) are deliberately left **out** of the versioned history so it never becomes a durable copy of your secrets — they are untracked, so a @@ -867,8 +868,10 @@ agentsync plugin upgrade --all --lossless # same, skipping bumps that would agentsync plugin upgrade atlassian # re-fetch one plugin, then re-apply ``` -Both `upgrade` forms end in a re-apply, so an upgrade lands in your agents in one -command rather than leaving them stale until the next `apply`. +Both `upgrade` forms end in a re-apply — and the re-apply **is** `apply`: same +scope resolution, same destination git backup, same removal counts and +translation report — so an upgrade lands in your agents in one command rather +than leaving them stale until the next `apply`. `plugin outdated` is not a pure read despite the `npm outdated` prior: it uses the network and it writes state (each marketplace's fetch timestamp and head @@ -882,7 +885,10 @@ fetch. It is simply the one the daily loop runs. Want nightly refreshes? agentsync ships no daemon — wire `agentsync plugin upgrade --all --lossless` into your own cron / launchd / -systemd / Task Scheduler. +systemd / Task Scheduler. Because the upgrade's re-apply is `apply`, an +unattended run under the default `prompt` git-backup mode prints the same hint +and baseline warning `apply` does; set `[destination_directory_git_backup] mode` +to `on` (or `off`) to silence it. --- @@ -988,7 +994,7 @@ Beta surface. `agentsync --help` is always authoritative. | `migrate subagents` | One-shot move of the retired canonical `agents/` directory to `subagents/`, rewriting that tree's recorded `source_id` values. Run once per tree (`--scope project` / `--project ` for a project tree). Refuses, listing the names, if a file exists under both directories. | `--scope --project` | | `mcp add\|remove\|list\|enable\|disable ` | Manage MCP servers. `enable`/`disable` flip the server's `enabled` bit — keeping the definition but stopping the render (`remove` deletes it). `--header "Name: Value"` (repeatable, http/sse only) sets request headers — the usual remote-auth secret site, e.g. `--header "Authorization: Bearer ${secret:TOKEN}"`. | `--type --command --args --url --env --agents --header` | | `marketplace add\|remove\|list ` | Manage marketplaces. | | -| `plugin add\|upgrade\|enable\|disable\|remove ` / `list` / `outdated` / `explain` | Manage plugins (the lifecycle subcommands all accept the same `id[@marketplace]` ref `add` accepts; the bare id also works, and a qualifier naming a different marketplace than the one the plugin was installed from is refused). `outdated` **(network)** polls the marketplaces and reports pending bumps — it also writes each marketplace's fetch timestamp + head SHA to state. `upgrade` **(network)** re-fetches one plugin, or with `--all` every plugin with a pending bump, and **re-applies** in both cases; `--lossless` skips an upgrade that would introduce a new translation loss, reporting it. `explain` shows per-agent translation coverage. | `outdated` · `upgrade [] --all --lossless --scope --project` · `explain [...] --all --json` | +| `plugin add\|upgrade\|enable\|disable\|remove ` / `list` / `outdated` / `explain` | Manage plugins (the lifecycle subcommands all accept the same `id[@marketplace]` ref `add` accepts; the bare id also works, and a qualifier naming a different marketplace than the one the plugin was installed from is refused). `outdated` **(network)** polls the marketplaces and reports pending bumps — it also writes each marketplace's fetch timestamp + head SHA to state. `upgrade` **(network)** re-fetches one plugin, or with `--all` every plugin with a pending bump, and **runs the full `apply`** in both cases (git backup, removal counts and translation report included); `--lossless` skips an upgrade that would introduce a new translation loss, reporting it. `explain` shows per-agent translation coverage. | `outdated` · `upgrade [] --all --lossless --scope --project` · `explain [...] --all --json` | | `secret set\|get\|list\|remove ` / `secret edit` | Manage age-encrypted secrets (`list` prints KEYS only; `edit` opens the whole vault, no ``; `set` refuses an empty value unless `--allow-empty`). All five require `[secrets].backend = "age"` (matched case-insensitively, exactly as `apply` matches it) and `[secrets].identity_file`; the three that re-encrypt — `set`, `edit`, `remove` — additionally require `[secrets].recipient`. | `set --stdin` | | `apply` | Render source → write agent configs (offline). Git-versions each user-scope destination dir into a local-only repo (opt-out) so a bad apply is revertible. A delete-only run (a component removed from source) reports `removed: N key(s), M file(s)` — key-removals and file-deletes counted distinctly — and a mixed run `applied: X ops, removed: …`, rather than mislabeling itself `up to date`/`applied: 0 ops`; `--dry-run` previews the same removal counts. | `--agents --dry-run --scope --project --no-git-backup` | | `revert ` | Roll a destination dir back to a prior apply checkpoint (append-only). Default undoes the most recent apply; prints an out-of-sync notice. `--to` must name one of the dir's own checkpoints (the current one or an ancestor) — anything else is refused. A dir under which a foreign git repo has appeared (or that isn't an agentsync-managed backup) is an **error** when you name the agent, and a **skip with a warning** under `--all` — strictness follows the invocation; there is no `--strict` flag. | `--agents --to --all --dry-run` | diff --git a/internal/cli/apply.go b/internal/cli/apply.go index 408f734d..a4c66fa9 100644 --- a/internal/cli/apply.go +++ b/internal/cli/apply.go @@ -558,8 +558,8 @@ func saveBestEffortState(s *state.Targets, statePath string, plan render.RenderP // loadProjectedForScope loads the canonical model with plugin projection AND // the active project overlay applied, returning the merged canonical plus the // resolved scope and project root. Every project-scope-aware command (apply, -// status, diff, reconcile, update re-apply) goes through it so they project and -// overlay identically. +// status, diff, reconcile, the `plugin upgrade` re-apply) goes through it so +// they project and overlay identically. // // At project scope the project's own source tree (/.agentsync/) is loaded // as a full canonical and overlaid onto the user canonical via project.Merge — diff --git a/internal/cli/reapply_state_order_internal_test.go b/internal/cli/apply_state_order_internal_test.go similarity index 50% rename from internal/cli/reapply_state_order_internal_test.go rename to internal/cli/apply_state_order_internal_test.go index 0dd6d854..7623157d 100644 --- a/internal/cli/reapply_state_order_internal_test.go +++ b/internal/cli/apply_state_order_internal_test.go @@ -8,8 +8,8 @@ import ( "testing" ) -// TestReapplyLoadsStateAfterSourceReload pins an ORDERING contract that has no -// behavioral test, and says plainly why. +// TestApplyPipelineLoadsStateAfterSourceReload pins an ORDERING contract that +// has no behavioral test, and says plainly why. // // `plugin upgrade ` runs under the global lock and finishes by re-applying. // The re-apply reloads the canonical source, and that reload can run a pending @@ -26,33 +26,45 @@ import ( // why asserting the observable proves nothing here. What is actually true is // structural: do not read state across a call that can rewrite it. // +// Since #231 the re-apply IS the apply pipeline (runApplyPipeline, apply.go), +// so the guard has two halves. Half A holds the original contract against the +// surviving implementation: runApplyPipeline must call loadProjectedForScope +// and then state.Load, in that order, and must not accept a *state.Targets. +// Half B is what makes "routed through the pipeline" a guarded property rather +// than a fact about today's tree: reapplyAfterPluginChange must delegate to +// runApplyPipeline and must NOT load state, plan, apply or save on its own — a +// future hand-rolled re-apply (the exact regression #231 closed) fails here by +// name, on both counts. +// // If this ever gains a genuine observable (a component the re-apply does not -// re-record), replace this with a test of that. -func TestReapplyLoadsStateAfterSourceReload(t *testing.T) { +// re-record), replace half A with a test of that. +func TestApplyPipelineLoadsStateAfterSourceReload(t *testing.T) { _, thisFile, _, ok := runtime.Caller(0) if !ok { t.Fatal("runtime.Caller failed") } - src, err := os.ReadFile(filepath.Join(filepath.Dir(thisFile), "plugin_poll.go")) + dir := filepath.Dir(thisFile) + + // Half A — the ordering contract, on the pipeline itself. + applySrc, err := os.ReadFile(filepath.Join(dir, "apply.go")) if err != nil { t.Fatal(err) } - - body := funcBody(string(src), "func reapplyAfterPluginChange(") + body := funcBody(string(applySrc), "func runApplyPipeline(") if body == "" { - t.Fatal("reapplyAfterPluginChange not found in plugin_poll.go — update this guard") + t.Fatal("runApplyPipeline not found in apply.go — update this guard") } reload := strings.Index(body, "loadProjectedForScope(") load := strings.Index(body, "state.Load(") switch { case reload < 0: - t.Fatal("reapplyAfterPluginChange no longer calls loadProjectedForScope — update this guard") + t.Fatal("runApplyPipeline no longer calls loadProjectedForScope — update this guard") case load < 0: - t.Fatal("reapplyAfterPluginChange no longer calls state.Load — if state is passed in again, " + + t.Fatal("runApplyPipeline no longer calls state.Load — if state is passed in again, " + "it is read before the reload that can rewrite it; see this test's doc comment") case load < reload: - t.Fatal("reapplyAfterPluginChange reads state.Load BEFORE loadProjectedForScope. That reload " + + t.Fatal("runApplyPipeline reads state.Load BEFORE loadProjectedForScope. That reload " + "can run the pending subagent migration, which rewrites recorded source_ids — so the copy " + "read here is stale and saving it reverts the rewrite. Load state AFTER the reload.") } @@ -60,11 +72,35 @@ func TestReapplyLoadsStateAfterSourceReload(t *testing.T) { // The signature must not accept state either: a caller-supplied // *state.Targets is read before this function runs, which has the same // defect and is how the bug originally shipped. - sig := funcSignature(string(src), "func reapplyAfterPluginChange(") + sig := funcSignature(string(applySrc), "func runApplyPipeline(") if strings.Contains(sig, "*state.Targets") { - t.Errorf("reapplyAfterPluginChange takes a *state.Targets again: %s\n"+ + t.Errorf("runApplyPipeline takes a *state.Targets again: %s\n"+ "A caller reads it before the source reload that can rewrite it. Load it inside, after the reload.", sig) } + + // Half B — the plugin re-apply must BE the pipeline, not a copy of it. The + // positive check and the four negatives are Errorf, not Fatal, so a + // hand-rolled re-apply reports every way it diverged in one run. + pollSrc, err := os.ReadFile(filepath.Join(dir, "plugin_poll.go")) + if err != nil { + t.Fatal(err) + } + reapply := funcBody(string(pollSrc), "func reapplyAfterPluginChange(") + if reapply == "" { + t.Fatal("reapplyAfterPluginChange not found in plugin_poll.go — update this guard") + } + if !strings.Contains(reapply, "runApplyPipeline(") { + t.Errorf("reapplyAfterPluginChange no longer calls runApplyPipeline. The plugin re-apply must be " + + "the apply pipeline itself — a second transcription of it is the divergence #231 closed (that " + + "copy had already lost the git baseline/checkpoint, the removal-aware headline, backup pruning " + + "and the translation report).") + } + for _, own := range []string{"state.Load(", "render.Plan(", "render.Apply(", "state.Save("} { + if strings.Contains(reapply, own) { + t.Errorf("reapplyAfterPluginChange calls %s itself. Everything between the source reload and "+ + "the report belongs to runApplyPipeline; a hand-rolled re-apply is the regression #231 closed.", own) + } + } } // funcBody returns the source text of the function whose declaration starts diff --git a/internal/cli/plugin.go b/internal/cli/plugin.go index d7fa2b73..b3e3063c 100644 --- a/internal/cli/plugin.go +++ b/internal/cli/plugin.go @@ -507,13 +507,8 @@ func pluginUpgradeRun(cmd *cobra.Command, args []string, lossless bool) error { // Re-apply so the upgraded plugin's components reach the agents now. Same // ending state as `plugin upgrade --all` — see the command's doc comment. - userHome := paths.HomeDir(paths.OSEnv{}) - statePath := filepath.Join(home, ".state", "targets.json") - // State is loaded inside reapplyAfterPluginChange, AFTER it reloads the - // source: that reload can run the pending subagent migration, which rewrites - // recorded source_ids, and a copy read here would be stale and would undo - // the rewrite when saved. - return reapplyAfterPluginChange(cmd, home, userHome, statePath) + // Source AND state are loaded inside the pipeline, in that order. + return reapplyAfterPluginChange(cmd, home) } // ---- enable ----------------------------------------------------------------- diff --git a/internal/cli/plugin_poll.go b/internal/cli/plugin_poll.go index 3f358274..08d4f933 100644 --- a/internal/cli/plugin_poll.go +++ b/internal/cli/plugin_poll.go @@ -234,83 +234,40 @@ func pollPluginsRun(cmd *cobra.Command, o pollOpts) error { if len(bumps) == 0 { return nil } - return reapplyAfterPluginChange(cmd, home, userHome, statePath) + return reapplyAfterPluginChange(cmd, home) } // reapplyAfterPluginChange re-renders the canonical to the agents after a // plugin's pinned version changed, so a plugin upgrade lands in the agents in // the same command rather than leaving them stale until the next `apply`. // -// It mirrors the apply pipeline deliberately: project-overlay merge, secret -// substitution, scope-aware state recording. Without the overlay a -// project-scope user would have their project state silently ignored, and -// without substitution ${secret:…} references would land literally in agent -// native files. -// The state is loaded HERE, after the source load, and deliberately not passed -// in by the caller. loadProjectedForScope can run the pending subagent -// migration, which rewrites this tree's recorded source_id values in -// targets.json — so a *state.Targets read before that call is stale the moment -// it happens, and saving it at the end silently undoes the rewrite. That was -// reachable via `plugin upgrade ` on an unmigrated tree; it used to fail -// loudly on a lock deadlock instead, which hid it. -func reapplyAfterPluginChange(cmd *cobra.Command, home, userHome, statePath string) error { - c2, sc, projectRoot, err := loadProjectedForScope(cmd, afero.NewOsFs(), home, false) - if err != nil { - return fmt.Errorf("reload source after upgrade: %w", err) - } - - st, err := state.Load(statePath) - if err != nil { - return fmt.Errorf("load state after upgrade: %w", err) - } - - secBackend := secrets.SelectBackend(c2.Config.Secrets, home, userHome) - envBackend := secrets.EnvBackend{} - resolved, serr := secrets.SubstituteCanonical(c2, secBackend, envBackend) - if serr != nil { - return fmt.Errorf("substitute secrets after upgrade: %w", serr) - } - - agents := []string{} - for name, ag := range c2.Config.Agents { - if ag.Enabled { - agents = append(agents, name) - } - } - reg := registryFactory() - plan, err := render.Plan(resolved, reg, agents, sc, projectRoot, st, userHome) - if err != nil { - return fmt.Errorf("plan after upgrade: %w", err) - } - collisions, written, _, applyErr := render.Apply(plan, reg, st, home, userHome, sc, projectRoot) - if applyErr != nil { - // Mirror `apply`: if render.Apply fails mid-pipeline, the files - // that already landed must be recorded so the next apply doesn't - // treat them as foreign collisions. Without this best-effort save, - // a half-applied bump leaves the dest diverged from state. - _ = saveBestEffortState(st, statePath, plan, userHome, sc, projectRoot, written) - return fmt.Errorf("apply after upgrade: %w", applyErr) - } - if len(collisions) > 0 { - ew := cmd.ErrOrStderr() - ep := printerOn(cmd, ew) - ep.Warnf("plugin upgrade backed up %d pre-existing target(s):", len(collisions)) - for _, r := range collisions { - ep.Fdetailf(ew, "%s", r.String()) - } - } - for name, res := range plan.PerAgent { - render.PruneStaleState(st, userHome, name, sc, projectRoot, res.Ops) - } - for name, res := range plan.PerAgent { - if err := render.RecordOpsState(st, userHome, name, sc, projectRoot, res.Ops); err != nil { - return err - } - } - if err := state.Save(statePath, st); err != nil { - return err +// It runs the REAL apply pipeline — runApplyPipeline, the same callable `apply` +// runs — rather than a transcription of it. The previous copy had already +// fallen behind: it took no pre-apply git baseline and no checkpoint +// (#118/#143), so an upgrade overwrote ~/.claude with nothing for `agentsync +// revert` to undo; it printed an unconditional "applied: N ops" instead of the +// removal-aware headline; it never pruned the collision backups; and it +// printed no translation report, which is what tells the user whether the new +// version still translates (#231). +// +// applyOpts{} is the right zero: the plugin commands define none of `apply`'s +// three flags (--dry-run, --no-git-backup, --agents), so the re-apply is a +// real apply of every enabled agent, with git backup governed by +// [destination_directory_git_backup] exactly as it is for `apply`. +// +// State is loaded INSIDE the pipeline, after the source reload, and +// deliberately not passed in by the caller. loadProjectedForScope can run the +// pending subagent migration, which rewrites this tree's recorded source_id +// values in targets.json — so a *state.Targets read before that call is stale +// the moment it happens, and saving it at the end silently undoes the rewrite. +// That was reachable via `plugin upgrade ` on an unmigrated tree; it used +// to fail loudly on a lock deadlock instead, which hid it. The ordering is +// pinned by TestApplyPipelineLoadsStateAfterSourceReload, whose second half +// also requires this function to stay a delegation. +func reapplyAfterPluginChange(cmd *cobra.Command, home string) error { + if err := runApplyPipeline(cmd, home, applyOpts{}); err != nil { + return fmt.Errorf("re-apply after plugin upgrade: %w", err) } - printerOn(cmd, cmd.OutOrStdout()).Successf(ui.EmojiApplied, "applied: %d ops", plan.Total()) return nil } diff --git a/internal/cli/plugin_reapply_pipeline_test.go b/internal/cli/plugin_reapply_pipeline_test.go new file mode 100644 index 00000000..17f7f845 --- /dev/null +++ b/internal/cli/plugin_reapply_pipeline_test.go @@ -0,0 +1,117 @@ +package cli_test + +import ( + "path/filepath" + "strings" + "testing" + + agit "github.com/spxrogers/agentsync/internal/git" +) + +// pluginReapplyFixture builds the home the two tests below share: an inited +// agentsync home with the named agents enabled, optionally +// `[destination_directory_git_backup] mode = "on"` (tests have no TTY, so the +// default `prompt` fails closed and never inits a repo), a SOURCE skill, and +// the versioned fixture marketplace with its `demo` plugin installed. It +// deliberately does NOT run `apply`: the first write into the destinations +// must be the upgrade's re-apply, or both tests would pass on what `apply` +// had already done. Returns the fixture's env and its target root. +// +// The skill matters: it renders to ~/.claude/skills/demo/SKILL.md, INSIDE a +// version root. An MCP-only config only touches ~/.claude.json at $HOME, which +// is never versioned (agentsync never inits a repo at $HOME) — a git-backup +// test built on it would pass with the backup entirely absent. +func pluginReapplyFixture(t *testing.T, gitBackup bool, agents ...string) (map[string]string, string) { + t.Helper() + tmp := t.TempDir() + env := map[string]string{"AGENTSYNC_TARGET_ROOT": tmp} + base := t.TempDir() + + mustRun(t, env, "init") + for _, a := range agents { + mustRun(t, env, "agent", "add", a) + } + if gitBackup { + enableGitBackupOn(t, tmp) + } + writeSkillSource(t, tmp, "demo", "body") + mpDir := makeVersionedMarketplace(t, base, "1.0.0") + mustRun(t, env, "marketplace", "add", mpDir) + mustRun(t, env, "plugin", "add", "demo@test-mp-v") + return env, tmp +} + +// TestPluginUpgrade_TakesGitBackupBaselineAndCheckpoint is the behavioural pin +// on the bug half of #231. Before it, `plugin upgrade` ended in a second, +// hand-maintained copy of the apply pipeline that had no +// [destination_directory_git_backup] pass at all: the upgrade overwrote a +// destination dir with NO pre-apply baseline and NO checkpoint, so `agentsync +// revert` could not undo it. The re-apply is now the real pipeline, and this +// test mirrors TestApply_GitBackupCheckpoint so the two read as one contract: +// with git backup on, the FIRST write under ~/.claude — here the upgrade's +// re-apply, not an `apply` — initializes the local repo and records a +// pre-apply baseline plus the apply checkpoint (≥2 commits, the oldest being +// the baseline). Without this test, dropping the git-backup pass from the +// plugin path (applyOpts{noGitBackup: true} at the call site) fails nothing. +func TestPluginUpgrade_TakesGitBackupBaselineAndCheckpoint(t *testing.T) { + env, tmp := pluginReapplyFixture(t, true, "claude") + + out, err := runCLI(t, env, "plugin", "upgrade", "demo") + if err != nil { + t.Fatalf("plugin upgrade demo: %v\n%s", err, out) + } + + claude := filepath.Join(tmp, ".claude") + st, err := agit.Detect(claude) + if err != nil { + t.Fatal(err) + } + if st != agit.StateAgentsyncOwned { + t.Fatalf("~/.claude state after plugin upgrade = %v, want agentsync-owned — the re-apply took no git backup:\n%s", st, out) + } + repo, err := agit.Open(claude) + if err != nil { + t.Fatal(err) + } + cps, err := repo.Log(0) + if err != nil { + t.Fatal(err) + } + if len(cps) < 2 { + t.Fatalf("want >=2 commits after the upgrade's re-apply (baseline + checkpoint), got %d:\n%s", len(cps), out) + } + if oldest := cps[len(cps)-1]; !strings.Contains(oldest.Subject, "pre-apply baseline") { + t.Fatalf("oldest commit should be the pre-apply baseline, got subject %q", oldest.Subject) + } +} + +// TestPluginUpgrade_RendersEveryEnabledAgent pins what the plugin path must +// NOT gain from sharing the pipeline: `apply --agents`' narrowing. The reason +// it cannot narrow today is structural, not a value: selectAgents returns every +// enabled agent unless the CALLING command reports +// cmd.Flags().Changed("agents"), and no plugin command registers that flag — +// so applyOpts.agentsCSV is inert on this path whatever it holds. What WOULD +// break this test is a pipeline that honoured a non-empty agentsCSV without +// the Changed gate, combined with a plugin call site that passes one — the +// mutation this test was written against. Two agents are enabled; after the +// upgrade's re-apply both must hold the plugin's MCP server. +func TestPluginUpgrade_RendersEveryEnabledAgent(t *testing.T) { + env, tmp := pluginReapplyFixture(t, false, "claude", "opencode") + + out, err := runCLI(t, env, "plugin", "upgrade", "demo") + if err != nil { + t.Fatalf("plugin upgrade demo: %v\n%s", err, out) + } + for _, dest := range []string{ + filepath.Join(tmp, ".claude.json"), + filepath.Join(tmp, ".config", "opencode", "opencode.json"), + } { + got, rerr := readFileString(t, dest) + if rerr != nil { + t.Fatalf("%s missing after plugin upgrade — the re-apply did not render every enabled agent: %v\n%s", dest, rerr, out) + } + if !strings.Contains(got, "demo-mcp") { + t.Fatalf("%s does not carry the plugin's demo-mcp server after plugin upgrade:\n%s", dest, got) + } + } +} diff --git a/internal/cli/plugin_verbs_test.go b/internal/cli/plugin_verbs_test.go index c8ecdd11..236580b3 100644 --- a/internal/cli/plugin_verbs_test.go +++ b/internal/cli/plugin_verbs_test.go @@ -49,6 +49,15 @@ func TestPluginOutdated_ReportsPendingBumps(t *testing.T) { // consolidation moved `update --apply`'s full re-apply into `plugin upgrade // --all`, so it is behavior-identical — pin bumped AND agents re-rendered in // one command. +// +// The re-apply is the real apply pipeline (#231), so its headline is apply's +// honest one: a version-only bump re-renders byte-identical destinations, and +// the pipeline reports that as `up to date: N ops, no changes` rather than the +// old copy's unconditional `applied: N ops` for a run that wrote nothing. +// Either headline is accepted below — and the oracle is then made STRONGER, +// not merely relaxed: the per-plugin translation report (`plugin: +// demo@test-mp-v`) is something only the real pipeline emits, so a +// re-divergence into a hand-rolled re-apply cannot fake it. func TestPluginUpgradeAll_UpgradesAndReapplies(t *testing.T) { tmp := t.TempDir() env := map[string]string{"AGENTSYNC_TARGET_ROOT": tmp} @@ -71,9 +80,12 @@ func TestPluginUpgradeAll_UpgradesAndReapplies(t *testing.T) { if !strings.Contains(demoTOML, "1.0.1") { t.Fatalf("plugin upgrade --all did not bump the pin:\n%s", demoTOML) } - if !strings.Contains(out, "applied:") { + if !strings.Contains(out, "applied:") && !strings.Contains(out, "up to date:") { t.Fatalf("plugin upgrade --all must re-apply after the bump; got:\n%s", out) } + if !strings.Contains(out, "plugin: demo@test-mp-v") { + t.Fatalf("plugin upgrade --all must end in the real apply pipeline (no translation report); got:\n%s", out) + } // The re-applied render must still verify on a follow-up apply. if out2, err2 := runCLI(t, env, "apply"); err2 != nil { t.Fatalf("apply after plugin upgrade --all: %v\n%s", err2, out2) @@ -83,6 +95,12 @@ func TestPluginUpgradeAll_UpgradesAndReapplies(t *testing.T) { // TestPluginUpgradeID_Reapplies pins the deliberate behavior CHANGE: the // single-id form now finishes with the same re-apply as --all, so one verb has // one ending state instead of two. +// +// As for --all above, that re-apply is the real apply pipeline (#231): a +// version-only bump re-renders identical bytes and is reported honestly as +// `up to date: N ops, no changes`, not `applied: N ops`, so either headline is +// accepted — and the translation report (`plugin: demo@test-mp-v`), which only +// the real pipeline emits, is required as the stronger positive signal. func TestPluginUpgradeID_Reapplies(t *testing.T) { tmp := t.TempDir() env := map[string]string{"AGENTSYNC_TARGET_ROOT": tmp} @@ -104,9 +122,12 @@ func TestPluginUpgradeID_Reapplies(t *testing.T) { if err != nil { t.Fatalf("plugin upgrade demo: %v\n%s", err, out) } - if !strings.Contains(out, "applied:") { + if !strings.Contains(out, "applied:") && !strings.Contains(out, "up to date:") { t.Fatalf("plugin upgrade must re-apply; got:\n%s", out) } + if !strings.Contains(out, "plugin: demo@test-mp-v") { + t.Fatalf("plugin upgrade must end in the real apply pipeline (no translation report); got:\n%s", out) + } demoTOML, _ := readFileString(t, filepath.Join(tmp, ".agentsync", "plugins", "demo.toml")) if !strings.Contains(demoTOML, "1.0.1") { t.Fatalf("plugin upgrade did not refresh the pin:\n%s", demoTOML) diff --git a/website/src/content/docs/guides/rollback.mdx b/website/src/content/docs/guides/rollback.mdx index 5156c8eb..c160279c 100644 --- a/website/src/content/docs/guides/rollback.mdx +++ b/website/src/content/docs/guides/rollback.mdx @@ -9,13 +9,15 @@ import { Aside, LinkCard } from '@astrojs/starlight/components'; `apply` can keep each user-scope destination dir (`~/.claude`, `~/.codex`, …) in its **own local-only git repo**, recording a checkpoint commit after every apply -that changes managed files there. If an apply ever goes wrong, you roll it back -with `agentsync revert` — no manual git surgery, no lost work. +that changes managed files there (including the apply at the end of `plugin +upgrade`). If an apply ever goes wrong, you roll it back with `agentsync revert` +— no manual git surgery, no lost work. ## The backup: what `apply` versions -After a successful user-scope apply that changes managed files, `apply` commits a -checkpoint to each affected destination dir's local repo. **Even the first apply is +After a successful user-scope apply that changes managed files — including the +apply at the end of `plugin upgrade` — `apply` commits a checkpoint to each +affected destination dir's local repo. **Even the first apply is revertible:** before that apply overwrites the dir, agentsync records a **pre-apply baseline** commit of the prior content of the files it is about to manage, so the apply checkpoint's parent is the genuine pre-apply state — there is no "the first diff --git a/website/src/content/docs/guides/updating.mdx b/website/src/content/docs/guides/updating.mdx index 241a9e0c..492ebff0 100644 --- a/website/src/content/docs/guides/updating.mdx +++ b/website/src/content/docs/guides/updating.mdx @@ -19,8 +19,10 @@ agentsync plugin upgrade --all --lossless # same, skipping bumps that would l agentsync plugin upgrade atlassian # re-fetch one plugin, then re-apply ``` -Both `upgrade` forms end in a re-apply, so an upgrade reaches your agents in one -command instead of leaving them stale until the next `apply`. +Both `upgrade` forms end in a re-apply — and the re-apply **is** `apply`: same +scope resolution, same destination git backup, same removal counts and +translation report — so an upgrade reaches your agents in one command instead +of leaving them stale until the next `apply`.