Install command aliases atomically

app.go

@@ -3,6 +3,7 @@ package commandkit
 import (
 	"fmt"
 
+	"gitea.maximumdirect.net/eric/CommandKit/internal/alias"
 	commandmeta "gitea.maximumdirect.net/eric/CommandKit/internal/commandmeta"
 )
 
@@ -12,6 +13,7 @@ type App struct {
 	identity     Identity
 	commands     map[string]*commandNode
 	routingRules []RoutingRule
+	aliases      *alias.Resolver
 	snapshot     *routingSnapshot
 }
 

docs/roadmap/implementation.md

@@ -1,6 +1,6 @@
 # Application-owned aliases implementation plan
 
-Status: in progress; Stages 1–4 completed.
+Status: in progress; Stages 1–5 completed.
 
 ## Authority, baseline, and scope
 
@@ -378,6 +378,13 @@ versus runnable operand space; valid longer aliases beneath `r → run`; failed
 registration after a snapshot was used. Explicitly reject
 `promptkit test full → promptkit test all` under a `test KIND` fixture.
 
+**Completed:** Replaced direct registry mutation with a candidate that validates
+the complete canonical tree and normalized routing rules before publishing.
+Alias chains and termination, missing or operand-bearing targets, canonical
+source and operand collisions, duplicate idempotence, later command conflicts,
+and failed-batch preservation are covered. Verified with
+`go test ./... -count=1`.
+
 ## Stage 6: Compile effective flag aliases during installation
 
 **Work**

registry.go

@@ -3,6 +3,10 @@ package commandkit
 import (
 	"errors"
 	"fmt"
+	"sort"
+
+	internalalias "gitea.maximumdirect.net/eric/CommandKit/internal/alias"
+	"gitea.maximumdirect.net/eric/CommandKit/internal/commandpath"
 )
 
 // Register adds a batch of declarations to the application. The complete batch
@@ -13,6 +17,31 @@ func (a *App) Register(declarations ...Declaration) error {
 	return a.installBatch(declarations...)
 }
 
