diff --git a/app.go b/app.go index 92e4723..60c8fce 100644 --- a/app.go +++ b/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 } diff --git a/docs/roadmap/implementation.md b/docs/roadmap/implementation.md index bc46bd4..0f87e61 100644 --- a/docs/roadmap/implementation.md +++ b/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** diff --git a/registry.go b/registry.go index eab2f3c..eff7ada 100644 --- a/registry.go +++ b/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) +} diff --git a/registry_test.go b/registry_test.go index 812b004..be731ae 100644 --- a/registry_test.go +++ b/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 {