fix(cli): identity warnings and remote plugin validation (D-1116, D-1151)

.github/wiki/Command-Canon.md

@@ -76,6 +76,8 @@ Total rows: 60
 ## Removal plan
 
 <!-- BEGIN PROSE:removal-plan -->
+Identity break-outs (`ci` to `ci`, `functions` to `functions`) keep their spelling and do not emit a deprecation warning, as an exception to the old-spelling warnings below. Longer moves beneath them, such as `ci eval` to `eval`, still warn.
+
 v1.5.0 ships this canon. Every old spelling above keeps working through all v1.5.x releases and prints a deprecation warning. **v1.6.0 removes every move, shim and retired hub in the table**: delete the rows from the canon fragments (`internal/canon/domains/`), run `make canon cmd-inventory wiki-commands parity`, and the old spellings are gone. `TestCanonShimLifetime` fails the build once the running version reaches a row's `removal_at`, so no shim can outlive v1.6.0 by accident. Break-outs and removals have no removal date: the command leaves core at v1.5.0. `scripts/ci/old-spellings.sh` prints every Go string literal and doc line that still names an old spelling (`<file>:<line> <old path> -> <new path>`); run it before the v1.6.0 removal and when updating docs.
 <!-- END PROSE:removal-plan -->
 

.github/wiki/database-migrations.md

@@ -59,6 +59,8 @@ Both `DELETE` statements execute inside the same transaction as the down DDL. Ei
 
 Both subcommands accept `--migration-dir <dir>` to override auto-detection. On remote targets (`--env staging|prod`) the flag is forwarded to the remote CLI; the pre-flight version-drift check guarantees the remote binary understands it.
 
+Remote migration commands validate `--plugin` locally before dispatch and forward it once. A directory value equal to `--plugin` remains a directory value and does not suppress plugin validation or forwarding.
+
 ## Migration file naming
 
 | Pattern | Meaning |

cmd/commands/canon_engine_argv.go

@@ -22,6 +22,7 @@ package commands
 import (
 	"strings"
 
+	"github.com/nself-org/cli/internal/cmdregistry"
 	"github.com/spf13/cobra"
 	"github.com/spf13/pflag"
 )
