fix(cli): harden restore cleanup and bundle rollback (D-0286 D-1162 D-0760)

.github/wiki/Bundles.md

@@ -20,6 +20,8 @@ How `nself bundle install` reaches an app-ready stack, and how nself records whi
 
 The first failing step stops the run; later steps show `skipped`. Outside an nSelf project the plugins are installed and recorded, the later steps are skipped, and the output says `NOT VERIFIED`; the exit code stays 0.
 
+If reconcile fails or is refused (including E403, E480 and E481), the install removes the members it newly added and their ledger records, and restores prior bundle ownership. Existing members and their schemas stay intact, including schemas kept by an earlier removal. Cleanup errors are returned alongside the reconcile error; a member that could not be removed keeps its ledger record. A failure in a later app-ready step keeps the installed members for a retry.
+
 `nself bundle install <bundle> --dry-run` prints the nine steps as `planned` and changes nothing: no files, no ledger, no hooks.
 
 A bundle that is not app-ready fails with **E135** (exit 2) naming the bundle, the step and the plugin. Starting the stack and the readiness, tracking and health steps are `NSELF_V15=1` behaviour. With `NSELF_V15` off the command installs, records the ledger and reconciles (a failed reconcile is an error in both modes), prints those four steps as `skipped` / `not verified (v1.5 behaviour, NSELF_V15=1)`, ends with `NOT VERIFIED` instead of `APP-READY`, and exits 0. A free bundle skips the ping_api bundle entitlement call; each plugin's licence is still checked. Rerun the command after fixing the cause; members that are already installed are not reinstalled.

.github/wiki/Nginx-Generation.md

@@ -74,6 +74,11 @@ The add writes files through reconcile, then, in the same apply, removes the ngi
 
 ### Returning to nginx
 
+Cleanup records dangling symlinks in the confirmed removal plan and unlinks
+them without following their targets. Directory traversal and deletion stay
+inside each generated output directory, including when a directory is replaced
+by a symlink during cleanup or before the confirmed plan is published.
+
 ```bash
 nself remove traefik --yes
 ```

cmd/commands/bundle_reconcile.go

@@ -13,6 +13,7 @@ import (
 	"os"
 
 	"github.com/nself-org/cli/internal/build"
+	"github.com/nself-org/cli/internal/bundle/appready"
 	"github.com/nself-org/cli/internal/config"
 	"github.com/nself-org/cli/internal/docker"
 	"github.com/nself-org/cli/internal/errs"
@@ -66,15 +67,22 @@ func reconcileAfterExtension(cmd *cobra.Command, trigger reconcile.Trigger) erro
 			return errs.New("E403", "extension files changed but regeneration needs confirmation").
 				WithWhy(err.Error()).WithFix("review the pending change with 'nself build --plan', then rerun with --yes (and --force for hand-edited files)")
 		}
-		return fmt.Errorf("regenerating %s:%s: %w", trigger.Kind, trigger.Subject, err)
+		wrapped := fmt.Errorf("regenerating %s:%s: %w", trigger.Kind, trigger.Subject, err)
+		if errors.Is(err, reconcile.ErrPublished) {
+			// The files were written and kept: a bundle install keeps its plugins.
+			return appready.Committed(wrapped)
+		}
+		return wrapped
 	}
+	// Apply wrote the generated files: from here a failure is post-commit, and
+	// a bundle install must keep its plugins (the files reference them).
 	if err := swapContainers(context.Background(), cmd, projectDir, plan); err != nil {
-		return err
+		return appready.Committed(err)
 	}
 	if plan.RequiresConfirmation || len(reconcile.HandEditedPaths(*plan)) > 0 {
 		return nil // Apply already printed the plan before its confirmation gate.
 	}
-	return reconcile.RenderHuman(cmd.ErrOrStderr(), *plan)
+	return appready.Committed(reconcile.RenderHuman(cmd.ErrOrStderr(), *plan))
 }
 
 // Seams for the container half of an extension reconcile; tests replace them.

cmd/commands/bundle_reconcile_orphans_test.go

@@ -7,12 +7,14 @@ package commands
 
 import (
 	"context"
+	"errors"
 	"os"
 	"path/filepath"
 	"reflect"
 	"strings"
 	"testing"
 
+	"github.com/nself-org/cli/internal/bundle/appready"
 	"github.com/nself-org/cli/internal/compat"
 	"github.com/nself-org/cli/internal/compat/compattest"
 	"github.com/nself-org/cli/internal/docker"
@@ -148,3 +150,87 @@ func TestExtensionComposeUsesProjectManifest(t *testing.T) {
 		t.Fatalf("env files must follow the project: %v", c.EnvFiles)
 	}
 }
