diff --git a/.github/wiki/Command-Canon.md b/.github/wiki/Command-Canon.md index 934b2cf66..18d004777 100644 --- a/.github/wiki/Command-Canon.md +++ b/.github/wiki/Command-Canon.md @@ -76,6 +76,8 @@ Total rows: 60 ## 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 (`: -> `); run it before the v1.6.0 removal and when updating docs. diff --git a/.github/wiki/database-migrations.md b/.github/wiki/database-migrations.md index 8f0f5700e..17068c26b 100644 --- a/.github/wiki/database-migrations.md +++ b/.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 ` 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 | diff --git a/cmd/commands/canon_engine_argv.go b/cmd/commands/canon_engine_argv.go index 55a8f08ca..440327a90 100644 --- a/cmd/commands/canon_engine_argv.go +++ b/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" } diff --git a/cmd/commands/canon_engine_argv_test.go b/cmd/commands/canon_engine_argv_test.go index 48f2e372a..10a1118b5 100644 --- a/cmd/commands/canon_engine_argv_test.go +++ b/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 } diff --git a/cmd/commands/db_hasura.go b/cmd/commands/db_hasura.go index 184e0aa3e..c9a7fc376 100644 --- a/cmd/commands/db_hasura.go +++ b/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) } diff --git a/cmd/commands/db_migrate.go b/cmd/commands/db_migrate.go index 90c52e8e7..ac55cdaff 100644 --- a/cmd/commands/db_migrate.go +++ b/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) } diff --git a/cmd/commands/db_remote_plugin.go b/cmd/commands/db_remote_plugin.go index 7e6bcbdc1..a4be88d3d 100644 --- a/cmd/commands/db_remote_plugin.go +++ b/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 " 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 " 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 } diff --git a/cmd/commands/db_remote_plugin_test.go b/cmd/commands/db_remote_plugin_test.go index 83fd62357..e20c7b5fd 100644 --- a/cmd/commands/db_remote_plugin_test.go +++ b/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") + } +} diff --git a/cmd/commands/plugin_flag.go b/cmd/commands/plugin_flag.go deleted file mode 100644 index 71544a8df..000000000 --- a/cmd/commands/plugin_flag.go +++ /dev/null @@ -1,20 +0,0 @@ -package commands - -// plugin_flag.go — the shared --plugin flag check for db commands. -// -// Purpose: refuse a path-shaped --plugin value before any directory is read -// (P7-PLUG-20). Split out of plugin_lifecycle.go for the 300-line cap. -// Inputs: the --plugin value ("" means the project's own migrations). -// Outputs: nil, or the E401 error from plugin.ValidateName. -// Constraints: no I/O. - -import "github.com/nself-org/cli/internal/plugin" - -// validatePluginFlag checks a non-empty --plugin value with plugin.ValidateName, -// so a path-shaped name is refused before any directory is read (P7-PLUG-20). -func validatePluginFlag(name string) error { - if name == "" { - return nil - } - return plugin.ValidateName(name) -} diff --git a/internal/cmdregistry/flag_argv.go b/internal/cmdregistry/flag_argv.go new file mode 100644 index 000000000..b9a7ece3a --- /dev/null +++ b/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 +} diff --git a/internal/cmdregistry/flags.go b/internal/cmdregistry/flags.go index 7874eb8eb..9b0e7533b 100644 --- a/internal/cmdregistry/flags.go +++ b/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{}