@@ -72,6 +73,10 @@ func rewriteOldToNew(t *canonTableT, root *cobra.Command, args []string, start i
 	if kind == "removed" {
 		return nil, nil, removedError(best)
 	}
+	// Identity breakouts keep their spelling and need no deprecation warning.
+	if strings.Join(best.From, " ") == strings.Join(best.To, " ") {
+		return args, nil, nil
+	}
 	removal := best.RemovalAt
 	if removal == "" {
 		removal = canonRemovalDefault
@@ -285,14 +290,7 @@ func rootFlag(root *cobra.Command, a string) *pflag.Flag {
 		return f
 	}
 	if a == "--help" || a == "-h" || a == "--version" {
-		return &pflag.Flag{Name: strings.TrimLeft(a, "-"), Value: boolValue{}}
+		return &pflag.Flag{Name: strings.TrimLeft(a, "-"), Value: cmdregistry.NoopBoolValue{}}
 	}
 	return nil
 }
-
-// boolValue is a stand-in bool flag value for the implicit help flags.
-type boolValue struct{}
-
-func (boolValue) String() string   { return "false" }
-func (boolValue) Set(string) error { return nil }
-func (boolValue) Type() string     { return "bool" }

cmd/commands/canon_engine_argv_test.go

@@ -20,6 +20,30 @@ import (
 
 func sp(s string) []string { return strings.Fields(s) }
 
+func TestCanonEngineArgvIdentityBreakouts(t *testing.T) {
+	root := newFixtureTree()
+	for _, input := range []string{"ci", "ci nodes ls", "--json ci nodes ls", "functions", "functions deploy --force"} {
+		t.Run(input, func(t *testing.T) {
+			args := sp(input)
+			got, notes, err := rewriteCanonArgsWith(&canonTable, root, args, true)
+			if err != nil {
+				t.Fatal(err)
+			}
+			if !reflect.DeepEqual(got, args) {
+				t.Errorf("identity breakout changed argv: got %v, want %v", got, args)
+			}
+			if len(notes) != 0 {
+				t.Errorf("identity breakout emitted deprecation notes: %+v", notes)
+			}
+		})
+	}
+	// The identity parent must not hide a longer, real move beneath it.
+	got, notes, err := rewriteCanonArgsWith(&canonTable, root, sp("ci eval run"), true)
+	if err != nil || strings.Join(got, " ") != "eval run" || len(notes) != 1 || notes[0].New != "eval" {
+		t.Fatalf("child breakout: argv=%v notes=%+v err=%v", got, notes, err)
+	}
+}
+
 func TestCanonEngineArgv(t *testing.T) {
 	tb := fixtureTable(t)
 	type tc struct{ in, v14, v15 string }

cmd/commands/db_hasura.go

@@ -11,6 +11,7 @@ import (
 	"os/exec"
 
 	"github.com/nself-org/cli/internal/database"
+	"github.com/nself-org/cli/internal/plugin"
 
 	"github.com/spf13/cobra"
 )
@@ -90,15 +91,17 @@ func runDBHasuraMetadataReload(cmd *cobra.Command, _ []string) error {
 }
 
 func runDBVerifyChecksums(cmd *cobra.Command, _ []string) error {
-	plugin, _ := cmd.Flags().GetString("plugin")
-	if err := validatePluginFlag(plugin); err != nil {
-		return err
+	pluginName, _ := cmd.Flags().GetString("plugin")
+	if pluginName != "" {
+		if err := plugin.ValidateName(pluginName); err != nil {
+			return err
+		}
 	}
 	cfg, err := loadProjectConfig()
 	if err != nil {
 		return err
 	}
-	mismatches, err := database.VerifyChecksums(cmd.Context(), cfg, plugin)
+	mismatches, err := database.VerifyChecksums(cmd.Context(), cfg, pluginName)
 	if err != nil {
 		return fmt.Errorf("verify checksums: %w", err)
 	}

cmd/commands/db_migrate.go

@@ -13,6 +13,7 @@ import (
 	"time"
 
 	"github.com/nself-org/cli/internal/database"
+	"github.com/nself-org/cli/internal/plugin"
 
 	"github.com/spf13/cobra"
 )
@@ -54,9 +55,11 @@ func runDBMigrateUp(cmd *cobra.Command, _ []string) error {
 		return fmt.Errorf("PostgreSQL is not running — start with 'nself start' first")
 	}
 
-	plugin, _ := cmd.Flags().GetString("plugin")
-	if err := validatePluginFlag(plugin); err != nil {
-		return err
+	pluginName, _ := cmd.Flags().GetString("plugin")
+	if pluginName != "" {
+		if err := plugin.ValidateName(pluginName); err != nil {
+			return err
+		}
 	}
 
 	// --migration-dir: apply all .sql files in the given directory (G-008).
@@ -96,7 +99,7 @@ func runDBMigrateUp(cmd *cobra.Command, _ []string) error {
 	// Default directory dry-run: PendingMigrations issues SELECTs only and
 	// creates nothing (P7-PROD-84); it never reaches MigrateUp.
 	if dryRun {
-		pending, err := database.PendingMigrations(cmd.Context(), cfg, plugin)
+		pending, err := database.PendingMigrations(cmd.Context(), cfg, pluginName)
 		if err != nil {
 			return fmt.Errorf("pending migrations: %w", err)
 		}
@@ -110,7 +113,7 @@ func runDBMigrateUp(cmd *cobra.Command, _ []string) error {
 		return nil
 	}
 
-	count, err := database.MigrateUp(cmd.Context(), cfg, plugin)
+	count, err := database.MigrateUp(cmd.Context(), cfg, pluginName)
 	if err != nil {
 		return fmt.Errorf("migrate up: %w", err)
 	}

cmd/commands/db_remote_plugin.go

@@ -4,14 +4,19 @@ package commands
 // Inputs: the cobra command (may carry a --plugin flag) and the remote argv a
 // db subcommand built.
 // Outputs: the argv with "--plugin <name>" appended, or the E401 error from
-// validatePluginFlag for an invalid name.
+// plugin.ValidateName for an invalid name.
 // Constraints: no I/O. The argv is copied, never mutated in place. A caller
-// that already forwards --plugin is left alone; the dedup matches the exact
-// "--plugin" token because no builder emits the "--plugin=x" form. Quoting
+// that already forwards --plugin is left alone; flag metadata distinguishes
+// flags from their values during deduplication. Quoting
 // happens later in shellQuoteArgs. An invalid name fails before any SSH call.
-// SPORT: cli/cmd/commands — see D-0232, db_remote.go, plugin_flag.go.
+// SPORT: cli/cmd/commands — see D-0232, db_remote.go, internal/plugin/name_validate.go.
 
-import "github.com/spf13/cobra"
+import (
+	"github.com/spf13/cobra"
+
+	"github.com/nself-org/cli/internal/cmdregistry"
+	"github.com/nself-org/cli/internal/plugin"
+)
 
 // withRemotePluginFlag appends "--plugin <name>" to remoteArgs when cmd carries
 // a non-empty --plugin value and the caller did not already forward it (D-0232).
@@ -26,16 +31,16 @@ func withRemotePluginFlag(cmd *cobra.Command, remoteArgs []string) ([]string, er
 	if f == nil || f.Value.String() == "" {
 		return remoteArgs, nil
 	}
-	for _, a := range remoteArgs {
-		if a == "--plugin" {
-			return remoteArgs, nil
-		}
-	}
 	name := f.Value.String()
-	if err := validatePluginFlag(name); err != nil {
+	if err := plugin.ValidateName(name); err != nil {
 		return nil, err
 	}
+	insertAt, forwarded := cmdregistry.FlagInsertionIndex(remoteArgs, "plugin", cmd.Flags(), cmd.InheritedFlags())
+	if forwarded {
+		return remoteArgs, nil
+	}
 	out := make([]string, 0, len(remoteArgs)+2)
-	out = append(out, remoteArgs...)
-	return append(out, "--plugin", name), nil
+	out = append(out, remoteArgs[:insertAt]...)
+	out = append(out, "--plugin", name)
+	return append(out, remoteArgs[insertAt:]...), nil
 }

cmd/commands/db_remote_plugin_test.go

@@ -6,6 +6,7 @@ package commands
 
 import (
 	"context"
+	"reflect"
 	"testing"
 
 	"github.com/spf13/cobra"
@@ -100,3 +101,56 @@ func TestWithRemotePluginFlag_NoDuplicateNoMutation(t *testing.T) {
 		t.Fatalf("duplicate forwarded: %q, err = %v", again, err)
 	}
 }
+
+func TestWithRemotePluginFlag_DistinguishesFlagsFromValues(t *testing.T) {
+	for _, tc := range []struct {
+		name      string
+		args      []string
+		forwarded bool
+	}{
+		{"string value", []string{"--migration-dir", "--plugin"}, false},
+		{"inline string value", []string{"--migration-dir=--plugin"}, false},
+		{"boolean then plugin", []string{"--dry-run", "--plugin", "claw"}, true},
+		{"inline plugin", []string{"--plugin=claw"}, true},
+	} {
+		t.Run(tc.name, func(t *testing.T) {
+			cmd := &cobra.Command{Use: "up"}
+			cmd.Flags().String("plugin", "claw", "")
+			cmd.Flags().String("migration-dir", "", "")
+			cmd.Flags().Bool("dry-run", false, "")
+			out, err := withRemotePluginFlag(cmd, tc.args)
+			want := append([]string(nil), tc.args...)
+			if !tc.forwarded {
+				want = append(want, "--plugin", "claw")
+			}
+			if err != nil || !reflect.DeepEqual(out, want) {
+				t.Fatalf("out = %q, err = %v; want %q", out, err, want)
+			}
+		})
+	}
+}
+
+func TestWithRemotePluginFlag_PositionalPluginIsNotAFlag(t *testing.T) {
+	cmd := &cobra.Command{Use: "up"}
+	cmd.Flags().String("plugin", "claw", "")
+	out, err := withRemotePluginFlag(cmd, []string{"db", "migrate", "up", "--", "--plugin"})
+	want := []string{"db", "migrate", "up", "--plugin", "claw", "--", "--plugin"}
+	if err != nil || !reflect.DeepEqual(out, want) {
+		t.Fatalf("out = %q, err = %v; want %q", out, err, want)
+	}
+}
+
+func TestRemoteUp_PluginLikeDirectoryStillValidatesPlugin(t *testing.T) {
+	sent, err := remoteUp(t, map[string]string{"migration-dir": "--plugin", "plugin": "../x"})
+	if err == nil || len(sent) != 0 {
+		t.Fatalf("err = %v, sent = %q; want validation error and no SSH", err, sent)
+	}
+}
+
+func TestWithRemotePluginFlag_ValidatesAlreadyForwardedPlugin(t *testing.T) {
+	cmd := &cobra.Command{Use: "up"}
+	cmd.Flags().String("plugin", "../x", "")
+	if _, err := withRemotePluginFlag(cmd, []string{"--plugin", "../x"}); err == nil {
+		t.Fatal("already forwarded plugin bypassed local validation")
+	}
+}

internal/cmdregistry/flag_argv.go

@@ -0,0 +1,34 @@
+package cmdregistry
+
+import (
+	"strings"
+
+	"github.com/spf13/pflag"
+)
+
+// FlagInsertionIndex finds the position before a positional separator where a
+// flag can be inserted, and reports whether name is already forwarded. Required
+// flag values are skipped even when they resemble flags.
+func FlagInsertionIndex(args []string, name string, local, inherited *pflag.FlagSet) (int, bool) {
+	for i := 0; i < len(args); i++ {
+		a := args[i]
+		if a == "--" {
+			return i, false
+		}
+		if !strings.HasPrefix(a, "--") {
+			continue
+		}
+		flagName, _, inlineValue := strings.Cut(strings.TrimPrefix(a, "--"), "=")
+		if flagName == name {
+			return i, true
+		}
+		flag := local.Lookup(flagName)
+		if flag == nil {
+			flag = inherited.Lookup(flagName)
+		}
+		if flag != nil && flag.NoOptDefVal == "" && !inlineValue {
+			i++
+		}
+	}
+	return len(args), false
+}

internal/cmdregistry/flags.go

@@ -68,3 +68,14 @@ func strPtr(s string) *string {
 	}
 	return &s
 }
+
+// NoopBoolValue is a stand-in bool flag value for implicit flags that should
+// never be set by the user (e.g. suppressed help flags). It satisfies
+// pflag.Value but ignores all writes.
+type NoopBoolValue struct{}
+
+func (NoopBoolValue) String() string   { return "false" }
+func (NoopBoolValue) Set(string) error { return nil }
+func (NoopBoolValue) Type() string     { return "bool" }
+
+var _ pflag.Value = NoopBoolValue{}