+
+// A failed container swap happens after Apply wrote the generated files, so the
+// error must say so (appready.ErrCommitted): the bundle install keeps its
+// plugins then. A refused apply wrote nothing and must not carry the mark.
+func TestExtensionReconcileSwapFailureIsCommitted(t *testing.T) {
+	trigger := reconcile.Trigger{Kind: reconcile.TriggerBundle, Subject: "zzfixture"}
+	running := fakeSwapRuntime{running: []reconcile.RunningContainer{
+		{Name: "proj_oldproxy", Service: "oldproxy", State: "running", ConfigHash: "x"},
+	}}
+	cases := map[string]func(){
+		"remove": func() {
+			extensionRemoveOrphans = func(context.Context, []docker.OrphanContainer) []error {
+				return []error{errors.New("docker rm failed")}
+			}
+		},
+		"start": func() {
+			extensionStartServices = func(context.Context, string, []string) error { return errors.New("compose up failed") }
+		},
+	}
+	for name, fail := range cases {
+		t.Run(name, func(t *testing.T) {
+			compattest.Both(t, func(t *testing.T) {
+				if !compat.V15() {
+					return
+				}
+				extensionFixture(t, "")
+				localPluginRegistry(t)
+				swapSeams(t, running)
+				fail()
+				err := reconcileAfterExtension(pluginInstallCmd, trigger)
+				if err == nil || !errors.Is(err, appready.ErrCommitted) {
+					t.Fatalf("err = %v, want a post-commit failure", err)
+				}
+			})
+		})
+	}
+	t.Run("refused", func(t *testing.T) {
+		compattest.Both(t, func(t *testing.T) {
+			if !compat.V15() {
+				return
+			}
+			extensionFixture(t, "prod")
+			localPluginRegistry(t)
+			swapSeams(t, running)
+			err := reconcileAfterExtension(pluginInstallCmd, trigger)
+			if err == nil || errors.Is(err, appready.ErrCommitted) {
+				t.Fatalf("err = %v, want a pre-commit refusal", err)
+			}
+		})
+	})
+}
+
+// Apply publishes the generated files and then fails to write its state record:
+// the files stay, so the error must carry the commit mark and a bundle install
+// keeps its plugins (the review finding on D-0760).
+func TestExtensionReconcileRecordFailureIsCommitted(t *testing.T) {
+	if os.Geteuid() == 0 {
+		t.Skip("root ignores directory permissions")
+	}
+	trigger := reconcile.Trigger{Kind: reconcile.TriggerBundle, Subject: "zzfixture"}
+	compattest.Both(t, func(t *testing.T) {
+		if !compat.V15() {
+			return
+		}
+		p := extensionFixture(t, "")
+		localPluginRegistry(t)
+		swapSeams(t, fakeSwapRuntime{})
+		stateDir := filepath.Dir(reconcile.GeneratedStatePath(p.project))
+		if err := os.MkdirAll(stateDir, 0o755); err != nil {
+			t.Fatal(err)
+		}
+		if err := os.Chmod(stateDir, 0o555); err != nil {
+			t.Fatal(err)
+		}
+		t.Cleanup(func() { _ = os.Chmod(stateDir, 0o755) })
+		err := reconcileAfterExtension(pluginInstallCmd, trigger)
+		if err == nil || !errors.Is(err, appready.ErrCommitted) {
+			t.Fatalf("err = %v, want a post-commit failure", err)
+		}
+		if _, statErr := os.Stat(filepath.Join(p.project, "docker-compose.yml")); statErr != nil {
+			t.Fatalf("the published compose file is missing: %v", statErr)
+		}
+	})
+}

internal/build/expect_publish.go

