feat(commands): wrapper UX hardening — persistent flags, -n drop, crashdump, kubeconfig hint, dmesg cushion, TUI refusal

README.md

@@ -90,7 +90,7 @@ floatingIP: ""
 
 Gather node information:
 ```bash
-talm -n 192.0.2.4 -e 192.0.2.4 template -t templates/controlplane.yaml -i > nodes/node1.yaml
+talm --nodes 192.0.2.4 -e 192.0.2.4 template -t templates/controlplane.yaml -i > nodes/node1.yaml
 ```
 
 Edit `nodes/node1.yaml` file:

main.go

@@ -73,8 +73,14 @@ func main() {
 	}
 }
 
-func Execute() error {
-	rootCmd.PersistentFlags().StringVar(
+// registerRootFlags installs the persistent flag set on rootCmd.
+// Extracted from Execute so tests can exercise the registration
+// without running cobra's executor. Single-call contract: cobra
+// panics on duplicate flag registration, so production calls this
+// exactly once from Execute; tests must build a fresh
+// *cobra.Command for each invocation.
+func registerRootFlags(cmd *cobra.Command) {
+	cmd.PersistentFlags().StringVar(
 		&commands.GlobalArgs.Talosconfig,
 		"talosconfig",
 		"",
@@ -84,13 +90,34 @@ func Execute() error {
 			filepath.Join(constants.ServiceAccountMountPath, constants.TalosconfigFilename),
 		),
 	)
-	rootCmd.PersistentFlags().StringVar(&commands.Config.RootDir, "root", ".", "root directory of the project")
-	rootCmd.PersistentFlags().StringVar(&commands.GlobalArgs.CmdContext, "context", "", "Context to be used in command")
-	rootCmd.PersistentFlags().StringSliceVarP(&commands.GlobalArgs.Nodes, "nodes", "n", []string{}, "target the specified nodes")
-	rootCmd.PersistentFlags().StringSliceVarP(&commands.GlobalArgs.Endpoints, "endpoints", "e", []string{}, "override default endpoints in Talos configuration")
-	rootCmd.PersistentFlags().StringVar(&commands.GlobalArgs.Cluster, "cluster", "", "Cluster to connect to if a proxy endpoint is used.")
-	rootCmd.PersistentFlags().BoolVar(&commands.GlobalArgs.SkipVerify, "skip-verify", false, "skip TLS certificate verification (keeps client authentication)")
-	rootCmd.PersistentFlags().Bool("version", false, "Print the version number of the application")
+	cmd.PersistentFlags().StringVar(&commands.Config.RootDir, "root", ".", "root directory of the project")
+	cmd.PersistentFlags().StringVar(&commands.GlobalArgs.CmdContext, "context", "", "Context to be used in command")
+	// --nodes is registered WITHOUT the `-n` shorthand. The
+	// previous registration carried `-n`, which silently captured
+	// any `-n <value>` an operator typed — for example
+	// `talm get hostnames -n network --nodes $NODE --endpoints
+	// $NODE` parsed `network` as a second node entry and then
+	// failed inside the gRPC name resolver with "produced zero
+	// addresses". Operators who type `-n namespace` for a
+	// subcommand argument (the muscle memory pattern from
+	// `kubectl`-style CLIs) now get a clean "flag -n not defined"
+	// from cobra — loud refusal instead of silent
+	// misinterpretation. The long form `--nodes` and modeline
+	// auto-population continue to work identically. Upstream
+	// talosctl does NOT register `-n` for `--namespace` on any
+	// subcommand (verified against image.go's PersistentFlags
+	// StringVar and get.go's local --namespace StringVar — both
+	// shorthand-free), so dropping `-n` from talm root closes a
+	// shadow trap without introducing any inherited-alias gap.
+	cmd.PersistentFlags().StringSliceVar(&commands.GlobalArgs.Nodes, "nodes", []string{}, "target the specified nodes")
+	cmd.PersistentFlags().StringSliceVarP(&commands.GlobalArgs.Endpoints, "endpoints", "e", []string{}, "override default endpoints in Talos configuration")
+	cmd.PersistentFlags().StringVar(&commands.GlobalArgs.Cluster, "cluster", "", "Cluster to connect to if a proxy endpoint is used.")
+	cmd.PersistentFlags().BoolVar(&commands.GlobalArgs.SkipVerify, "skip-verify", false, "skip TLS certificate verification (keeps client authentication)")
+	cmd.PersistentFlags().Bool("version", false, "Print the version number of the application")
+}
+
+func Execute() error {
+	registerRootFlags(rootCmd)
 
 	cmd, err := rootCmd.ExecuteContextC(context.Background())
 	if err != nil && !common.SuppressErrors {

main_test.go

@@ -182,6 +182,41 @@ func TestLoadConfig_EmptyApplyTimeoutResolvesDefault(t *testing.T) {
 	}
 }
 
+// TestRegisterRootFlags_NodesHasNoShorthand pins that talm's
+// root `--nodes` does NOT claim the `-n` shorthand. With `-n`
+// registered as the alias for `--nodes`, the global captures any
+// `-n <value>` an operator types — `talm get hostnames -n network
+// --nodes $NODE --endpoints $NODE` quietly parses `network` as a
+// second node entry, then fails inside the gRPC name resolver with
+// "produced zero addresses". Operators who type `-n namespace`
+// (kubectl muscle memory) now get a clean cobra "flag -n not
+// defined" error — loud refusal instead of silent
+// misinterpretation. The long form `--nodes` keeps working.
+// Upstream talosctl does not register `-n` for `--namespace` on
+// any wrapped subcommand (image's PersistentFlags --namespace and
+// get's local --namespace are both shorthand-free), so the change
+// closes a shadow trap without introducing an inherited-alias gap.
+func TestRegisterRootFlags_NodesHasNoShorthand(t *testing.T) {
+	// registerRootFlags writes default empty strings back through
+	// the cobra/pflag StringVar bindings into commands.GlobalArgs
+	// and commands.Config, which are package-level mutables.
+	// snapshotConfigState (defined above) saves+restores them so
+	// other tests aren't poisoned.
+	snapshotConfigState(t)
+
+	cmd := &cobra.Command{Use: "talm-test"}
+	registerRootFlags(cmd)
+
+	flag := cmd.PersistentFlags().Lookup("nodes")
+	if flag == nil {
+		t.Fatal("expected --nodes to be registered, got nil")
+	}
+
+	if flag.Shorthand != "" {
+		t.Errorf("--nodes shorthand: got %q, want empty (otherwise it shadows upstream `-n / --namespace`)", flag.Shorthand)
+	}
+}
+
 func TestSkipConfigCommands(t *testing.T) {
 	tests := []struct {
 		name     string

pkg/commands/crashdump_handler.go

@@ -0,0 +1,126 @@
+// Copyright Cozystack Authors
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package commands
+
+import "github.com/spf13/cobra"
+
+// wrapCrashdumpCommand pre-populates GlobalArgs.Nodes from upstream
+// crashdump's per-class node flags (--init-node, --control-plane-nodes,
+// --worker-nodes) so the upstream WithClient guard in
+// `cmd/talosctl/pkg/talos/global/client.go` (which checks
+// `len(GlobalArgs.Nodes) > 0`) is satisfied before crashdump's own
+// RunE runs.
+//
+// Without this pre-population an operator who follows talosctl
+// crashdump's documented help text — passing only --control-plane-nodes
+// and --endpoints — hits a "nodes are not set for the command" error
+// from the global client wrapper instead of crashdump's own
+// "deprecated, please use `talosctl support` instead" message
+// (crashdump is hidden + deprecated upstream, but the wrapper still
+// imports it through `taloscommands.Commands`).
+//
+// Wrapper populates GlobalArgs.Nodes from the per-class flags
+// BEFORE chaining to the original PreRunE. wrapTalosCommand's
+// PreRunE syncs `taloscommands.GlobalArgs = commands.GlobalArgs`
+// near its end — populating after the chain would update only
+// talm's side and leave upstream still seeing an empty list.
+//
+// Resulting precedence:
+//
+//   - Explicit `--nodes` on the CLI wins (it lands in
+//     GlobalArgs.Nodes during cobra's flag parse, before any
+//     PreRunE runs; the populate gate sees it non-empty and
+//     skips).
+//   - Per-class flags win over modeline. The chain order
+//     forced by the upstream-sync constraint runs the populate
+//     before the wrapTalosCommand PreRunE's modeline merge,
+//     and that merge captures `nodesFromArgs := len(GlobalArgs.
+//     Nodes) > 0` from the now-populated talm side, telling it
+//     NOT to overwrite. This is a deliberate trade-off: the
+//     upstream guard surface-fires unless GlobalArgs.Nodes is
+//     populated before sync, and the per-class flags carry
+//     operator intent more specifically than the modeline
+//     default for a deprecated command.
+//   - Modeline wins only when GlobalArgs.Nodes is empty AND no
+//     per-class flag was passed.
+//
+// Contract with the caller: wrapCrashdumpCommand MUST be installed
+// AFTER wrapTalosCommand's PreRunE assignment in
+// talosctl_wrapper.go, so its `originalPreRunE` capture points at
+// the wrapTalosCommand closure rather than a nil. If a future
+// refactor reorders the dispatch in talosctl_wrapper.go to call
+// wrapCrashdumpCommand earlier, the chain order inverts and the
+// upstream guard fires again.
+func wrapCrashdumpCommand(wrappedCmd *cobra.Command) {
+	originalPreRunE := wrappedCmd.PreRunE
+
+	wrappedCmd.PreRunE = func(cmd *cobra.Command, args []string) error {
+		populateNodesFromPerClassFlags(cmd)
+
+		if originalPreRunE != nil {
+			return originalPreRunE(cmd, args)
+		}
+
+		return nil
+	}
+}
+
+// populateNodesFromPerClassFlags fills GlobalArgs.Nodes from the
+// union of --init-node, --control-plane-nodes, and --worker-nodes
+// when GlobalArgs.Nodes is otherwise empty. Used by the wrappers
+// for upstream commands that consume these per-class lists rather
+// than the global --nodes (crashdump, rotate-ca).
+//
+// MUST be called BEFORE wrapTalosCommand's PreRunE assignment in
+// the chain — that closure syncs
+// `taloscommands.GlobalArgs = commands.GlobalArgs` near its end,
+// and any population after the sync only updates talm's side,
+// leaving the upstream WithClient guard still seeing an empty
+// list. Per-command wrappers (wrapCrashdumpCommand,
+// wrapRotateCACommand) capture the original PreRunE first, then
+// install a closure that calls this helper BEFORE chaining.
+//
+// Walk order is stable (init -> control-plane -> worker) so the
+// resolved list is deterministic when multiple classes are set.
+// Explicit --nodes (CLI or modeline) takes precedence: the gate
+// is len(GlobalArgs.Nodes) == 0.
+//
+// Per-class flag lookups are tolerant: missing flag definitions
+// return errors that are silently swallowed, so the helper is
+// safe to call on any command that may or may not have all three
+// registered.
+func populateNodesFromPerClassFlags(cmd *cobra.Command) {
+	if len(GlobalArgs.Nodes) > 0 {
+		return
+	}
+
+	populated := []string{}
+
+	if initNode, err := cmd.Flags().GetString("init-node"); err == nil && initNode != "" {
+		populated = append(populated, initNode)
+	}
+
+	if cps, err := cmd.Flags().GetStringSlice("control-plane-nodes"); err == nil {
+		populated = append(populated, cps...)
+	}
+
+	if workers, err := cmd.Flags().GetStringSlice("worker-nodes"); err == nil {
+		populated = append(populated, workers...)
+	}
+
+	if len(populated) > 0 {
+		GlobalArgs.Nodes = populated
+	}
+}

pkg/commands/dmesg_handler.go

@@ -0,0 +1,68 @@
+// Copyright Cozystack Authors
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package commands
+
+import (
+	"strings"
+
+	"github.com/cockroachdb/errors"
+	"github.com/spf13/cobra"
+)
+
+// dmesgCmdName labels the wrapped dmesg subcommand in the dispatch.
+// Hoisted so a future rename touches one site.
+const dmesgCmdName = "dmesg"
+
+// wrapDmesgCommand installs a FlagErrorFunc that rewrites the
+// cryptic ParseBool error from a numeric --tail value into an
+// operator-friendly hint.
+//
+// Upstream talosctl registers --tail as a BoolVarP toggling
+// tail-mode for --follow (`Dmesg(ctx, follow, tail bool)` on the
+// wire). Operators' first instinct is `tail(1)`-style line count;
+// `talm dmesg --tail=3` then surfaces
+// `strconv.ParseBool: parsing "3": invalid syntax` with no hint
+// at the actual contract.
+//
+// The original ParseBool error is wrapped (not replaced) so it
+// stays reachable via errors.Unwrap and verbose fmt.Sprintf("%+v",
+// err) rendering — debugging the underlying pflag failure remains
+// possible while the operator-facing top line describes what --tail
+// actually does and what to do instead.
+func wrapDmesgCommand(wrappedCmd *cobra.Command) {
+	wrappedCmd.SetFlagErrorFunc(func(_ *cobra.Command, err error) error {
+		// Substring detection on pflag's error format. Two tokens
+		// — `--tail` plus `ParseBool` — together are specific
+		// enough to avoid false positives (pflag only emits
+		// ParseBool errors for bool flags). Matching on `--tail`
+		// without surrounding quotes is durable to pflag's
+		// rendering difference between shorthand-present and
+		// shorthand-absent flags: today's `BoolVarP(..., "tail",
+		// "", ...)` emits `"--tail"`, but a future upstream that
+		// adds a shorthand would emit `-T, --tail` and would
+		// otherwise silently bypass the cushion. pflag does not
+		// export a typed error for invalid-argument failures, so
+		// substring is the only available detection path.
+		msg := err.Error()
+		if strings.Contains(msg, "--tail") && strings.Contains(msg, "ParseBool") {
+			return errors.WithHint(
+				errors.Wrap(err, "talm dmesg --tail is a boolean toggle (tail-mode for --follow), not a line count"),
+				"for the last N lines, pipe to tail(1): `talm dmesg --nodes <node> | tail -n N`; to stream only new messages, use `--follow --tail`",
+			)
+		}
+
+		return err
+	})
+}

pkg/commands/kubeconfig_handler.go

@@ -170,11 +170,18 @@ Otherwise, kubeconfig will be written to PWD.`
 
 		return nil
 	}
-	// Set custom Args validation: allow no args by default, but allow args when --login is set
+	// Set custom Args validation: allow no args by default, but allow
+	// args when --login is set. The error message describes the
+	// wrapper's actual contract (default writes to project root;
+	// --login redirects to the system kubeconfig) instead of the
+	// previous misleading wording about "--login to pass arguments".
 	wrappedCmd.Args = func(cmd *cobra.Command, args []string) error {
 		loginFlagValue, _ := cmd.Flags().GetBool("login")
 		if !loginFlagValue && len(args) > 0 {
-			return errors.New("kubeconfig command does not accept arguments (use --login flag to pass arguments)")
+			return errors.WithHint(
+				errors.New("talm kubeconfig writes to project root by default; positional path is not consumed in this mode"),
+				"to write to a specific path, pass --login /your/path (the positional argument is honoured under --login); --login with no positional defaults to ~/.kube/config (the system kubeconfig)",
+			)
 		}
 
 		return nil

pkg/commands/rotate_ca_handler.go

@@ -57,7 +57,7 @@ The command works by:
 3. Gracefully rolling out the new CAs to all nodes
 4. Updating local configs (talosconfig, secrets.yaml, kubeconfig)
 
-IMPORTANT: You must specify exactly ONE control-plane node via --endpoints/-e or --nodes/-n
+IMPORTANT: You must specify exactly ONE control-plane node via --endpoints/-e or --nodes
 flags, or through a single config file (-f). The node must be a control-plane node.
 
 By default, both Talos API CA and Kubernetes API CA are rotated. Use --talos=false
@@ -92,6 +92,18 @@ The command runs in dry-run mode by default. Use --dry-run=false to perform actu
 	originalPreRunE := wrappedCmd.PreRunE
 
 	wrappedCmd.PreRunE = func(cmd *cobra.Command, args []string) error {
+		// Populate GlobalArgs.Nodes from upstream's per-class node
+		// flags (--init-node / --control-plane-nodes / --worker-nodes)
+		// BEFORE chaining the original PreRunE. See
+		// populateNodesFromPerClassFlags godoc for the chain-order
+		// rationale (upstream WithClient guard reads from
+		// taloscommands.GlobalArgs after the wrapper's sync; populating
+		// after the sync leaves upstream blind to the per-class lists).
+		// Upstream rotate-ca's contract is "exactly one CP node" — the
+		// multi-node guard below catches the case where the populated
+		// list ends up with more than one entry.
+		populateNodesFromPerClassFlags(cmd)
+
 		// Run original PreRunE first (processes modeline, syncs GlobalArgs, etc.)
 		if originalPreRunE != nil {
 			if err := originalPreRunE(cmd, args); err != nil {
@@ -110,7 +122,7 @@ The command runs in dry-run mode by default. Use --dry-run=false to perform actu
 		if len(GlobalArgs.Nodes) > 1 {
 			return errors.WithHint(
 				errors.Newf("rotate-ca requires exactly one control-plane node, but %d nodes were provided", len(GlobalArgs.Nodes)),
-				"the rotate-ca command coordinates CA rotation across the entire cluster from a single control-plane node; specify only one node via --nodes or a single config file",
+				"the rotate-ca command coordinates CA rotation across the entire cluster from a single control-plane node; specify only one node via --nodes, --control-plane-nodes with a single IP, or a single config file",
 			)
 		}