fix(cli): plugin upgrade re-applies through the real apply pipeline (#231)

[spxrogers]

Summary

Closes #231. Fourth PR in the #226 code-quality series (after #240, #244, #247, #249). One bug fix on the plugin upgrade path, one refactor that makes that class of bug impossible, and two tidies the issue asked for while in the file.

1. One apply pipeline (commit 1, 3b5cd58, internal/cli/apply.go only). applyRun's body becomes runApplyPipeline(cmd, home, applyOpts), where applyOpts{dryRun, noGitBackup, agentsCSV} are exactly apply's three flags and nothing else — a behaviour gate added there would recreate, one field at a time, the divergence this PR closes — and 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). Zero test edits. apply is byte-identical, proven rather than argued: both binaries (main and commit 1) were run through five fixture scenarios / 48 steps with stdout, stderr and exit status captured per step (144 files), diff -r empty, from a working directory outside any repository carrying its own .agentsync/.

2. The plugin re-apply is the pipeline (commit 2, 24f601f). reapplyAfterPluginChange was a second, hand-maintained transcription of the pipeline that had already fallen behind: no pre-apply git baseline and no checkpoint (#118/#143), so an upgrade overwrote ~/.claude with nothing for agentsync revert to undo; an unconditional applied: N ops for runs that wrote nothing; no backup pruning; no translation report. It is now a six-line delegation to runApplyPipeline(cmd, home, applyOpts{}). What the plugin path gains, each measured before→after on a fixture and documented: the git baseline + checkpoint (the Fixed bullet), the scope: line, the honest five-case headline (up to date: N ops, no changes on an idempotent upgrade), a warning instead of applied: 0 ops when no agents are enabled, backup pruning, the per-plugin translation report, apply's collision wording (printed even when the apply then fails partway), one re-apply after plugin upgrade: error prefix — and apply's git-backup policy in both forms: interactively, the one-time "enable git backup for this dir?" question can now appear during an upgrade and yes / don't ask again persists the mode to agentsync.toml; unattended under the default prompt mode, apply's hint and could not take a pre-apply baseline warning print on every run. What it must not gain: --dry-run and --agents. The latter is structurally inert (selectAgents early-returns unless the calling command reports Changed("agents"), and no plugin command registers the flag); the applyOpts.agentsCSV doc names that seam and its one residual (mcp add --agents is a same-named flag with a different meaning). plugin upgrade --no-git-backup is deliberately not added: the config knob is the documented answer for cron, and the non-TTY prompt fails closed.

The source-text guard TestReapplyLoadsStateAfterSourceReload (state must be loaded after the source reload that can run the subagent migration) is renamed TestApplyPipelineLoadsStateAfterSourceReload, re-pointed at runApplyPipeline, split into two subtests, and gains a second half: reapplyAfterPluginChange must call runApplyPipeline and must not state.Load / render.Plan / render.Apply / state.Save on its own — a hand-rolled re-apply, the exact regression #231 closes, now fails by name on every count. Its capture is checked for truncation by requiring the function's own top-level return nil at the end.

3. --lossless reporting extracted (commit 3, e168961): reportLosslessExclusions takes the 41-line announce block out of pollPluginsRun (194 → 160 lines); filterSafeBumps stays a pure partition. No test edits; the emission order the comment defends was already pinned by TestLossless_UnevaluableIsNotReportedAsLossy.

4. One spelling of the default-marketplace sentinel (commit 4, b18e8ba). The issue's description of the idiom is wrong in a way that changes the fix: the tree has three copies of "" → "default" plus one inverse (pluginUpgradeRun maps the sentinel back to ""), and resolveMarketplaceName already existed. So: a defaultMarketplaceSentinel const and a recordedMarketplace(storedID) helper; the inverse site uses the constant and says it is the inverse. Live "default" literals 6 → 0. Behaviour-preserving on purpose — measuring it surfaced a real, pre-existing bug (a bare-id-installed plugin is recorded as name@default, which the poll engine's fetched index never keys unless a marketplace is literally registered under that name, so plugin outdated / plugin upgrade --all never see it), filed as #251 and cited from the helper's doc rather than papered over.

