diff --git a/CHANGELOG.md b/CHANGELOG.md index d5a84b0b..d89f6471 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,27 @@ 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, + as in `apply`, is printed even when the apply then fails partway), and the + five `… after upgrade:` error prefixes collapse into one + `re-apply after plugin upgrade:`. Interactively, the question `apply` asks + before its first write into an untracked destination dir (enable git backup + for it?) can now appear during an upgrade, and answering `yes` / `don't ask + again` persists the mode to `agentsync.toml` exactly as it does for `apply`. + 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 @@ -734,8 +765,10 @@ source layout, CLI surface, and state schema are stabilizing but may still chang - **`plugin upgrade` now re-applies, in BOTH forms.** `--all` carries over `update --apply`'s complete re-apply (scope resolution, secret - substitution, plan/apply, state recording), so it is behavior-identical to - what `update --apply` did. The single-id `plugin upgrade ` gains that + substitution, plan/apply, state recording), so it keeps `update --apply`'s + ending state; #231 above then made that re-apply `apply`'s own pipeline, + which prints more and — see Fixed, above — git-backs-up the destinations + it overwrites. The single-id `plugin upgrade ` gains that same re-apply — a **behavior change**: it used to re-fetch and leave your agents stale until the next `apply`. One verb, one ending state. - **`--auto-safe` became `--lossless` on the plugin side**, because the name @@ -816,9 +849,10 @@ source layout, CLI surface, and state schema are stabilizing but may still chang - **`plugin upgrade`'s one-line help now names `apply` instead of saying "re-apply".** Both `upgrade` forms genuinely run the full apply pipeline - (`render.Plan` + `render.Apply`, honoring `--scope`/`--project`) — but in the - `agentsync plugin` command list, "and re-apply" read as loose jargon rather - than as "runs `agentsync apply`", so the one behavior a reader most needs to + (`render.Plan` + `render.Apply`, honoring `--scope`/`--project`; since #231 + above, the one `apply` itself runs) — but in the `agentsync plugin` command + list, "and re-apply" read as loose jargon rather than as "runs + `agentsync apply`", so the one behavior a reader most needs to know about the command — that it writes to your agents' native config, not just to the plugin cache — was the one the summary buried. Reworded to "and run apply" (and matched in the website CLI reference table). Behavior is diff --git a/docs/architecture.md b/docs/architecture.md index e0fd8901..2490ffbc 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,19 @@ 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, with destination git backup governed by +`[destination_directory_git_backup]` exactly as for `apply`. 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..c02fe4bf 100644 --- a/docs/components.md +++ b/docs/components.md @@ -45,11 +45,16 @@ 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 - `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 - resolved cleartext in `op.Content`; `destReadPath` / `readDestText` + `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 apply, not + opted out of git backup (the `[destination_directory_git_backup]` mode still + governs it); `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 resolved cleartext in `op.Content`; `destReadPath` / `readDestText` (`destread.go`) — the whole-file destination readers that carry the symlink policy (`AGENTSYNC_ALLOW_SYMLINK_DEST`), mirroring `iox.AtomicWrite`'s. - **Commands:** `init`, `agent {add,remove,list,enable,disable}`, `apply`, diff --git a/docs/concepts.md b/docs/concepts.md index 02063670..04cb7aa0 100644 --- a/docs/concepts.md +++ b/docs/concepts.md @@ -261,7 +261,7 @@ plugin manager. These are deliberate outcomes, so they are never rendered in the ### Polling (the networked verb of the daily loop) `agentsync plugin outdated` is the command that touches the network in the daily loop: it polls marketplaces, refreshes the cache, and recomputes version pins. -`agentsync plugin upgrade --all` then re-pins every pending bump and re-applies. +`agentsync plugin upgrade --all` then re-pins every pending bump and runs the full `apply`. `apply` itself runs entirely from cache — the split keeps it fast, reproducible, and offline-safe. It is not the only networked command (`plugin add`, `marketplace add`, `import :plugin`, and `init ` all fetch), just @@ -283,7 +283,7 @@ the one the loop runs. (dest ▶ source capture) agentsync plugin outdated ─network─▶ refresh marketplace cache & pins - agentsync plugin upgrade --all re-pin pending bumps, then re-apply + agentsync plugin upgrade --all re-pin pending bumps, then run apply (plain apply renders from cache, offline) ``` diff --git a/docs/user-guide.md b/docs/user-guide.md index 6b4dcad8..2dadd5eb 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,13 @@ 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`. Interactively, that includes +the question `apply` asks before its first write into a destination dir it has +not versioned yet (enable git backup for it?); answering `yes` or `don't ask +again` persists the mode to `agentsync.toml`, just as it does for `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 +888,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 +997,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 070d09e8..ed96fcd3 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,45 @@ 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, not opted out of destination git +// backup (the [destination_directory_git_backup] mode still governs it) — +// 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 body of the apply command (callers must hold the global lock; only +// `apply --dry-run`, which writes nothing, runs without it). 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 +145,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 +176,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 +265,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) @@ -522,8 +559,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/apply_state_order_internal_test.go b/internal/cli/apply_state_order_internal_test.go new file mode 100644 index 00000000..69c14f5c --- /dev/null +++ b/internal/cli/apply_state_order_internal_test.go @@ -0,0 +1,149 @@ +package cli + +import ( + "os" + "path/filepath" + "runtime" + "strings" + "testing" +) + +// 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 +// subagent migration, which rewrites this tree's recorded source_id values in +// .state/targets.json. Reading targets.json BEFORE that reload and saving the +// copy afterwards therefore writes stale ids over the rewrite. +// +// WHY THIS IS A SOURCE GUARD AND NOT A BEHAVIORAL TEST: the same re-apply then +// calls PruneStaleState + RecordOpsState over the fresh plan, which re-records +// every op it renders — so for a subagent the agent still renders, the stale +// write is immediately healed and the end state is indistinguishable. I wrote +// the obvious end-to-end test first; it passed with the bug deliberately +// reintroduced, which is precisely why the bug shipped unnoticed and precisely +// 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 half A with a test of that. +func TestApplyPipelineLoadsStateAfterSourceReload(t *testing.T) { + _, thisFile, _, ok := runtime.Caller(0) + if !ok { + t.Fatal("runtime.Caller failed") + } + dir := filepath.Dir(thisFile) + + // The two halves are independent contracts; separate subtests so a Fatal + // in one never hides a failure in the other. + t.Run("pipeline loads state after the source reload", func(t *testing.T) { + applySrc, err := os.ReadFile(filepath.Join(dir, "apply.go")) + if err != nil { + t.Fatal(err) + } + body := funcBody(string(applySrc), "func runApplyPipeline(") + if body == "" { + t.Fatal("runApplyPipeline not found in apply.go — update this guard") + } + // funcBody stops at the first line that is just "}", so a raw string + // literal with a column-0 brace inside the function would silently + // shrink the text under test. A complete capture ends with the + // function's own final statement — its top-level `return nil`, indented + // one tab — so anything else at the end means the capture was cut short + // somewhere, possibly below the calls this guard asserts on. + if !strings.HasSuffix(body, "\n\treturn nil") { + t.Fatal("funcBody captured a truncated runApplyPipeline (the body does not end at the " + + "function's top-level `return nil`). Either a column-0 '}' inside the function cut the " + + "capture short (fix funcBody or the function), or the function's final statement changed — " + + "then update this check") + } + + reload := strings.Index(body, "loadProjectedForScope(") + load := strings.Index(body, "state.Load(") + switch { + case reload < 0: + t.Fatal("runApplyPipeline no longer calls loadProjectedForScope — update this guard") + case load < 0: + 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("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.") + } + + // 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(applySrc), "func runApplyPipeline(") + if strings.Contains(sig, "*state.Targets") { + 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) + } + }) + + t.Run("plugin re-apply delegates to the pipeline", func(t *testing.T) { + // 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 +// with decl, from the opening brace to the first line that is exactly "}". +func funcBody(src, decl string) string { + i := strings.Index(src, decl) + if i < 0 { + return "" + } + rest := src[i:] + j := strings.Index(rest, "\n}\n") + if j < 0 { + return rest + } + return rest[:j] +} + +// funcSignature returns the declaration line of the function starting with decl. +func funcSignature(src, decl string) string { + i := strings.Index(src, decl) + if i < 0 { + return "" + } + rest := src[i:] + if j := strings.Index(rest, "\n"); j >= 0 { + return rest[:j] + } + return rest +} diff --git a/internal/cli/plugin.go b/internal/cli/plugin.go index d7fa2b73..c61e14d3 100644 --- a/internal/cli/plugin.go +++ b/internal/cli/plugin.go @@ -418,11 +418,12 @@ func pluginUpgradeRun(cmd *cobra.Command, args []string, lossless bool) error { } // Parse marketplace name from the stored id "name@marketplace". The - // "default" sentinel (bare-id install) names no real marketplace — map it - // to "" so the entry is re-resolved by searching all caches, exactly like - // the original install. + // defaultMarketplaceSentinel (bare-id install) names no real marketplace — + // map it to "" so the entry is re-resolved by searching all caches, exactly + // like the original install. This is the ONE inverse of recordedMarketplace: + // every other reader folds "" TO the sentinel; this site folds it back. _, mpName := splitPluginRef(existing.Plugin.ID.Unverified()) - if mpName == "default" { + if mpName == defaultMarketplaceSentinel { mpName = "" } @@ -507,13 +508,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 ----------------------------------------------------------------- @@ -739,15 +735,16 @@ func checkPluginRefMarketplace(existing pluginTOML, id, typedMP string) error { if recorded == typedMP { return nil } - // A bare-id install records the "default" SENTINEL (resolveMarketplaceName: - // the marketplace was searched across all caches, not named). "default" - // identifies no real marketplace, so a typed qualifier can be neither - // confirmed nor contradicted — refuse like the no-record branch below with - // an honest message, rather than claiming the plugin was "installed from - // marketplace \"default\"" (a marketplace that doesn't exist; the update - // flow depends on the sentinel staying recorded as-is, so re-recording the - // resolved name at install is a bigger change than this guard should make). - if recorded == "" || recorded == "default" { + // A bare-id install records defaultMarketplaceSentinel + // (resolveMarketplaceName: the marketplace was searched across all caches, + // not named). It identifies no real marketplace, so a typed qualifier can + // be neither confirmed nor contradicted — refuse like the no-record branch + // below with an honest message, rather than claiming the plugin was + // "installed from marketplace \"default\"" (a marketplace that doesn't + // exist; the update flow depends on the sentinel staying recorded as-is, so + // re-recording the resolved name at install is a bigger change than this + // guard should make). + if recorded == "" || recorded == defaultMarketplaceSentinel { return fmt.Errorf("plugin %q was installed by bare id (its marketplace is not recorded), so the ref %q cannot be verified; use the bare id %q", id, id+"@"+typedMP, id) } @@ -755,20 +752,43 @@ func checkPluginRefMarketplace(existing pluginTOML, id, typedMP string) error { id, recorded, typedMP, id+"@"+recorded, id) } -// resolveMarketplaceName returns the marketplace name, defaulting to "default" -// if empty. NOTE: "default" is a SENTINEL, not a marketplace — but nothing -// reserves the name, so a user-registered marketplace literally named -// "default" is ambiguous with it (its qualified refs get the "installed by -// bare id" refusal and its upgrades re-search all caches). Accepted residual: -// reserving the name would break any existing marketplace so named, and the -// all-caches search still resolves the right entry. +// defaultMarketplaceSentinel is the sentinel recorded in a plugin's stored id +// ("@default") when the plugin was installed by bare id, i.e. the +// marketplace was searched across all caches rather than named. It identifies +// no real marketplace. The one place that needs a real name again +// (pluginUpgradeRun's re-resolve) maps it BACK to "" so the entry is searched +// across all caches, exactly like the original install. Nothing reserves the +// name, so a user-registered marketplace literally named "default" is +// ambiguous with it (its qualified refs get the "installed by bare id" refusal +// and its upgrades re-search all caches). Accepted residual: reserving the +// name would break any existing marketplace so named, and the all-caches +// search still resolves the right entry. +const defaultMarketplaceSentinel = "default" + +// resolveMarketplaceName returns the marketplace name, folding an empty one +// to defaultMarketplaceSentinel. func resolveMarketplaceName(name string) string { if name == "" { - return "default" + return defaultMarketplaceSentinel } return name } +// recordedMarketplace returns the marketplace a stored plugin id +// ("@") names, folding an unqualified id to +// defaultMarketplaceSentinel. The sentinel names no marketplace: the poll +// engine's fetched index is keyed by REGISTERED marketplace names +// (plugin_poll.go), so it has no entry for the sentinel unless a marketplace +// is literally registered as "default" (nothing reserves the name — see the +// constant's doc), and a bare-id install is therefore invisible to `plugin +// outdated` and `plugin upgrade --all` (#251). This helper only unifies the +// spelling of splitPluginRef + the sentinel default; it does not make that +// lookup work. +func recordedMarketplace(storedID string) string { + _, mpName := splitPluginRef(storedID) + return resolveMarketplaceName(mpName) +} + // pluginCacheDir returns the cache directory for a plugin. The id is // sanitized so a hostile marketplace publishing a plugin named // "../../etc/foo" cannot cause writes outside .state/cache/plugins/. diff --git a/internal/cli/plugin_poll.go b/internal/cli/plugin_poll.go index 3f358274..6f342cfe 100644 --- a/internal/cli/plugin_poll.go +++ b/internal/cli/plugin_poll.go @@ -141,46 +141,12 @@ func pollPluginsRun(cmd *cobra.Command, o pollOpts) error { // Compute pending bumps. bumps := marketplace.ComputePendingBumps(st, c.Marketplaces, c.Plugins, fetched, c.Config.Updates.DefaultMode) - // --lossless: drop bumps whose candidate version - // would introduce a new translation loss (an adapter Skip) for any enabled - // agent. Each bump is evaluated by projecting the plugin's installed vs - // candidate manifest and diffing the skip identities a render emits; - // comparing both under identical conditions makes any render quirk cancel, so - // the delta is exactly the bump's effect. An excluded bump is REPORTED, never - // silently dropped. Evaluation failures are excluded too (conservative), but - // reported as what they are rather than as measured losses. + // --lossless: partition the bumps and report every exclusion. See + // reportLosslessExclusions for what is measured and why the caveat is + // printed where it is. var excluded int if o.lossless { - safe, lossy, unevaluable := filterSafeBumps(home, bumps, fetched, c.Config, userHome, warnW) - for _, b := range lossy { - p.Infof("lossless: skipping lossy bump %s %s → %s (candidate version drops translation for an agent)", - b.ID, b.From, b.To) - } - // Printed ONCE for the whole run rather than per bump: the caveat is a - // property of the check, not of any one bump, and repeating it per line - // would bury the bumps it is meant to qualify. - // - // Gated on `lossy` alone, NOT on everything excluded: the caveat says the - // loss may fall on an agent this plugin is not projected to, and offers - // re-running without --lossless. Neither sentence is true of a bump that - // could not be evaluated — there is no identified loss and no agent — and - // telling that user to drop the flag would invert a refusal that exists - // precisely because nothing is known. - // - // Emitted between the two loops, not after both. Detailf is an UNLABELED - // continuation line that hangs under whatever diagnostic precedes it, so - // printing it last would render it attached to an unevaluable bump — the - // very misattribution the gate above exists to prevent. Gating and - // placement have to agree. - if len(lossy) > 0 { - p.Detailf("%s", losslessTargetingCaveat) - } - for _, b := range unevaluable { - p.Infof("lossless: skipping bump %s %s → %s (could not be evaluated; see the warning above)", - b.ID, b.From, b.To) - } - excluded = len(lossy) + len(unevaluable) - bumps = safe + bumps, excluded = reportLosslessExclusions(p, home, bumps, fetched, c.Config, userHome, warnW) } if len(bumps) == 0 { @@ -234,83 +200,81 @@ func pollPluginsRun(cmd *cobra.Command, o pollOpts) error { if len(bumps) == 0 { return nil } - return reapplyAfterPluginChange(cmd, home, userHome, statePath) + return reapplyAfterPluginChange(cmd, home) +} + +// reportLosslessExclusions is the `--lossless` gate of `plugin upgrade --all`: +// it drops every bump whose candidate version would introduce a new +// translation loss (an adapter Skip) for any enabled agent, and REPORTS each +// exclusion — a dropped bump is never silent. A bump that cannot be evaluated +// is excluded too (conservative), but reported as what it is rather than as a +// measured loss. Returns the safe bumps and how many were excluded. +// filterSafeBumps decides (and bumpIsLossy says how a bump is measured); this +// function only announces. +func reportLosslessExclusions(p *ui.Printer, home string, bumps []marketplace.Bump, fetched map[string]map[string]marketplace.PluginEntry, cfg source.Config, userHome string, warn io.Writer) (safe []marketplace.Bump, excluded int) { + var lossy, unevaluable []marketplace.Bump + safe, lossy, unevaluable = filterSafeBumps(home, bumps, fetched, cfg, userHome, warn) + for _, b := range lossy { + p.Infof("lossless: skipping lossy bump %s %s → %s (candidate version drops translation for an agent)", + b.ID, b.From, b.To) + } + // Printed ONCE for the whole run rather than per bump: the caveat is a + // property of the check, not of any one bump, and repeating it per line + // would bury the bumps it is meant to qualify. + // + // Gated on `lossy` alone, NOT on everything excluded: the caveat says the + // loss may fall on an agent this plugin is not projected to, and offers + // re-running without --lossless. Neither sentence is true of a bump that + // could not be evaluated — there is no identified loss and no agent — and + // telling that user to drop the flag would invert a refusal that exists + // precisely because nothing is known. + // + // Emitted between the two loops, not after both. Detailf is an UNLABELED + // continuation line that hangs under whatever diagnostic precedes it, so + // printing it last would render it attached to an unevaluable bump — the + // very misattribution the gate above exists to prevent. Gating and + // placement have to agree. + if len(lossy) > 0 { + p.Detailf("%s", losslessTargetingCaveat) + } + for _, b := range unevaluable { + p.Infof("lossless: skipping bump %s %s → %s (could not be evaluated; see the warning above)", + b.ID, b.From, b.To) + } + return safe, len(lossy) + len(unevaluable) } // 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 } @@ -341,10 +305,7 @@ func computeFreshPluginSHAs(home string, plugins []source.Plugin, fetched map[st // plID is the raw plugin id for filesystem/map use; pl.ID prints itself // sanitized when used directly in the warnings below. plID := pl.ID.Unverified() - _, mpName := splitPluginRef(pl.Plugin.ID.Unverified()) - if mpName == "" { - mpName = "default" - } + mpName := recordedMarketplace(pl.Plugin.ID.Unverified()) entries, ok := fetched[mpName] if !ok { continue @@ -407,10 +368,7 @@ func applyPluginBump(home string, b marketplace.Bump, fetched map[string]map[str } // Find the marketplace entry for re-fetch. - _, mpName := splitPluginRef(existing.Plugin.ID.Unverified()) - if mpName == "" { - mpName = "default" - } + mpName := recordedMarketplace(existing.Plugin.ID.Unverified()) entries, ok := fetched[mpName] if !ok { @@ -540,10 +498,7 @@ func bumpIsLossy(home string, b marketplace.Bump, fetched map[string]map[string] if err != nil { return false, err } - _, mpName := splitPluginRef(existing.Plugin.ID.Unverified()) - if mpName == "" { - mpName = "default" - } + mpName := recordedMarketplace(existing.Plugin.ID.Unverified()) entries, ok := fetched[mpName] if !ok { return false, fmt.Errorf("marketplace %q not in fetched index", mpName) diff --git a/internal/cli/plugin_poll_test.go b/internal/cli/plugin_poll_test.go index 9f3e6fbe..9e0c5e6f 100644 --- a/internal/cli/plugin_poll_test.go +++ b/internal/cli/plugin_poll_test.go @@ -184,7 +184,14 @@ func TestPluginPoll_UpgradeAllPartialFailureRescuesState(t *testing.T) { t.Fatal(err) } - if _, err := runCLI(t, env, "plugin", "upgrade", "--all"); err == nil { + out, err := runCLI(t, env, "plugin", "upgrade", "--all") + // The one error prefix the plugin path adds (CHANGELOG: the five + // `… after upgrade:` prefixes collapsed into it). The root command silences + // cobra's error echo, so the returned error is the only place it can be. + if err != nil && !strings.Contains(err.Error(), "re-apply after plugin upgrade:") { + t.Fatalf("plugin upgrade --all failure must carry the re-apply prefix; got err=%v\n%s", err, out) + } + if err == nil { t.Fatal("expected plugin upgrade --all to fail when the skills dir is blocked") } diff --git a/internal/cli/plugin_reapply_pipeline_test.go b/internal/cli/plugin_reapply_pipeline_test.go new file mode 100644 index 00000000..2a7f2bc7 --- /dev/null +++ b/internal/cli/plugin_reapply_pipeline_test.go @@ -0,0 +1,166 @@ +package cli_test + +import ( + "fmt" + "os" + "path/filepath" + "strings" + "testing" + + agit "github.com/spxrogers/agentsync/internal/git" + "github.com/spxrogers/agentsync/internal/render" +) + +// pluginReapplyFixture builds the home the tests below share: an inited +// agentsync home with the named agents enabled, 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. A test +// that needs git backup sets `[destination_directory_git_backup] mode = "on"` +// itself (enableGitBackupOn): tests have no TTY, so the default `prompt` +// fails closed and never inits a repo. +// +// 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, 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) + } + 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, "claude") + enableGitBackupOn(t, tmp) + + 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, "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) + } + } +} + +// TestPluginUpgrade_WarnsWhenNoAgentsEnabled pins one of the headlines the +// plugin path gained by routing through the pipeline (CHANGELOG): with no +// agent enabled, the old copy printed `applied: 0 ops`; the pipeline warns +// that there is nothing to apply and exits 0, exactly as `apply` does. +func TestPluginUpgrade_WarnsWhenNoAgentsEnabled(t *testing.T) { + env, _ := pluginReapplyFixture(t) // no agents + + out, err := runCLI(t, env, "plugin", "upgrade", "demo") + if err != nil { + t.Fatalf("plugin upgrade demo with no agents should exit 0: %v\n%s", err, out) + } + if !strings.Contains(out, "no agents are enabled") { + t.Fatalf("plugin upgrade demo with no agents should warn like apply does; got:\n%s", out) + } + if strings.Contains(out, "applied: 0 ops") { + t.Fatalf("plugin upgrade demo must not claim `applied: 0 ops` for a no-op run; got:\n%s", out) + } +} + +// TestPluginUpgrade_PrunesOldBackups pins the other pipeline-only step the +// CHANGELOG names for the plugin path: after a successful re-apply the +// collision-backup directory under .state/backups is pruned to +// render.DefaultBackupKeep entries, as `apply` prunes it. The old copy never +// pruned, so a cron-driven upgrade loop grew that directory without bound. +func TestPluginUpgrade_PrunesOldBackups(t *testing.T) { + env, tmp := pluginReapplyFixture(t, "claude") + root := filepath.Join(tmp, ".agentsync", ".state", "backups") + seeded := render.DefaultBackupKeep + 3 + for i := range seeded { + if err := os.MkdirAll(filepath.Join(root, fmt.Sprintf("20260101T%06dZ-000000001", i)), 0o755); err != nil { + t.Fatal(err) + } + } + + out, err := runCLI(t, env, "plugin", "upgrade", "demo") + if err != nil { + t.Fatalf("plugin upgrade demo: %v\n%s", err, out) + } + left, rerr := os.ReadDir(root) + if rerr != nil { + t.Fatal(rerr) + } + if len(left) != render.DefaultBackupKeep { + t.Fatalf("after the upgrade's re-apply %d backup dirs remain, want %d (the pipeline prunes; the old copy did not):\n%s", len(left), render.DefaultBackupKeep, out) + } +} diff --git a/internal/cli/plugin_test.go b/internal/cli/plugin_test.go index 6a09fac0..b5525e41 100644 --- a/internal/cli/plugin_test.go +++ b/internal/cli/plugin_test.go @@ -1337,6 +1337,11 @@ func TestPlugin_BareIDInstallSentinelQualifierHonest(t *testing.T) { if out, err := runCLI(t, env, "plugin", "disable", "demo"); err != nil { t.Fatalf("bare-id disable: %v\n%s", err, out) } + // The recorded id carries the sentinel ON DISK — `demo@default`, never a + // bare `demo@` — which is what every later reader folds back from. + if raw, _ := readFileString(t, filepath.Join(tmp, ".agentsync", "plugins", "demo.toml")); !strings.Contains(raw, "demo@default") { + t.Fatalf("bare-id install must record the @default sentinel; got:\n%s", raw) + } p := readPluginTOMLFixture(t, filepath.Join(tmp, ".agentsync", "plugins", "demo.toml")) if !p.Plugin.Disabled { t.Fatal("bare-id disable should have set disabled=true") diff --git a/internal/cli/plugin_verbs_test.go b/internal/cli/plugin_verbs_test.go index c8ecdd11..4d598780 100644 --- a/internal/cli/plugin_verbs_test.go +++ b/internal/cli/plugin_verbs_test.go @@ -49,6 +49,16 @@ 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. +// The fixture's bump is version-only, so that headline is deterministic and +// is asserted by its prefix — it is the user-visible change this PR ships — and the +// per-plugin translation report (`plugin: demo@test-mp-v`), which only the +// real pipeline emits, is required too, so a re-divergence into a hand-rolled +// re-apply cannot fake either. func TestPluginUpgradeAll_UpgradesAndReapplies(t *testing.T) { tmp := t.TempDir() env := map[string]string{"AGENTSYNC_TARGET_ROOT": tmp} @@ -71,8 +81,11 @@ 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:") { - t.Fatalf("plugin upgrade --all must re-apply after the bump; got:\n%s", out) + if !strings.Contains(out, "up to date:") { + t.Fatalf("plugin upgrade --all must re-apply through the pipeline, whose headline for a version-only bump is `up to date`; 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 (the translation report is missing); 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 { @@ -83,6 +96,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`, never `applied: N ops` — asserted by its prefix — +// and the translation report (`plugin: demo@test-mp-v`), which only the real +// pipeline emits, is required as the second positive signal. func TestPluginUpgradeID_Reapplies(t *testing.T) { tmp := t.TempDir() env := map[string]string{"AGENTSYNC_TARGET_ROOT": tmp} @@ -104,8 +123,11 @@ func TestPluginUpgradeID_Reapplies(t *testing.T) { if err != nil { t.Fatalf("plugin upgrade demo: %v\n%s", err, out) } - if !strings.Contains(out, "applied:") { - t.Fatalf("plugin upgrade must re-apply; got:\n%s", out) + if !strings.Contains(out, "up to date:") { + t.Fatalf("plugin upgrade must re-apply through the pipeline, whose headline for a version-only bump is `up to date`; got:\n%s", out) + } + if !strings.Contains(out, "plugin: demo@test-mp-v") { + t.Fatalf("plugin upgrade must end in the real apply pipeline (the translation report is missing); got:\n%s", out) } demoTOML, _ := readFileString(t, filepath.Join(tmp, ".agentsync", "plugins", "demo.toml")) if !strings.Contains(demoTOML, "1.0.1") { diff --git a/internal/cli/reapply_state_order_internal_test.go b/internal/cli/reapply_state_order_internal_test.go deleted file mode 100644 index 0dd6d854..00000000 --- a/internal/cli/reapply_state_order_internal_test.go +++ /dev/null @@ -1,96 +0,0 @@ -package cli - -import ( - "os" - "path/filepath" - "runtime" - "strings" - "testing" -) - -// TestReapplyLoadsStateAfterSourceReload 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 -// subagent migration, which rewrites this tree's recorded source_id values in -// .state/targets.json. Reading targets.json BEFORE that reload and saving the -// copy afterwards therefore writes stale ids over the rewrite. -// -// WHY THIS IS A SOURCE GUARD AND NOT A BEHAVIORAL TEST: the same re-apply then -// calls PruneStaleState + RecordOpsState over the fresh plan, which re-records -// every op it renders — so for a subagent the agent still renders, the stale -// write is immediately healed and the end state is indistinguishable. I wrote -// the obvious end-to-end test first; it passed with the bug deliberately -// reintroduced, which is precisely why the bug shipped unnoticed and precisely -// why asserting the observable proves nothing here. What is actually true is -// structural: do not read state across a call that can rewrite it. -// -// 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) { - _, thisFile, _, ok := runtime.Caller(0) - if !ok { - t.Fatal("runtime.Caller failed") - } - src, err := os.ReadFile(filepath.Join(filepath.Dir(thisFile), "plugin_poll.go")) - if err != nil { - t.Fatal(err) - } - - body := funcBody(string(src), "func reapplyAfterPluginChange(") - if body == "" { - t.Fatal("reapplyAfterPluginChange not found in plugin_poll.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") - case load < 0: - t.Fatal("reapplyAfterPluginChange 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 " + - "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.") - } - - // 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(") - if strings.Contains(sig, "*state.Targets") { - t.Errorf("reapplyAfterPluginChange 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) - } -} - -// funcBody returns the source text of the function whose declaration starts -// with decl, from the opening brace to the first line that is exactly "}". -func funcBody(src, decl string) string { - i := strings.Index(src, decl) - if i < 0 { - return "" - } - rest := src[i:] - j := strings.Index(rest, "\n}\n") - if j < 0 { - return rest - } - return rest[:j] -} - -// funcSignature returns the declaration line of the function starting with decl. -func funcSignature(src, decl string) string { - i := strings.Index(src, decl) - if i < 0 { - return "" - } - rest := src[i:] - if j := strings.Index(rest, "\n"); j >= 0 { - return rest[:j] - } - return rest -} diff --git a/website/src/content/docs/guides/rollback.mdx b/website/src/content/docs/guides/rollback.mdx index 5156c8eb..e490b976 100644 --- a/website/src/content/docs/guides/rollback.mdx +++ b/website/src/content/docs/guides/rollback.mdx @@ -14,8 +14,9 @@ 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..ec8e63cb 100644 --- a/website/src/content/docs/guides/updating.mdx +++ b/website/src/content/docs/guides/updating.mdx @@ -19,8 +19,13 @@ 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`. Interactively, that includes +the question `apply` asks before its first write into a destination dir it has +not versioned yet (enable git backup for it?); answering `yes` or `don't ask +again` persists the mode to `agentsync.toml`, just as it does for `apply`.