From aa2bc864a59f6b42f3458ce1fbbc462ed4b57ef0 Mon Sep 17 00:00:00 2001 From: Aric Camarata Date: Sun, 25 Oct 2026 13:50:01 -0400 Subject: [PATCH 1/5] fix(bundle): reverse-dep scan reports unreadable manifests and returns directory names; install clears a stale remove intent (D-0662) --- .github/wiki/Bundles.md | 2 + .github/wiki/cmd-bundle.md | 4 +- internal/bundle/installer_appready.go | 13 +++- internal/bundle/installer_intent_test.go | 40 ++++++++++ internal/bundle/remover_deps.go | 17 ++++- .../bundle/remover_deps_unreadable_test.go | 73 +++++++++++++++++++ internal/plugin/installer_remove_update.go | 47 ++++++++++-- internal/plugin/reverse_deps.go | 10 ++- internal/plugin/reverse_deps_test.go | 58 ++++++++++++++- 9 files changed, 247 insertions(+), 17 deletions(-) create mode 100644 internal/bundle/installer_intent_test.go create mode 100644 internal/bundle/remover_deps_unreadable_test.go diff --git a/.github/wiki/Bundles.md b/.github/wiki/Bundles.md index e7319c91a..e7bec92c9 100644 --- a/.github/wiki/Bundles.md +++ b/.github/wiki/Bundles.md @@ -88,6 +88,8 @@ A bundle remove deletes a plugin only when the bundle was the last one in its `i - **Not recorded.** The ledger at `/.nself-state/bundles.json` records a bundle only when all its plugins are installed. For a bundle it does not record, `bundle remove` fails with E134 (exit 1): nothing it installed is removable as a unit. Remove the individual plugins with `nself plugin remove `. - **`--dry-run`** prints the plan and changes nothing; it never creates the ledger. `nself remove --dry-run` prints the plugin, the data rule, the referencing bundles and whether E133 would refuse, and changes nothing. - **A failure stops the run.** The ledger then keeps the bundle recorded and drops only the plugins already removed, so running the same command again continues from there. +- **Resume.** The remove is recorded in `.nself-state/remove-intent.json` in the plugin directory before the first plugin goes. If it is interrupted, run the same command again: it prints `resuming an interrupted remove` and removes only what is left. `nself bundle install ` clears that bundle's intent, so a later remove starts fresh. +- **Kept for a dependent.** A member that an installed plugin outside the plan lists in its `plugin.json` `dependencies` is kept and recorded `explicit` (`kept: px (needed by outsider; recorded explicit)`); dependents are matched by plugin directory name. If an outside plugin's `plugin.json` cannot be read, the remove refuses before removing anything, because that plugin might depend on a member: fix or remove it, then rerun. `nself plugin remove` refuses the same way unless `--force`. - `--force` keeps its meaning (overwrite hand-edited generated files). `--keep-data` is a hidden, deprecated alias; from v1.5 it does nothing and prints a warning. It is removed in v1.6.0. `nself remove ` (and `nself plugin remove`) refuses a plugin an installed bundle still references: **E133**, exit 1, naming the bundle. Remove the bundle, or pass `--force` to remove only that plugin; the ledger then drops the plugin's record. diff --git a/.github/wiki/cmd-bundle.md b/.github/wiki/cmd-bundle.md index 2260755f2..8c16df639 100644 --- a/.github/wiki/cmd-bundle.md +++ b/.github/wiki/cmd-bundle.md @@ -166,7 +166,9 @@ An interrupted remove is resumed by running the same command again (the progress is kept in `.nself-state/remove-intent.json` in the plugin directory). A remove interrupted after the ledger file was deleted cannot resume: the leftover plugins are kept as explicit, and `nself plugin remove` removes them. -A member that a kept plugin depends on is kept and recorded as explicit. +A member that a plugin outside the plan depends on is kept and recorded as explicit. +If such a plugin's `plugin.json` cannot be read, the remove refuses before removing +anything. `nself bundle install` clears the bundle's old remove intent. ``` nself bundle remove [--dry-run] [--keep-data] diff --git a/internal/bundle/installer_appready.go b/internal/bundle/installer_appready.go index adacf9de3..6eec06568 100644 --- a/internal/bundle/installer_appready.go +++ b/internal/bundle/installer_appready.go @@ -81,7 +81,7 @@ func finishInstall(ctx context.Context, rn *appready.Runner, store *ledger.Store if len(members) == 0 { return "no installed members to record", nil } - if err := store.Update(func(l *ledger.Ledger) error { + if err := store.Locked(func(tx *ledger.Tx, l ledger.Ledger) error { // A member that was on disk before this install and has no ledger // record was installed on its own: record it explicit so a later // bundle remove keeps it. (Errs toward keeping the plugin.) @@ -95,7 +95,16 @@ func finishInstall(ctx context.Context, rn *appready.Runner, store *ledger.Store } } l.AddBundle(result.Bundle, members, time.Now()) - addCatalogHolders(l, result.Bundle, members, time.Now()) + addCatalogHolders(&l, result.Bundle, members, time.Now()) + if err := tx.Write(l); err != nil { + return err + } + // A remove that was interrupted before this install is superseded: the + // bundle is installed again, so a later remove must not "resume" it + // (D-0662). A failed clear is a warning, never a failed install. + if err := tx.ClearRemoveIntent(result.Bundle); err != nil { + _, _ = fmt.Fprintf(out, "warning: could not clear the old remove intent of bundle %s: %v\n", result.Bundle, err) + } return nil }); err != nil { return "", fmt.Errorf("the plugins are installed (not rolled back) but recording them in %s failed: %w", store.Path(), err) diff --git a/internal/bundle/installer_intent_test.go b/internal/bundle/installer_intent_test.go new file mode 100644 index 000000000..6afbff42c --- /dev/null +++ b/internal/bundle/installer_intent_test.go @@ -0,0 +1,40 @@ +package bundle + +// installer_intent_test.go — D-0662: installing a bundle supersedes an old +// remove intent of that bundle, so a later remove does not print "resuming". + +import ( + "bytes" + "testing" + "time" + + "github.com/nself-org/cli/internal/bundle/ledger" +) + +func TestBundleInstallClearsStaleRemoveIntent(t *testing.T) { + pluginDir := isolate(t) + setupOfflineRegistry(t, mockNotificationsOnly()) + store := newLedgerStore(pluginDir) + for _, slug := range []string{"task", "other"} { + err := store.Locked(func(tx *ledger.Tx, _ ledger.Ledger) error { + return tx.SetRemoveIntent(slug, ledger.RemoveIntent{StartedAt: time.Now(), Remaining: []string{"notifications"}}) + }) + if err != nil { + t.Fatal(err) + } + } + var out bytes.Buffer + opts := freeOpts(t, pluginDir, &out, func(name string) error { + writeMember(t, pluginDir, name, 1, nil) + return nil + }) + if _, err := Install(opCtx(t), "task", opts); err != nil { + t.Fatalf("install: %v\n%s", err, out.String()) + } + if _, ok := intentOf(t, pluginDir, "task"); ok { + t.Error("the install left task's stale remove intent") + } + if _, ok := intentOf(t, pluginDir, "other"); !ok { + t.Error("the install cleared another bundle's remove intent") + } +} diff --git a/internal/bundle/remover_deps.go b/internal/bundle/remover_deps.go index a8eadce28..1a90fdeb6 100644 --- a/internal/bundle/remover_deps.go +++ b/internal/bundle/remover_deps.go @@ -9,11 +9,15 @@ // "needed by ; recorded explicit") and their records in plan.work // restored as explicit, so a later bundle remove keeps them too. // Constraints: iterates to a fixpoint (a kept plugin is itself an outside -// dependent of what it needs). A scan error refuses the plan: nothing is -// removed. No I/O beyond the scan. +// dependent of what it needs). Dependents are plugin directory names, the same +// names the plan holds. A scan error refuses the plan: nothing is removed. So +// does an outside plugin whose plugin.json cannot be read (it may depend on a +// member); an unreadable manifest of a planned member is no obstacle (D-0662). +// No I/O beyond the scan. package bundle import ( + "errors" "fmt" "sort" "strings" @@ -43,6 +47,15 @@ func keepDependedOn(plan *removePlan, orig ledger.Ledger, pluginDir string, scan continue } deps, err := scan(pluginDir, name) + var unreadable *plugin.UnreadableManifestsError + if errors.As(err, &unreadable) { + if outside := outsidePlan(unreadable.Dirs, planned); len(outside) == 0 { + err = nil // only planned members are unreadable: they go anyway + } else { + return fmt.Errorf("checking which installed plugins depend on %s: %w; nothing was removed, fix or remove it and rerun", + name, &plugin.UnreadableManifestsError{Dirs: outside}) + } + } if err != nil { return fmt.Errorf("checking which installed plugins depend on %s: %w", name, err) } diff --git a/internal/bundle/remover_deps_unreadable_test.go b/internal/bundle/remover_deps_unreadable_test.go new file mode 100644 index 000000000..63c775b53 --- /dev/null +++ b/internal/bundle/remover_deps_unreadable_test.go @@ -0,0 +1,73 @@ +package bundle + +// remover_deps_unreadable_test.go — D-0662: an outside plugin whose plugin.json +// cannot be read refuses the plan; an unreadable planned member does not; and +// dependents are matched by directory name, not manifest name. + +import ( + "encoding/json" + "os" + "path/filepath" + "strings" + "testing" +) + +func breakManifest(t *testing.T, dir, name string) { + t.Helper() + if err := os.WriteFile(filepath.Join(dir, name, "plugin.json"), []byte(`{"name":`), 0o644); err != nil { + t.Fatal(err) + } +} + +func TestRemoveRefusesWhenAnOutsiderManifestIsUnreadable(t *testing.T) { + dir := chatDir(t) + breakManifest(t, dir, "outsider") + before := ledgerBytes(t, dir) + _, _, rec, err := removeChat(t, dir, nil) + if err == nil || !strings.Contains(err.Error(), "outsider") || !strings.Contains(err.Error(), "nothing was removed") { + t.Fatalf("err = %v, want a refusal naming outsider", err) + } + if len(rec.calls) != 0 || !isAlreadyInstalled(dir, "px") || string(before) != string(ledgerBytes(t, dir)) { + t.Errorf("a refused plan changed state; removed %v", rec.names()) + } +} + +func TestRemoveIgnoresAnUnreadableManifestOfAPlannedMember(t *testing.T) { + dir := chatDir(t) + breakManifest(t, dir, "px") + _, out, rec, err := removeChat(t, dir, nil) + if err != nil { + t.Fatalf("remove: %v\n%s", err, out) + } + if !sameStrings(rec.names(), []string{"py", "px"}) { + t.Errorf("removed %v, want [py px]", rec.names()) + } +} + +// A dependent's manifest name equals a planned member, but it lives in another +// directory: it is outside the plan, so the member it needs is kept. +func TestRemoveMatchesDependentsByDirectoryNotManifestName(t *testing.T) { + dir := chatDir(t) + setDeps(t, dir, "outsider", "px") + path := filepath.Join(dir, "outsider", "plugin.json") + raw, err := os.ReadFile(path) + if err != nil { + t.Fatal(err) + } + var m map[string]any + if err := json.Unmarshal(raw, &m); err != nil { + t.Fatal(err) + } + m["name"], m["schema"] = "py", "np_py" + out, _ := json.Marshal(m) + if err := os.WriteFile(path, out, 0o644); err != nil { + t.Fatal(err) + } + _, outp, rec, err := removeChat(t, dir, nil) + if err != nil { + t.Fatalf("remove: %v\n%s", err, outp) + } + if !sameStrings(rec.names(), []string{"py"}) || !strings.Contains(outp, "kept: px (needed by outsider; recorded explicit)") { + t.Errorf("removed %v, want only py\n%s", rec.names(), outp) + } +} diff --git a/internal/plugin/installer_remove_update.go b/internal/plugin/installer_remove_update.go index 899298918..7681dd32a 100644 --- a/internal/plugin/installer_remove_update.go +++ b/internal/plugin/installer_remove_update.go @@ -9,9 +9,11 @@ package plugin import ( "context" + "errors" "fmt" "os" "path/filepath" + "sort" "strings" "github.com/nself-org/cli/internal/config" @@ -31,9 +33,28 @@ func logDangerousPermissions(pluginName string, permissions []string) { } } +// UnreadableManifestsError reports installed plugin directories whose +// plugin.json exists but cannot be read or parsed (D-0662). Such a plugin might +// depend on the plugin being removed, so a reverse-dependency scan cannot rule +// it out. The scan still returns every dependent it could read alongside it. +type UnreadableManifestsError struct { + // Dirs lists the plugin directory names (sorted). + Dirs []string +} + +func (e *UnreadableManifestsError) Error() string { + return fmt.Sprintf("plugin.json of %s cannot be read, so it cannot be ruled out as a dependent", + strings.Join(e.Dirs, ", ")) +} + // checkReverseDependencies scans all installed plugins in pluginDir and returns -// the names of any that declare name as a dependency. This prevents silently -// breaking dependent plugins during uninstall. +// the directory names of any that declare name as a dependency. This prevents +// silently breaking dependent plugins during uninstall. +// +// A directory without a plugin.json is not a plugin and is skipped, as are +// dot-prefixed folders (install staging, .nself-state) and ".prev" backups. A +// plugin.json that exists but cannot be read or parsed is reported through an +// *UnreadableManifestsError (the dependents found so far are still returned). func checkReverseDependencies(pluginDir, name string) ([]string, error) { entries, err := os.ReadDir(pluginDir) if err != nil { @@ -43,23 +64,33 @@ func checkReverseDependencies(pluginDir, name string) ([]string, error) { return nil, fmt.Errorf("reading plugin directory: %w", err) } - var dependents []string + var dependents, unreadable []string for _, entry := range entries { - if !entry.IsDir() || entry.Name() == name { + if !entry.IsDir() || entry.Name() == name || strings.HasPrefix(entry.Name(), ".") { continue } manifestPath := filepath.Join(pluginDir, entry.Name(), "plugin.json") + if _, statErr := os.Lstat(manifestPath); os.IsNotExist(statErr) { + continue + } m, err := parseManifest(manifestPath) if err != nil { - continue // skip directories without valid manifests + if !strings.HasSuffix(entry.Name(), ".prev") { + unreadable = append(unreadable, entry.Name()) + } + continue } for _, dep := range m.Dependencies { if strings.EqualFold(dep, name) { - dependents = append(dependents, m.Name) + dependents = append(dependents, entry.Name()) break } } } + if len(unreadable) > 0 { + sort.Strings(unreadable) + return dependents, &UnreadableManifestsError{Dirs: unreadable} + } return dependents, nil } @@ -87,6 +118,10 @@ func Remove(ctx context.Context, cfg *config.Config, name string, pluginDir stri // Check for reverse dependencies before doing anything destructive. if !force { dependents, err := checkReverseDependencies(pluginDir, name) + var unreadable *UnreadableManifestsError + if errors.As(err, &unreadable) { + return fmt.Errorf("checking reverse dependencies: %w. Fix or remove it, or use --force to remove anyway", err) + } if err != nil { return fmt.Errorf("checking reverse dependencies: %w", err) } diff --git a/internal/plugin/reverse_deps.go b/internal/plugin/reverse_deps.go index 420d5628f..48554ce10 100644 --- a/internal/plugin/reverse_deps.go +++ b/internal/plugin/reverse_deps.go @@ -3,14 +3,16 @@ // Purpose: let `nself bundle remove` keep a member that a plugin outside the // bundle still needs, using the same scan `plugin remove` uses. // Inputs: the plugin directory and a plugin name. -// Outputs: the manifest names of the dependents. +// Outputs: the directory names of the dependents. // Constraints: read only; the name is validated before the scan. package plugin -// ReverseDependencies returns the manifest names of installed plugins whose +// ReverseDependencies returns the directory names of installed plugins whose // plugin.json lists name as a dependency (compared without regard to case). -// Directories without a valid manifest are skipped. A missing plugin directory -// has no dependents. An invalid name is an error. +// Directories without a plugin.json are skipped. A plugin.json that exists but +// cannot be read or parsed comes back as an *UnreadableManifestsError next to +// the dependents that could be read (D-0662). A missing plugin directory has no +// dependents. An invalid name is an error. func ReverseDependencies(pluginDir, name string) ([]string, error) { if err := ValidateName(name); err != nil { return nil, err diff --git a/internal/plugin/reverse_deps_test.go b/internal/plugin/reverse_deps_test.go index cc7ad316b..d5da3021c 100644 --- a/internal/plugin/reverse_deps_test.go +++ b/internal/plugin/reverse_deps_test.go @@ -4,11 +4,16 @@ package plugin // manifests, its case rules, skipped directories and name validation. import ( + "context" + "errors" "fmt" "os" "path/filepath" "sort" + "strings" "testing" + + "github.com/nself-org/cli/internal/config" ) func writeDepManifest(t *testing.T, pluginDir, dir, manifest string) { @@ -35,8 +40,9 @@ func TestReverseDependencies(t *testing.T) { t.Fatal(err) } got, err := ReverseDependencies(dir, "target") - if err != nil { - t.Fatal(err) + var unreadable *UnreadableManifestsError + if !errors.As(err, &unreadable) || len(unreadable.Dirs) != 1 || unreadable.Dirs[0] != "broken" { + t.Fatalf("err = %v, want unreadable [broken]", err) } sort.Strings(got) if len(got) != 2 || got[0] != "dep-one" || got[1] != "dep-two" { @@ -54,3 +60,51 @@ func TestReverseDependenciesEdges(t *testing.T) { } } } + +// D-0662: an unreadable plugin.json is reported, not skipped; dependents are +// directory names; staging folders and ".prev" backups are not plugins. +func TestReverseDependenciesReportsUnreadableManifests(t *testing.T) { + dir := t.TempDir() + base := `{"name":"%s","version":"1.0.0","description":"d","category":"utility","license":"MIT"%s}` + writeDepManifest(t, dir, "dep-dir", fmt.Sprintf(base, "other-name", `,"dependencies":["target"]`)) + writeDepManifest(t, dir, "broken", `{not json`) + writeDepManifest(t, dir, ".staging-x", `{not json`) + writeDepManifest(t, dir, "old.prev", `{not json`) + if err := os.MkdirAll(filepath.Join(dir, "no-manifest"), 0o755); err != nil { + t.Fatal(err) + } + got, err := ReverseDependencies(dir, "target") + var unreadable *UnreadableManifestsError + if !errors.As(err, &unreadable) || len(unreadable.Dirs) != 1 || unreadable.Dirs[0] != "broken" { + t.Fatalf("err = %v, want unreadable [broken]", err) + } + if len(got) != 1 || got[0] != "dep-dir" { + t.Errorf("dependents = %v, want the directory name [dep-dir]", got) + } +} + +func TestCheckReverseDependenciesNoUnreadable(t *testing.T) { + dir := t.TempDir() + writeDepManifest(t, dir, "fine", `{"name":"fine","version":"1.0.0","description":"d","category":"utility","license":"MIT"}`) + if got, err := checkReverseDependencies(dir, "target"); err != nil || len(got) != 0 { + t.Errorf("got %v, %v; want none", got, err) + } +} + +// D-0662: plugin remove refuses while another plugin's manifest is unreadable +// (it could depend on the target); --force removes anyway. +func TestRemoveRefusesWhenAnotherManifestIsUnreadable(t *testing.T) { + dir := t.TempDir() + writeDepManifest(t, dir, "target", `{"name":"target","version":"1.0.0","description":"d","category":"utility","license":"MIT"}`) + writeDepManifest(t, dir, "midupdate", `{"name":`) + err := Remove(context.Background(), &config.Config{}, "target", dir, true, false) + if err == nil || !strings.Contains(err.Error(), "midupdate") || !strings.Contains(err.Error(), "--force") { + t.Fatalf("err = %v, want a refusal naming midupdate and --force", err) + } + if _, statErr := os.Stat(filepath.Join(dir, "target")); statErr != nil { + t.Errorf("target was removed: %v", statErr) + } + if err := Remove(context.Background(), &config.Config{}, "target", dir, true, true); err != nil { + t.Fatalf("forced remove: %v", err) + } +} From bc6b8502e80646d7db85292f511096bc5a37f519 Mon Sep 17 00:00:00 2001 From: Aric Camarata Date: Sun, 25 Oct 2026 13:50:50 -0400 Subject: [PATCH 2/5] docs(config): describe the unquoted-dollar warning on the config validate page (D-0751) --- .github/wiki/cmd-config.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/wiki/cmd-config.md b/.github/wiki/cmd-config.md index aa37baf09..9c49a5dd1 100644 --- a/.github/wiki/cmd-config.md +++ b/.github/wiki/cmd-config.md @@ -33,7 +33,7 @@ With `--json` in v1.5 mode an applied change prints the envelope with `applied: **Rotating a live secret.** Put the new value in `.env.secrets` and let the regeneration carry it: `nself config set HASURA_GRAPHQL_ADMIN_SECRET --env secrets --yes` on a prod-class env. The generated env and compose files are updated; the running containers keep the old value until they are recreated (`nself start`). `nself secrets rotate` is a different tool: it rotates the encrypted secret store (`.secrets/.age`), which the build does not read, so it does not regenerate anything. Its schedule tracks `HASURA_GRAPHQL_ADMIN_SECRET`, the name the stack reads. -The `validate` subcommand runs all registered configuration validators and reports pass/fail per rule. It is automatically run during `nself build`, but you can run it independently to check config without rebuilding. +The `validate` subcommand runs all registered configuration validators and reports pass/fail per rule. It is automatically run during `nself build`, but you can run it independently to check config without rebuilding. It also warns, on stderr, about each env-file value whose unquoted or double-quoted `$` the loader would expand (`.env.secrets:12: KEY holds an unquoted '$' ...`): the warning gives the file, line and key only, never the value; a `${NAME}` reference is not flagged; and warnings do not change the exit code or the `--json` document. Single-quote the value if the `$` is literal. ### JSON output From 45e9d2df312c477818321d60c065ca6b8758b162 Mon Sep 17 00:00:00 2001 From: Aric Camarata Date: Sun, 25 Oct 2026 13:52:22 -0400 Subject: [PATCH 3/5] fix(migrate): record the v1.4 consent warning in meta.deprecations under --json; guides pass --yes to rollback (D-0762) --- .github/wiki/Upgrade-From-v0.9.md | 8 +++-- .github/wiki/Upgrading-from-v1.md | 6 ++-- .github/wiki/cmd-migrate.md | 6 ++-- cmd/commands/data_consent.go | 14 +++++++-- cmd/commands/data_consent_test.go | 52 +++++++++++++++++++++++++++++++ 5 files changed, 77 insertions(+), 9 deletions(-) diff --git a/.github/wiki/Upgrade-From-v0.9.md b/.github/wiki/Upgrade-From-v0.9.md index a92d3523f..1319aa3dc 100644 --- a/.github/wiki/Upgrade-From-v0.9.md +++ b/.github/wiki/Upgrade-From-v0.9.md @@ -302,7 +302,7 @@ If `nself migrate run` fails partway through: nself migrate rollback --list # Restore the last backup -nself migrate rollback +nself migrate rollback --yes ``` The rollback restores all files that were backed up before the failure. @@ -329,12 +329,14 @@ If you need to revert to v0.9 after migrating: nself migrate rollback --list # Restore the most recent backup -nself migrate rollback +nself migrate rollback --yes # Or restore a specific backup by timestamp -nself migrate rollback --backup 20260420-143022 +nself migrate rollback --backup 20260420-143022 --yes ``` +`--yes` confirms the restore without a prompt. From v1.5 a rollback without `--yes` asks on a terminal and is refused with E403 anywhere else (scripts, CI); in v1.4 it still runs but prints a deprecation warning (under `--json`, in `meta.deprecations`). + After rollback, reinstall the v0.9 CLI (use the Homebrew formula version pin or download from GitHub releases). --- diff --git a/.github/wiki/Upgrading-from-v1.md b/.github/wiki/Upgrading-from-v1.md index d0271181b..8b2f9aae1 100644 --- a/.github/wiki/Upgrading-from-v1.md +++ b/.github/wiki/Upgrading-from-v1.md @@ -108,15 +108,17 @@ Then restore your data: `nself db restore /path/to/backup.sql` If migration fails, roll back to v1 state: ```bash -nself migrate rollback +nself migrate rollback --yes ``` Or restore a specific backup: ```bash -nself migrate rollback --backup 20260328-143022 +nself migrate rollback --backup 20260328-143022 --yes ``` +`--yes` confirms the restore without a prompt. From v1.5 a rollback without `--yes` asks on a terminal and is refused with E403 anywhere else (scripts, CI); in v1.4 it still runs but prints a deprecation warning (under `--json`, in `meta.deprecations`). + --- See [[Guide-Migration-from-v1]] for a detailed walkthrough with troubleshooting steps. diff --git a/.github/wiki/cmd-migrate.md b/.github/wiki/cmd-migrate.md index 17a92d3a3..264798ea7 100644 --- a/.github/wiki/cmd-migrate.md +++ b/.github/wiki/cmd-migrate.md @@ -93,15 +93,17 @@ nself migrate detect nself migrate run # Restore from the most recent backup -nself migrate rollback +nself migrate rollback --yes # List available backups nself migrate rollback --list # Restore from a specific backup -nself migrate rollback --backup 20260417-143022 +nself migrate rollback --backup 20260417-143022 --yes ``` +`--yes` confirms the restore without a prompt. From v1.5 a rollback without `--yes` asks on a terminal and is refused with E403 anywhere else (scripts, CI); in v1.4 it still runs but prints a deprecation warning (under `--json`, in `meta.deprecations`). + See [[Upgrade-From-v0.9]] for the full step-by-step migration guide. diff --git a/cmd/commands/data_consent.go b/cmd/commands/data_consent.go index 2679051bc..1bbcc4720 100644 --- a/cmd/commands/data_consent.go +++ b/cmd/commands/data_consent.go @@ -10,7 +10,8 @@ package commands // // Behaviour (ADR 0021 compat switch): // - --yes set: runs, silent. -// - v1.4 (default), no --yes: runs, with one stderr deprecation line. +// - v1.4 (default), no --yes: runs, with one stderr deprecation line (and, +// under --json, a meta.deprecations entry). // - v1.5 (NSELF_V15=1), no --yes: asks on a terminal, otherwise refuses // with E403 before any side effect. // - a read-only or preview flag (--dry-run, --list) skips the gate entirely. @@ -22,9 +23,13 @@ import ( "github.com/nself-org/cli/internal/compat" "github.com/nself-org/cli/internal/errs" + "github.com/nself-org/cli/internal/output" "github.com/spf13/cobra" ) +// consentRefusedFrom is the release from which a bare data-loss command is refused. +const consentRefusedFrom = "v1.5.0" + // consentFlagUsage is the help text of the --yes flag on the gated commands. const consentFlagUsage = "Confirm the data loss without a prompt (required from v1.5 unless you answer a terminal prompt)" @@ -90,8 +95,13 @@ func requireDataLossConsent(cmd *cobra.Command, what string) error { return errs.New("E403", fmt.Sprintf("%s %s and was not confirmed", cmd.CommandPath(), what)). WithFix("pass --yes to confirm, or run it on a terminal and answer y") } + path := strings.TrimPrefix(cmd.CommandPath(), "nself ") + // A --json caller gets the notice in meta.deprecations like a canon move + // notice (D-0762), even when the stderr line is silenced. + if output.JSONRequestedFromArgs(os.Args) { + output.AddDeprecation("nself "+path+" without --yes", "nself "+path+" --yes", consentRefusedFrom) + } if !warningsSilenced(os.Args) { - path := strings.TrimPrefix(cmd.CommandPath(), "nself ") _, _ = fmt.Fprintf(cmd.ErrOrStderr(), "warning: nself %s without --yes is deprecated (%s); from v1.5 it needs --yes or a terminal confirmation. Pass --yes now.\n", path, what) } return nil diff --git a/cmd/commands/data_consent_test.go b/cmd/commands/data_consent_test.go index d6070a0eb..ab4b0b207 100644 --- a/cmd/commands/data_consent_test.go +++ b/cmd/commands/data_consent_test.go @@ -7,11 +7,13 @@ package commands import ( "bytes" "errors" + "io" "os" "strings" "testing" "github.com/nself-org/cli/internal/errs" + "github.com/nself-org/cli/internal/output" "github.com/spf13/cobra" "golang.org/x/term" ) @@ -167,3 +169,53 @@ func TestDataLossGateSkipFlagsExist(t *testing.T) { t.Fatal("rollback lost its --list flag") } } + +// TestDataLossGateJSONDeprecation: a v1.4 --json caller sees the consent +// warning in meta.deprecations, even when the stderr line is silenced, and a +// run with --yes or without --json records nothing (D-0762). +func TestDataLossGateJSONDeprecation(t *testing.T) { + cases := []struct { + name string + argv []string + yes bool + want bool + }{ + {"json bare", []string{"nself", "backup", "prune", "--json"}, false, true}, + {"json bare silenced", []string{"nself", "backup", "prune", "--json", "--quiet"}, false, true}, + {"json yes", []string{"nself", "backup", "prune", "--json", "--yes"}, true, false}, + {"text bare", []string{"nself", "backup", "prune"}, false, false}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + t.Setenv("NSELF_V15", "0") + output.ResetState() + t.Cleanup(output.ResetState) + withArgs(t, tc.argv...) + var buf bytes.Buffer + c := gateCmd(&buf) + if tc.yes { + if err := c.Flags().Set("yes", "true"); err != nil { + t.Fatal(err) + } + } + if err := requireDataLossConsent(c, "deletes old backups"); err != nil { + t.Fatal(err) + } + var doc bytes.Buffer + if err := output.EmitData(output.Writer{Out: &doc, Err: io.Discard}, "backup prune", map[string]string{"ok": "1"}); err != nil { + t.Fatal(err) + } + got := strings.Contains(doc.String(), `"deprecations"`) + if got != tc.want { + t.Fatalf("meta.deprecations present = %v, want %v:\n%s", got, tc.want, doc.String()) + } + if tc.want { + for _, w := range []string{`"old": "nself backup prune without --yes"`, `"new": "nself backup prune --yes"`, `"removal_at": "v1.5.0"`} { + if !strings.Contains(doc.String(), w) { + t.Errorf("meta lacks %s:\n%s", w, doc.String()) + } + } + } + }) + } +} From 5144c4b5ac551ed3c607cd113c9c56b19f7895f2 Mon Sep 17 00:00:00 2001 From: Aric Camarata Date: Sun, 25 Oct 2026 13:53:21 -0400 Subject: [PATCH 4/5] ci(proxy-swap): run the round-trip and kill-mid-remove legs on pull requests (D-1165) --- .github/workflows/proxy-swap-e2e.yml | 4 +- internal/repoqa/proxy_swap_legs_test.go | 52 +++++++++++++++++++++++++ 2 files changed, 54 insertions(+), 2 deletions(-) create mode 100644 internal/repoqa/proxy_swap_legs_test.go diff --git a/.github/workflows/proxy-swap-e2e.yml b/.github/workflows/proxy-swap-e2e.yml index 4d5e7983f..f474420cd 100644 --- a/.github/workflows/proxy-swap-e2e.yml +++ b/.github/workflows/proxy-swap-e2e.yml @@ -60,10 +60,10 @@ jobs: if: ${{ github.event_name == 'pull_request' || inputs.leg == 'add' }} run: bash scripts/e2e/proxy-swap.sh --add - name: Round-trip leg - if: ${{ github.event_name == 'workflow_dispatch' && inputs.leg == 'round-trip' }} + if: ${{ github.event_name == 'pull_request' || (github.event_name == 'workflow_dispatch' && inputs.leg == 'round-trip') }} run: bash scripts/e2e/proxy-swap.sh --round-trip --assert-identical - name: Kill-mid-remove leg - if: ${{ github.event_name == 'workflow_dispatch' && inputs.leg == 'round-trip' }} + if: ${{ github.event_name == 'pull_request' || (github.event_name == 'workflow_dispatch' && inputs.leg == 'round-trip') }} run: bash scripts/e2e/proxy-swap.sh --kill-mid-remove - name: Renew leg if: ${{ github.event_name == 'pull_request' || (github.event_name == 'workflow_dispatch' && inputs.leg == 'renew') }} diff --git a/internal/repoqa/proxy_swap_legs_test.go b/internal/repoqa/proxy_swap_legs_test.go new file mode 100644 index 000000000..6c7bf25af --- /dev/null +++ b/internal/repoqa/proxy_swap_legs_test.go @@ -0,0 +1,52 @@ +package repoqa + +import ( + "os" + "path/filepath" + "strings" + "testing" + + "gopkg.in/yaml.v3" +) + +// TestProxySwapRecoveryLegsRunOnPullRequests: the round-trip and kill-mid-remove +// legs of the proxy swap e2e used to run only on manual dispatch, so a pull +// request that broke the way back from a swap, or the recovery after a kill, +// stayed green (D-1165). Every leg step must run on pull_request. +func TestProxySwapRecoveryLegsRunOnPullRequests(t *testing.T) { + data, err := os.ReadFile(filepath.Join(repoRoot(t), ".github/workflows/proxy-swap-e2e.yml")) + if err != nil { + t.Fatal(err) + } + var workflow struct { + Jobs map[string]struct { + Steps []struct { + Name string `yaml:"name"` + Run string `yaml:"run"` + If string `yaml:"if"` + } `yaml:"steps"` + } `yaml:"jobs"` + } + if err := yaml.Unmarshal(data, &workflow); err != nil { + t.Fatal(err) + } + seen := map[string]bool{} + for _, job := range workflow.Jobs { + for _, step := range job.Steps { + for _, leg := range []string{"--add", "--round-trip", "--kill-mid-remove", "--renew"} { + if !strings.Contains(step.Run, "proxy-swap.sh "+leg) { + continue + } + seen[leg] = true + if !strings.Contains(step.If, "github.event_name == 'pull_request'") { + t.Errorf("step %q (proxy-swap.sh %s) does not run on pull_request: if = %q", step.Name, leg, step.If) + } + } + } + } + for _, leg := range []string{"--add", "--round-trip", "--kill-mid-remove", "--renew"} { + if !seen[leg] { + t.Errorf("no workflow step runs proxy-swap.sh %s", leg) + } + } +} From b63b39ba9858390c63d12d550dfcffcd466a7fae Mon Sep 17 00:00:00 2001 From: Aric Camarata Date: Sun, 25 Oct 2026 14:06:50 -0400 Subject: [PATCH 5/5] test(commands): share one nself build and drop the doubled live pilot runs to cut the package run time (D-0735) --- cmd/commands/deprecation_wiring_test.go | 13 ++------ cmd/commands/mcp_integration_test.go | 26 +++++---------- cmd/commands/pilot_json_test.go | 15 +++++++++ internal/repoqa/test_binary_builds_test.go | 39 ++++++++++++++++++++++ 4 files changed, 66 insertions(+), 27 deletions(-) create mode 100644 internal/repoqa/test_binary_builds_test.go diff --git a/cmd/commands/deprecation_wiring_test.go b/cmd/commands/deprecation_wiring_test.go index ca991e7cd..27b7e75d1 100644 --- a/cmd/commands/deprecation_wiring_test.go +++ b/cmd/commands/deprecation_wiring_test.go @@ -246,16 +246,9 @@ func TestInstalledBinaryEmitsDeprecationWarning(t *testing.T) { t.Skip("binary staging in this test is POSIX-only") } - root := findRepoRootForTest(t) - installDir := t.TempDir() - binPath := filepath.Join(installDir, "nself") - - build := exec.Command("go", "build", "-mod=vendor", "-o", binPath, "./cmd/nself/") - build.Dir = root - build.Env = append(os.Environ(), "CGO_ENABLED=0") - if out, err := build.CombinedOutput(); err != nil { - t.Fatalf("build failed: %v\n%s", err, out) - } + // pilotBinary is one shared build of ./cmd/nself in a directory of its own, + // with no source tree next to it (D-0735: no second compile and link). + binPath := pilotBinary(t) // Run from a *different* empty directory so neither the binary's own // directory nor the working directory contains a source tree. diff --git a/cmd/commands/mcp_integration_test.go b/cmd/commands/mcp_integration_test.go index ad841ddd3..b65ff1b09 100644 --- a/cmd/commands/mcp_integration_test.go +++ b/cmd/commands/mcp_integration_test.go @@ -32,20 +32,14 @@ import ( "github.com/mark3labs/mcp-go/mcp" ) -// buildNSelfBinary compiles ./cmd/nself into dir and returns its path. Used -// so mcpExecSelf's re-exec has a real binary to invoke — os.Executable() -// inside `go test` resolves to the test harness, which doesn't understand -// "build"/"start" as subcommands, hence the NSELF_MCP_EXEC_OVERRIDE hook. -func buildNSelfBinary(t *testing.T, dir string) string { +// buildNSelfBinary returns the nself binary the suite builds once per test +// process (pilotBinary), so mcpExecSelf's re-exec has a real binary to invoke: +// os.Executable() inside `go test` resolves to the test harness, which doesn't +// understand "build"/"start" as subcommands, hence the NSELF_MCP_EXEC_OVERRIDE +// hook. Reusing it spares a second ./cmd/nself compile and link (D-0735). +func buildNSelfBinary(t *testing.T) string { t.Helper() - bin := filepath.Join(dir, "nself") - cmd := exec.Command("go", "build", "-mod=vendor", "-o", bin, "../../cmd/nself") - cmd.Dir = mustGetwd(t) - out, err := cmd.CombinedOutput() - if err != nil { - t.Fatalf("building nself for integration test: %v\n%s", err, out) - } - return bin + return pilotBinary(t) } func mustGetwd(t *testing.T) string { @@ -64,8 +58,7 @@ func TestMCPIntegration_InitBuildVerify(t *testing.T) { t.Skip("skipping binary-build integration test in -short mode") } - binDir := t.TempDir() - bin := buildNSelfBinary(t, binDir) + bin := buildNSelfBinary(t) projectDir := t.TempDir() t.Setenv("NSELF_ALLOW_SOURCE_DIR", "1") @@ -138,8 +131,7 @@ func TestMCPIntegration_StartStatusStop(t *testing.T) { t.Skip("skipping in -short mode") } - binDir := t.TempDir() - bin := buildNSelfBinary(t, binDir) + bin := buildNSelfBinary(t) projectDir := t.TempDir() t.Setenv(mcpSelfExecOverrideEnv, bin) diff --git a/cmd/commands/pilot_json_test.go b/cmd/commands/pilot_json_test.go index 0b9ed5f79..71141892c 100644 --- a/cmd/commands/pilot_json_test.go +++ b/cmd/commands/pilot_json_test.go @@ -207,6 +207,18 @@ func runPilot(t *testing.T, proj string, v15 bool, extra []string, args ...strin return pilotRun{code: code, stdout: o.String(), stderr: e.String()} } +// skipLiveUnderV15Guard skips a subprocess ("live") subtest when it runs inside +// TestPilotJSONUnderV15Guard. runPilot rebuilds the child environment without +// any NSELF_ variable and sets NSELF_V15 itself, so the guard's t.Setenv changes +// nothing for a live run: the standalone test already covers it, and running it +// twice doubled the slowest tests of the package (D-0735). +func skipLiveUnderV15Guard(t *testing.T) { + t.Helper() + if strings.HasPrefix(t.Name(), "TestPilotJSONUnderV15Guard/") { + t.Skip("live runs rebuild the child environment; the standalone test covers them") + } +} + // ---- status ---------------------------------------------------------------- func statusReport(results ...health.HealthResult) *health.HealthReport { @@ -281,6 +293,7 @@ func TestPilotJSONStatus(t *testing.T) { }) t.Run("live", func(t *testing.T) { + skipLiveUnderV15Guard(t) proj := pilotProject(t) r := runPilot(t, proj, true, nil, "status", "--json") data := validateEnvelope(t, r.stdout, "status") @@ -408,6 +421,7 @@ func TestPilotJSONDoctor(t *testing.T) { }) t.Run("live", func(t *testing.T) { + skipLiveUnderV15Guard(t) proj := pilotProject(t) for _, args := range [][]string{{"doctor", "--json"}, {"doctor", "--format", "json"}, {"doctor", "--json", "--deep"}} { r := runPilot(t, proj, true, nil, args...) @@ -561,6 +575,7 @@ func TestPilotJSONConfig(t *testing.T) { }) t.Run("live", func(t *testing.T) { + skipLiveUnderV15Guard(t) p := pilotProject(t) // v1.5: schema-valid envelopes, secrets masked, --reveal honoured. for _, tc := range []struct { diff --git a/internal/repoqa/test_binary_builds_test.go b/internal/repoqa/test_binary_builds_test.go new file mode 100644 index 000000000..cd0143310 --- /dev/null +++ b/internal/repoqa/test_binary_builds_test.go @@ -0,0 +1,39 @@ +package repoqa + +import ( + "os" + "path/filepath" + "strings" + "testing" +) + +// TestCommandsTestsShareOneNselfBuild: the cmd/commands tests compile and link +// ./cmd/nself once per test process (pilotBinary). Each extra build cost about +// half a minute on the macOS Intel runner and pushed the package toward its +// 10m go test timeout (D-0735). A test that needs the binary calls +// pilotBinary(t). TestSurfaceIdentity re-executes itself in a fresh process, so +// it cannot share the build and is the one allowed exception. +func TestCommandsTestsShareOneNselfBuild(t *testing.T) { + allowed := map[string]bool{ + "pilot_json_test.go": true, // pilotBinary itself + "surface_identity_test.go": true, // runs in a fresh process + } + files, err := filepath.Glob(filepath.Join(repoRoot(t), "cmd/commands/*_test.go")) + if err != nil || len(files) == 0 { + t.Fatalf("no cmd/commands tests found: %v", err) + } + for _, f := range files { + if allowed[filepath.Base(f)] { + continue + } + data, err := os.ReadFile(f) + if err != nil { + t.Fatal(err) + } + for i, line := range strings.Split(string(data), "\n") { + if strings.Contains(line, `"go", "build"`) && strings.Contains(line, "cmd/nself") { + t.Errorf("%s:%d builds ./cmd/nself itself; call pilotBinary(t) to share the one build", filepath.Base(f), i+1) + } + } + } +}