+type installationCandidate struct {
+	commands     map[string]*commandNode
+	routingRules []RoutingRule
+	aliases      *internalalias.Resolver
+	changed      bool
+}
+
+type canonicalNode struct {
+	command  Command
+	path     commandpath.Path
+	children map[string]*canonicalNode
+}
+
+type commandAliasTrace struct {
+	exact        *canonicalNode
+	operandOwner *canonicalNode
+}
+
+type routingRuleKey struct {
+	kind   routingRuleKind
+	source string
+	target string
+	scope  string
+}
+
 func (a *App) installBatch(declarations ...Declaration) error {
 	if a == nil {
 		return fmt.Errorf("cannot register commands on a nil app")
@@ -42,43 +71,259 @@ func (a *App) installBatch(declarations ...Declaration) error {
 		}
 	}
 
-	pending := make(map[string]*commandNode, len(commands))
+	candidate, err := a.buildInstallationCandidate(commands, rules)
+	if err != nil {
+		return err
+	}
+	if !candidate.changed {
+		return nil
+	}
+	a.commands = candidate.commands
+	a.routingRules = candidate.routingRules
+	a.aliases = candidate.aliases
+	a.snapshot = nil
+	return nil
+}
+
+func (a *App) buildInstallationCandidate(commands []Command, addedRules []RoutingRule) (*installationCandidate, error) {
+	candidate := &installationCandidate{
+		commands: make(map[string]*commandNode, len(a.commands)+len(commands)),
+	}
+	for name, node := range a.commands {
+		candidate.commands[name] = node
+	}
 	for _, command := range commands {
 		if err := validateCommandPlacement(command); err != nil {
-			return err
+			return nil, err
 		}
 		if command.record.absent {
 			continue
 		}
 		name := command.record.name
-		if _, exists := a.commands[name]; exists {
-			return fmt.Errorf("command path %q is already registered", name)
+		if _, exists := candidate.commands[name]; exists {
+			return nil, fmt.Errorf("command path %q is already registered", name)
 		}
-		if _, exists := pending[name]; exists {
-			return fmt.Errorf("command path %q is already registered", name)
-		}
-		pending[name] = &commandNode{command: command}
+		candidate.commands[name] = &commandNode{command: command}
+		candidate.changed = true
 	}
+
+	rules, err := normalizeRoutingRules(append(append([]RoutingRule(nil), a.routingRules...), addedRules...))
+	if err != nil {
+		return nil, err
+	}
+	candidate.routingRules = rules
+	if !sameRoutingRules(rules, a.routingRules) {
+		candidate.changed = true
+	}
+
+	var mappings []internalalias.Mapping
+	for _, rule := range rules {
+		if rule.record.kind == commandAliasRule {
+			mappings = append(mappings, internalalias.Mapping{
+				Source: rule.record.sourcePath.Clone(),
+				Target: rule.record.targetPath.Clone(),
+			})
+		}
+	}
+	if err := internalalias.ValidateTermination(mappings); err != nil {
+		return nil, err
+	}
+	resolver, err := internalalias.NewResolver(mappings)
+	if err != nil {
+		return nil, err
+	}
+	candidate.aliases = resolver
+
+	tree := buildCanonicalTree(candidate.commands)
+	if err := validateCommandAliases(rules, resolver, tree); err != nil {
+		return nil, err
+	}
+	return candidate, nil
+}
+
+func normalizeRoutingRules(rules []RoutingRule) ([]RoutingRule, error) {
+	unique := make(map[routingRuleKey]RoutingRule, len(rules))
+	commandTargets := make(map[string]string)
 	for _, rule := range rules {
 		if rule.record == nil {
-			return fmt.Errorf("invalid zero routing rule declaration")
+			return nil, fmt.Errorf("invalid zero routing rule declaration")
 		}
 		if len(rule.record.issues) > 0 {
-			return fmt.Errorf("routing rule: %w", errors.Join(rule.record.issues...))
+			return nil, fmt.Errorf("routing rule: %w", errors.Join(rule.record.issues...))
+		}
+		key, err := routingKey(rule.record)
+		if err != nil {
+			return nil, err
+		}
+		if key.kind == commandAliasRule {
+			if previous, exists := commandTargets[key.source]; exists && previous != key.target {
+				return nil, fmt.Errorf("conflicting command aliases for %q", rule.record.sourcePath.String())
+			}
+			commandTargets[key.source] = key.target
+		}
+		unique[key] = rule
+	}
+	keys := make([]routingRuleKey, 0, len(unique))
+	for key := range unique {
+		keys = append(keys, key)
+	}
+	sort.Slice(keys, func(i, j int) bool {
+		left, right := keys[i], keys[j]
+		if left.kind != right.kind {
+			return left.kind < right.kind
+		}
+		if left.source != right.source {
+			return left.source < right.source
+		}
+		if left.scope != right.scope {
+			return left.scope < right.scope
+		}
+		return left.target < right.target
+	})
+	result := make([]RoutingRule, 0, len(keys))
+	for _, key := range keys {
+		result = append(result, unique[key])
+	}
+	return result, nil
+}
+
+func routingKey(record *routingRuleRecord) (routingRuleKey, error) {
+	key := routingRuleKey{kind: record.kind}
+	switch record.kind {
+	case commandAliasRule:
+		if len(record.sourcePath) == 0 || len(record.targetPath) == 0 {
+			return routingRuleKey{}, fmt.Errorf("invalid command alias path")
+		}
+		key.source = record.sourcePath.Key()
+		key.target = record.targetPath.Key()
+	case globalFlagAliasRule:
+		key.source = flagSpellingKey(record.sourceFlag)
+		key.target = flagSpellingKey(record.targetFlag)
+	case scopedFlagAliasRule:
+		if len(record.scopePath) == 0 {
+			return routingRuleKey{}, fmt.Errorf("invalid flag alias command scope")
+		}
+		key.source = flagSpellingKey(record.sourceFlag)
+		key.target = flagSpellingKey(record.targetFlag)
+		key.scope = record.scopePath.Key()
+	default:
+		return routingRuleKey{}, fmt.Errorf("unsupported routing rule kind %d", record.kind)
+	}
+	return key, nil
+}
+
+func flagSpellingKey(spelling capturedFlagSpelling) string {
+	if spelling.short {
+		return "short:" + spelling.name
+	}
+	return "long:" + spelling.name
+}
+
+func sameRoutingRules(left, right []RoutingRule) bool {
+	if len(left) != len(right) {
+		return false
+	}
+	leftKeys := make(map[routingRuleKey]struct{}, len(left))
+	for _, rule := range left {
+		key, err := routingKey(rule.record)
+		if err != nil {
+			return false
+		}
+		leftKeys[key] = struct{}{}
+	}
+	for _, rule := range right {
+		key, err := routingKey(rule.record)
+		if err != nil {
+			return false
+		}
+		if _, exists := leftKeys[key]; !exists {
+			return false
 		}
 	}
-	if len(pending) == 0 && len(rules) == 0 {
-		return nil
+	return true
+}
+
+func buildCanonicalTree(commands map[string]*commandNode) map[string]*canonicalNode {
+	roots := make(map[string]*canonicalNode, len(commands))
+	for name, node := range commands {
+		roots[name] = buildCanonicalNode(node.command, commandpath.Path{name})
 	}
-	if a.commands == nil {
-		a.commands = make(map[string]*commandNode, len(pending))
+	return roots
+}
+
+func buildCanonicalNode(command Command, path commandpath.Path) *canonicalNode {
+	node := &canonicalNode{command: command, path: path.Clone(), children: make(map[string]*canonicalNode, len(command.record.children))}
+	for _, child := range command.record.children {
+		if child.record == nil || child.record.absent {
+			continue
+		}
+		childPath := append(path.Clone(), child.record.name)
+		node.children[child.record.name] = buildCanonicalNode(child, childPath)
 	}
-	for name, node := range pending {
-		a.commands[name] = node
-	}
-	a.routingRules = append(a.routingRules, rules...)
-	if len(pending) > 0 {
-		a.snapshot = nil
+	return node
+}
+
+func validateCommandAliases(rules []RoutingRule, resolver *internalalias.Resolver, roots map[string]*canonicalNode) error {
+	for _, rule := range rules {
+		if rule.record.kind != commandAliasRule {
+			continue
+		}
+		record := rule.record
+		sourceTrace := traceCanonicalPath(roots, record.sourcePath)
+		if sourceTrace.exact != nil {
+			return fmt.Errorf("command alias source %q conflicts with canonical command %q", record.sourcePath.String(), sourceTrace.exact.path.String())
+		}
+		if sourceTrace.operandOwner != nil {
+			return fmt.Errorf("command alias source %q intercepts operands of canonical command %q", record.sourcePath.String(), sourceTrace.operandOwner.path.String())
+		}
+
+		resolvedTarget := resolver.Resolve(record.targetPath)
+		trace := traceCanonicalPath(roots, resolvedTarget)
+		if trace.exact != nil {
+			continue
+		}
+		if trace.operandOwner != nil {
+			return fmt.Errorf("command alias %q target %q includes operands for canonical command %q", record.sourcePath.String(), record.targetPath.String(), trace.operandOwner.path.String())
+		}
+		return fmt.Errorf("command alias %q target %q does not resolve to a canonical command or group", record.sourcePath.String(), record.targetPath.String())
 	}
 	return nil
 }
+
+func traceCanonicalPath(roots map[string]*canonicalNode, path commandpath.Path) commandAliasTrace {
+	var current *canonicalNode
+	children := roots
+	for index, component := range path {
+		next := children[component]
+		if next != nil {
+			current = next
+			children = current.children
+			continue
+		}
+		operandCount := len(path) - index
+		if current != nil && acceptsOperandCount(current.command, operandCount) {
+			return commandAliasTrace{operandOwner: current}
+		}
+		return commandAliasTrace{}
+	}
+	return commandAliasTrace{exact: current}
+}
+
+func acceptsOperandCount(command Command, count int) bool {
+	if command.record == nil || command.record.payload == nil || !command.record.payload.hasRun() {
+		return false
+	}
+	arguments := command.record.payload.bridgeArguments()
+	minimum, maximum := 0, 0
+	for _, argument := range arguments {
+		minimum += argument.Min
+		if maximum >= 0 {
+			if argument.Max < 0 {
+				maximum = -1
+			} else {
+				maximum += argument.Max
+			}
+		}
+	}
+	return count >= minimum && (maximum < 0 || count <= maximum)
+}

registry_test.go

@@ -2,7 +2,10 @@ package commandkit
 
 import (
 	"errors"
+	"fmt"
 	"testing"
+
+	"gitea.maximumdirect.net/eric/CommandKit/internal/commandpath"
 )
 
 type registryInput struct{ value string }
@@ -41,11 +44,11 @@ func TestCapturedDeclarationsRetainCallbacksWithoutCallingThem(t *testing.T) {
 }
 
 func TestRoutingRuleFactoriesCaptureNormalizedValues(t *testing.T) {
-	commandRule := Alias("  old\tpath ", "new path")
+	commandRule := Alias("  old\tpath ", " new ")
 	if got, want := commandRule.record.sourcePath.String(), "old path"; got != want {
 		t.Fatalf("source path = %q, want %q", got, want)
 	}
-	if got, want := commandRule.record.targetPath.String(), "new path"; got != want {
+	if got, want := commandRule.record.targetPath.String(), "new"; got != want {
 		t.Fatalf("target path = %q, want %q", got, want)
 	}
 	globalRule := FlagAlias("--colour", "-c")
@@ -117,6 +120,155 @@ func TestNilAndZeroDeclarationsFailWithoutMutation(t *testing.T) {
 	}
 }
 
+func TestAliasTargetsCanBeDeclaredInEitherOrder(t *testing.T) {
+	canonical := simpleRunnable("canonical")
+	for name, declarations := range map[string][]Declaration{
+		"targets last": {
+			Alias("shortcut", "alternate"),
+			Alias("alternate", "canonical"),
+			canonical,
+		},
+		"targets first": {
+			canonical,
+			Alias("alternate", "canonical"),
+			Alias("shortcut", "alternate"),
+		},
+	} {
+		t.Run(name, func(t *testing.T) {
+			app, err := New(Identity{Name: "Example"}, declarations...)
+			if err != nil {
+				t.Fatal(err)
+			}
+			if got := app.aliases.Resolve(commandpath.Path{"shortcut"}); !got.Equal(commandpath.Path{"canonical"}) {
+				t.Fatalf("shortcut resolves to %#v", got)
+			}
+		})
+	}
+}
+
+func TestIdenticalCommandAliasesAreIdempotentAcrossRegistrations(t *testing.T) {
+	app, err := New(Identity{Name: "Example"}, simpleRunnable("canonical"))
+	if err != nil {
+		t.Fatal(err)
+	}
+	rule := Alias("shortcut", "canonical")
+	if err := app.Register(rule, Alias(" shortcut ", " canonical ")); err != nil {
+		t.Fatal(err)
+	}
+	if len(app.routingRules) != 1 {
+		t.Fatalf("routing rules = %d, want one normalized rule", len(app.routingRules))
+	}
+	snapshot := app.routeSnapshot()
+	if err := app.Register(Alias("shortcut", "canonical")); err != nil {
+		t.Fatal(err)
+	}
+	if len(app.routingRules) != 1 || app.routeSnapshot() != snapshot {
+		t.Fatal("identical registration changed aliases or invalidated the snapshot")
+	}
+}
+
+func TestCommandAliasesRejectMissingAndCompetingTargets(t *testing.T) {
+	if _, err := New(Identity{Name: "Example"}, Alias("shortcut", "missing")); err == nil {
+		t.Fatal("New() accepted a missing alias target")
+	}
+	_, err := New(Identity{Name: "Example"},
+		simpleRunnable("canonical"),
+		Alias("first", "canonical"),
+		Alias("second", "canonical"),
+		Alias("shortcut", "first"),
+		Alias("shortcut", "second"),
+	)
+	if err == nil {
+		t.Fatal("New() accepted competing declared targets with a shared endpoint")
+	}
+}
+
+func TestCommandAliasesRejectCanonicalRoutesAndOperands(t *testing.T) {
+	group := Group("group", "group", simpleRunnable("show"))
+	if _, err := New(Identity{Name: "Example"}, group, simpleRunnable("target"), Alias("group show", "target")); err == nil {
+		t.Fatal("New() accepted an alias source that replaces a canonical child")
+	}
+
+	run := runnableWithOperands("run", 1, 1)
+	if _, err := New(Identity{Name: "Example"}, run, simpleRunnable("inspect"), Alias("run full", "inspect")); err == nil {
+		t.Fatal("New() accepted an alias source that intercepts a valid operand")
+	}
+	if _, err := New(Identity{Name: "Example"}, run, Alias("fast", "run production")); err == nil {
+		t.Fatal("New() accepted an operand-bearing alias target")
+	}
+
+	promptkit := Group("promptkit", "promptkit", runnableWithOperands("test", 1, 1))
+	if _, err := New(Identity{Name: "Example"}, promptkit, Alias("promptkit test full", "promptkit test all")); err == nil {
+		t.Fatal("New() accepted an alias that replaces the test command's KIND operand")
+	}
+}
+
+func TestLongerAliasBeneathAlternateSpellingCanUseOperandWords(t *testing.T) {
+	app, err := New(Identity{Name: "Example"},
+		runnableWithOperands("run", 1, 1),
+		simpleRunnable("inspect"),
+		Alias("r", "run"),
+		Alias("r full", "inspect"),
+		Alias("run too many words", "inspect"),
+	)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if got := app.aliases.Resolve(commandpath.Path{"r", "full"}); !got.Equal(commandpath.Path{"inspect"}) {
+		t.Fatalf("longer alternate route resolves to %#v", got)
+	}
+}
+
+func TestFailedAliasRegistrationPreservesCommandsRulesAndSnapshot(t *testing.T) {
+	app, err := New(Identity{Name: "Example"}, simpleRunnable("canonical"), Alias("existing", "canonical"))
+	if err != nil {
+		t.Fatal(err)
+	}
+	snapshot := app.routeSnapshot()
+	oldResolver := app.aliases
+	retained := errors.New("retained construction failure")
+	invalid := WithError(simpleRunnable("new"), fmt.Errorf("integration: %w", retained))
+	err = app.Register(invalid, Alias("later", "missing"))
+	if !errors.Is(err, retained) {
+		t.Fatalf("Register() error = %v, want retained cause", err)
+	}
+	if app.commands["new"] != nil || len(app.routingRules) != 1 || app.routeSnapshot() != snapshot || app.aliases != oldResolver {
+		t.Fatal("failed registration changed the app registry or derived metadata")
+	}
+}
+
+func TestNewCanonicalCommandIsCheckedAgainstExistingAliases(t *testing.T) {
+	app, err := New(Identity{Name: "Example"}, simpleRunnable("canonical"), Alias("shortcut", "canonical"))
+	if err != nil {
+		t.Fatal(err)
+	}
+	snapshot := app.routeSnapshot()
+	if err := app.Register(simpleRunnable("shortcut")); err == nil {
+		t.Fatal("Register() accepted a canonical command that collides with an alias source")
+	}
+	if app.commands["shortcut"] != nil || len(app.routingRules) != 1 || app.routeSnapshot() != snapshot {
+		t.Fatal("failed canonical registration changed the app registry or snapshot")
+	}
+}
+
+func simpleRunnable(name string) Command {
+	return NewCommand(CommandSpec[struct{}]{
+		Name: name, Summary: name,
+		Run: func(Invocation, struct{}) Outcome { return Success() },
+	})
+}
+
+func runnableWithOperands(name string, min, max int) Command {
+	return NewCommand(CommandSpec[struct{}]{
+		Name: name, Summary: name,
+		Arguments: []Argument[struct{}]{RepeatArgument(ArgumentMeta{Name: "ARG"}, min, max,
+			func(value string) (string, error) { return value, nil },
+			func(*struct{}, Supplied[[]string]) {},
+		)},
+		Run: func(Invocation, struct{}) Outcome { return Success() },
+	})
+}
+
 func TestAppsHaveIndependentRegistries(t *testing.T) {
 	first, err := New(Identity{Name: "First"})
 	if err != nil {