@@ -92,6 +92,24 @@ func (p *artifactPublisher) target(path string) (*os.Root, string, error) {
 	if err != nil {
 		return nil, "", err
 	}
+	for scope, expected := range p.sink.cleanupRoots {
+		if rel, inside := under(scope, abs); inside {
+			root := p.roots[scope]
+			if root == nil {
+				root, err = os.OpenRoot(scope)
+				if err != nil {
+					return nil, "", err
+				}
+				info, statErr := root.Stat(".")
+				if statErr != nil || !os.SameFile(expected, info) {
+					_ = root.Close()
+					return nil, "", errDeviated(scope, "cleanup directory was replaced")
+				}
+				p.roots[scope] = root
+			}
+			return root, filepath.FromSlash(rel), nil
+		}
+	}
 	base := filepath.VolumeName(abs) + string(filepath.Separator)
 	rel, inside := under(p.sink.root, abs)
 	if inside {

internal/build/expect_rollback.go

@@ -83,7 +83,10 @@ func (p *artifactPublisher) restore(before artifactBefore) error {
 		if err := root.Mkdir(rel, before.info.Mode().Perm()); err != nil && !errors.Is(err, fs.ErrExist) {
 			return err
 		}
-		return root.Chmod(rel, before.info.Mode().Perm())
+		if err := root.Chmod(rel, before.info.Mode().Perm()); err != nil {
+			return err
+		}
+		return p.refreshCleanupRoot(before.path, root, rel)
 	}
 	if before.link != "" {
 		if err := root.Remove(rel); err != nil && !errors.Is(err, fs.ErrNotExist) {
@@ -103,3 +106,30 @@ func (p *artifactPublisher) restore(before artifactBefore) error {
 	}
 	return p.write(before.path, data, before.info.Mode().Perm())
 }
+
+// refreshCleanupRoot rebinds rollback's descendants to the directory rollback
+// just recreated, rather than the cached descriptor for its unlinked predecessor.
+func (p *artifactPublisher) refreshCleanupRoot(path string, parent *os.Root, rel string) error {
+	abs, err := filepath.Abs(p.sink.abs(path))
+	if err != nil {
+		return err
+	}
+	if _, tracked := p.sink.cleanupRoots[abs]; !tracked {
+		return nil
+	}
+	root, err := parent.OpenRoot(rel)
+	if err != nil {
+		return err
+	}
+	info, err := root.Stat(".")
+	if err != nil {
+		_ = root.Close()
+		return err
+	}
+	if previous := p.roots[abs]; previous != nil {
+		_ = previous.Close()
+	}
+	p.roots[abs] = root
+	p.sink.cleanupRoots[abs] = info
+	return nil
+}

internal/build/expect_sink.go

@@ -1,19 +1,8 @@
 package build
 
-// expect_sink.go — a write-mode Sink that holds a build to a confirmed render
-// (P7-LIVE-03, review M3 / EPIC ruling B).
-//
-// Purpose: `nself build` shows a plan, asks for confirmation, then writes. The
-// write must be that render and nothing else, even if an input changed after
-// the plan was made. expectSink stages the render: every write must carry
-// exactly the bytes the confirmed render planned for that path, every removal
-// and chmod must be one it planned, and anything else stops the build with an
-// error before publishing any artifact. After the build, verify checks the
-// complete staged render; commit publishes it with rollback on failure.
-// Inputs: the PlannedBuild of the confirmed render (BuildOptions.Expect).
-// Outputs: errors naming the path that deviated.
-// Constraints: write mode only; reads see staged writes before disk.
-// Effects are not asserted here (they run through writeEffects).
+// expectSink stages exactly the confirmed render (P7-LIVE-03): unplanned bytes,
+// removals or modes fail before publication. Reads overlay staged writes on disk.
+// Commit verifies completeness and publishes with rollback; host effects are separate.
 
 import (
 	"bytes"
@@ -28,26 +17,25 @@ import (
 	"github.com/nself-org/cli/internal/errs"
 )
 
-// deviationError is a write, removal or chmod that is not part of the
-// confirmed render.
+// deviationError identifies an operation outside the confirmed render.
 type deviationError struct{ msg string }
 
 func (e *deviationError) Error() string { return e.msg }
 
-// errDeviated builds a deviationError.
 func errDeviated(path, why string) error {
 	return &deviationError{msg: fmt.Sprintf("the project changed after the plan was made: %s %s; nothing further was written (re-run nself build --plan)", path, why)}
 }
 
 // expectSink is the checking disk sink.
 type expectSink struct {
 	*diskSink
-	exp        *PlannedBuild
-	keyr       *memSink // canonicalises paths exactly as plan mode did
-	dirs       map[string]fs.FileMode
-	atomic     map[string]bool
-	chmods     map[string]fs.FileMode
-	writeModes map[string]fs.FileMode
+	exp          *PlannedBuild
+	keyr         *memSink // canonicalises paths exactly as plan mode did
+	dirs         map[string]fs.FileMode
+	atomic       map[string]bool
+	chmods       map[string]fs.FileMode
+	writeModes   map[string]fs.FileMode
+	cleanupRoots map[string]fs.FileInfo
 }
 
 func newExpectSink(root string, exp *PlannedBuild) *expectSink {
@@ -104,6 +92,20 @@ func (e *expectSink) Remove(p string) error {
 	return e.keyr.Remove(p)
 }
 
+// RemoveWithin stages an unlink and remembers the cleanup directory identity
+// so deferred publication cannot be redirected to a different directory.
+func (e *expectSink) RemoveWithin(root *os.Root, _ string, p string) error {
+	info, err := root.Stat(".")
+	if err != nil {
+		return err
+	}
+	if e.cleanupRoots == nil {
+		e.cleanupRoots = map[string]fs.FileInfo{}
+	}
+	e.cleanupRoots[root.Name()] = info
+	return e.Remove(p)
+}
+
 func (e *expectSink) Chmod(p string, perm fs.FileMode) error {
 	k := e.keyr.key(p)
 	if f, ok := e.exp.Files[k]; ok && f.Perm == perm {