Review provenance. The plan and execution spec were reviewed by a fresh reviewer, who replicated all four commits in probe clones and found two issues (the default-mode unattended warning was measured on the wrong fixture and undocumented; the sentinel helper's doc claimed something #251 disproves) — both fixed before execution.

Review loop (three rounds, four read-only lenses each — correctness, adversarial, API design, test rigor; commits 5–7). Round 1 on b18e8ba: the zero-value doc said "git backup on" in three places; the interactive prompt (and its persisted config write) on plugin upgrade was undocumented; the up to date: headline was unpinned repo-wide; the sentinel helper's doc over-claimed; plus nits — fixed in 9a64e8e. Round 2 on 9a64e8e: the two guides' prompt sentence overclaimed for not now; the guard's truncation needle rationale was false; round 1's parenthetical made an older CHANGELOG entry self-refuting; two CHANGELOG claims about the plugin path had no test — fixed in 00f05e9 (incl. TestPluginUpgrade_WarnsWhenNoAgentsEnabled and TestPluginUpgrade_PrunesOldBackups). Round 3 on 00f05e9: the needle matched the dry-run branch's earlier call, so a truncation below it was invisible (three lenses measured it); the reworded CHANGELOG tail under-claimed; a stale count in the fixture doc — fixed in 59c8aa1. Three of four lenses said ship it in round 3 and the remaining findings were text this loop itself wrote, so the loop stopped there with two rounds of budget unused. Declined across rounds, with reasons on the PR's ledger: a plugin-path interactive-prompt test (the prompt is apply's own path, pinned there; a copy would rebuild the fixture in package cli), rewording gitbackup.go's unattended hint, restructuring docs/components.md's Key list (#235), and having the older plugin_verbs setups adopt the new fixture.

Type of change

• Bug fix
• New feature / enhancement
• Refactor (no behavior change)
• Docs
• Tests / CI / tooling

Test plan

New (internal/cli/plugin_reapply_pipeline_test.go, one fixture that deliberately never runs apply so the upgrade's re-apply is the first write): TestPluginUpgrade_TakesGitBackupBaselineAndCheckpoint (with git backup on, ~/.claude is agentsync-owned with ≥2 commits, the oldest the pre-apply baseline), TestPluginUpgrade_RendersEveryEnabledAgent (two agents; both destinations carry the plugin's server), TestPluginUpgrade_WarnsWhenNoAgentsEnabled (warns, exits 0, never applied: 0 ops), TestPluginUpgrade_PrunesOldBackups (DefaultBackupKeep+3 seeded backup dirs → exactly DefaultBackupKeep after the upgrade).

Rewritten (not retired): TestApplyPipelineLoadsStateAfterSourceReload (two subtests, above); TestPluginUpgradeAll_UpgradesAndReapplies / TestPluginUpgradeID_Reapplies assert the up to date: headline (the fixture's bump is version-only, so it is deterministic) and require the translation-report line plugin: demo@test-mp-v, which only the real pipeline emits; TestPluginPoll_UpgradeAllPartialFailureRescuesState pins the re-apply after plugin upgrade: prefix; TestPlugin_BareIDInstallSentinelQualifierHonest pins the on-disk demo@default form.

Break-verifies run before each commit (literal failure sets): every applyOpts field ignored inside the pipeline → the apply tests that own it; applyOpts{dryRun: true} / {noGitBackup: true} / a pipeline honouring agentsCSV at the plugin site → the new tests; a hand-rolled re-apply body → the guard's second half on every branch; the headline format changed to applied: → both plugin_verbs tests; the wrap dropped → the prefix pin; resolveMarketplaceName made identity → the on-disk pin; the zero-agents warning replaced → its test; the PruneBackups call removed → its test (and the guard stays quiet); a raw-string column-0 brace anywhere in runApplyPipeline, including between its two BuildReport calls → the guard's truncation check; the caveat moved after the unevaluable loop → TestLossless_UnevaluableIsNotReportedAsLossy. Dropping the sentinel default in recordedMarketplace fails 0 tests: a pre-existing gap reported as-is (#251), not papered over.

Secrets invariants on the final tree: 0 .Canonical() unwraps in internal/cli, no nolint delta, SubstituteCanonical call sites 5 → 4.

Gates, each commit independently and again on the head: go build, go vet, gofmt -l empty, gofumpt, AGENTSYNC_TEST_IN_CONTAINER=1 go test ./... (29 packages ok), go test -race ./internal/cli/..., go test -tags=e2e ./test/e2e/..., go test -tags=bdd ./test/bdd/..., GOTOOLCHAIN=go1.26.2 golangci-lint@v2.12.2 run ./... → 0 issues.

• just test-release is green (the release bar) — just is not installable in this session; every layer the recipe orchestrates (vet → build → race → e2e → bdd) was run directly as listed above, and CI's hermetic test-release job is green on the PR.
• just lint is clean — run directly with the pinned toolchain; go.mod/go.sum untouched.

Checklist

• Conventional commit messages with a scope (e.g. fix(secrets): …).
• Tests added/updated for the behavior changed.
• If this touches internal/secrets, internal/capture, or any
  source.Write* path, I've re-read the secret-handling invariants in
  CLAUDE.md / SECURITY.md and not weakened them. — Not touched; the pipeline still renders from secrets.Resolved and the extraction moves no unwrap and adds no dest→source write (counts above).
• Docs updated if behavior, CLI surface, or capability coverage changed. — docs/architecture.md §4 (step 9 clause + "One implementation" paragraph), docs/components.md, docs/concepts.md, docs/user-guide.md (four places incl. the cron paragraph), website/src/content/docs/reference/cli.mdx, guides/updating.mdx (incl. the scheduling section), guides/rollback.mdx, CHANGELOG.md (Fixed + Changed, plus two older [Unreleased] entries reconciled). The dated docs/superpowers/plans/… file that names applyRun is a historical record and is left alone.

🤖 Generated with Claude Code

https://claude.ai/code/session_01M4VNyoCuGXx7pYxNfLVbFG